DamageableSystem cleanup & performance improvements (#20820)

Content.Server/Bible/BibleSystem.cs

@@ -133,7 +133,7 @@ namespace Content.Server.Bible
 
             var damage = _damageableSystem.TryChangeDamage(args.Target.Value, component.Damage, true, origin: uid);
 
-            if (damage == null || damage.Total == 0)
+            if (damage == null || damage.Empty)
             {
                 var othersMessage = Loc.GetString(component.LocPrefix + "-heal-success-none-others", ("user", Identity.Entity(args.User, EntityManager)),("target", Identity.Entity(args.Target.Value, EntityManager)),("bible", uid));
                 _popupSystem.PopupEntity(othersMessage, args.User, Filter.PvsExcept(args.User), true, PopupType.Medium);

Content.Server/Electrocution/ElectrocutionSystem.cs

@@ -177,7 +177,7 @@ public sealed class ElectrocutionSystem : SharedElectrocutionSystem
         if (!electrified.OnAttacked)
             return;
 
-        if (_meleeWeapon.GetDamage(args.Used, args.User).Total == 0)
+        if (!_meleeWeapon.GetDamage(args.Used, args.User).Any())
             return;
 
         TryDoElectrifiedAct(uid, args.User, 1, electrified);
@@ -192,7 +192,7 @@ public sealed class ElectrocutionSystem : SharedElectrocutionSystem
     private void OnLightAttacked(EntityUid uid, PoweredLightComponent component, AttackedEvent args)
     {
 
-        if (_meleeWeapon.GetDamage(args.Used, args.User).Total == 0)
+        if (!_meleeWeapon.GetDamage(args.Used, args.User).Any())
             return;
 
         if (args.Used != args.User)

Content.Server/Projectiles/ProjectileSystem.cs

@@ -53,7 +53,7 @@ public sealed class ProjectileSystem : SharedProjectileSystem
 
         if (modifiedDamage is not null && EntityManager.EntityExists(component.Shooter))
         {
-            if (modifiedDamage.Total > FixedPoint2.Zero && !deleted)
+            if (modifiedDamage.Any() && !deleted)
             {
                 _color.RaiseEffect(Color.Red, new List<EntityUid> { target }, Filter.Pvs(target, entityManager: EntityManager));
             }

Content.Server/Weapons/Melee/MeleeWeaponSystem.cs

@@ -62,7 +62,7 @@ public sealed class MeleeWeaponSystem : SharedMeleeWeaponSystem
 
         var damageSpec = GetDamage(uid, args.User, component);
 
-        if (damageSpec.Total == FixedPoint2.Zero)
+        if (damageSpec.Empty)
             return;
 
         _damageExamine.AddDamageExamine(args.Message, damageSpec, Loc.GetString("damage-melee"));

Content.Server/Weapons/Ranged/Systems/GunSystem.Battery.cs

@@ -85,7 +85,7 @@ public sealed partial class GunSystem
             {
                 var p = (ProjectileComponent) projectile.Component;
 
-                if (p.Damage.Total > FixedPoint2.Zero)
+                if (!p.Damage.Empty)
                 {
                     return p.Damage;
                 }

Content.Server/Weapons/Ranged/Systems/GunSystem.Cartridges.cs

@@ -37,7 +37,7 @@ public sealed partial class GunSystem
         {
             var p = (ProjectileComponent) projectile.Component;
 
-            if (p.Damage.Total > FixedPoint2.Zero)
+            if (!p.Damage.Empty)
             {
                 return p.Damage;
             }

Content.Server/Weapons/Ranged/Systems/GunSystem.cs

@@ -239,7 +239,7 @@ public sealed partial class GunSystem : SharedGunSystem
                         {
                             if (!Deleted(hitEntity))
                             {
-                                if (dmg.Total > FixedPoint2.Zero)
+                                if (dmg.Any())
                                 {
                                     _color.RaiseEffect(Color.Red, new List<EntityUid>() { hitEntity }, Filter.Pvs(hitEntity, entityManager: EntityManager));
                                 }

Content.Shared/Damage/DamageModifierSet.cs

@@ -5,11 +5,15 @@ using Robust.Shared.Serialization.TypeSerializers.Implementations.Custom.Prototy
 namespace Content.Shared.Damage
 {
     /// <summary>
-    ///     A set of coefficients or flat modifiers to damage types.. Can be applied to <see cref="DamageSpecifier"/> using <see
+    ///     A set of coefficients or flat modifiers to damage types. Can be applied to <see cref="DamageSpecifier"/> using <see
     ///     cref="DamageSpecifier.ApplyModifierSet(DamageSpecifier, DamageModifierSet)"/>. This can be done several times as the
     ///     <see cref="DamageSpecifier"/> is passed to it's final target. By default the receiving <see cref="DamageableComponent"/>, will
     ///     also apply it's own <see cref="DamageModifierSet"/>.
     /// </summary>
+    /// <remarks>
+    /// The modifier will only ever be applied to damage that is being dealt. Healing is unmodified.
+    /// The modifier can also never convert damage into healing.
+    /// </remarks>
     [DataDefinition]
     [Serializable, NetSerializable]
     [Virtual]

Content.Shared/Damage/DamageSpecifier.cs

@@ -37,16 +37,42 @@ namespace Content.Shared.Damage
         [IncludeDataField(customTypeSerializer: typeof(DamageSpecifierDictionarySerializer), readOnly: true)]
         public Dictionary<string, FixedPoint2> DamageDict { get; set; } = new();
 
+        [JsonIgnore]
+        [Obsolete("Use GetTotal()")]
+        public FixedPoint2 Total => GetTotal();
+
         /// <summary>
-        ///     Sum of the damage values.
+        ///     Returns a sum of the damage values.
         /// </summary>
         /// <remarks>
         ///     Note that this being zero does not mean this damage has no effect. Healing in one type may cancel damage
-        ///     in another. For this purpose, you should instead use <see cref="TrimZeros()"/> and then check the <see
+        ///     in another. Consider using <see cref="Any()"/> or <see cref="Empty"/> instead.
-        ///     cref="Empty"/> property.
         /// </remarks>
-        [JsonIgnore]
+        public FixedPoint2 GetTotal()
-        public FixedPoint2 Total => DamageDict.Values.Sum();
+        {
+            var total = FixedPoint2.Zero;
+            foreach (var value in DamageDict.Values)
+            {
+                total += value;
+            }
+            return total;
+        }
+
+        /// <summary>
+        /// Returns true if the specifier contains any positive damage values.
+        /// Differs from <see cref="Empty"/> as a damage specifier might contain entries with zeroes.
+        /// This also returns false if the specifier only contains negative values.
+        /// </summary>
+        public bool Any()
+        {
+            foreach (var value in DamageDict.Values)
+            {
+                if (value > FixedPoint2.Zero)
+                    return true;
+            }
+
+            return false;
+        }
 
         /// <summary>
         ///     Whether this damage specifier has any entries.
@@ -100,41 +126,39 @@ namespace Content.Shared.Damage
         /// </summary>
         /// <remarks>
         ///     Only applies resistance to a damage type if it is dealing damage, not healing.
+        ///     This will never convert damage into healing.
         /// </remarks>
         public static DamageSpecifier ApplyModifierSet(DamageSpecifier damageSpec, DamageModifierSet modifierSet)
         {
             // Make a copy of the given data. Don't modify the one passed to this function. I did this before, and weapons became
             // duller as you hit walls. Neat, but not FixedPoint2ended. And confusing, when you realize your fists don't work no
             // more cause they're just bloody stumps.
-            DamageSpecifier newDamage = new(damageSpec);
+            DamageSpecifier newDamage = new();
+            newDamage.DamageDict.EnsureCapacity(damageSpec.DamageDict.Count);
 
-            foreach (var entry in newDamage.DamageDict)
+            foreach (var (key, value) in damageSpec.DamageDict)
             {
-                if (entry.Value <= 0) continue;
+                if (value == 0)
+                    continue;
 
-                float newValue = entry.Value.Float();
+                if (value < 0)
-
-                if (modifierSet.FlatReduction.TryGetValue(entry.Key, out var reduction))
                 {
-                    newValue -= reduction;
+                    newDamage.DamageDict[key] = value;
-                    if (newValue <= 0)
-                    {
-                        // flat reductions cannot heal you
-                        newDamage.DamageDict[entry.Key] = FixedPoint2.Zero;
                     continue;
                 }
+
+                float newValue = value.Float();
+
+                if (modifierSet.FlatReduction.TryGetValue(key, out var reduction))
+                    newValue -= reduction;
+
+                if (modifierSet.Coefficients.TryGetValue(key, out var coefficient))
+                    newValue *= coefficient;
+
+                if (newValue > 0)
+                    newDamage.DamageDict[key] = FixedPoint2.New(newValue);
             }
 
-                if (modifierSet.Coefficients.TryGetValue(entry.Key, out var coefficient))
-                {
-                    // negative coefficients **can** heal you.
-                    newValue = newValue * coefficient;
-                }
-
-                newDamage.DamageDict[entry.Key] = FixedPoint2.New(newValue);
-            }
-
-            newDamage.TrimZeros();
             return newDamage;
         }
 
@@ -146,13 +170,19 @@ namespace Content.Shared.Damage
         /// <returns></returns>
         public static DamageSpecifier ApplyModifierSets(DamageSpecifier damageSpec, IEnumerable<DamageModifierSet> modifierSets)
         {
-            DamageSpecifier newDamage = new(damageSpec);
+            bool any = false;
+            DamageSpecifier newDamage = damageSpec;
             foreach (var set in modifierSets)
             {
-                // this is probably really inefficient. just don't call this in a hot path I guess.
+                // This creates a new damageSpec for each modifier when we really onlt need to create one.
+                // This is quite inefficient, but hopefully this shouldn't ever be called frequently.
                 newDamage = ApplyModifierSet(newDamage, set);
+                any = true;
             }
 
+            if (!any)
+                newDamage = new DamageSpecifier(damageSpec);
+
             return newDamage;
         }
 
@@ -224,9 +254,10 @@ namespace Content.Shared.Damage
         {
             foreach (var (type, value) in other.DamageDict)
             {
-                if (DamageDict.ContainsKey(type))
+                // CollectionsMarshal my beloved.
+                if (DamageDict.TryGetValue(type, out var existing))
                 {
-                    DamageDict[type] += value;
+                    DamageDict[type] = existing + value;
                 }
             }
         }
@@ -262,19 +293,23 @@ namespace Content.Shared.Damage
         ///     total of each group. If no members of a group are present in this <see cref="DamageSpecifier"/>, the
         ///     group is not included in the resulting dictionary.
         /// </remarks>
-        public Dictionary<string, FixedPoint2> GetDamagePerGroup(IPrototypeManager? protoManager = null)
+        public Dictionary<string, FixedPoint2> GetDamagePerGroup(IPrototypeManager protoManager)
         {
-            IoCManager.Resolve(ref protoManager);
+            var dict = new Dictionary<string, FixedPoint2>();
-            var damageGroupDict = new Dictionary<string, FixedPoint2>();
+            GetDamagePerGroup(protoManager, dict);
+            return dict;
+        }
+
+        /// <inheritdoc cref="GetDamagePerGroup(Robust.Shared.Prototypes.IPrototypeManager)"/>
+        public void GetDamagePerGroup(IPrototypeManager protoManager, Dictionary<string, FixedPoint2> dict)
+        {
+            dict.Clear();
             foreach (var group in protoManager.EnumeratePrototypes<DamageGroupPrototype>())
             {
                 if (TryGetDamageInGroup(group, out var value))
-                {
+                    dict.Add(group.ID, value);
-                    damageGroupDict.Add(group.ID, value);
             }
         }
-            return damageGroupDict;
-        }
 
         #region Operators
         public static DamageSpecifier operator *(DamageSpecifier damageSpec, FixedPoint2 factor)
@@ -372,6 +407,8 @@ namespace Content.Shared.Damage
 
             return true;
         }
+
+        public FixedPoint2 this[string key] => DamageDict[key];
     }
     #endregion
 }

Content.Shared/Damage/Systems/DamageableSystem.cs

@@ -20,6 +20,9 @@ namespace Content.Shared.Damage
         [Dependency] private readonly INetManager _netMan = default!;
         [Dependency] private readonly MobThresholdSystem _mobThreshold = default!;
 
+        private EntityQuery<AppearanceComponent> _appearanceQuery;
+        private EntityQuery<DamageableComponent> _damageableQuery;
+
         public override void Initialize()
         {
             SubscribeLocalEvent<DamageableComponent, ComponentInit>(DamageableInit);
@@ -27,6 +30,9 @@ namespace Content.Shared.Damage
             SubscribeLocalEvent<DamageableComponent, ComponentGetState>(DamageableGetState);
             SubscribeLocalEvent<DamageableComponent, OnIrradiatedEvent>(OnIrradiated);
             SubscribeLocalEvent<DamageableComponent, RejuvenateEvent>(OnRejuvenate);
+
+            _appearanceQuery = GetEntityQuery<AppearanceComponent>();
+            _damageableQuery = GetEntityQuery<DamageableComponent>();
         }
 
         /// <summary>
@@ -45,9 +51,9 @@ namespace Content.Shared.Damage
                     component.Damage.DamageDict.TryAdd(type, FixedPoint2.Zero);
                 }
 
-                foreach (var groupID in damageContainerPrototype.SupportedGroups)
+                foreach (var groupId in damageContainerPrototype.SupportedGroups)
                 {
-                    var group = _prototypeManager.Index<DamageGroupPrototype>(groupID);
+                    var group = _prototypeManager.Index<DamageGroupPrototype>(groupId);
                     foreach (var type in group.DamageTypes)
                     {
                         component.Damage.DamageDict.TryAdd(type, FixedPoint2.Zero);
@@ -63,8 +69,8 @@ namespace Content.Shared.Damage
                 }
             }
 
-            component.DamagePerGroup = component.Damage.GetDamagePerGroup(_prototypeManager);
+            component.Damage.GetDamagePerGroup(_prototypeManager, component.DamagePerGroup);
-            component.TotalDamage = component.Damage.Total;
+            component.TotalDamage = component.Damage.GetTotal();
         }
 
         /// <summary>
@@ -90,11 +96,11 @@ namespace Content.Shared.Damage
         public void DamageChanged(EntityUid uid, DamageableComponent component, DamageSpecifier? damageDelta = null,
             bool interruptsDoAfters = true, EntityUid? origin = null)
         {
-            component.DamagePerGroup = component.Damage.GetDamagePerGroup(_prototypeManager);
+            component.Damage.GetDamagePerGroup(_prototypeManager, component.DamagePerGroup);
-            component.TotalDamage = component.Damage.Total;
+            component.TotalDamage = component.Damage.GetTotal();
-            Dirty(component);
+            Dirty(uid, component);
 
-            if (EntityManager.TryGetComponent<AppearanceComponent>(uid, out var appearance) && damageDelta != null)
+            if (_appearanceQuery.TryGetComponent(uid, out var appearance) && damageDelta != null)
             {
                 var data = new DamageVisualizerGroupData(component.DamagePerGroup.Keys.ToList());
                 _appearance.SetData(uid, DamageVisualizerKeys.DamageUpdateGroups, data, appearance);
@@ -117,7 +123,7 @@ namespace Content.Shared.Damage
         public DamageSpecifier? TryChangeDamage(EntityUid? uid, DamageSpecifier damage, bool ignoreResistances = false,
             bool interruptsDoAfters = true, DamageableComponent? damageable = null, EntityUid? origin = null)
         {
-            if (!uid.HasValue || !Resolve(uid.Value, ref damageable, false))
+            if (!uid.HasValue || !_damageableQuery.Resolve(uid.Value, ref damageable, false))
             {
                 // TODO BODY SYSTEM pass damage onto body system
                 return null;
@@ -140,6 +146,8 @@ namespace Content.Shared.Damage
                 if (damageable.DamageModifierSetId != null &&
                     _prototypeManager.TryIndex<DamageModifierSetPrototype>(damageable.DamageModifierSetId, out var modifierSet))
                 {
+                    // TODO DAMAGE PERFORMANCE
+                    // use a local private field instead of creating a new dictionary here..
                     damage = DamageSpecifier.ApplyModifierSet(damage, modifierSet);
                 }
 
@@ -153,20 +161,30 @@ namespace Content.Shared.Damage
                 }
             }
 
-            // Copy the current damage, for calculating the difference
+            // TODO DAMAGE PERFORMANCE
-            DamageSpecifier oldDamage = new(damageable.Damage);
+            // Consider using a local private field instead of creating a new dictionary here.
+            // Would need to check that nothing ever tries to cache the delta.
+            var delta = new DamageSpecifier();
+            delta.DamageDict.EnsureCapacity(damage.DamageDict.Count);
 
-            damageable.Damage.ExclusiveAdd(damage);
+            var dict = damageable.Damage.DamageDict;
-            damageable.Damage.ClampMin(FixedPoint2.Zero);
+            foreach (var (type, value) in damage.DamageDict)
-
-            var delta = damageable.Damage - oldDamage;
-            delta.TrimZeros();
-
-            if (!delta.Empty)
             {
-                DamageChanged(uid.Value, damageable, delta, interruptsDoAfters, origin);
+                // CollectionsMarshal my beloved.
+                if (!dict.TryGetValue(type, out var oldValue))
+                    continue;
+
+                var newValue = FixedPoint2.Max(FixedPoint2.Zero, oldValue + value);
+                if (newValue == oldValue)
+                    continue;
+
+                dict[type] = newValue;
+                delta.DamageDict[type] = newValue - oldValue;
             }
 
+            if (delta.DamageDict.Count > 0)
+                DamageChanged(uid.Value, damageable, delta, interruptsDoAfters, origin);
+
             return delta;
         }
 
@@ -196,12 +214,11 @@ namespace Content.Shared.Damage
 
         public void SetDamageModifierSetId(EntityUid uid, string damageModifierSetId, DamageableComponent? comp = null)
         {
-            if (!Resolve(uid, ref comp))
+            if (!_damageableQuery.Resolve(uid, ref comp))
                 return;
 
             comp.DamageModifierSetId = damageModifierSetId;
-
+            Dirty(uid, comp);
-            Dirty(comp);
         }
 
         private void DamageableGetState(EntityUid uid, DamageableComponent component, ref ComponentGetState args)
@@ -265,7 +282,7 @@ namespace Content.Shared.Damage
     ///     Raised before damage is done, so stuff can cancel it if necessary.
     /// </summary>
     [ByRefEvent]
-    public record struct BeforeDamageChangedEvent(DamageSpecifier Delta, EntityUid? Origin = null, bool Cancelled = false);
+    public record struct BeforeDamageChangedEvent(DamageSpecifier Damage, EntityUid? Origin = null, bool Cancelled = false);
 
     /// <summary>
     ///     Raised on an entity when damage is about to be dealt,
@@ -312,14 +329,14 @@ namespace Content.Shared.Damage
         /// <summary>
         ///     Was any of the damage change dealing damage, or was it all healing?
         /// </summary>
-        public readonly bool DamageIncreased = false;
+        public readonly bool DamageIncreased;
 
         /// <summary>
         ///     Does this event interrupt DoAfters?
         ///     Note: As provided in the constructor, this *does not* account for DamageIncreased.
         ///     As written into the event, this *does* account for DamageIncreased.
         /// </summary>
-        public readonly bool InterruptsDoAfters = false;
+        public readonly bool InterruptsDoAfters;
 
         /// <summary>
         ///     Contains the entity which caused the change in damage, if any was responsible.

Content.Shared/Weapons/Melee/SharedMeleeWeaponSystem.cs

@@ -495,7 +495,7 @@ public abstract class SharedMeleeWeaponSystem : EntitySystem
         var modifiedDamage = DamageSpecifier.ApplyModifierSets(damage + hitEvent.BonusDamage + attackedEvent.BonusDamage, hitEvent.ModifiersList);
         var damageResult = Damageable.TryChangeDamage(target, modifiedDamage, origin:user);
 
-        if (damageResult != null && damageResult.Total > FixedPoint2.Zero)
+        if (damageResult != null && damageResult.Any())
         {
             // If the target has stamina and is taking blunt damage, they should also take stamina damage based on their blunt to stamina factor
             if (damageResult.DamageDict.TryGetValue("Blunt", out var bluntDamage))
@@ -522,7 +522,7 @@ public abstract class SharedMeleeWeaponSystem : EntitySystem
             {
                 Audio.PlayPredicted(hitEvent.HitSoundOverride, meleeUid, user);
             }
-            else if (GetDamage(meleeUid, user, component).Total.Equals(FixedPoint2.Zero) && component.HitSound != null)
+            else if (!GetDamage(meleeUid, user, component).Any() && component.HitSound != null)
             {
                 Audio.PlayPredicted(component.HitSound, meleeUid, user);
             }

Content.Tests/Shared/DamageTest.cs

@@ -17,15 +17,15 @@ namespace Content.Tests.Shared
     public sealed class DamageTest : ContentUnitTest
     {
 
-        static private Dictionary<string, float> _resistanceCoefficientDict = new()
+        private static Dictionary<string, float> _resistanceCoefficientDict = new()
         {
             // "missing" blunt entry
-            { "Piercing", -2 },// Turn Piercing into Healing
+            { "Piercing", -2 }, // negative multipliers just cause the damage to be ignored.
             { "Slash", 3 },
             { "Radiation", 1.5f },
         };
 
-        static private Dictionary<string, float> _resistanceReductionDict = new()
+        private static Dictionary<string, float> _resistanceReductionDict = new()
         {
             { "Blunt", - 5 },
             // "missing" piercing entry
@@ -152,14 +152,14 @@ namespace Content.Tests.Shared
             // Apply once
             damageSpec = DamageSpecifier.ApplyModifierSet(damageSpec, modifierSet);
             Assert.That(damageSpec.DamageDict["Blunt"], Is.EqualTo(FixedPoint2.New(25)));
-            Assert.That(damageSpec.DamageDict["Piercing"], Is.EqualTo(FixedPoint2.New(-40))); // became healing
+            Assert.That(!damageSpec.DamageDict.ContainsKey("Piercing"));  // Cannot convert damage into healing.
             Assert.That(damageSpec.DamageDict["Slash"], Is.EqualTo(FixedPoint2.New(6)));
             Assert.That(damageSpec.DamageDict["Radiation"], Is.EqualTo(FixedPoint2.New(44.25)));
 
             // And again, checking for some other behavior
             damageSpec = DamageSpecifier.ApplyModifierSet(damageSpec, modifierSet);
             Assert.That(damageSpec.DamageDict["Blunt"], Is.EqualTo(FixedPoint2.New(30)));
-            Assert.That(damageSpec.DamageDict["Piercing"], Is.EqualTo(FixedPoint2.New(-40))); // resistances don't apply to healing
+            Assert.That(!damageSpec.DamageDict.ContainsKey("Piercing"));
             Assert.That(!damageSpec.DamageDict.ContainsKey("Slash"));  // Reduction reduced to 0, and removed from specifier
             Assert.That(damageSpec.DamageDict["Radiation"], Is.EqualTo(FixedPoint2.New(65.63)));
         }