From b217c8d04ff951a7eec4a58009740abaaa0cb119 Mon Sep 17 00:00:00 2001 From: Lex Date: Sat, 19 Sep 2026 02:48:28 -0300 Subject: [PATCH 1/3] fix(cues): tally attribute changes per operation Cues and the modifier-success check read a change set scoped to the operation, so nested effects, rebuilds after a set change and outer pending deltas no longer leak into each other. --- .../Cues/CueMagnitudeReentrancyTests.cs | 146 +++++++++++++++--- Forge/Attributes/AttributeChangeSet.cs | 91 +++++++++++ Forge/Attributes/EntityAttribute.cs | 10 +- Forge/Core/EntityAttributes.cs | 57 ++++++- Forge/Cues/CuesManager.cs | 117 ++++++-------- Forge/Effects/ActiveEffect.cs | 31 ++-- Forge/Effects/Effect.cs | 14 +- Forge/Effects/EffectsManager.cs | 90 ++++++----- 8 files changed, 407 insertions(+), 149 deletions(-) create mode 100644 Forge/Attributes/AttributeChangeSet.cs diff --git a/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs b/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs index baf7746..e2faba7 100644 --- a/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs +++ b/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs @@ -18,12 +18,13 @@ namespace Gamesmiths.Forge.Tests.Cues; /// -/// An cue reads the deltas an effect has left pending on its -/// target, and any effect landing on that same target flushes them. A hook that runs between the effect's attribute -/// writes and its cues — an executed hook raising an event that activates an ability, a changed hook applying -/// threshold effects — can therefore land such an effect, and the cue must still report what its own effect did. -/// The cue handlers themselves are arbitrary code too, so they keep running after the hooks: reading early is what -/// protects the magnitudes, not dispatching early. +/// An cue and the modifier-success check read the changes the +/// effect's own operation made to its target, tallied apart from every other operation on that entity. A hook that +/// runs between the effect's attribute writes and its cues — an executed hook raising an event that activates an +/// ability, a changed hook applying threshold effects — can land another effect on the same target, and neither that +/// effect's cues nor the original's may read each other's changes. The cue handlers themselves are arbitrary code +/// too, so they keep running after the hooks: closing the tally early is what protects the magnitudes, not +/// dispatching early. /// /// The fixture providing the and . /// @@ -31,10 +32,14 @@ public class CueMagnitudeReentrancyTests(TagsAndCuesFixture tagsAndCuesFixture) { private const string CueAttribute = "TestAttributeSet.Attribute90"; private const string SideAttribute = "TestAttributeSet.Attribute1"; + private const string KeptAttribute = "TestAttributeSet.Attribute1000"; + private const string ArrivingAttribute = "VitalAttributeSet.CurrentHealth"; private readonly TagsManager _tagsManager = tagsAndCuesFixture.TagsManager; private readonly CuesManager _cuesManager = tagsAndCuesFixture.CuesManager; private readonly TestCue _cue = tagsAndCuesFixture.TestCueInstances[0]; + private readonly TestCue _sideCue = tagsAndCuesFixture.TestCueInstances[1]; + private readonly TestCue _otherCue = tagsAndCuesFixture.TestCueInstances[2]; [Fact] [Trait("Execute", null)] @@ -134,12 +139,96 @@ public void Update_cue_handlers_run_after_the_changed_hooks_so_a_handler_removin } } - private static Effect CreateSideEffect(TestEntity target) + [Fact] + [Trait("Execute", null)] + public void A_nested_effects_cues_read_the_changes_of_its_own_execution_and_not_the_outer_ones() + { + var target = new TestEntity(_tagsManager, _cuesManager); + _cue.Reset(); + _sideCue.Reset(); + _otherCue.Reset(); + + // The side effect lands while the outer hit's change to Attribute90 is still pending on the entity. Its cue on + // that attribute must read nothing, and only its cue on the attribute it touched must read its own change. + target.Events.Subscribe( + EventTag(), + _ => target.EffectsManager.ApplyEffect(CreateSideEffect( + target, + CreateCue("test.cue2"), + CreateCue("test.cue3", SideAttribute)))); + + target.EffectsManager.ApplyEffect(CreateInstantEffect(target)); + + _cue.ExecuteData.Value.Should().Be(-10); + _sideCue.ExecuteData.Count.Should().Be(1); + _sideCue.ExecuteData.Value.Should().Be(0); + _otherCue.ExecuteData.Count.Should().Be(1); + _otherCue.ExecuteData.Value.Should().Be(1); + } + + [Fact] + [Trait("Execute", null)] + public void Modifier_success_is_judged_on_the_effects_own_execution() + { + var target = new TestEntity(_tagsManager, _cuesManager); + _cue.Reset(); + _sideCue.Reset(); + + // A nested effect whose modifier changes nothing must not have its cues fired by the outer hit's pending + // change. + target.Events.Subscribe( + EventTag(), + _ => target.EffectsManager.ApplyEffect(CreateSideEffect( + target, + CreateCue("test.cue2"), + magnitude: 0, + requireModifierSuccess: true))); + + target.EffectsManager.ApplyEffect(CreateInstantEffect(target)); + + _cue.ExecuteData.Value.Should().Be(-10); + _sideCue.ExecuteData.Count.Should().Be(0); + } + + [Fact] + [Trait("Update", null)] + public void Update_cues_after_an_attribute_set_arrives_report_what_the_effect_now_contributes() { + var target = new TestEntity(_tagsManager, _cuesManager); + _cue.Reset(); + _sideCue.Reset(); + + // Applied while the entity lacks the set, so only the kept attribute is modified at first. + target.EffectsManager.ApplyEffect(CreateCrossSetEffect(target)); + + target.Attributes.AddAttributeSet(new VitalAttributeSet()); + + // The arriving attribute takes the effect's full modifier; the kept one is unapplied and re-applied, which nets + // to nothing. Both are read after every effect was rebuilt and the attributes flushed. + target.Attributes[ArrivingAttribute].CurrentValue.Should().Be(90); + _cue.UpdateData.Count.Should().Be(1); + _cue.UpdateData.Value.Should().Be(-10); + _sideCue.UpdateData.Count.Should().Be(1); + _sideCue.UpdateData.Value.Should().Be(0); + } + + private static Effect CreateSideEffect( + TestEntity target, + CueData? cue = null, + CueData? otherCue = null, + float magnitude = 1, + bool requireModifierSuccess = false) + { + CueTriggerRequirement requirement = requireModifierSuccess + ? CueTriggerRequirement.OnExecute + : CueTriggerRequirement.None; + var effectData = new EffectData( "Side Effect", new DurationData(DurationType.Instant), - [CreateModifier(SideAttribute, 1)]); + [CreateModifier(SideAttribute, magnitude)], + requireModifierSuccessToTriggerCue: requirement, + cues: [.. new[] { cue, otherCue }.OfType()]); return new Effect(effectData, new EffectOwnership(target, target)); } @@ -152,6 +241,37 @@ private static Modifier CreateModifier(string attribute, float magnitude) new ModifierMagnitude(MagnitudeCalculationType.ScalableFloat, new ScalableFloat(magnitude))); } + private static CueData CreateCue(Tag cueTag, string attribute) + { + return new CueData( + cueTag.GetSingleTagContainer(), + -100, + 100, + CueMagnitudeType.AttributeValueChange, + attribute); + } + + private CueData CreateCue(Tag? cueTag = null) + { + return CreateCue(cueTag ?? Tag.RequestTag(_tagsManager, "test.cue1"), CueAttribute); + } + + private CueData CreateCue(string cueTagName, string attribute = CueAttribute) + { + return CreateCue(Tag.RequestTag(_tagsManager, cueTagName), attribute); + } + + private Effect CreateCrossSetEffect(TestEntity target) + { + var effectData = new EffectData( + "Cross Set Buff", + new DurationData(DurationType.Infinite), + [CreateModifier(KeptAttribute, 10), CreateModifier(ArrivingAttribute, -10)], + cues: [CreateCue("test.cue1", ArrivingAttribute), CreateCue("test.cue2", KeptAttribute)]); + + return new Effect(effectData, new EffectOwnership(target, target)); + } + private Effect CreateInstantEffect(TestEntity target) { var effectData = new EffectData( @@ -201,16 +321,6 @@ private Effect CreateStackableEffect(TestEntity target, Tag? cueTag = null) return new Effect(effectData, new EffectOwnership(target, target)); } - private CueData CreateCue(Tag? cueTag = null) - { - return new CueData( - (cueTag ?? Tag.RequestTag(_tagsManager, "test.cue1")).GetSingleTagContainer(), - -100, - 100, - CueMagnitudeType.AttributeValueChange, - CueAttribute); - } - private Tag EventTag() { return Tag.RequestTag(_tagsManager, "tag"); diff --git a/Forge/Attributes/AttributeChangeSet.cs b/Forge/Attributes/AttributeChangeSet.cs new file mode 100644 index 0000000..e30801f --- /dev/null +++ b/Forge/Attributes/AttributeChangeSet.cs @@ -0,0 +1,91 @@ +// Copyright © Gamesmiths Guild. + +using Gamesmiths.Forge.Core; + +namespace Gamesmiths.Forge.Attributes; + +/// +/// The net change one operation — an effect executing, applying, restacking or being rebuilt — made to each attribute +/// of an entity, kept apart from whatever else changed those attributes meanwhile. +/// +/// +/// +/// An also keeps an entity-wide pending change, but that one accumulates across every +/// operation until the next flush publishes it, so a nested operation — a raised event activating an ability that +/// commits its cost while a hit is still landing — would read the hit's deltas as its own. Cues and the +/// modifier-success check read this instead, through the scope opens +/// around the operation. +/// +/// +/// Pooled by its , so an execution costs no allocation once warm. A few entries searched +/// linearly are cheaper than a dictionary for the handful of attributes one effect touches. +/// +/// +internal sealed class AttributeChangeSet +{ + private readonly List _changes = []; + + /// + /// Gets a value indicating whether the operation left any attribute's current value different from before. + /// + internal bool HasChanges + { + get + { + foreach (AttributeChange change in _changes) + { + if (change.Delta != 0) + { + return true; + } + } + + return false; + } + } + + /// + /// Gets the net change the operation made to an attribute's current value; zero for one it never touched. + /// + /// The attribute to look up. + /// The net change to the attribute's current value. + internal int DeltaOf(EntityAttribute attribute) + { + int index = IndexOf(attribute); + + return index < 0 ? 0 : _changes[index].Delta; + } + + internal void Record(EntityAttribute attribute, int delta) + { + int index = IndexOf(attribute); + + if (index < 0) + { + _changes.Add(new AttributeChange(attribute, delta)); + return; + } + + _changes[index] = new AttributeChange(attribute, _changes[index].Delta + delta); + } + + internal void Clear() + { + _changes.Clear(); + } + + private int IndexOf(EntityAttribute attribute) + { + for (int i = 0; i < _changes.Count; i++) + { + if (ReferenceEquals(_changes[i].Attribute, attribute)) + { + return i; + } + } + + return -1; + } + + private readonly record struct AttributeChange(EntityAttribute Attribute, int Delta); +} diff --git a/Forge/Attributes/EntityAttribute.cs b/Forge/Attributes/EntityAttribute.cs index 6a49562..951ba9f 100644 --- a/Forge/Attributes/EntityAttribute.cs +++ b/Forge/Attributes/EntityAttribute.cs @@ -110,6 +110,12 @@ public sealed class EntityAttribute internal int PendingValueChange { get; private set; } + /// + /// Gets or sets the entity's attribute container this attribute is attached to, which tallies its changes per + /// operation; while it belongs to no entity. + /// + internal EntityAttributes? Container { get; set; } + internal EntityAttribute( StringKey key, int defaultValue, @@ -440,7 +446,9 @@ private void RefreshValues(int oldValue) if (CurrentValue != oldValue) { - PendingValueChange += CurrentValue - oldValue; + int delta = CurrentValue - oldValue; + PendingValueChange += delta; + Container?.RecordChange(this, delta); } } diff --git a/Forge/Core/EntityAttributes.cs b/Forge/Core/EntityAttributes.cs index 9b2cf32..9b2db8d 100644 --- a/Forge/Core/EntityAttributes.cs +++ b/Forge/Core/EntityAttributes.cs @@ -21,6 +21,8 @@ public class EntityAttributes(IForgeEntity owner) : IEnumerable private readonly Dictionary _attributes = []; private readonly List _attributeSets = []; private readonly HashSet _dependentEffects = []; + private readonly List _openChanges = []; + private readonly Stack _changeSetPool = new(); /// /// Event invoked when an attribute set is added to this entity, carrying the set. @@ -214,16 +216,64 @@ internal void RegisterDependent(ActiveEffect activeEffect) } internal void UnregisterDependent(ActiveEffect activeEffect) -#pragma warning restore T0009 // Internal Styling Rule T0009 { _dependentEffects.Remove(activeEffect); } + /// + /// Opens a scope that tallies, into the returned set, the net change every attribute write makes until + /// closes it. Scopes nest, and a write lands in the innermost open one, so an operation + /// that re-enters through a hook — an event activating an ability that applies its own effects — keeps its tally + /// apart from theirs. + /// + /// A set closed earlier to keep adding to, when one operation spans two scopes, or + /// for a fresh one. + /// The set the scope tallies into. + internal AttributeChangeSet BeginChanges(AttributeChangeSet? changes = null) + { + changes ??= _changeSetPool.TryPop(out AttributeChangeSet? pooled) ? pooled : new AttributeChangeSet(); + _openChanges.Add(changes); + + return changes; + } + + /// + /// Closes the innermost scope. Its set stays readable until hands it back. + /// + /// The set the scope being closed tallies into. + internal void EndChanges(AttributeChangeSet changes) + { + Validation.Assert( + _openChanges.Count > 0 && _openChanges[^1] == changes, + "Change scopes close innermost first."); + + _openChanges.RemoveAt(_openChanges.Count - 1); + } + + /// + /// Returns a set whose scope is closed to the pool once nothing reads it anymore. + /// + /// The set to hand back. + internal void ReleaseChanges(AttributeChangeSet changes) + { + changes.Clear(); + _changeSetPool.Push(changes); + } + + internal void RecordChange(EntityAttribute attribute, int delta) + { + if (_openChanges.Count > 0) + { + _openChanges[^1].Record(attribute, delta); + } + } + private void AttachAttributeSet(AttributeSet attributeSet) { foreach (KeyValuePair attribute in attributeSet.AttributesMap) { _attributes.Add(attribute.Key, attribute.Value); + attribute.Value.Container = this; } _attributeSets.Add(attributeSet); @@ -231,9 +281,10 @@ private void AttachAttributeSet(AttributeSet attributeSet) private void DetachAttributeSet(AttributeSet attributeSet) { - foreach (StringKey attributeKey in attributeSet.AttributesMap.Keys) + foreach (KeyValuePair attribute in attributeSet.AttributesMap) { - _attributes.Remove(attributeKey); + _attributes.Remove(attribute.Key); + attribute.Value.Container = null; } _attributeSets.Remove(attributeSet); diff --git a/Forge/Cues/CuesManager.cs b/Forge/Cues/CuesManager.cs index 5bf72a7..5b13479 100644 --- a/Forge/Cues/CuesManager.cs +++ b/Forge/Cues/CuesManager.cs @@ -1,5 +1,6 @@ // Copyright © Gamesmiths Guild. +using Gamesmiths.Forge.Attributes; using Gamesmiths.Forge.Core; using Gamesmiths.Forge.Effects; using Gamesmiths.Forge.Tags; @@ -11,11 +12,6 @@ namespace Gamesmiths.Forge.Cues; /// public sealed class CuesManager { - /// - /// The most cues one effect can have before the magnitudes held across its hooks move from the stack to the heap. - /// - internal const int MaxStackCueMagnitudes = 8; - private readonly Dictionary> _registeredCues = []; /// @@ -131,56 +127,29 @@ public void UpdateCue(Tag cueTag, IForgeEntity? target, CueParameters? parameter } /// - /// Decides whether an effect's cues fire for a trigger and, when they do, reads the magnitude of each one into - /// , one slot per entry of the effect's cues. + /// Applies the cues of an effect that just landed on its target. /// /// - /// The read and the dispatch are separate on purpose. The magnitudes come from the attribute deltas still pending - /// from the operation that fires the cues, and any hook that runs before the handlers — a component, a manager - /// event — can land another effect on the target whose own application flushes them. Reading here and dispatching - /// through or - /// once the hooks are done keeps the cues describing that operation alone, without moving the handlers ahead of - /// the components that expect to run first. + /// The cues of an operation read the the operation tallied its attribute writes + /// into, never the entity-wide pending values: those accumulate across every operation until the next flush, so a + /// nested one — a raised event activating an ability that applies its own effects while a hit is still landing — + /// would read the hit's deltas as its own. The set is closed before any hook runs, which is also what lets the + /// handlers keep running after the components and change notifications rather than ahead of them. /// - /// The evaluated data of the effect whose cues are being read. - /// The trigger the cues are being read for. - /// Receives one magnitude per cue; at least as long as the effect's cues. - /// if the cues fire and was filled; otherwise, - /// . - internal static bool TryCaptureCueMagnitudes( - in EffectEvaluatedData effectEvaluatedData, - CueTriggerRequirement triggerRequirement, - Span magnitudes) + /// The evaluated data of the applied effect. + /// The changes the application made to the target's attributes. + internal void ApplyCues(in EffectEvaluatedData effectEvaluatedData, AttributeChangeSet changes) { EffectData effectData = effectEvaluatedData.Effect.EffectData; - EntityAttributes targetAttributes = effectEvaluatedData.Target.Attributes; - if (!ShouldTriggerCue(in effectData, in targetAttributes, triggerRequirement)) - { - return false; - } - - for (int i = 0; i < effectData.Cues.Length; i++) - { - magnitudes[i] = CalculateMagnitude(in effectData.Cues[i], in effectEvaluatedData); - } - - return true; - } - - internal void ApplyCues(in EffectEvaluatedData effectEvaluatedData) - { - EffectData effectData = effectEvaluatedData.Effect.EffectData; - - EntityAttributes targetAttributes = effectEvaluatedData.Target.Attributes; - if (!ShouldTriggerCue(in effectData, in targetAttributes, CueTriggerRequirement.OnApply)) + if (!ShouldTriggerCue(in effectData, changes, CueTriggerRequirement.OnApply)) { return; } foreach (CueData cueData in effectData.Cues) { - int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData); + int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData, changes); if (cueData.CueTags is null) { @@ -219,13 +188,23 @@ internal void RemoveCues(in EffectEvaluatedData effectEvaluatedData, bool interr } } - internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySpan magnitudes) + /// + /// Executes the cues of an effect that just executed on its target. + /// + /// The evaluated data of the executed effect. + /// The changes the execution made to the target's attributes. + internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, AttributeChangeSet changes) { - CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; + EffectData effectData = effectEvaluatedData.Effect.EffectData; - for (int i = 0; i < cues.Length; i++) + if (!ShouldTriggerCue(in effectData, changes, CueTriggerRequirement.OnExecute)) { - CueData cueData = cues[i]; + return; + } + + foreach (CueData cueData in effectData.Cues) + { + int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData, changes); if (cueData.CueTags is null) { @@ -238,32 +217,32 @@ internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySp cueTag, effectEvaluatedData.Target, new CueParameters( - magnitudes[i], - cueData.NormalizedMagnitude(magnitudes[i]), + magnitude, + cueData.NormalizedMagnitude(magnitude), effectEvaluatedData.Effect.Ownership.Source, effectEvaluatedData.CustomCueParameters)); } } } - internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData) + /// + /// Updates the cues of an active effect that just changed on its target. + /// + /// The evaluated data of the changed effect. + /// The changes the operation made to the target's attributes, or + /// when it made none. + internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData, AttributeChangeSet? changes) { - CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; - Span magnitudes = cues.Length <= MaxStackCueMagnitudes ? stackalloc int[cues.Length] : new int[cues.Length]; + EffectData effectData = effectEvaluatedData.Effect.EffectData; - if (TryCaptureCueMagnitudes(in effectEvaluatedData, CueTriggerRequirement.OnUpdate, magnitudes)) + if (!ShouldTriggerCue(in effectData, changes, CueTriggerRequirement.OnUpdate)) { - UpdateCues(in effectEvaluatedData, magnitudes); + return; } - } - internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySpan magnitudes) - { - CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; - - for (int i = 0; i < cues.Length; i++) + foreach (CueData cueData in effectData.Cues) { - CueData cueData = cues[i]; + int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData, changes); if (cueData.CueTags is null) { @@ -276,8 +255,8 @@ internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySpa cueTag, effectEvaluatedData.Target, new CueParameters( - magnitudes[i], - cueData.NormalizedMagnitude(magnitudes[i]), + magnitude, + cueData.NormalizedMagnitude(magnitude), effectEvaluatedData.Effect.Ownership.Source, effectEvaluatedData.CustomCueParameters)); } @@ -286,7 +265,7 @@ internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySpa private static bool ShouldTriggerCue( in EffectData effectData, - in EntityAttributes attributes, + AttributeChangeSet? changes, CueTriggerRequirement triggerRequirements) { if (!effectData.RequireModifierSuccessToTriggerCue.HasFlag(triggerRequirements)) @@ -294,12 +273,13 @@ private static bool ShouldTriggerCue( return true; } - return attributes.Any(x => x.PendingValueChange != 0); + return changes?.HasChanges == true; } private static int CalculateMagnitude( in CueData cueData, - in EffectEvaluatedData effectEvaluatedData) + in EffectEvaluatedData effectEvaluatedData, + AttributeChangeSet? changes) { switch (cueData.MagnitudeType) { @@ -314,12 +294,13 @@ private static int CalculateMagnitude( cueData.MagnitudeAttribute is not null, "Cues with CueMagnitudeType.AttributeMagnitude must contains a configured MagnitudeAttribute."); - if (!effectEvaluatedData.Target.Attributes.ContainsAttribute(cueData.MagnitudeAttribute)) + if (changes is null + || !effectEvaluatedData.Target.Attributes.ContainsAttribute(cueData.MagnitudeAttribute)) { return 0; } - return effectEvaluatedData.Target.Attributes[cueData.MagnitudeAttribute].PendingValueChange; + return changes.DeltaOf(effectEvaluatedData.Target.Attributes[cueData.MagnitudeAttribute]); case CueMagnitudeType.AttributeBaseValue: Validation.Assert( diff --git a/Forge/Effects/ActiveEffect.cs b/Forge/Effects/ActiveEffect.cs index fd83a72..f3d88ef 100644 --- a/Forge/Effects/ActiveEffect.cs +++ b/Forge/Effects/ActiveEffect.cs @@ -2,7 +2,6 @@ using Gamesmiths.Forge.Attributes; using Gamesmiths.Forge.Core; -using Gamesmiths.Forge.Cues; using Gamesmiths.Forge.Effects.Components; using Gamesmiths.Forge.Effects.Duration; using Gamesmiths.Forge.Effects.Magnitudes; @@ -294,8 +293,9 @@ internal bool AddStack(Effect effect, int stacks = 1) } else { + // Nothing was re-applied, so there are no attribute changes for the update cues to report. EffectEvaluatedData effectEvaluatedData = EffectEvaluatedData; - effectEvaluatedData.Target.EffectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData); + effectEvaluatedData.Target.EffectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, null); } if (stackingData.ApplicationRefreshPolicy == StackApplicationRefreshPolicy.RefreshOnSuccessfulApplication) @@ -543,36 +543,31 @@ private void UnregisterAttributeDependencies() private void ReapplyEffect(Effect effect, int? level = null, bool isStackingCall = false) { + // Tallied across the unapply and the re-application, and closed before any hook runs, so the update cues report + // the net change this re-application made and nothing a changed hook goes on to apply. + EntityAttributes targetAttributes = EffectEvaluatedData.Target.Attributes; + AttributeChangeSet changes = targetAttributes.BeginChanges(); + Unapply(true); EffectEvaluatedData.ReEvaluate(effect, StackCount, level); Apply(reApplication: true); + targetAttributes.EndChanges(changes); + EffectEvaluatedData effectEvaluatedData = EffectEvaluatedData; EffectsManager effectsManager = effectEvaluatedData.Target.EffectsManager; - // Same two phases as an execution: the update cues read this re-application's pending deltas, which a changed - // hook — a stack threshold applying its effects, say — can flush by landing another effect on the target, so - // their magnitudes are read before the hooks and their handlers run after, on a handle the hooks saw whole. - CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; - Span cueMagnitudes = cues.Length <= CuesManager.MaxStackCueMagnitudes - ? stackalloc int[cues.Length] - : new int[cues.Length]; - bool triggerCues = (!Effect.EffectData.SuppressStackingCues || !isStackingCall) - && CuesManager.TryCaptureCueMagnitudes( - in effectEvaluatedData, - CueTriggerRequirement.OnUpdate, - cueMagnitudes); - effectsManager.OnActiveEffectChanged_InternalCall(this); - if (triggerCues) + if (!Effect.EffectData.SuppressStackingCues || !isStackingCall) { - effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, cueMagnitudes); + effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, changes); } - effectEvaluatedData.Target.Attributes.ApplyPendingValueChanges(); + targetAttributes.ReleaseChanges(changes); + targetAttributes.ApplyPendingValueChanges(); } private void ApplyModifiers(bool unapply = false) diff --git a/Forge/Effects/Effect.cs b/Forge/Effects/Effect.cs index 1e29a81..a14169f 100644 --- a/Forge/Effects/Effect.cs +++ b/Forge/Effects/Effect.cs @@ -1,5 +1,6 @@ // Copyright © Gamesmiths Guild. +using Gamesmiths.Forge.Attributes; using Gamesmiths.Forge.Core; using Gamesmiths.Forge.Effects.Calculator; using Gamesmiths.Forge.Effects.Components; @@ -151,6 +152,11 @@ internal static void Execute( { CustomExecution[] customExecutions = effectEvaluatedData.Effect.EffectData.CustomExecutions; + // Tallied per execution, and closed before any hook runs, so the cues report what this execution did to the + // attributes and nothing an executed hook goes on to apply. + EntityAttributes targetAttributes = effectEvaluatedData.Target.Attributes; + AttributeChangeSet changes = targetAttributes.BeginChanges(); + if (customExecutions.Length > 0) { var allModifiers = new List(effectEvaluatedData.ModifierCount); @@ -217,11 +223,15 @@ internal static void Execute( } } + targetAttributes.EndChanges(changes); + effectEvaluatedData.Target.EffectsManager.OnEffectExecuted_InternalCall( effectEvaluatedData, - componentInstances); + componentInstances, + changes); - effectEvaluatedData.Target.Attributes.ApplyPendingValueChanges(); + targetAttributes.ReleaseChanges(changes); + targetAttributes.ApplyPendingValueChanges(); } /// diff --git a/Forge/Effects/EffectsManager.cs b/Forge/Effects/EffectsManager.cs index 7bffb5e..2ac22a2 100644 --- a/Forge/Effects/EffectsManager.cs +++ b/Forge/Effects/EffectsManager.cs @@ -1,5 +1,6 @@ // Copyright © Gamesmiths Guild. +using Gamesmiths.Forge.Attributes; using Gamesmiths.Forge.Core; using Gamesmiths.Forge.Cues; using Gamesmiths.Forge.Effects.Components; @@ -47,8 +48,8 @@ public class EffectsManager(IForgeEntity owner, CuesManager cuesManager) /// necessarily landed yet: an instant effect has yet to execute, and a duration effect has yet to apply its /// modifiers. For stack applications, the existing active effect may already have re-evaluated and applied its /// modifiers by the time this event fires. Use , , - /// or when you need a - /// settled post-change view. + /// or when you need a settled + /// post-change view. /// /// /// For the buff-bar lifecycle — one event per active effect appearing and disappearing — use @@ -102,7 +103,7 @@ public class EffectsManager(IForgeEntity owner, CuesManager cuesManager) /// /// /// The owner's attributes still carry the effect's modifiers at this point; they are released immediately after, - /// and is the seam for observing that. + /// and is the seam for observing that. /// /// public event Action? OnActiveEffectRemoved; @@ -373,21 +374,9 @@ public void UnregisterApplicationBlocker(IEffectApplicationBlocker blocker) internal void OnEffectExecuted_InternalCall( EffectEvaluatedData executedEffectEvaluatedData, - IEffectComponent[]? componentInstances) - { - // Cues read the attribute deltas still pending from this execution, and a hook can land another effect on the - // owner — a raised event activating an ability that commits its cost — whose own application flushes them. - // Their magnitudes are read before the hooks and their handlers run after, so they describe this execution - // alone while the components keep running first, which is what an accumulator tallying it relies on. - CueData[] cues = executedEffectEvaluatedData.Effect.EffectData.Cues; - Span cueMagnitudes = cues.Length <= CuesManager.MaxStackCueMagnitudes - ? stackalloc int[cues.Length] - : new int[cues.Length]; - bool triggerCues = CuesManager.TryCaptureCueMagnitudes( - in executedEffectEvaluatedData, - CueTriggerRequirement.OnExecute, - cueMagnitudes); - + IEffectComponent[]? componentInstances, + AttributeChangeSet changes) + { foreach (IEffectComponent component in componentInstances ?? executedEffectEvaluatedData.Effect.EffectData.EffectComponents) { @@ -396,10 +385,7 @@ internal void OnEffectExecuted_InternalCall( OnEffectExecuted?.Invoke(executedEffectEvaluatedData); - if (triggerCues) - { - _cuesManager.ExecuteCues(in executedEffectEvaluatedData, cueMagnitudes); - } + _cuesManager.ExecuteCues(in executedEffectEvaluatedData, changes); } internal void OnActiveEffectUnapplied_InternalCall(ActiveEffect removedEffect, EffectRemovalReason reason) @@ -436,16 +422,11 @@ internal void OnActiveEffectChanged_InternalCall(ActiveEffect removedEffect) OnActiveEffectChanged?.Invoke(removedEffect.Handle); } - internal void TriggerCuesUpdate_InternalCall(in EffectEvaluatedData effectEvaluatedData) - { - _cuesManager.UpdateCues(in effectEvaluatedData); - } - internal void TriggerCuesUpdate_InternalCall( in EffectEvaluatedData effectEvaluatedData, - ReadOnlySpan magnitudes) + AttributeChangeSet? changes) { - _cuesManager.UpdateCues(in effectEvaluatedData, magnitudes); + _cuesManager.UpdateCues(in effectEvaluatedData, changes); } internal void RemoveActiveEffect_InternalCall(ActiveEffect effect) @@ -477,26 +458,45 @@ internal void RebuildAroundAttributeChange(Action applyChange) { ActiveEffect[] activeEffects = [.. _activeEffects, .. Owner.Attributes.DependentEffects]; - foreach (ActiveEffect activeEffect in activeEffects) + // Each effect's writes are tallied across its unapply here and its rebuild below, so its update cues report + // the net change the membership change made through it — nothing for an attribute it modified before and + // after, its full modifier for one that just arrived. The scope belongs to the effect's own target, which is + // where a dependent effect's modifiers live. + var changeSets = new AttributeChangeSet[activeEffects.Length]; + + for (int i = 0; i < activeEffects.Length; i++) { + ActiveEffect activeEffect = activeEffects[i]; + EntityAttributes targetAttributes = activeEffect.EffectEvaluatedData.Target.Attributes; + + changeSets[i] = targetAttributes.BeginChanges(); activeEffect.DetachAttributeBindings(); activeEffect.Unapply(reApplication: true); + targetAttributes.EndChanges(changeSets[i]); } applyChange(); - var changedEffects = new List(); + var changedEffects = new List(); - foreach (ActiveEffect activeEffect in activeEffects) + for (int i = 0; i < activeEffects.Length; i++) { + ActiveEffect activeEffect = activeEffects[i]; + if (!IsStillActive(activeEffect)) { continue; } - if (activeEffect.RebuildAfterAttributeChange()) + EntityAttributes targetAttributes = activeEffect.EffectEvaluatedData.Target.Attributes; + + targetAttributes.BeginChanges(changeSets[i]); + bool changed = activeEffect.RebuildAfterAttributeChange(); + targetAttributes.EndChanges(changeSets[i]); + + if (changed) { - changedEffects.Add(activeEffect); + changedEffects.Add(i); } } @@ -507,8 +507,10 @@ internal void RebuildAroundAttributeChange(Action applyChange) activeEffect.EffectEvaluatedData.Target.Attributes.ApplyPendingValueChanges(); } - foreach (ActiveEffect activeEffect in changedEffects) + foreach (int i in changedEffects) { + ActiveEffect activeEffect = activeEffects[i]; + if (!IsStillActive(activeEffect)) { continue; @@ -518,7 +520,12 @@ internal void RebuildAroundAttributeChange(Action applyChange) effectsManager.OnActiveEffectChanged_InternalCall(activeEffect); EffectEvaluatedData effectEvaluatedData = activeEffect.EffectEvaluatedData; - effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData); + effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, changeSets[i]); + } + + for (int i = 0; i < activeEffects.Length; i++) + { + activeEffects[i].EffectEvaluatedData.Target.Attributes.ReleaseChanges(changeSets[i]); } foreach (ActiveEffect activeEffect in activeEffects) @@ -788,6 +795,8 @@ private ActiveEffect ApplyNewEffect(Effect effect, EffectApplicationContext? app OnEffectApplied?.Invoke(activeEffect.EffectEvaluatedData); EffectEvaluatedData effectEvaluatedData = activeEffect.EffectEvaluatedData; + EntityAttributes targetAttributes = effectEvaluatedData.Target.Attributes; + AttributeChangeSet changes = targetAttributes.BeginChanges(); bool triggerApplyCuesEarly = effect.EffectData.PeriodicData.HasValue && effect.EffectData.PeriodicData.Value.ExecuteOnApplication @@ -795,17 +804,20 @@ private ActiveEffect ApplyNewEffect(Effect effect, EffectApplicationContext? app if (triggerApplyCuesEarly) { - _cuesManager.ApplyCues(in effectEvaluatedData); + _cuesManager.ApplyCues(in effectEvaluatedData, changes); } activeEffect.Apply(inhibited: !remainActive); + targetAttributes.EndChanges(changes); + if (!triggerApplyCuesEarly) { - _cuesManager.ApplyCues(in effectEvaluatedData); + _cuesManager.ApplyCues(in effectEvaluatedData, changes); } - effectEvaluatedData.Target.Attributes.ApplyPendingValueChanges(); + targetAttributes.ReleaseChanges(changes); + targetAttributes.ApplyPendingValueChanges(); foreach (IEffectComponent component in activeEffect.ComponentInstances) { From 57b38816090000a070627814d582fe3756a769ad Mon Sep 17 00:00:00 2001 From: Lex Date: Sat, 19 Sep 2026 02:48:29 -0300 Subject: [PATCH 2/3] docs(cues): describe the per-operation tally AttributeValueChange and RequireModifierSuccessToTriggerCue now judge the effect's own operation only. --- docs/cues.md | 2 +- docs/effects/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cues.md b/docs/cues.md index ce88b45..36ffb2f 100644 --- a/docs/cues.md +++ b/docs/cues.md @@ -69,7 +69,7 @@ var cueData = new CueData( - **AttributeMax**: Uses an attribute's maximum value constraint (requires `magnitudeAttribute`). - **AttributeMagnitudeEvaluatedUpToChannel**: Uses an attribute's magnitude calculated up to a specific channel (requires `magnitudeAttribute` and `finalChannel`). -The attribute-based magnitudes are read the moment the effect's own attribute writes land, before its components and change notifications run, and the handlers are called afterwards with those values. Anything a component or listener applies in turn — an ability committing its cost, a threshold effect — is therefore never counted in a cue of the effect that triggered it. +`AttributeValueChange` reports the net change the effect's own operation made to the attribute — an execution, an application, a stack or level change, or a rebuild after an attribute set arrived or left — tallied apart from every other operation on the entity. Anything a component, listener or cue handler applies in turn is an operation of its own with its own tally, so an ability committing its cost or a threshold effect landing is never counted in the cue of the effect that triggered it, and an effect applied while another is still landing never reads that one's changes as its own. The handlers themselves run after the effect's components and change notifications. ```csharp // Magnitude based on effect level diff --git a/docs/effects/README.md b/docs/effects/README.md index b1a676e..2c38d55 100644 --- a/docs/effects/README.md +++ b/docs/effects/README.md @@ -839,7 +839,7 @@ var cueEnabledEffectData = new EffectData( **Cue-related properties:** -- **RequireModifierSuccessToTriggerCue**: Specifies for which effect lifecycle events a cue should only trigger if at least one attribute was successfully modified. +- **RequireModifierSuccessToTriggerCue**: Specifies for which effect lifecycle events a cue should only trigger if at least one attribute was successfully modified by the effect's own operation — changes another effect is making to the same entity at the time do not count. - `None`: No modifier success required; cues may trigger regardless of success. - `OnApply`: Only trigger cues on application if at least one attribute is modified. - `OnUpdate`: Only trigger cues on update if at least one attribute is modified. From 61b5983ed3948c6e6c57f89ceba98eda295086b9 Mon Sep 17 00:00:00 2001 From: Lex Date: Sat, 19 Sep 2026 11:45:43 -0300 Subject: [PATCH 3/3] fix(cues): snapshot cues and key deltas by name All cue magnitudes are read once, right after the operation writes, and dispatched after its hooks; deltas are looked up by attribute key so a departed or swapped attribute keeps its change. --- .../Cues/CueMagnitudeReentrancyTests.cs | 93 ++++++++++--- Forge/Attributes/AttributeChangeSet.cs | 22 ++- Forge/Core/EntityAttributes.cs | 1 + Forge/Cues/CuesManager.cs | 130 ++++++++++++------ Forge/Effects/ActiveEffect.cs | 22 ++- Forge/Effects/EffectsManager.cs | 66 ++++++++- docs/cues.md | 2 +- 7 files changed, 267 insertions(+), 69 deletions(-) diff --git a/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs b/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs index e2faba7..956a089 100644 --- a/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs +++ b/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs @@ -154,8 +154,8 @@ public void A_nested_effects_cues_read_the_changes_of_its_own_execution_and_not_ EventTag(), _ => target.EffectsManager.ApplyEffect(CreateSideEffect( target, - CreateCue("test.cue2"), - CreateCue("test.cue3", SideAttribute)))); + cue: CreateCue("test.cue2"), + otherCue: CreateCue("test.cue3", SideAttribute)))); target.EffectsManager.ApplyEffect(CreateInstantEffect(target)); @@ -180,8 +180,8 @@ public void Modifier_success_is_judged_on_the_effects_own_execution() EventTag(), _ => target.EffectsManager.ApplyEffect(CreateSideEffect( target, - CreateCue("test.cue2"), magnitude: 0, + cue: CreateCue("test.cue2"), requireModifierSuccess: true))); target.EffectsManager.ApplyEffect(CreateInstantEffect(target)); @@ -190,6 +190,65 @@ public void Modifier_success_is_judged_on_the_effects_own_execution() _sideCue.ExecuteData.Count.Should().Be(0); } + [Fact] + [Trait("Execute", null)] + public void Every_cue_of_an_execution_is_read_before_its_hooks_and_handlers_can_move_the_attributes() + { + var target = new TestEntity(_tagsManager, _cuesManager); + _cue.Reset(); + _sideCue.Reset(); + + // Both an executed hook and the first cue's own handler take another 5 off the attribute; the second cue, read + // after the execution's writes and before either ran, still reports the 80 this execution left. + target.Events.Subscribe( + EventTag(), + _ => target.EffectsManager.ApplyEffect(CreateSideEffect(target, CueAttribute, -5))); + + void ReenterFromCue() + { + target.EffectsManager.ApplyEffect(CreateSideEffect(target, CueAttribute, -5)); + } + + _cue.OnExecuted += ReenterFromCue; + + try + { + target.EffectsManager.ApplyEffect(CreateInstantEffect( + target, + CreateCue("test.cue2", CueAttribute, CueMagnitudeType.AttributeCurrentValue))); + + target.PlayerAttributeSet.Attribute90.CurrentValue.Should().Be(70); + _cue.ExecuteData.Value.Should().Be(-10); + _sideCue.ExecuteData.Value.Should().Be(80); + } + finally + { + _cue.OnExecuted -= ReenterFromCue; + } + } + + [Fact] + [Trait("Update", null)] + public void Update_cues_after_an_attribute_set_leaves_report_the_modifier_it_took_with_it() + { + var target = new TestEntity(_tagsManager, _cuesManager); + var vitalSet = new VitalAttributeSet(); + target.Attributes.AddAttributeSet(vitalSet); + _cue.Reset(); + _sideCue.Reset(); + + target.EffectsManager.ApplyEffect(CreateCrossSetEffect(target)); + target.Attributes[ArrivingAttribute].CurrentValue.Should().Be(90); + + // The departing attribute is detached before the update cues run, but the unapply that gave it its 10 back was + // this effect's doing and is still reported, the way its listeners still hear that last change. + target.Attributes.RemoveAttributeSet(vitalSet); + + _cue.UpdateData.Count.Should().Be(1); + _cue.UpdateData.Value.Should().Be(10); + _sideCue.UpdateData.Value.Should().Be(0); + } + [Fact] [Trait("Update", null)] public void Update_cues_after_an_attribute_set_arrives_report_what_the_effect_now_contributes() @@ -214,9 +273,10 @@ public void Update_cues_after_an_attribute_set_arrives_report_what_the_effect_no private static Effect CreateSideEffect( TestEntity target, + string attribute = SideAttribute, + float magnitude = 1, CueData? cue = null, CueData? otherCue = null, - float magnitude = 1, bool requireModifierSuccess = false) { CueTriggerRequirement requirement = requireModifierSuccess @@ -226,7 +286,7 @@ private static Effect CreateSideEffect( var effectData = new EffectData( "Side Effect", new DurationData(DurationType.Instant), - [CreateModifier(SideAttribute, magnitude)], + [CreateModifier(attribute, magnitude)], requireModifierSuccessToTriggerCue: requirement, cues: [.. new[] { cue, otherCue }.OfType()]); @@ -241,14 +301,12 @@ private static Modifier CreateModifier(string attribute, float magnitude) new ModifierMagnitude(MagnitudeCalculationType.ScalableFloat, new ScalableFloat(magnitude))); } - private static CueData CreateCue(Tag cueTag, string attribute) + private static CueData CreateCue( + Tag cueTag, + string attribute, + CueMagnitudeType magnitudeType = CueMagnitudeType.AttributeValueChange) { - return new CueData( - cueTag.GetSingleTagContainer(), - -100, - 100, - CueMagnitudeType.AttributeValueChange, - attribute); + return new CueData(cueTag.GetSingleTagContainer(), -100, 100, magnitudeType, attribute); } private CueData CreateCue(Tag? cueTag = null) @@ -256,9 +314,12 @@ private CueData CreateCue(Tag? cueTag = null) return CreateCue(cueTag ?? Tag.RequestTag(_tagsManager, "test.cue1"), CueAttribute); } - private CueData CreateCue(string cueTagName, string attribute = CueAttribute) + private CueData CreateCue( + string cueTagName, + string attribute = CueAttribute, + CueMagnitudeType magnitudeType = CueMagnitudeType.AttributeValueChange) { - return CreateCue(Tag.RequestTag(_tagsManager, cueTagName), attribute); + return CreateCue(Tag.RequestTag(_tagsManager, cueTagName), attribute, magnitudeType); } private Effect CreateCrossSetEffect(TestEntity target) @@ -272,7 +333,7 @@ private Effect CreateCrossSetEffect(TestEntity target) return new Effect(effectData, new EffectOwnership(target, target)); } - private Effect CreateInstantEffect(TestEntity target) + private Effect CreateInstantEffect(TestEntity target, CueData? extraCue = null) { var effectData = new EffectData( "Instant Effect", @@ -282,7 +343,7 @@ private Effect CreateInstantEffect(TestEntity target) [ new RaiseEventEffectComponent(EventTag().GetSingleTagContainer()!, EffectEventTrigger.Executed) ], - cues: [CreateCue()]); + cues: [.. new[] { CreateCue(), extraCue }.OfType()]); return new Effect(effectData, new EffectOwnership(target, target)); } diff --git a/Forge/Attributes/AttributeChangeSet.cs b/Forge/Attributes/AttributeChangeSet.cs index e30801f..cf56bd0 100644 --- a/Forge/Attributes/AttributeChangeSet.cs +++ b/Forge/Attributes/AttributeChangeSet.cs @@ -45,15 +45,27 @@ internal bool HasChanges } /// - /// Gets the net change the operation made to an attribute's current value; zero for one it never touched. + /// Gets the net change the operation made to the current value of the attribute with a key; zero for one it never + /// touched. /// - /// The attribute to look up. + /// + /// Looked up by key rather than through the entity's attributes so the change survives the attribute leaving: an + /// attribute set being removed unapplies its modifiers, which is recorded here, and detaches the attributes before + /// the update cues read it, and a hook can swap a set for a fresh instance between an operation and its cues. + /// + /// The key of the attribute to look up. /// The net change to the attribute's current value. - internal int DeltaOf(EntityAttribute attribute) + internal int DeltaOf(StringKey attributeKey) { - int index = IndexOf(attribute); + for (int i = 0; i < _changes.Count; i++) + { + if (_changes[i].Attribute.Key == attributeKey) + { + return _changes[i].Delta; + } + } - return index < 0 ? 0 : _changes[index].Delta; + return 0; } internal void Record(EntityAttribute attribute, int delta) diff --git a/Forge/Core/EntityAttributes.cs b/Forge/Core/EntityAttributes.cs index 9b2db8d..3a124fd 100644 --- a/Forge/Core/EntityAttributes.cs +++ b/Forge/Core/EntityAttributes.cs @@ -267,6 +267,7 @@ internal void RecordChange(EntityAttribute attribute, int delta) _openChanges[^1].Record(attribute, delta); } } +#pragma warning restore T0009 // Internal Styling Rule T0009 private void AttachAttributeSet(AttributeSet attributeSet) { diff --git a/Forge/Cues/CuesManager.cs b/Forge/Cues/CuesManager.cs index 5b13479..dbd4aad 100644 --- a/Forge/Cues/CuesManager.cs +++ b/Forge/Cues/CuesManager.cs @@ -12,6 +12,11 @@ namespace Gamesmiths.Forge.Cues; /// public sealed class CuesManager { + /// + /// The most cues one effect can have before the magnitudes held across its hooks move from the stack to the heap. + /// + internal const int MaxStackCueMagnitudes = 8; + private readonly Dictionary> _registeredCues = []; /// @@ -127,29 +132,72 @@ public void UpdateCue(Tag cueTag, IForgeEntity? target, CueParameters? parameter } /// - /// Applies the cues of an effect that just landed on its target. + /// Decides whether an effect's cues fire for a trigger and, when they do, reads the magnitude of each one into + /// , one slot per entry of the effect's cues. /// /// - /// The cues of an operation read the the operation tallied its attribute writes - /// into, never the entity-wide pending values: those accumulate across every operation until the next flush, so a + /// + /// The read and the dispatch are separate on purpose: every cue of an operation is read at once, right after the + /// operation's own writes, and the handlers run later with those values. Reading at dispatch instead would let a + /// hook, or an earlier handler of the same operation, move a value before a later cue looks at it, so the cues of + /// one operation would not describe one state. + /// + /// + /// An attribute's change comes from the the operation tallied its writes into, + /// never from the entity-wide pending values: those accumulate across every operation until the next flush, so a /// nested one — a raised event activating an ability that applies its own effects while a hit is still landing — - /// would read the hit's deltas as its own. The set is closed before any hook runs, which is also what lets the - /// handlers keep running after the components and change notifications rather than ahead of them. + /// would read the hit's deltas as its own. + /// /// + /// The evaluated data of the effect whose cues are being read. + /// The trigger the cues are being read for. + /// The changes the operation made to the target's attributes, or + /// when it made none. + /// Receives one magnitude per cue; at least as long as the effect's cues. + /// if the cues fire and was filled; otherwise, + /// . + internal static bool TryCaptureCueMagnitudes( + in EffectEvaluatedData effectEvaluatedData, + CueTriggerRequirement triggerRequirement, + AttributeChangeSet? changes, + Span magnitudes) + { + EffectData effectData = effectEvaluatedData.Effect.EffectData; + + if (!ShouldTriggerCue(in effectData, changes, triggerRequirement)) + { + return false; + } + + for (int i = 0; i < effectData.Cues.Length; i++) + { + magnitudes[i] = CalculateMagnitude(in effectData.Cues[i], in effectEvaluatedData, changes); + } + + return true; + } + + /// + /// Applies the cues of an effect that just landed on its target. Nothing runs between an application and its + /// cues, so they are read and dispatched here in one go. + /// /// The evaluated data of the applied effect. /// The changes the application made to the target's attributes. internal void ApplyCues(in EffectEvaluatedData effectEvaluatedData, AttributeChangeSet changes) { - EffectData effectData = effectEvaluatedData.Effect.EffectData; + CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; + Span magnitudes = cues.Length <= MaxStackCueMagnitudes + ? stackalloc int[cues.Length] + : new int[cues.Length]; - if (!ShouldTriggerCue(in effectData, changes, CueTriggerRequirement.OnApply)) + if (!TryCaptureCueMagnitudes(in effectEvaluatedData, CueTriggerRequirement.OnApply, changes, magnitudes)) { return; } - foreach (CueData cueData in effectData.Cues) + for (int i = 0; i < cues.Length; i++) { - int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData, changes); + CueData cueData = cues[i]; if (cueData.CueTags is null) { @@ -162,8 +210,8 @@ internal void ApplyCues(in EffectEvaluatedData effectEvaluatedData, AttributeCha cueTag, effectEvaluatedData.Target, new CueParameters( - magnitude, - cueData.NormalizedMagnitude(magnitude), + magnitudes[i], + cueData.NormalizedMagnitude(magnitudes[i]), effectEvaluatedData.Effect.Ownership.Source, effectEvaluatedData.CustomCueParameters)); } @@ -189,22 +237,18 @@ internal void RemoveCues(in EffectEvaluatedData effectEvaluatedData, bool interr } /// - /// Executes the cues of an effect that just executed on its target. + /// Executes the cues of an effect that just executed on its target, with the magnitudes + /// read before the execution's hooks ran. /// /// The evaluated data of the executed effect. - /// The changes the execution made to the target's attributes. - internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, AttributeChangeSet changes) + /// The captured magnitude of each cue. + internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySpan magnitudes) { - EffectData effectData = effectEvaluatedData.Effect.EffectData; - - if (!ShouldTriggerCue(in effectData, changes, CueTriggerRequirement.OnExecute)) - { - return; - } + CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; - foreach (CueData cueData in effectData.Cues) + for (int i = 0; i < cues.Length; i++) { - int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData, changes); + CueData cueData = cues[i]; if (cueData.CueTags is null) { @@ -217,8 +261,8 @@ internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, AttributeC cueTag, effectEvaluatedData.Target, new CueParameters( - magnitude, - cueData.NormalizedMagnitude(magnitude), + magnitudes[i], + cueData.NormalizedMagnitude(magnitudes[i]), effectEvaluatedData.Effect.Ownership.Source, effectEvaluatedData.CustomCueParameters)); } @@ -226,23 +270,37 @@ internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, AttributeC } /// - /// Updates the cues of an active effect that just changed on its target. + /// Updates the cues of an active effect whose change ran no hooks, reading and dispatching them in one go. /// /// The evaluated data of the changed effect. /// The changes the operation made to the target's attributes, or /// when it made none. internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData, AttributeChangeSet? changes) { - EffectData effectData = effectEvaluatedData.Effect.EffectData; + CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; + Span magnitudes = cues.Length <= MaxStackCueMagnitudes + ? stackalloc int[cues.Length] + : new int[cues.Length]; - if (!ShouldTriggerCue(in effectData, changes, CueTriggerRequirement.OnUpdate)) + if (TryCaptureCueMagnitudes(in effectEvaluatedData, CueTriggerRequirement.OnUpdate, changes, magnitudes)) { - return; + UpdateCues(in effectEvaluatedData, magnitudes); } + } - foreach (CueData cueData in effectData.Cues) + /// + /// Updates the cues of an active effect that just changed on its target, with the magnitudes + /// read before the change's hooks ran. + /// + /// The evaluated data of the changed effect. + /// The captured magnitude of each cue. + internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySpan magnitudes) + { + CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; + + for (int i = 0; i < cues.Length; i++) { - int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData, changes); + CueData cueData = cues[i]; if (cueData.CueTags is null) { @@ -255,8 +313,8 @@ internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData, AttributeCh cueTag, effectEvaluatedData.Target, new CueParameters( - magnitude, - cueData.NormalizedMagnitude(magnitude), + magnitudes[i], + cueData.NormalizedMagnitude(magnitudes[i]), effectEvaluatedData.Effect.Ownership.Source, effectEvaluatedData.CustomCueParameters)); } @@ -294,13 +352,7 @@ private static int CalculateMagnitude( cueData.MagnitudeAttribute is not null, "Cues with CueMagnitudeType.AttributeMagnitude must contains a configured MagnitudeAttribute."); - if (changes is null - || !effectEvaluatedData.Target.Attributes.ContainsAttribute(cueData.MagnitudeAttribute)) - { - return 0; - } - - return changes.DeltaOf(effectEvaluatedData.Target.Attributes[cueData.MagnitudeAttribute]); + return changes?.DeltaOf(cueData.MagnitudeAttribute) ?? 0; case CueMagnitudeType.AttributeBaseValue: Validation.Assert( diff --git a/Forge/Effects/ActiveEffect.cs b/Forge/Effects/ActiveEffect.cs index f3d88ef..ea8a84f 100644 --- a/Forge/Effects/ActiveEffect.cs +++ b/Forge/Effects/ActiveEffect.cs @@ -2,6 +2,7 @@ using Gamesmiths.Forge.Attributes; using Gamesmiths.Forge.Core; +using Gamesmiths.Forge.Cues; using Gamesmiths.Forge.Effects.Components; using Gamesmiths.Forge.Effects.Duration; using Gamesmiths.Forge.Effects.Magnitudes; @@ -295,7 +296,9 @@ internal bool AddStack(Effect effect, int stacks = 1) { // Nothing was re-applied, so there are no attribute changes for the update cues to report. EffectEvaluatedData effectEvaluatedData = EffectEvaluatedData; - effectEvaluatedData.Target.EffectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, null); + effectEvaluatedData.Target.EffectsManager.TriggerCuesUpdate_InternalCall( + in effectEvaluatedData, + changes: null); } if (stackingData.ApplicationRefreshPolicy == StackApplicationRefreshPolicy.RefreshOnSuccessfulApplication) @@ -559,11 +562,24 @@ private void ReapplyEffect(Effect effect, int? level = null, bool isStackingCall EffectEvaluatedData effectEvaluatedData = EffectEvaluatedData; EffectsManager effectsManager = effectEvaluatedData.Target.EffectsManager; + // Read before the changed hooks, on the state this re-application left, and dispatched after them, on a handle + // the hooks saw whole. + CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; + Span cueMagnitudes = cues.Length <= CuesManager.MaxStackCueMagnitudes + ? stackalloc int[cues.Length] + : new int[cues.Length]; + bool triggerCues = (!Effect.EffectData.SuppressStackingCues || !isStackingCall) + && CuesManager.TryCaptureCueMagnitudes( + in effectEvaluatedData, + CueTriggerRequirement.OnUpdate, + changes, + cueMagnitudes); + effectsManager.OnActiveEffectChanged_InternalCall(this); - if (!Effect.EffectData.SuppressStackingCues || !isStackingCall) + if (triggerCues) { - effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, changes); + effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, cueMagnitudes); } targetAttributes.ReleaseChanges(changes); diff --git a/Forge/Effects/EffectsManager.cs b/Forge/Effects/EffectsManager.cs index 2ac22a2..0dc8748 100644 --- a/Forge/Effects/EffectsManager.cs +++ b/Forge/Effects/EffectsManager.cs @@ -377,6 +377,19 @@ internal void OnEffectExecuted_InternalCall( IEffectComponent[]? componentInstances, AttributeChangeSet changes) { + // Read before the hooks, so every cue of this execution sees the state its own writes left — the change from + // the closed set, the rest from the attributes as they stand — and dispatched after them, so the handlers keep + // running behind the components, which an accumulator tallying this execution relies on. + CueData[] cues = executedEffectEvaluatedData.Effect.EffectData.Cues; + Span cueMagnitudes = cues.Length <= CuesManager.MaxStackCueMagnitudes + ? stackalloc int[cues.Length] + : new int[cues.Length]; + bool triggerCues = CuesManager.TryCaptureCueMagnitudes( + in executedEffectEvaluatedData, + CueTriggerRequirement.OnExecute, + changes, + cueMagnitudes); + foreach (IEffectComponent component in componentInstances ?? executedEffectEvaluatedData.Effect.EffectData.EffectComponents) { @@ -385,7 +398,10 @@ internal void OnEffectExecuted_InternalCall( OnEffectExecuted?.Invoke(executedEffectEvaluatedData); - _cuesManager.ExecuteCues(in executedEffectEvaluatedData, changes); + if (triggerCues) + { + _cuesManager.ExecuteCues(in executedEffectEvaluatedData, cueMagnitudes); + } } internal void OnActiveEffectUnapplied_InternalCall(ActiveEffect removedEffect, EffectRemovalReason reason) @@ -429,6 +445,13 @@ internal void TriggerCuesUpdate_InternalCall( _cuesManager.UpdateCues(in effectEvaluatedData, changes); } + internal void TriggerCuesUpdate_InternalCall( + in EffectEvaluatedData effectEvaluatedData, + ReadOnlySpan magnitudes) + { + _cuesManager.UpdateCues(in effectEvaluatedData, magnitudes); + } + internal void RemoveActiveEffect_InternalCall(ActiveEffect effect) { RemoveActiveEffect(effect, EffectRemovalReason.Expired); @@ -507,9 +530,35 @@ internal void RebuildAroundAttributeChange(Action applyChange) activeEffect.EffectEvaluatedData.Target.Attributes.ApplyPendingValueChanges(); } - foreach (int i in changedEffects) + // Every changed effect's cues are read here, on the settled attributes, before any of their hooks run; a hook + // of one effect must not move what another's cues report. A rare path, so the buffers can live on the heap. + int[]?[] cueMagnitudes = new int[changedEffects.Count][]; + + for (int j = 0; j < changedEffects.Count; j++) { - ActiveEffect activeEffect = activeEffects[i]; + ActiveEffect activeEffect = activeEffects[changedEffects[j]]; + + if (!IsStillActive(activeEffect)) + { + continue; + } + + EffectEvaluatedData effectEvaluatedData = activeEffect.EffectEvaluatedData; + int[] magnitudes = new int[effectEvaluatedData.Effect.EffectData.Cues.Length]; + + if (CuesManager.TryCaptureCueMagnitudes( + in effectEvaluatedData, + CueTriggerRequirement.OnUpdate, + changeSets[changedEffects[j]], + magnitudes)) + { + cueMagnitudes[j] = magnitudes; + } + } + + for (int j = 0; j < changedEffects.Count; j++) + { + ActiveEffect activeEffect = activeEffects[changedEffects[j]]; if (!IsStillActive(activeEffect)) { @@ -519,8 +568,11 @@ internal void RebuildAroundAttributeChange(Action applyChange) EffectsManager effectsManager = activeEffect.EffectEvaluatedData.Target.EffectsManager; effectsManager.OnActiveEffectChanged_InternalCall(activeEffect); - EffectEvaluatedData effectEvaluatedData = activeEffect.EffectEvaluatedData; - effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, changeSets[i]); + if (cueMagnitudes[j] is int[] magnitudes) + { + EffectEvaluatedData effectEvaluatedData = activeEffect.EffectEvaluatedData; + effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, magnitudes); + } } for (int i = 0; i < activeEffects.Length; i++) @@ -804,7 +856,11 @@ private ActiveEffect ApplyNewEffect(Effect effect, EffectApplicationContext? app if (triggerApplyCuesEarly) { + // Handlers only ever run on a closed set: a handler writing an attribute directly must not tally into the + // application it is answering. The set is still empty here and is reopened for the application itself. + targetAttributes.EndChanges(changes); _cuesManager.ApplyCues(in effectEvaluatedData, changes); + targetAttributes.BeginChanges(changes); } activeEffect.Apply(inhibited: !remainActive); diff --git a/docs/cues.md b/docs/cues.md index 36ffb2f..29e0b0b 100644 --- a/docs/cues.md +++ b/docs/cues.md @@ -69,7 +69,7 @@ var cueData = new CueData( - **AttributeMax**: Uses an attribute's maximum value constraint (requires `magnitudeAttribute`). - **AttributeMagnitudeEvaluatedUpToChannel**: Uses an attribute's magnitude calculated up to a specific channel (requires `magnitudeAttribute` and `finalChannel`). -`AttributeValueChange` reports the net change the effect's own operation made to the attribute — an execution, an application, a stack or level change, or a rebuild after an attribute set arrived or left — tallied apart from every other operation on the entity. Anything a component, listener or cue handler applies in turn is an operation of its own with its own tally, so an ability committing its cost or a threshold effect landing is never counted in the cue of the effect that triggered it, and an effect applied while another is still landing never reads that one's changes as its own. The handlers themselves run after the effect's components and change notifications. +`AttributeValueChange` reports the net change the effect's own operation made to the attribute — an execution, an application, a stack or level change, or a rebuild after an attribute set arrived or left — tallied apart from every other operation on the entity. Anything a component, listener or cue handler applies in turn is an operation of its own with its own tally, so an ability committing its cost or a threshold effect landing is never counted in the cue of the effect that triggered it, and an effect applied while another is still landing never reads that one's changes as its own. An attribute that leaves with its set still reports the change its departure made through the effect, the same way its listeners hear that last change. Every cue of an operation is read at once, right after the operation's own writes, and the handlers run after the effect's components and change notifications with those values — so neither a hook nor an earlier handler can move what a later cue reports. ```csharp // Magnitude based on effect level