From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com> Date: Sun, 8 Oct 2023 16:27:41 +0000 (+1100) Subject: DamageableSystem cleanup & performance improvements (#20820) X-Git-Url: https://git.smokeofanarchy.ru/gitweb.cgi?a=commitdiff_plain;h=364c9b7f0a9784641417ba6f49a932a23aeaa318;p=space-station-14.git DamageableSystem cleanup & performance improvements (#20820) --- diff --git a/Content.Server/Bible/BibleSystem.cs b/Content.Server/Bible/BibleSystem.cs index 18f34ba1cc..e2cdc8c744 100644 --- a/Content.Server/Bible/BibleSystem.cs +++ b/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); diff --git a/Content.Server/Electrocution/ElectrocutionSystem.cs b/Content.Server/Electrocution/ElectrocutionSystem.cs index 844d526c58..48415c3953 100644 --- a/Content.Server/Electrocution/ElectrocutionSystem.cs +++ b/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) diff --git a/Content.Server/Projectiles/ProjectileSystem.cs b/Content.Server/Projectiles/ProjectileSystem.cs index 60fc0c3b5c..c52e712e74 100644 --- a/Content.Server/Projectiles/ProjectileSystem.cs +++ b/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 { target }, Filter.Pvs(target, entityManager: EntityManager)); } diff --git a/Content.Server/Weapons/Melee/MeleeWeaponSystem.cs b/Content.Server/Weapons/Melee/MeleeWeaponSystem.cs index a0a3f6d5d7..94ddc09e73 100644 --- a/Content.Server/Weapons/Melee/MeleeWeaponSystem.cs +++ b/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")); diff --git a/Content.Server/Weapons/Ranged/Systems/GunSystem.Battery.cs b/Content.Server/Weapons/Ranged/Systems/GunSystem.Battery.cs index ab2553e31b..f4deffd113 100644 --- a/Content.Server/Weapons/Ranged/Systems/GunSystem.Battery.cs +++ b/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; } diff --git a/Content.Server/Weapons/Ranged/Systems/GunSystem.Cartridges.cs b/Content.Server/Weapons/Ranged/Systems/GunSystem.Cartridges.cs index 6a5dd2d02d..e7bd3683d3 100644 --- a/Content.Server/Weapons/Ranged/Systems/GunSystem.Cartridges.cs +++ b/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; } diff --git a/Content.Server/Weapons/Ranged/Systems/GunSystem.cs b/Content.Server/Weapons/Ranged/Systems/GunSystem.cs index 0859cb9442..adda6a94e3 100644 --- a/Content.Server/Weapons/Ranged/Systems/GunSystem.cs +++ b/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() { hitEntity }, Filter.Pvs(hitEntity, entityManager: EntityManager)); } diff --git a/Content.Shared/Damage/DamageModifierSet.cs b/Content.Shared/Damage/DamageModifierSet.cs index 2dfc38715e..bd074ec30f 100644 --- a/Content.Shared/Damage/DamageModifierSet.cs +++ b/Content.Shared/Damage/DamageModifierSet.cs @@ -5,11 +5,15 @@ using Robust.Shared.Serialization.TypeSerializers.Implementations.Custom.Prototy namespace Content.Shared.Damage { /// - /// A set of coefficients or flat modifiers to damage types.. Can be applied to using using . This can be done several times as the /// is passed to it's final target. By default the receiving , will /// also apply it's own . /// + /// + /// 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. + /// [DataDefinition] [Serializable, NetSerializable] [Virtual] diff --git a/Content.Shared/Damage/DamageSpecifier.cs b/Content.Shared/Damage/DamageSpecifier.cs index 903ee605f1..b7181e297f 100644 --- a/Content.Shared/Damage/DamageSpecifier.cs +++ b/Content.Shared/Damage/DamageSpecifier.cs @@ -37,16 +37,42 @@ namespace Content.Shared.Damage [IncludeDataField(customTypeSerializer: typeof(DamageSpecifierDictionarySerializer), readOnly: true)] public Dictionary DamageDict { get; set; } = new(); + [JsonIgnore] + [Obsolete("Use GetTotal()")] + public FixedPoint2 Total => GetTotal(); + /// - /// Sum of the damage values. + /// Returns a sum of the damage values. /// /// /// 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 and then check the property. + /// in another. Consider using or instead. /// - [JsonIgnore] - public FixedPoint2 Total => DamageDict.Values.Sum(); + public FixedPoint2 GetTotal() + { + var total = FixedPoint2.Zero; + foreach (var value in DamageDict.Values) + { + total += value; + } + return total; + } + + /// + /// Returns true if the specifier contains any positive damage values. + /// Differs from as a damage specifier might contain entries with zeroes. + /// This also returns false if the specifier only contains negative values. + /// + public bool Any() + { + foreach (var value in DamageDict.Values) + { + if (value > FixedPoint2.Zero) + return true; + } + + return false; + } /// /// Whether this damage specifier has any entries. @@ -100,41 +126,39 @@ namespace Content.Shared.Damage /// /// /// Only applies resistance to a damage type if it is dealing damage, not healing. + /// This will never convert damage into healing. /// 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; - - float newValue = entry.Value.Float(); + if (value == 0) + continue; - if (modifierSet.FlatReduction.TryGetValue(entry.Key, out var reduction)) + if (value < 0) { - newValue -= reduction; - if (newValue <= 0) - { - // flat reductions cannot heal you - newDamage.DamageDict[entry.Key] = FixedPoint2.Zero; - continue; - } + newDamage.DamageDict[key] = value; + continue; } - if (modifierSet.Coefficients.TryGetValue(entry.Key, out var coefficient)) - { - // negative coefficients **can** heal you. - newValue = newValue * coefficient; - } + float newValue = value.Float(); + + if (modifierSet.FlatReduction.TryGetValue(key, out var reduction)) + newValue -= reduction; + + if (modifierSet.Coefficients.TryGetValue(key, out var coefficient)) + newValue *= coefficient; - newDamage.DamageDict[entry.Key] = FixedPoint2.New(newValue); + if (newValue > 0) + newDamage.DamageDict[key] = FixedPoint2.New(newValue); } - newDamage.TrimZeros(); return newDamage; } @@ -146,13 +170,19 @@ namespace Content.Shared.Damage /// public static DamageSpecifier ApplyModifierSets(DamageSpecifier damageSpec, IEnumerable 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,18 +293,22 @@ namespace Content.Shared.Damage /// total of each group. If no members of a group are present in this , the /// group is not included in the resulting dictionary. /// - public Dictionary GetDamagePerGroup(IPrototypeManager? protoManager = null) + public Dictionary GetDamagePerGroup(IPrototypeManager protoManager) + { + var dict = new Dictionary(); + GetDamagePerGroup(protoManager, dict); + return dict; + } + + /// + public void GetDamagePerGroup(IPrototypeManager protoManager, Dictionary dict) { - IoCManager.Resolve(ref protoManager); - var damageGroupDict = new Dictionary(); + dict.Clear(); foreach (var group in protoManager.EnumeratePrototypes()) { if (TryGetDamageInGroup(group, out var value)) - { - damageGroupDict.Add(group.ID, value); - } + dict.Add(group.ID, value); } - return damageGroupDict; } #region Operators @@ -372,6 +407,8 @@ namespace Content.Shared.Damage return true; } + + public FixedPoint2 this[string key] => DamageDict[key]; } #endregion } diff --git a/Content.Shared/Damage/Systems/DamageableSystem.cs b/Content.Shared/Damage/Systems/DamageableSystem.cs index a3cdd14ef7..8f6ccc20e6 100644 --- a/Content.Shared/Damage/Systems/DamageableSystem.cs +++ b/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 _appearanceQuery; + private EntityQuery _damageableQuery; + public override void Initialize() { SubscribeLocalEvent(DamageableInit); @@ -27,6 +30,9 @@ namespace Content.Shared.Damage SubscribeLocalEvent(DamageableGetState); SubscribeLocalEvent(OnIrradiated); SubscribeLocalEvent(OnRejuvenate); + + _appearanceQuery = GetEntityQuery(); + _damageableQuery = GetEntityQuery(); } /// @@ -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(groupID); + var group = _prototypeManager.Index(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.TotalDamage = component.Damage.Total; + component.Damage.GetDamagePerGroup(_prototypeManager, component.DamagePerGroup); + component.TotalDamage = component.Damage.GetTotal(); } /// @@ -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.TotalDamage = component.Damage.Total; - Dirty(component); + component.Damage.GetDamagePerGroup(_prototypeManager, component.DamagePerGroup); + component.TotalDamage = component.Damage.GetTotal(); + Dirty(uid, component); - if (EntityManager.TryGetComponent(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(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 - DamageSpecifier oldDamage = new(damageable.Damage); + // TODO DAMAGE PERFORMANCE + // 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); - damageable.Damage.ClampMin(FixedPoint2.Zero); + var dict = damageable.Damage.DamageDict; + foreach (var (type, value) in damage.DamageDict) + { + // CollectionsMarshal my beloved. + if (!dict.TryGetValue(type, out var oldValue)) + continue; - var delta = damageable.Damage - oldDamage; - delta.TrimZeros(); + var newValue = FixedPoint2.Max(FixedPoint2.Zero, oldValue + value); + if (newValue == oldValue) + continue; - if (!delta.Empty) - { - DamageChanged(uid.Value, damageable, delta, interruptsDoAfters, origin); + 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(comp); + Dirty(uid, 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. /// [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); /// /// Raised on an entity when damage is about to be dealt, @@ -312,14 +329,14 @@ namespace Content.Shared.Damage /// /// Was any of the damage change dealing damage, or was it all healing? /// - public readonly bool DamageIncreased = false; + public readonly bool DamageIncreased; /// /// 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. /// - public readonly bool InterruptsDoAfters = false; + public readonly bool InterruptsDoAfters; /// /// Contains the entity which caused the change in damage, if any was responsible. diff --git a/Content.Shared/Weapons/Melee/SharedMeleeWeaponSystem.cs b/Content.Shared/Weapons/Melee/SharedMeleeWeaponSystem.cs index 9b61fd03b7..ebe1a21e67 100644 --- a/Content.Shared/Weapons/Melee/SharedMeleeWeaponSystem.cs +++ b/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); } diff --git a/Content.Tests/Shared/DamageTest.cs b/Content.Tests/Shared/DamageTest.cs index 4f8647b3e4..4221ff25a2 100644 --- a/Content.Tests/Shared/DamageTest.cs +++ b/Content.Tests/Shared/DamageTest.cs @@ -17,15 +17,15 @@ namespace Content.Tests.Shared public sealed class DamageTest : ContentUnitTest { - static private Dictionary _resistanceCoefficientDict = new() + private static Dictionary _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 _resistanceReductionDict = new() + private static Dictionary _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))); }