diff --git a/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs b/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs new file mode 100644 index 00000000..baf77462 --- /dev/null +++ b/Forge.Tests/Cues/CueMagnitudeReentrancyTests.cs @@ -0,0 +1,247 @@ +// Copyright © Gamesmiths Guild. + +using FluentAssertions; +using Gamesmiths.Forge.Core; +using Gamesmiths.Forge.Cues; +using Gamesmiths.Forge.Effects; +using Gamesmiths.Forge.Effects.Components; +using Gamesmiths.Forge.Effects.Duration; +using Gamesmiths.Forge.Effects.Magnitudes; +using Gamesmiths.Forge.Effects.Modifiers; +using Gamesmiths.Forge.Effects.Periodic; +using Gamesmiths.Forge.Effects.Stacking; +using Gamesmiths.Forge.Tags; +using Gamesmiths.Forge.Tests.Helpers; + +using static Gamesmiths.Forge.Tests.Helpers.TagsAndCuesFixture; + +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. +/// +/// The fixture providing the and . +/// +public class CueMagnitudeReentrancyTests(TagsAndCuesFixture tagsAndCuesFixture) : IClassFixture +{ + private const string CueAttribute = "TestAttributeSet.Attribute90"; + private const string SideAttribute = "TestAttributeSet.Attribute1"; + + private readonly TagsManager _tagsManager = tagsAndCuesFixture.TagsManager; + private readonly CuesManager _cuesManager = tagsAndCuesFixture.CuesManager; + private readonly TestCue _cue = tagsAndCuesFixture.TestCueInstances[0]; + + [Fact] + [Trait("Execute", null)] + public void Execute_cue_keeps_its_attribute_change_when_an_executed_hook_applies_an_effect_to_the_target() + { + var target = new TestEntity(_tagsManager, _cuesManager); + _cue.Reset(); + + // The Thorns recipe: the executed event activates an ability whose commit lands its cost on the same entity. + target.Events.Subscribe(EventTag(), _ => target.EffectsManager.ApplyEffect(CreateSideEffect(target))); + + target.EffectsManager.ApplyEffect(CreateInstantEffect(target)); + + target.PlayerAttributeSet.Attribute90.CurrentValue.Should().Be(80); + _cue.ExecuteData.Count.Should().Be(1); + _cue.ExecuteData.Value.Should().Be(-10); + } + + [Fact] + [Trait("Update", null)] + public void Update_cue_keeps_its_attribute_change_when_a_changed_hook_applies_an_effect_to_the_target() + { + var target = new TestEntity(_tagsManager, _cuesManager); + _cue.Reset(); + + target.EffectsManager.OnActiveEffectChanged += + _ => target.EffectsManager.ApplyEffect(CreateSideEffect(target)); + + Effect effect = CreateStackableEffect(target); + target.EffectsManager.ApplyEffect(effect); + target.EffectsManager.ApplyEffect(effect); + + target.PlayerAttributeSet.Attribute90.CurrentValue.Should().Be(80); + _cue.UpdateData.Count.Should().Be(1); + _cue.UpdateData.Value.Should().Be(-5); + } + + [Fact] + [Trait("Execute", null)] + public void Execute_cue_handlers_run_after_the_components_so_an_accumulator_tallies_the_execution_they_reenter() + { + var target = new TestEntity(_tagsManager, _cuesManager); + var tallyTag = Tag.RequestTag(_tagsManager, "other.tag"); + _cue.Reset(); + + // A cue handler landing another effect on the target flushes this execution's deltas; the accumulator has to + // have tallied them by then, or its baseline moves past them and the execution counts for nothing. + void ReenterFromCue() + { + target.EffectsManager.ApplyEffect(CreateSideEffect(target)); + } + + _cue.OnExecuted += ReenterFromCue; + + try + { + ActiveEffectHandle handle = target.EffectsManager.ApplyEffect(CreatePeriodicEffect(target, tallyTag))!; + target.EffectsManager.UpdateEffects(1); + + target.PlayerAttributeSet.Attribute90.CurrentValue.Should().Be(70); + handle.GetComponent()!.Total.Should().Be(20); + _cue.ExecuteData.Count.Should().Be(2); + _cue.ExecuteData.Value.Should().Be(-10); + } + finally + { + _cue.OnExecuted -= ReenterFromCue; + } + } + + [Fact] + [Trait("Update", null)] + public void Update_cue_handlers_run_after_the_changed_hooks_so_a_handler_removing_the_effect_leaves_them_a_live_handle() + { + var target = new TestEntity(_tagsManager, _cuesManager); + var cueTag = Tag.RequestTag(_tagsManager, "other.tag"); + var removeOnUpdate = new RemoveOnUpdateCueHandler(); + _cuesManager.RegisterCue(cueTag, removeOnUpdate); + + try + { + bool? handleWasValidInChangedHook = null; + target.EffectsManager.OnActiveEffectChanged += changed => handleWasValidInChangedHook = changed.IsValid; + + Effect effect = CreateStackableEffect(target, cueTag); + ActiveEffectHandle handle = target.EffectsManager.ApplyEffect(effect)!; + removeOnUpdate.Handle = handle; + target.EffectsManager.ApplyEffect(effect); + + handleWasValidInChangedHook.Should().BeTrue(); + handle.IsValid.Should().BeFalse(); + removeOnUpdate.LastMagnitude.Should().Be(-5); + } + finally + { + _cuesManager.UnregisterCue(cueTag, removeOnUpdate); + } + } + + private static Effect CreateSideEffect(TestEntity target) + { + var effectData = new EffectData( + "Side Effect", + new DurationData(DurationType.Instant), + [CreateModifier(SideAttribute, 1)]); + + return new Effect(effectData, new EffectOwnership(target, target)); + } + + private static Modifier CreateModifier(string attribute, float magnitude) + { + return new Modifier( + attribute, + ModifierOperation.FlatBonus, + new ModifierMagnitude(MagnitudeCalculationType.ScalableFloat, new ScalableFloat(magnitude))); + } + + private Effect CreateInstantEffect(TestEntity target) + { + var effectData = new EffectData( + "Instant Effect", + new DurationData(DurationType.Instant), + [CreateModifier(CueAttribute, -10)], + effectComponents: + [ + new RaiseEventEffectComponent(EventTag().GetSingleTagContainer()!, EffectEventTrigger.Executed) + ], + cues: [CreateCue()]); + + return new Effect(effectData, new EffectOwnership(target, target)); + } + + private Effect CreatePeriodicEffect(TestEntity target, Tag tallyTag) + { + var effectData = new EffectData( + "Periodic Effect", + new DurationData( + DurationType.HasDuration, + new ModifierMagnitude(MagnitudeCalculationType.ScalableFloat, new ScalableFloat(10))), + [CreateModifier(CueAttribute, -10)], + periodicData: new PeriodicData(new ScalableFloat(1), true, PeriodInhibitionRemovedPolicy.NeverReset), + effectComponents: [new AttributeAccumulatorEffectComponent(CueAttribute, tallyTag)], + cues: [CreateCue()]); + + return new Effect(effectData, new EffectOwnership(target, target)); + } + + private Effect CreateStackableEffect(TestEntity target, Tag? cueTag = null) + { + var effectData = new EffectData( + "Stackable Effect", + new DurationData(DurationType.Infinite), + [CreateModifier(CueAttribute, -5)], + new StackingData( + new ScalableInt(2), + new ScalableInt(1), + StackPolicy.AggregateBySource, + StackLevelPolicy.AggregateLevels, + StackMagnitudePolicy.Sum, + StackOverflowPolicy.DenyApplication, + StackExpirationPolicy.ClearEntireStack), + cues: [CreateCue(cueTag)]); + + 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"); + } + + /// + /// A cue handler that takes its effect off the target from the update, the way a "dispel on this cue" handler + /// would. + /// + private sealed class RemoveOnUpdateCueHandler : ICueHandler + { + public ActiveEffectHandle? Handle { get; set; } + + public int? LastMagnitude { get; private set; } + + public void OnApply(IForgeEntity? target, CueParameters? parameters) + { + } + + public void OnExecute(IForgeEntity? target, CueParameters? parameters) + { + } + + public void OnRemove(IForgeEntity? target, bool interrupted) + { + } + + public void OnUpdate(IForgeEntity? target, CueParameters? parameters) + { + LastMagnitude = parameters?.Magnitude; + Handle?.Target?.EffectsManager.RemoveEffect(Handle, true); + } + } +} diff --git a/Forge/Cues/CuesManager.cs b/Forge/Cues/CuesManager.cs index c0faf26f..5bf72a7d 100644 --- a/Forge/Cues/CuesManager.cs +++ b/Forge/Cues/CuesManager.cs @@ -11,6 +11,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 = []; /// @@ -125,6 +130,44 @@ 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. + /// + /// + /// 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 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) + { + 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; @@ -176,19 +219,13 @@ internal void RemoveCues(in EffectEvaluatedData effectEvaluatedData, bool interr } } - internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData) + internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData, ReadOnlySpan magnitudes) { - EffectData effectData = effectEvaluatedData.Effect.EffectData; + CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; - EntityAttributes targetAttributes = effectEvaluatedData.Target.Attributes; - if (!ShouldTriggerCue(in effectData, in targetAttributes, CueTriggerRequirement.OnExecute)) + for (int i = 0; i < cues.Length; i++) { - return; - } - - foreach (CueData cueData in effectData.Cues) - { - int magnitude = CalculateMagnitude(in cueData, in effectEvaluatedData); + CueData cueData = cues[i]; if (cueData.CueTags is null) { @@ -201,8 +238,8 @@ internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData) cueTag, effectEvaluatedData.Target, new CueParameters( - magnitude, - cueData.NormalizedMagnitude(magnitude), + magnitudes[i], + cueData.NormalizedMagnitude(magnitudes[i]), effectEvaluatedData.Effect.Ownership.Source, effectEvaluatedData.CustomCueParameters)); } @@ -211,17 +248,22 @@ internal void ExecuteCues(in EffectEvaluatedData effectEvaluatedData) internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData) { - EffectData effectData = effectEvaluatedData.Effect.EffectData; + CueData[] cues = effectEvaluatedData.Effect.EffectData.Cues; + Span magnitudes = cues.Length <= MaxStackCueMagnitudes ? stackalloc int[cues.Length] : new int[cues.Length]; - EntityAttributes targetAttributes = effectEvaluatedData.Target.Attributes; - if (!ShouldTriggerCue(in effectData, in targetAttributes, CueTriggerRequirement.OnUpdate)) + if (TryCaptureCueMagnitudes(in effectEvaluatedData, CueTriggerRequirement.OnUpdate, magnitudes)) { - return; + UpdateCues(in effectEvaluatedData, magnitudes); } + } - foreach (CueData cueData in effectData.Cues) + 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); + CueData cueData = cues[i]; if (cueData.CueTags is null) { @@ -234,8 +276,8 @@ internal void UpdateCues(in EffectEvaluatedData effectEvaluatedData) cueTag, effectEvaluatedData.Target, new CueParameters( - magnitude, - cueData.NormalizedMagnitude(magnitude), + magnitudes[i], + cueData.NormalizedMagnitude(magnitudes[i]), effectEvaluatedData.Effect.Ownership.Source, effectEvaluatedData.CustomCueParameters)); } diff --git a/Forge/Effects/ActiveEffect.cs b/Forge/Effects/ActiveEffect.cs index 89927aa7..fd83a723 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; @@ -548,13 +549,27 @@ private void ReapplyEffect(Effect effect, int? level = null, bool isStackingCall Apply(reApplication: true); - EffectEvaluatedData.Target.EffectsManager.OnActiveEffectChanged_InternalCall(this); - EffectEvaluatedData effectEvaluatedData = EffectEvaluatedData; - - if (!Effect.EffectData.SuppressStackingCues || !isStackingCall) - { - EffectEvaluatedData.Target.EffectsManager.TriggerCuesUpdate_InternalCall(in 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) + { + effectsManager.TriggerCuesUpdate_InternalCall(in effectEvaluatedData, cueMagnitudes); } effectEvaluatedData.Target.Attributes.ApplyPendingValueChanges(); diff --git a/Forge/Effects/EffectsManager.cs b/Forge/Effects/EffectsManager.cs index b71063d1..7bffb5e3 100644 --- a/Forge/Effects/EffectsManager.cs +++ b/Forge/Effects/EffectsManager.cs @@ -63,7 +63,8 @@ public class EffectsManager(IForgeEntity owner, CuesManager cuesManager) /// /// The manager-wide counterpart of : only instant and periodic /// effects execute, and a periodic effect raises it on every tick. The execution has already changed the base - /// values by this point, so handlers read post-execution attributes. + /// values by this point, so handlers read post-execution attributes. Its cues fire after the handlers, but read + /// their magnitudes before, so a handler that applies further effects to the owner cannot disturb what they report. /// public event Action? OnEffectExecuted; @@ -374,6 +375,19 @@ 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); + foreach (IEffectComponent component in componentInstances ?? executedEffectEvaluatedData.Effect.EffectData.EffectComponents) { @@ -382,7 +396,10 @@ internal void OnEffectExecuted_InternalCall( OnEffectExecuted?.Invoke(executedEffectEvaluatedData); - _cuesManager.ExecuteCues(in executedEffectEvaluatedData); + if (triggerCues) + { + _cuesManager.ExecuteCues(in executedEffectEvaluatedData, cueMagnitudes); + } } internal void OnActiveEffectUnapplied_InternalCall(ActiveEffect removedEffect, EffectRemovalReason reason) @@ -424,6 +441,13 @@ internal void TriggerCuesUpdate_InternalCall(in EffectEvaluatedData effectEvalua _cuesManager.UpdateCues(in effectEvaluatedData); } + internal void TriggerCuesUpdate_InternalCall( + in EffectEvaluatedData effectEvaluatedData, + ReadOnlySpan magnitudes) + { + _cuesManager.UpdateCues(in effectEvaluatedData, magnitudes); + } + internal void RemoveActiveEffect_InternalCall(ActiveEffect effect) { RemoveActiveEffect(effect, EffectRemovalReason.Expired); diff --git a/docs/cues.md b/docs/cues.md index e7507ed7..ce88b454 100644 --- a/docs/cues.md +++ b/docs/cues.md @@ -69,6 +69,8 @@ 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. + ```csharp // Magnitude based on effect level var levelBasedCue = new CueData( diff --git a/docs/statescript/subgraphs.md b/docs/statescript/subgraphs.md index bdff07db..4eece6b7 100644 --- a/docs/statescript/subgraphs.md +++ b/docs/statescript/subgraphs.md @@ -26,6 +26,8 @@ Every state node has both an **OnActivate** port (Event port) and a **Subgraph** - **OnActivate** → downstream nodes are **independent siblings**. They outlive the port and must manage their own lifetime. - **Subgraph** → downstream nodes are **owned by the port**. They are automatically disabled when the parent node deactivates. +The distinction is made where the disable signal *starts*: a node that deactivates on its own sends it through its Subgraph ports only. A node that *receives* the signal forwards it through every one of its output ports, event ports included, so once a node is inside a subgraph its whole downstream tree goes with it, however the links inside are wired. Chaining `A → OnActivate → B → OnActivate → C` under a Subgraph port is therefore fine: `B` and `C` are still torn down with the subgraph. Only a chain hanging off the OnActivate port of a node that ends *itself* — the root of a graph, a timer running out — survives that node. + ### Example: The Difference in Practice ```csharp