From cb474712f8bc8a21f21ca5692ee931c375932226 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Thu, 30 Jun 2022 12:07:08 -0700 Subject: [PATCH] fix: Removes side effect of setting skill mod when changing Owner (#1101) ## Breaking Change! Setting `mod.Owner` will no longer update a skill mod. **Please stick to the API and use `mobile.AddSkillMod(mod)` for all situations, including equipping.** ## Fixes - [X] Fixes memory leak in factions. - [X] Changes `List` to `HashSet`. - [X] Eliminates skill mods adding/removing twice. --- Projects/Server/Mobiles/Mobile.cs | 114 ++++++++---------- Projects/Server/Mobiles/Mods/MobileMod.cs | 2 +- Projects/Server/Mobiles/Mods/SkillMod.cs | 22 +--- Projects/Server/Skills.cs | 32 ++--- .../Engines/Factions/Core/Faction.cs | 9 +- Projects/UOContent/Misc/AOS.cs | 10 +- 6 files changed, 80 insertions(+), 109 deletions(-) diff --git a/Projects/Server/Mobiles/Mobile.cs b/Projects/Server/Mobiles/Mobile.cs index c86ef7a40..ece4d3fc9 100644 --- a/Projects/Server/Mobiles/Mobile.cs +++ b/Projects/Server/Mobiles/Mobile.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Runtime.CompilerServices; using Server.Accounting; using Server.Buffers; +using Server.Collections; using Server.ContextMenus; using Server.Guilds; using Server.Gumps; @@ -424,7 +425,7 @@ namespace Server public object Party { get; set; } - public List SkillMods { get; private set; } + public HashSet SkillMods { get; private set; } [CommandProperty(AccessLevel.GameMaster)] public int VirtualArmorMod @@ -1795,7 +1796,7 @@ namespace Server /// /// Gets a list of all StatMod's currently active for the Mobile. /// - public List StatMods { get; private set; } + public HashSet StatMods { get; private set; } /// /// Gets or sets the base, unmodified, strength of the Mobile. Ranges from 1 to 65000, inclusive. @@ -3379,9 +3380,8 @@ namespace Server { ValidateSkillMods(); - for (var i = 0; i < SkillMods.Count; ++i) + foreach (var mod in SkillMods) { - var mod = SkillMods[i]; var sk = Skills[mod.Skill]; sk?.Update(); } @@ -3389,18 +3389,18 @@ namespace Server public virtual void ValidateSkillMods() { - for (var i = 0; i < SkillMods.Count;) + using var queue = PooledRefQueue.Create(8); + foreach (var mod in SkillMods) { - var mod = SkillMods[i]; + if (!mod.CheckCondition()) + { + queue.Enqueue(mod); + } + } - if (mod.CheckCondition()) - { - ++i; - } - else - { - InternalRemoveSkillMod(mod); - } + while (queue.Count > 0) + { + InternalRemoveSkillMod(queue.Dequeue()); } } @@ -3413,9 +3413,8 @@ namespace Server ValidateSkillMods(); - if (!SkillMods.Contains(mod)) + if (SkillMods.Add(mod)) { - SkillMods.Add(mod); mod.Owner = this; var sk = Skills[mod.Skill]; @@ -3430,16 +3429,14 @@ namespace Server return; } - ValidateSkillMods(); - InternalRemoveSkillMod(mod); + ValidateSkillMods(); } private void InternalRemoveSkillMod(SkillMod mod) { - if (SkillMods.Contains(mod)) + if (SkillMods.Remove(mod)) { - SkillMods.Remove(mod); mod.Owner = null; var sk = Skills[mod.Skill]; @@ -6272,8 +6269,8 @@ namespace Server m_DexLock = (StatLockType)reader.ReadByte(); m_IntLock = (StatLockType)reader.ReadByte(); - StatMods = new List(); - SkillMods = new List(); + StatMods = new HashSet(); + SkillMods = new HashSet(); if (version < 32) { @@ -7621,8 +7618,8 @@ namespace Server m_FollowersMax = 5; Skills = new Skills(this); Items = new List(); - StatMods = new List(); - SkillMods = new List(); + StatMods = new HashSet(); + SkillMods = new HashSet(); Map = Map.Internal; AutoPageNotify = true; Aggressors = new List(); @@ -8317,19 +8314,16 @@ namespace Server public bool RemoveStatMod(string name) { - StatMods ??= new List(); + StatMods ??= new HashSet(); - for (var i = 0; i < StatMods.Count; ++i) + StatMod mod = GetStatMod(name); + + if (mod != null) { - var check = StatMods[i]; - - if (check.Name == name) - { - StatMods.RemoveAt(i); - CheckStatTimers(); - Delta(MobileDelta.Stat | GetStatDelta(check.Type)); - return true; - } + StatMods.Remove(mod); + CheckStatTimers(); + Delta(MobileDelta.Stat | GetStatDelta(mod.Type)); + return true; } return false; @@ -8337,12 +8331,10 @@ namespace Server public StatMod GetStatMod(string name) { - StatMods ??= new List(); + StatMods ??= new HashSet(); - for (var i = 0; i < StatMods.Count; ++i) + foreach (var check in StatMods) { - var check = StatMods[i]; - if (check.Name == name) { return check; @@ -8354,19 +8346,7 @@ namespace Server public void AddStatMod(StatMod mod) { - StatMods ??= new List(); - - for (var i = 0; i < StatMods.Count; ++i) - { - var check = StatMods[i]; - - if (check.Name == mod.Name) - { - Delta(MobileDelta.Stat | GetStatDelta(check.Type)); - StatMods.RemoveAt(i); - break; - } - } + RemoveStatMod(mod.Name); StatMods.Add(mod); Delta(MobileDelta.Stat | GetStatDelta(mod.Type)); @@ -8402,23 +8382,29 @@ namespace Server { var offset = 0; - StatMods ??= new List(); + StatMods ??= new HashSet(); - for (var i = 0; i < StatMods.Count; ++i) + if (StatMods.Count > 0) { - var mod = StatMods[i]; - - if (mod.HasElapsed()) + using var queue = PooledRefQueue.Create(8); + foreach (var mod in StatMods) { - StatMods.RemoveAt(i); + if (mod.HasElapsed()) + { + queue.Enqueue(mod); + } + else if ((mod.Type & type) != 0) + { + offset += mod.Offset; + } + } + + while (queue.Count > 0) + { + var mod = queue.Dequeue(); + StatMods.Remove(mod); Delta(MobileDelta.Stat | GetStatDelta(mod.Type)); CheckStatTimers(); - - --i; - } - else if ((mod.Type & type) != 0) - { - offset += mod.Offset; } } diff --git a/Projects/Server/Mobiles/Mods/MobileMod.cs b/Projects/Server/Mobiles/Mods/MobileMod.cs index be1083c0d..797e0e543 100644 --- a/Projects/Server/Mobiles/Mods/MobileMod.cs +++ b/Projects/Server/Mobiles/Mods/MobileMod.cs @@ -21,7 +21,7 @@ namespace Server; public partial class MobileMod { [DirtyTrackingEntity] - public virtual Mobile Owner { get; set; } + public Mobile Owner { get; set; } public MobileMod(Mobile owner) => Owner = owner; } diff --git a/Projects/Server/Mobiles/Mods/SkillMod.cs b/Projects/Server/Mobiles/Mods/SkillMod.cs index d5b600241..680d92b88 100644 --- a/Projects/Server/Mobiles/Mods/SkillMod.cs +++ b/Projects/Server/Mobiles/Mods/SkillMod.cs @@ -13,6 +13,7 @@ * along with this program. If not, see . * *************************************************************************/ +using System.Runtime.CompilerServices; using ModernUO.Serialization; namespace Server; @@ -50,21 +51,6 @@ public abstract partial class SkillMod : MobileMod } } - public override Mobile Owner - { - get => base.Owner; - set - { - var owner = base.Owner; - if (owner != value) - { - owner?.RemoveSkillMod(this); - base.Owner = owner = value; - owner?.AddSkillMod(this); - } - } - } - [SerializableField(1)] public SkillName Skill { @@ -136,10 +122,8 @@ public abstract partial class SkillMod : MobileMod } } - public void Remove() - { - Owner = null; - } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public void Remove() => Owner?.RemoveSkillMod(this); public abstract bool CheckCondition(); } diff --git a/Projects/Server/Skills.cs b/Projects/Server/Skills.cs index 0bb156e35..26532d06f 100644 --- a/Projects/Server/Skills.cs +++ b/Projects/Server/Skills.cs @@ -276,30 +276,30 @@ namespace Server double bonusObey = 0.0, bonusNotObey = 0.0; - for (var i = 0; i < mods.Count; ++i) + foreach (var mod in mods) { - var mod = mods[i]; - - if (mod.Skill == (SkillName)Info.SkillID) + if (mod.Skill != (SkillName)Info.SkillID) { - if (mod.Relative) + continue; + } + + if (mod.Relative) + { + if (mod.ObeyCap) { - if (mod.ObeyCap) - { - bonusObey += mod.Value; - } - else - { - bonusNotObey += mod.Value; - } + bonusObey += mod.Value; } else { - bonusObey = 0.0; - bonusNotObey = 0.0; - value = mod.Value; + bonusNotObey += mod.Value; } } + else + { + bonusObey = 0.0; + bonusNotObey = 0.0; + value = mod.Value; + } } value += bonusNotObey; diff --git a/Projects/UOContent/Engines/Factions/Core/Faction.cs b/Projects/UOContent/Engines/Factions/Core/Faction.cs index 9345a7328..f4f126922 100644 --- a/Projects/UOContent/Engines/Factions/Core/Faction.cs +++ b/Projects/UOContent/Engines/Factions/Core/Faction.cs @@ -1320,7 +1320,7 @@ namespace Server.Factions var context = new SkillLossContext(); m_SkillLoss[mob] = context; - var mods = context.m_Mods = new List(); + var mods = context.m_Mods = new HashSet(); for (var i = 0; i < mob.Skills.Length; ++i) { @@ -1352,11 +1352,12 @@ namespace Server.Factions var mods = context.m_Mods; - for (var i = 0; i < mods.Count; ++i) + foreach (var mod in mods) { - mob.RemoveSkillMod(mods[i]); + mod.Remove(); } + context.m_Mods = null; context._timerToken.Cancel(); return true; @@ -1376,7 +1377,7 @@ namespace Server.Factions private class SkillLossContext { - public List m_Mods; + public HashSet m_Mods; public TimerExecutionToken _timerToken; } } diff --git a/Projects/UOContent/Misc/AOS.cs b/Projects/UOContent/Misc/AOS.cs index 18f7f3210..ed395c055 100644 --- a/Projects/UOContent/Misc/AOS.cs +++ b/Projects/UOContent/Misc/AOS.cs @@ -955,7 +955,7 @@ namespace Server public sealed class AosSkillBonuses : BaseAttributes { - private List m_Mods; + private HashSet m_Mods; public AosSkillBonuses(Item owner) : base(owner) { @@ -1068,7 +1068,7 @@ namespace Server continue; } - m_Mods ??= new List(); + m_Mods ??= new HashSet(); SkillMod sk = new DefaultSkillMod(skill, true, bonus); sk.ObeyCap = true; @@ -1084,10 +1084,10 @@ namespace Server return; } - for (var i = 0; i < m_Mods.Count; ++i) + foreach (var mod in m_Mods) { - var m = m_Mods[i].Owner; - m_Mods[i].Remove(); + var m = mod.Owner; + mod.Remove(); if (Core.ML) {