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<SkillMod>` to `HashSet<SkillMod>`.
- [X] Eliminates skill mods adding/removing twice.
This commit is contained in:
Kamron Batman 2022-06-30 12:07:08 -07:00 committed by GitHub
parent 414ef8d8d6
commit cb474712f8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 80 additions and 109 deletions

View file

@ -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<SkillMod> SkillMods { get; private set; }
public HashSet<SkillMod> SkillMods { get; private set; }
[CommandProperty(AccessLevel.GameMaster)]
public int VirtualArmorMod
@ -1795,7 +1796,7 @@ namespace Server
/// <summary>
/// Gets a list of all <see cref="StatMod">StatMod's</see> currently active for the Mobile.
/// </summary>
public List<StatMod> StatMods { get; private set; }
public HashSet<StatMod> StatMods { get; private set; }
/// <summary>
/// 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<SkillMod>.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<StatMod>();
SkillMods = new List<SkillMod>();
StatMods = new HashSet<StatMod>();
SkillMods = new HashSet<SkillMod>();
if (version < 32)
{
@ -7621,8 +7618,8 @@ namespace Server
m_FollowersMax = 5;
Skills = new Skills(this);
Items = new List<Item>();
StatMods = new List<StatMod>();
SkillMods = new List<SkillMod>();
StatMods = new HashSet<StatMod>();
SkillMods = new HashSet<SkillMod>();
Map = Map.Internal;
AutoPageNotify = true;
Aggressors = new List<AggressorInfo>();
@ -8317,19 +8314,16 @@ namespace Server
public bool RemoveStatMod(string name)
{
StatMods ??= new List<StatMod>();
StatMods ??= new HashSet<StatMod>();
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<StatMod>();
StatMods ??= new HashSet<StatMod>();
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<StatMod>();
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<StatMod>();
StatMods ??= new HashSet<StatMod>();
for (var i = 0; i < StatMods.Count; ++i)
if (StatMods.Count > 0)
{
var mod = StatMods[i];
if (mod.HasElapsed())
using var queue = PooledRefQueue<StatMod>.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;
}
}

View file

@ -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;
}

View file

@ -13,6 +13,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>. *
*************************************************************************/
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();
}

View file

@ -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;

View file

@ -1320,7 +1320,7 @@ namespace Server.Factions
var context = new SkillLossContext();
m_SkillLoss[mob] = context;
var mods = context.m_Mods = new List<SkillMod>();
var mods = context.m_Mods = new HashSet<SkillMod>();
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<SkillMod> m_Mods;
public HashSet<SkillMod> m_Mods;
public TimerExecutionToken _timerToken;
}
}

View file

@ -955,7 +955,7 @@ namespace Server
public sealed class AosSkillBonuses : BaseAttributes
{
private List<SkillMod> m_Mods;
private HashSet<SkillMod> m_Mods;
public AosSkillBonuses(Item owner) : base(owner)
{
@ -1068,7 +1068,7 @@ namespace Server
continue;
}
m_Mods ??= new List<SkillMod>();
m_Mods ??= new HashSet<SkillMod>();
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)
{