From 7ed18883ab9e3eb6100df9a4c5a6658c83f33425 Mon Sep 17 00:00:00 2001 From: Lex Date: Sat, 5 Sep 2026 18:52:12 -0300 Subject: [PATCH 1/6] feat(statescript): add a fixed update rail and an update stamp Graphs only had the frame, so a node moving a body pushed harder at 120fps than at 60. OnFixedUpdate is the second rail; UpdateStamp counts passes over both so a resolver can tell them apart. --- Forge.Tests/Helpers/StatescriptTestHelpers.cs | 89 ++++++ Forge.Tests/Statescript/FixedUpdateTests.cs | 284 ++++++++++++++++++ Forge.Tests/Statescript/UpdateStampTests.cs | 132 ++++++++ Forge/Abilities/Ability.cs | 8 + Forge/Abilities/IAbilityBehavior.cs | 13 + Forge/Core/EntityAbilities.cs | 18 ++ Forge/Statescript/GraphAbilityBehavior.cs | 6 + Forge/Statescript/GraphContext.cs | 26 ++ Forge/Statescript/GraphProcessor.cs | 51 +++- Forge/Statescript/Node.cs | 15 + Forge/Statescript/Nodes/StateNode.cs | 46 ++- 11 files changed, 673 insertions(+), 15 deletions(-) create mode 100644 Forge.Tests/Statescript/FixedUpdateTests.cs create mode 100644 Forge.Tests/Statescript/UpdateStampTests.cs diff --git a/Forge.Tests/Helpers/StatescriptTestHelpers.cs b/Forge.Tests/Helpers/StatescriptTestHelpers.cs index d2c69065..dd7611a1 100644 --- a/Forge.Tests/Helpers/StatescriptTestHelpers.cs +++ b/Forge.Tests/Helpers/StatescriptTestHelpers.cs @@ -285,6 +285,95 @@ private static AbilityData CreateAbilityData(string name, Func } } +/// +/// A state node that stays active and counts what it was called with, so a test can tell the frame update and the +/// fixed update apart by which counter moved. +/// +internal sealed class TrackingStateNode : StateNode +{ + public int ActivateCount { get; private set; } + + public int DeactivateCount { get; private set; } + + public int UpdateCount { get; private set; } + + public int FixedUpdateCount { get; private set; } + + public double LastUpdateDelta { get; private set; } + + public double LastFixedUpdateDelta { get; private set; } + + public ulong LastSeenUpdateStamp { get; private set; } + + protected override void OnActivate(GraphContext graphContext) + { + ActivateCount++; + } + + protected override void OnDeactivate(GraphContext graphContext) + { + DeactivateCount++; + } + + protected override void OnUpdate(double deltaTime, GraphContext graphContext) + { + UpdateCount++; + LastUpdateDelta = deltaTime; + LastSeenUpdateStamp = graphContext.UpdateStamp; + } + + protected override void OnFixedUpdate(double deltaTime, GraphContext graphContext) + { + FixedUpdateCount++; + LastFixedUpdateDelta = deltaTime; + LastSeenUpdateStamp = graphContext.UpdateStamp; + } +} + +/// +/// An ability behavior that overrides only the frame hook, so the fixed hook falls through to the interface default. +/// +internal sealed class FrameOnlyBehavior : IAbilityBehavior +{ + public int UpdateCount { get; private set; } + + public void OnStarted(AbilityBehaviorContext context) + { + } + + public void OnEnded(AbilityBehaviorContext context) + { + } + + public void OnUpdate(double deltaTime) + { + UpdateCount++; + } +} + +/// +/// A state node that deactivates another one from inside its own fixed update, for the case where the set of active +/// nodes changes while it is being walked. +/// +/// The state node to deactivate from the fixed update. +internal sealed class DeactivateOnFixedUpdateNode(TrackingStateNode target) : StateNode +{ + private readonly TrackingStateNode _target = target; + + protected override void OnActivate(GraphContext graphContext) + { + } + + protected override void OnDeactivate(GraphContext graphContext) + { + } + + protected override void OnFixedUpdate(double deltaTime, GraphContext graphContext) + { + _target.InputPorts[AbortPort].ReceiveMessage(graphContext); + } +} + internal sealed class TrackingActionNode(string? name = null, List? executionLog = null) : ActionNode { private readonly string? _name = name; diff --git a/Forge.Tests/Statescript/FixedUpdateTests.cs b/Forge.Tests/Statescript/FixedUpdateTests.cs new file mode 100644 index 00000000..7179877d --- /dev/null +++ b/Forge.Tests/Statescript/FixedUpdateTests.cs @@ -0,0 +1,284 @@ +// Copyright © Gamesmiths Guild. + +using FluentAssertions; +using Gamesmiths.Forge.Abilities; +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.Statescript; +using Gamesmiths.Forge.Statescript.Nodes; +using Gamesmiths.Forge.Statescript.Nodes.State; +using Gamesmiths.Forge.Tags; +using Gamesmiths.Forge.Tests.Helpers; + +using static Gamesmiths.Forge.Tests.Helpers.NodeBindings; + +namespace Gamesmiths.Forge.Tests.Statescript; + +public class FixedUpdateTests(TagsAndCuesFixture fixture) : IClassFixture +{ + private readonly TagsManager _tagsManager = fixture.TagsManager; + private readonly CuesManager _cuesManager = fixture.CuesManager; + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Frame_update_reaches_only_the_frame_hook() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.UpdateGraph(0.25); + + node.UpdateCount.Should().Be(1); + node.FixedUpdateCount.Should().Be(0); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Fixed_update_reaches_only_the_fixed_hook() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.FixedUpdateGraph(0.25); + + node.FixedUpdateCount.Should().Be(1); + node.UpdateCount.Should().Be(0); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Each_rail_carries_its_own_delta() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.UpdateGraph(0.008); + processor.FixedUpdateGraph(1.0 / 60.0); + + node.LastUpdateDelta.Should().Be(0.008); + node.LastFixedUpdateDelta.Should().Be(1.0 / 60.0); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Both_rails_can_drive_the_same_graph_in_one_tick() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.UpdateGraph(0.016); + processor.FixedUpdateGraph(0.016); + processor.UpdateGraph(0.016); + + node.UpdateCount.Should().Be(2); + node.FixedUpdateCount.Should().Be(1); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void A_timer_does_not_advance_on_fixed_updates() + { + var graph = new Graph(); + graph.VariableDefinitions.DefineVariable("duration", 1.0); + + TimerNode timer = CreateTimerNode("duration"); + graph.AddNode(timer); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + timer.InputPorts[StateNode.InputPort])); + + var processor = new GraphProcessor(graph); + processor.StartGraph(); + + processor.FixedUpdateGraph(5.0); + + processor.GraphContext.IsActive.Should().BeTrue( + "a timer counts wall-clock time and is not driven by the fixed step"); + + processor.UpdateGraph(1.0); + + processor.GraphContext.IsActive.Should().BeFalse(); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Fixed_update_skips_a_node_that_has_been_aborted() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + node.InputPorts[StateNode.AbortPort].ReceiveMessage(processor.GraphContext); + node.DeactivateCount.Should().Be(1); + + processor.FixedUpdateGraph(0.016); + + node.FixedUpdateCount.Should().Be(0); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Fixed_update_does_nothing_before_the_graph_starts() + { + var node = new TrackingStateNode(); + + var graph = new Graph(); + graph.AddNode(node); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + node.InputPorts[StateNode.InputPort])); + + var processor = new GraphProcessor(graph); + + processor.FixedUpdateGraph(0.016); + + node.ActivateCount.Should().Be(0); + node.FixedUpdateCount.Should().Be(0); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Fixed_update_does_nothing_after_the_graph_stops() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.StopGraph(); + processor.FixedUpdateGraph(0.016); + + node.FixedUpdateCount.Should().Be(0); + } + + [Fact] + [Trait("Graph", "FixedUpdate")] + public void Fixed_update_survives_a_node_deactivating_another_mid_walk() + { + var target = new TrackingStateNode(); + var deactivator = new DeactivateOnFixedUpdateNode(target); + + var graph = new Graph(); + graph.AddNode(deactivator); + graph.AddNode(target); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + deactivator.InputPorts[StateNode.InputPort])); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + target.InputPorts[StateNode.InputPort])); + + var processor = new GraphProcessor(graph); + processor.StartGraph(); + + target.ActivateCount.Should().Be(1); + + // The walk runs off a snapshot, so removing a node from the active set part way through it neither throws nor + // skips the rest of the set. + processor.Invoking(x => x.FixedUpdateGraph(0.016)).Should().NotThrow(); + + target.DeactivateCount.Should().Be(1); + + // Whether the target was reached before the abort is down to the order the active set happens to be in, which + // nothing promises. What is promised is that no walk after the abort reaches it. + int updatesBeforeAbort = target.FixedUpdateCount; + + processor.FixedUpdateGraph(0.016); + processor.FixedUpdateGraph(0.016); + + target.FixedUpdateCount.Should().Be(updatesBeforeAbort); + } + + [Fact] + [Trait("GraphBehavior", "FixedUpdate")] + public void Fixed_update_abilities_drives_the_graph_behind_an_ability() + { + var node = new TrackingStateNode(); + + var graph = new Graph(); + graph.AddNode(node); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + node.InputPorts[StateNode.InputPort])); + + var entity = new TestEntity(_tagsManager, _cuesManager); + var behavior = new GraphAbilityBehavior(graph); + + AbilityHandle handle = GrantAndActivate(entity, behavior); + handle.IsActive.Should().BeTrue(); + + entity.Abilities.FixedUpdateAbilities(1.0 / 60.0); + + node.FixedUpdateCount.Should().Be(1); + node.UpdateCount.Should().Be(0); + + entity.Abilities.UpdateAbilities(0.016); + + node.FixedUpdateCount.Should().Be(1); + node.UpdateCount.Should().Be(1); + } + + [Fact] + [Trait("GraphBehavior", "FixedUpdate")] + public void A_behavior_with_no_fixed_half_is_left_alone() + { + var entity = new TestEntity(_tagsManager, _cuesManager); + var behavior = new FrameOnlyBehavior(); + + AbilityHandle handle = GrantAndActivate(entity, behavior); + handle.Should().NotBeNull(); + + entity.Abilities.FixedUpdateAbilities(1.0 / 60.0); + + behavior.UpdateCount.Should().Be(0, "the default fixed hook does nothing"); + + entity.Abilities.UpdateAbilities(0.016); + + behavior.UpdateCount.Should().Be(1); + } + + private static GraphProcessor StartGraphWith(TrackingStateNode node) + { + var graph = new Graph(); + graph.AddNode(node); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + node.InputPorts[StateNode.InputPort])); + + var processor = new GraphProcessor(graph); + processor.StartGraph(); + + node.ActivateCount.Should().Be(1); + + return processor; + } + + private static AbilityHandle GrantAndActivate(TestEntity entity, IAbilityBehavior behavior) + { + var abilityData = new AbilityData("FixedGraph", behaviorFactory: () => behavior); + + var grantConfig = new GrantAbilityConfig( + abilityData, + new ScalableInt(1), + AbilityDeactivationPolicy.CancelImmediately, + AbilityDeactivationPolicy.CancelImmediately, + false, + false, + LevelComparison.Higher); + + var effectData = new EffectData( + "GrantFixedGraph", + new DurationData(DurationType.Infinite), + effectComponents: [new GrantAbilityEffectComponent([grantConfig])]); + + entity.EffectsManager.ApplyEffect(new Effect(effectData, new EffectOwnership(entity, entity))); + + AbilityHandle? handle = entity.Abilities.GrantedAbilities.First(); + handle.Should().NotBeNull(); + handle.TryActivate(out AbilityActivationFailures failureFlags).Should().BeTrue(); + failureFlags.Should().Be(AbilityActivationFailures.None); + + return handle; + } +} diff --git a/Forge.Tests/Statescript/UpdateStampTests.cs b/Forge.Tests/Statescript/UpdateStampTests.cs new file mode 100644 index 00000000..3745f938 --- /dev/null +++ b/Forge.Tests/Statescript/UpdateStampTests.cs @@ -0,0 +1,132 @@ +// Copyright © Gamesmiths Guild. + +using FluentAssertions; +using Gamesmiths.Forge.Statescript; +using Gamesmiths.Forge.Statescript.Nodes; +using Gamesmiths.Forge.Tests.Helpers; + +namespace Gamesmiths.Forge.Tests.Statescript; + +/// +/// Covers the update stamp: the monotonic counter a resolver keys on when it knows its own answer cannot change +/// within one pass. +/// +public class UpdateStampTests +{ + [Fact] + [Trait("Graph", "UpdateStamp")] + public void A_graph_that_has_not_been_updated_is_on_stamp_zero() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.GraphContext.UpdateStamp.Should().Be(0); + } + + [Fact] + [Trait("Graph", "UpdateStamp")] + public void A_frame_update_advances_the_stamp() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.UpdateGraph(0.016); + processor.GraphContext.UpdateStamp.Should().Be(1); + + processor.UpdateGraph(0.016); + processor.GraphContext.UpdateStamp.Should().Be(2); + } + + [Fact] + [Trait("Graph", "UpdateStamp")] + public void A_fixed_update_advances_the_stamp() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.FixedUpdateGraph(1.0 / 60.0); + processor.GraphContext.UpdateStamp.Should().Be(1); + + processor.FixedUpdateGraph(1.0 / 60.0); + processor.GraphContext.UpdateStamp.Should().Be(2); + } + + [Fact] + [Trait("Graph", "UpdateStamp")] + public void Both_rails_count_as_separate_passes() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.UpdateGraph(0.016); + processor.FixedUpdateGraph(1.0 / 60.0); + + processor.GraphContext.UpdateStamp.Should().Be( + 2, + "the frame and the fixed step are separate passes over separate world state"); + } + + [Fact] + [Trait("Graph", "UpdateStamp")] + public void A_node_sees_the_stamp_of_the_pass_it_is_running_in() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.UpdateGraph(0.016); + node.LastSeenUpdateStamp.Should().Be(1); + + processor.FixedUpdateGraph(1.0 / 60.0); + node.LastSeenUpdateStamp.Should().Be(2); + } + + [Fact] + [Trait("Graph", "UpdateStamp")] + public void The_stamp_does_not_move_before_the_graph_starts() + { + var node = new TrackingStateNode(); + + var graph = new Graph(); + graph.AddNode(node); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + node.InputPorts[StateNode.InputPort])); + + var processor = new GraphProcessor(graph); + + processor.UpdateGraph(0.016); + processor.FixedUpdateGraph(0.016); + + processor.GraphContext.UpdateStamp.Should().Be(0); + } + + [Fact] + [Trait("Graph", "UpdateStamp")] + public void The_stamp_does_not_move_after_the_graph_stops() + { + var node = new TrackingStateNode(); + GraphProcessor processor = StartGraphWith(node); + + processor.UpdateGraph(0.016); + processor.StopGraph(); + + processor.UpdateGraph(0.016); + processor.FixedUpdateGraph(0.016); + + processor.GraphContext.UpdateStamp.Should().Be(1); + } + + private static GraphProcessor StartGraphWith(TrackingStateNode node) + { + var graph = new Graph(); + graph.AddNode(node); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + node.InputPorts[StateNode.InputPort])); + + var processor = new GraphProcessor(graph); + processor.StartGraph(); + + return processor; + } +} diff --git a/Forge/Abilities/Ability.cs b/Forge/Abilities/Ability.cs index f515fce6..979b7bf1 100644 --- a/Forge/Abilities/Ability.cs +++ b/Forge/Abilities/Ability.cs @@ -339,6 +339,14 @@ internal void UpdateBehaviors(double deltaTime) } } + internal void FixedUpdateBehaviors(double deltaTime) + { + foreach (BehaviorBinding binding in _behaviors.Values) + { + binding.Behavior.OnFixedUpdate(deltaTime); + } + } + internal bool CanActivate(IForgeEntity? abilityTarget, out AbilityActivationFailures failureFlags) { bool canActivate = true; diff --git a/Forge/Abilities/IAbilityBehavior.cs b/Forge/Abilities/IAbilityBehavior.cs index d5211f13..8e22aacd 100644 --- a/Forge/Abilities/IAbilityBehavior.cs +++ b/Forge/Abilities/IAbilityBehavior.cs @@ -27,6 +27,19 @@ public interface IAbilityBehavior void OnUpdate(double deltaTime) { } + + /// + /// Called on the host's fixed step to advance the parts of the behavior that have to run at a rate agreed in + /// advance. The default implementation does nothing. + /// + /// + /// Separate from because a body moved at the frame rate moves a different amount per second + /// on every machine, while the fixed step does not vary. A host with no fixed step never calls this. + /// + /// The length of the fixed step, in seconds. + void OnFixedUpdate(double deltaTime) + { + } } /// diff --git a/Forge/Core/EntityAbilities.cs b/Forge/Core/EntityAbilities.cs index b4ab8553..3920ef8a 100644 --- a/Forge/Core/EntityAbilities.cs +++ b/Forge/Core/EntityAbilities.cs @@ -491,6 +491,24 @@ public void UpdateAbilities(double deltaTime) } } + /// + /// Advances the parts of every active ability behavior that have to run at a rate agreed in advance. Call this from + /// the game's fixed callback - a physics step, or a network tick - alongside + /// in its frame callback. + /// + /// + /// A game with no fixed step of its own never calls this, and the behaviors that need one stop running rather + /// than running at the frame rate. + /// + /// The length of the fixed step, in seconds. + public void FixedUpdateAbilities(double deltaTime) + { + foreach (AbilityHandle handle in GrantedAbilities) + { + handle.Ability?.FixedUpdateBehaviors(deltaTime); + } + } + internal AbilityHandle GrantAbility( AbilityData abilityData, int abilityLevel, diff --git a/Forge/Statescript/GraphAbilityBehavior.cs b/Forge/Statescript/GraphAbilityBehavior.cs index e82a288d..cfa77206 100644 --- a/Forge/Statescript/GraphAbilityBehavior.cs +++ b/Forge/Statescript/GraphAbilityBehavior.cs @@ -42,6 +42,12 @@ public void OnUpdate(double deltaTime) Processor.UpdateGraph(deltaTime); } + /// + public void OnFixedUpdate(double deltaTime) + { + Processor.FixedUpdateGraph(deltaTime); + } + /// /// Starts the graph processor, wiring up the callback to /// automatically end the ability instance when the graph finishes. diff --git a/Forge/Statescript/GraphContext.cs b/Forge/Statescript/GraphContext.cs index 3df4c1e5..1c66563c 100644 --- a/Forge/Statescript/GraphContext.cs +++ b/Forge/Statescript/GraphContext.cs @@ -45,6 +45,32 @@ public sealed class GraphContext /// public Variables GraphVariables { get; } = new Variables(); + /// + /// Gets the number of update passes this execution has run, counting frame and fixed updates alike. + /// + /// + /// A monotonic stamp for "the same pass as last time I looked", which is what a value worth computing once + /// per pass needs and what neither delta time nor a host frame counter gives: delta time says how long the pass + /// was rather than which one it is, and a host counter cannot distinguish a frame update from the fixed update + /// beside it. + /// It counts both rails deliberately, because they are separate passes over separate world state - a value + /// computed on the frame and reused on the fixed step would be reused across exactly the boundary the two hooks + /// exist to keep apart. One counter rather than one per rail for the same reason: a resolver cannot tell which + /// rail it is being evaluated on, since nodes also run in cascades that begin on either or on neither, so a + /// per-rail counter would be read unchanged across passes that are not the same pass. + /// This is a cache epoch, not a clock, and in particular it is not a network tick. It is local to one + /// graph execution, starts at zero every time a graph starts, and only ever counts up. A simulation tick shared + /// between peers is none of those things - it has to mean the same number on every machine and has to be settable, + /// because reconciliation rewinds it. A stamp that could go backwards would serve a value cached before the rewind + /// as though it were current. + /// It is also not a licence to memoize a property definition on. Every resolver bound to a node input + /// is defined as a property, and nodes run in cascades between passes - an event listener firing, an + /// ability activating - so a value held for a whole pass would be read after a Set Variable or a Set Position that + /// changed it. What this is for is a resolver that knows its own answer cannot change within a pass, and that is a + /// judgement only that resolver can make. + /// + public ulong UpdateStamp { get; internal set; } + internal Dictionary InternalNodeActivationStatus { get; } = []; internal HashSet ActiveStateNodes { get; } = []; diff --git a/Forge/Statescript/GraphProcessor.cs b/Forge/Statescript/GraphProcessor.cs index dee41088..f25f6dd9 100644 --- a/Forge/Statescript/GraphProcessor.cs +++ b/Forge/Statescript/GraphProcessor.cs @@ -81,20 +81,39 @@ public void StartGraph(Action? variableOverrides = null) /// The time elapsed since the last update, in seconds. public void UpdateGraph(double deltaTime) { - if (!GraphContext.HasStarted) + if (!BufferActiveNodes()) { return; } - _updateBuffer.Clear(); - _updateBuffer.AddRange(GraphContext.ActiveStateNodes); - for (int i = 0; i < _updateBuffer.Count; i++) { _updateBuffer[i].Update(deltaTime, GraphContext); } } + /// + /// Updates all active state nodes on the host's fixed step. Call this from the game's fixed callback - a physics + /// step, or a network tick - alongside in its frame callback. + /// + /// + /// A host with no fixed step of its own simply never calls this, and the nodes that need one stop running rather + /// than running at the frame rate. Nothing in the graph requires both to be driven. + /// + /// The length of the fixed step, in seconds. + public void FixedUpdateGraph(double deltaTime) + { + if (!BufferActiveNodes()) + { + return; + } + + for (int i = 0; i < _updateBuffer.Count; i++) + { + _updateBuffer[i].FixedUpdate(deltaTime, GraphContext); + } + } + /// /// Stops the execution of the graph. This method calls the entry node's stop method to halt the graph's processing /// and then removes all node contexts from the graph context to clean up any state associated with the graph's @@ -108,9 +127,9 @@ public void StopGraph() return; } - // Clear HasStarted first so the disable cascade is re-entrancy safe (e.g. an ExitNode triggering StopGraph, or a - // state node reaching FinalizeGraph) without nulling Processor yet. Keeping Processor set throughout the cascade - // lets action nodes on OnDeactivate paths still resolve property-backed inputs. + // Clear HasStarted first so the disable cascade is re-entrancy safe (e.g. an ExitNode triggering StopGraph, or + // a state node reaching FinalizeGraph) without nulling Processor yet. Keeping Processor set throughout the + // cascade lets action nodes on OnDeactivate paths still resolve property-backed inputs. GraphContext.HasStarted = false; Graph.EntryNode.StopGraph(GraphContext); GraphContext.Processor = null; @@ -139,4 +158,22 @@ internal void FinalizeGraph() GraphContext.RemoveAllNodeContext(); OnGraphCompleted?.Invoke(); } + + // Snapshots the active nodes before either update walks them, because a node updated part way through can + // deactivate itself or another and modify the set being walked. Stamping here rather than in each caller is what + // makes the frame and fixed updates count as the separate passes they are. + private bool BufferActiveNodes() + { + if (!GraphContext.HasStarted) + { + return false; + } + + GraphContext.UpdateStamp++; + + _updateBuffer.Clear(); + _updateBuffer.AddRange(GraphContext.ActiveStateNodes); + + return true; + } } diff --git a/Forge/Statescript/Node.cs b/Forge/Statescript/Node.cs index d2ed0b5d..3c06e465 100644 --- a/Forge/Statescript/Node.cs +++ b/Forge/Statescript/Node.cs @@ -221,6 +221,21 @@ internal virtual void Update(double deltaTime, GraphContext graphContext) { } + /// + /// Updates this node on the host's fixed step. The default implementation does nothing. + /// + /// + /// Separate from because the two run at different rates and answer different questions: the + /// frame rate is whatever the machine manages, while the fixed rate is agreed in advance and is the only rate at + /// which repeating the same work twice gives the same answer twice. Physics is the usual reason a host has a fixed + /// step; a networked simulation, where every peer must reproduce the same steps, is the other. + /// + /// The length of the fixed step, in seconds. + /// The graph context. + internal virtual void FixedUpdate(double deltaTime, GraphContext graphContext) + { + } + /// /// Creates a port of the specified type with the given index. /// diff --git a/Forge/Statescript/Nodes/StateNode.cs b/Forge/Statescript/Nodes/StateNode.cs index 1c7a1153..bb139544 100644 --- a/Forge/Statescript/Nodes/StateNode.cs +++ b/Forge/Statescript/Nodes/StateNode.cs @@ -69,19 +69,23 @@ public abstract class StateNode : Node internal override void Update(double deltaTime, GraphContext graphContext) #pragma warning restore SA1202 // Elements should be ordered by access { - if (!graphContext.HasNodeContext(NodeID)) + if (IsNodeActive(graphContext)) { - return; + OnUpdate(deltaTime, graphContext); } + } - StateNodeContext nodeContext = graphContext.GetNodeContext(NodeID); - - if (!nodeContext.Active) + /// + /// Updates this state node on the host's fixed step. Only processes the update if the node is currently active. + /// + /// The length of the fixed step, in seconds. + /// The graph's context. + internal override void FixedUpdate(double deltaTime, GraphContext graphContext) + { + if (IsNodeActive(graphContext)) { - return; + OnFixedUpdate(deltaTime, graphContext); } - - OnUpdate(deltaTime, graphContext); } /// @@ -131,6 +135,32 @@ protected virtual void OnUpdate(double deltaTime, GraphContext graphContext) { } + /// + /// Called on every fixed step while the node is active. Override this instead of for logic + /// that has to advance at a rate agreed in advance rather than at whatever rate the machine renders: moving a + /// body, steering a character, asking the physics world a question, or anything a networked peer has to be able to + /// reproduce step for step. + /// + /// + /// Both hooks exist because the two rates are different and neither substitutes for the other. The frame + /// rate is whatever the machine manages and can drift far above or below the fixed rate, so a body driven from + /// is pushed a different amount per second on a fast machine than on a slow one, and a + /// physics query asked from there is asked several times about a world that has not changed, or not at all in the + /// step where it did. + /// The name says fixed rather than physics because the interval is the guarantee and + /// physics is only the most common reason to want one: a dedicated server with physics switched off still runs + /// this rail, and a networked simulation drives it from its own clock rather than from the engine's. + /// A host that never drives the fixed step simply never calls this, and a node that overrides it stops + /// running rather than running at the wrong rate - which is the honest failure, since silently falling back to + /// the frame would reintroduce exactly what overriding this avoids. Timers, animations and anything counting + /// wall-clock time belong in , whose delta is the one the player experiences. + /// + /// The length of the fixed step, in seconds. + /// The graph's context. + protected virtual void OnFixedUpdate(double deltaTime, GraphContext graphContext) + { + } + /// /// Called once the node has finished activating, after , after /// and have been emitted, and after any messages deferred From ef4151f187e073f51f359c82c65aaba9e3eaa75b Mon Sep 17 00:00:00 2001 From: Lex Date: Sun, 6 Sep 2026 00:34:44 -0300 Subject: [PATCH 2/6] docs(statescript): document the two update rails Adds a Two update rails section to the state node guide, the fixed hooks to the ability integration and overview pages, and a caching section to the resolver guide that says what UpdateStamp is for and why it is not a network tick. --- docs/abilities.md | 2 +- docs/statescript/README.md | 4 ++-- docs/statescript/ability-integration.md | 16 ++++++++++++--- docs/statescript/custom-resolvers.md | 27 +++++++++++++++++++++++++ docs/statescript/nodes/state/README.md | 17 +++++++++++++++- 5 files changed, 59 insertions(+), 7 deletions(-) diff --git a/docs/abilities.md b/docs/abilities.md index 916689cd..bf681b37 100644 --- a/docs/abilities.md +++ b/docs/abilities.md @@ -1050,7 +1050,7 @@ entity.Events.Raise(new EventData Abilities can be driven by Statescript graphs instead of handwritten `IAbilityBehavior` classes. This is done through `GraphAbilityBehavior`, which connects the ability lifecycle to a graph's execution: - When the ability **starts**, the graph begins processing from its Entry node. -- Each frame, `OnUpdate(deltaTime)` advances all active state nodes in the graph. +- Each frame, `OnUpdate(deltaTime)` advances all active state nodes in the graph; each fixed step, `OnFixedUpdate(deltaTime)` advances the ones on that rail. - When the graph **completes** (all state nodes deactivate) or an Exit node is reached, the ability instance ends. - When the ability is **canceled**, the graph is stopped and all active nodes are disabled. diff --git a/docs/statescript/README.md b/docs/statescript/README.md index 05ff3748..e6683506 100644 --- a/docs/statescript/README.md +++ b/docs/statescript/README.md @@ -25,7 +25,7 @@ When the graph starts: 4. **Condition nodes** evaluate and route the message to the True or False output. 5. **State nodes** activate when they receive a message and remain active over time. -Once all synchronous propagation is complete, only **state nodes** remain active. These nodes are updated each frame via `GraphProcessor.UpdateGraph(deltaTime)`. When a state node deactivates (e.g., a timer expires), it may emit messages that trigger further actions, conditions, or other state nodes. +Once all synchronous propagation is complete, only **state nodes** remain active. These nodes are updated each frame via `GraphProcessor.UpdateGraph(deltaTime)`, and on each fixed step via `GraphProcessor.FixedUpdateGraph(deltaTime)`. When a state node deactivates (e.g., a timer expires), it may emit messages that trigger further actions, conditions, or other state nodes. **The graph completes when no state nodes remain active.** @@ -88,7 +88,7 @@ When a filter needs three states rather than two, use a separate explicit flag i Statescript integrates with the Abilities system through `GraphAbilityBehavior`: 1. When the ability **activates**, the graph starts processing. -2. Each frame, `OnUpdate(deltaTime)` drives `GraphProcessor.UpdateGraph(deltaTime)`. +2. Each frame, `OnUpdate(deltaTime)` drives `GraphProcessor.UpdateGraph(deltaTime)`; each fixed step, `OnFixedUpdate(deltaTime)` drives `GraphProcessor.FixedUpdateGraph(deltaTime)`. 3. When the graph **completes** or an **Exit node** is reached, the ability instance ends. 4. If the ability is **canceled**, the graph is stopped and all active nodes are disabled. diff --git a/docs/statescript/ability-integration.md b/docs/statescript/ability-integration.md index 7a86f3c9..1451ef16 100644 --- a/docs/statescript/ability-integration.md +++ b/docs/statescript/ability-integration.md @@ -9,7 +9,7 @@ For an overview of the Statescript system, see the [Statescript overview](README `GraphAbilityBehavior` bridges the ability lifecycle and the graph processor: - When the ability **starts** → the graph begins processing from its Entry node. -- Each frame → `OnUpdate(deltaTime)` advances all active state nodes. +- Each frame → `OnUpdate(deltaTime)` advances all active state nodes; each fixed step → `OnFixedUpdate(deltaTime)` advances the ones on that rail. - When the graph **completes** (all state nodes deactivate) or an Exit node is reached → the ability instance ends automatically. - When the ability is **canceled** → the graph is stopped and all active nodes are disabled. @@ -51,15 +51,25 @@ When the ability is canceled: ### The Update Loop -`GraphAbilityBehavior` implements `IAbilityBehavior.OnUpdate(deltaTime)`, which calls `GraphProcessor.UpdateGraph(deltaTime)` to tick all active state nodes. The Abilities system calls this automatically each frame for active ability instances. +`GraphAbilityBehavior` implements both update hooks. `IAbilityBehavior.OnUpdate(deltaTime)` calls `GraphProcessor.UpdateGraph(deltaTime)`, and `IAbilityBehavior.OnFixedUpdate(deltaTime)` calls `GraphProcessor.FixedUpdateGraph(deltaTime)`. Each advances the state nodes that override the matching hook — see [Two update rails](nodes/state/README.md#two-update-rails). ```csharp // In your game loop, update the entity's abilities // (This is handled by whatever drives your abilities, typically alongside EffectsManager) behavior.Processor.UpdateGraph(deltaTime); + +// And from the fixed callback, if your host has one +behavior.Processor.FixedUpdateGraph(fixedDeltaTime); +``` + +If you're using `GraphAbilityBehavior` through the standard Abilities system, drive the entity instead and both rails reach the graph: + +```csharp +entity.Abilities.UpdateAbilities(deltaTime); // from the frame callback +entity.Abilities.FixedUpdateAbilities(fixedDeltaTime); // from the fixed callback ``` -If you're using `GraphAbilityBehavior` through the standard Abilities system, the update is called automatically by the ability instance. +**Drive whichever rails your host has.** A turn-based game may call only `UpdateAbilities`; a host with no fixed step never calls `FixedUpdateAbilities`, and nodes on that rail simply do not run. Effects have no fixed half — `EffectsManager.UpdateEffects` counts wall-clock time, so durations and periods belong on the frame rail regardless. ## Typed Activation Data diff --git a/docs/statescript/custom-resolvers.md b/docs/statescript/custom-resolvers.md index c79c0186..c149785b 100644 --- a/docs/statescript/custom-resolvers.md +++ b/docs/statescript/custom-resolvers.md @@ -384,3 +384,30 @@ When a node reads a named value through `GraphContext.TryResolve()`: 2. **Property definitions** (resolvers) are checked as a fallback (read-only, computed values). This means a graph variable with the same name as a property definition will shadow the resolver. This can be useful for overriding a computed value with a fixed one during specific graph executions. + +## Caching an Expensive Resolver + +`Resolve` runs every time a node reads the bound property, which for a resolver bound inside an array lambda means once per element. If a resolver is expensive — a spatial query, a scan of the scene — and you know its answer cannot change within a single update pass, you can cache it against `GraphContext.UpdateStamp`: + +```csharp +private ulong _stamp; +private Variant128 _cached; + +public Variant128 Resolve(GraphContext graphContext) +{ + if (graphContext.UpdateStamp == _stamp) + { + return _cached; + } + + _stamp = graphContext.UpdateStamp; + _cached = /* the expensive computation */; + return _cached; +} +``` + +`UpdateStamp` is a monotonic counter of update passes, advanced by the processor on both the frame and fixed rails so the two never share a value. + +**Only the resolver can decide whether this is safe, and for most resolvers it is not.** Nodes also run in cascades *between* passes — an event listener firing, an ability activating, one node's message reaching the next — so a value held for a whole pass can be read after a Set Variable or a Set Position that changed what it was computed from. Anything reading graph variables, entity state, or world transforms is a poor candidate. This is why nothing in the framework caches property definitions on your behalf. + +`UpdateStamp` is **not a clock and not a network tick**. It is local to one graph execution, restarts at zero each time the graph starts, and only counts up — a shared simulation tick has to mean the same number on every peer and has to be settable, because reconciliation rewinds it. Read a network tick from your networking layer, never from here. diff --git a/docs/statescript/nodes/state/README.md b/docs/statescript/nodes/state/README.md index 03e054f5..48c85490 100644 --- a/docs/statescript/nodes/state/README.md +++ b/docs/statescript/nodes/state/README.md @@ -23,12 +23,27 @@ State nodes **persist over time**. They activate when receiving a message, remai 1. Message on **Input** → node activates → `OnActivate()` is called. 2. **OnActivate** and **Subgraph** ports emit regular messages. -3. Each frame, `OnUpdate(deltaTime)` is called by the graph processor. +3. Each frame, `OnUpdate(deltaTime)` is called by the graph processor. On each fixed step, `OnFixedUpdate(deltaTime)` is called instead — see [Two update rails](#two-update-rails). 4. When internal logic completes → `OnDeactivate` emits, Subgraph ports send disable signals. 5. If **Abort** receives a message → `OnAbort` emits, then node deactivates normally. **Deferred actions:** If activation logic triggers immediate deactivation (e.g., a timer with duration 0), the deactivation is **deferred** until activation completes. This guarantees that OnActivate and Subgraph ports fire before any deactivation processing begins. +## Two update rails + +A state node can be advanced on either of two independent rails, and it should override exactly one of them. + +| Hook | Driven by | Delta | Use it for | +|---|---|---|---| +| `OnUpdate(deltaTime, graphContext)` | `GraphProcessor.UpdateGraph` | The frame's elapsed time | Timers, animations, input, anything counting wall-clock time | +| `OnFixedUpdate(deltaTime, graphContext)` | `GraphProcessor.FixedUpdateGraph` | The fixed step's length | Moving a body, steering a character, querying the physics world, anything a networked peer must reproduce step for step | + +The two rates differ and neither substitutes for the other. The frame rate is whatever the machine manages and drifts far above or below the fixed rate, so a body driven from `OnUpdate` is pushed a different amount per second on a fast machine than on a slow one, and a query asked from there is asked several times about a world that has not changed, or not at all in the step where it did. + +The hook is called **fixed** rather than **physics** because the interval is the guarantee and physics is only the most common reason to want one. A dedicated server with physics switched off still runs this rail, and a networked simulation drives it from its own clock rather than from the engine's. + +**A host drives whichever rails it has.** Godot calls `UpdateGraph` from `_Process` and `FixedUpdateGraph` from `_PhysicsProcess`; a turn-based game may call only the first. A host with no fixed step never calls `FixedUpdateGraph`, and nodes that override `OnFixedUpdate` simply do not run — which is the honest failure, since falling back to the frame would reintroduce exactly what overriding it avoids. + ## Creating Custom State Nodes Extend `StateNode` where `T` is a context class inheriting from `StateNodeContext`: From 3fa86c933488251911cd6e34ac59526617e16332 Mon Sep 17 00:00:00 2001 From: Lex Date: Sun, 6 Sep 2026 01:13:32 -0300 Subject: [PATCH 3/6] fix(abilities): snapshot before dispatching fixed updates Review of #66: a behavior granting an ability from its fixed update threw, because adding invalidates the open enumerator. Both fixed loops now walk a reused buffer and re-check membership, so a callback may grant, clear or end from inside one. Also corrects the update stamp docs, which claimed a reset that does not happen. --- Forge.Tests/Helpers/StatescriptTestHelpers.cs | 136 ++++++++++++++++++ Forge.Tests/Statescript/FixedUpdateTests.cs | 52 +++++++ Forge/Abilities/Ability.cs | 24 +++- Forge/Core/EntityAbilities.cs | 32 ++++- Forge/Statescript/GraphContext.cs | 18 ++- docs/statescript/custom-resolvers.md | 14 +- 6 files changed, 260 insertions(+), 16 deletions(-) diff --git a/Forge.Tests/Helpers/StatescriptTestHelpers.cs b/Forge.Tests/Helpers/StatescriptTestHelpers.cs index dd7611a1..ec6748d1 100644 --- a/Forge.Tests/Helpers/StatescriptTestHelpers.cs +++ b/Forge.Tests/Helpers/StatescriptTestHelpers.cs @@ -351,6 +351,142 @@ public void OnUpdate(double deltaTime) } } +/// +/// An ability behavior that ends its own instance from inside an update, which is what a graph completing on that +/// rail does. Ending removes the behavior from the dictionary the dispatch loop is walking. +/// +/// Whether to end from the fixed hook rather than the frame hook. +internal sealed class SelfEndingBehavior(bool endOnFixedUpdate) : IAbilityBehavior +{ + private readonly bool _endOnFixedUpdate = endOnFixedUpdate; + + private AbilityBehaviorContext? _context; + + public void OnStarted(AbilityBehaviorContext context) + { + _context = context; + } + + public void OnEnded(AbilityBehaviorContext context) + { + } + + public void OnUpdate(double deltaTime) + { + if (!_endOnFixedUpdate) + { + _context?.InstanceHandle.End(); + } + } + + public void OnFixedUpdate(double deltaTime) + { + if (_endOnFixedUpdate) + { + _context?.InstanceHandle.End(); + } + } +} + +/// +/// An ability behavior that clears its own ability from inside an update, removing the handle from the granted set the +/// entity-level dispatch loop is walking. +/// +/// The entity whose ability is cleared. +/// Whether to clear from the fixed hook rather than the frame hook. +internal sealed class SelfClearingBehavior(TestEntity entity, bool clearOnFixedUpdate) : IAbilityBehavior +{ + private readonly TestEntity _entity = entity; + private readonly bool _clearOnFixedUpdate = clearOnFixedUpdate; + + private AbilityBehaviorContext? _context; + + public void OnStarted(AbilityBehaviorContext context) + { + _context = context; + } + + public void OnEnded(AbilityBehaviorContext context) + { + } + + public void OnUpdate(double deltaTime) + { + if (!_clearOnFixedUpdate) + { + Clear(); + } + } + + public void OnFixedUpdate(double deltaTime) + { + if (_clearOnFixedUpdate) + { + Clear(); + } + } + + private void Clear() + { + if (_context is not null) + { + _entity.Abilities.ClearAbility(_context.AbilityHandle); + } + } +} + +/// +/// An ability behavior that grants a second ability from inside an update, adding to the granted set the entity-level +/// dispatch loop is walking. Adding invalidates an enumerator where removing no longer does. +/// +/// The entity the new ability is granted to. +/// The ability to grant. +/// Whether to grant from the fixed hook rather than the frame hook. +internal sealed class GrantingBehavior(TestEntity entity, AbilityData granted, bool grantOnFixedUpdate) + : IAbilityBehavior +{ + private readonly TestEntity _entity = entity; + private readonly AbilityData _granted = granted; + private readonly bool _grantOnFixedUpdate = grantOnFixedUpdate; + + private bool _done; + + public void OnStarted(AbilityBehaviorContext context) + { + } + + public void OnEnded(AbilityBehaviorContext context) + { + } + + public void OnUpdate(double deltaTime) + { + if (!_grantOnFixedUpdate) + { + Grant(); + } + } + + public void OnFixedUpdate(double deltaTime) + { + if (_grantOnFixedUpdate) + { + Grant(); + } + } + + private void Grant() + { + if (_done) + { + return; + } + + _done = true; + _entity.Abilities.GrantAbilityPermanently(_granted, 1, LevelComparison.Higher, null); + } +} + /// /// A state node that deactivates another one from inside its own fixed update, for the case where the set of active /// nodes changes while it is being walked. diff --git a/Forge.Tests/Statescript/FixedUpdateTests.cs b/Forge.Tests/Statescript/FixedUpdateTests.cs index 7179877d..b01f568b 100644 --- a/Forge.Tests/Statescript/FixedUpdateTests.cs +++ b/Forge.Tests/Statescript/FixedUpdateTests.cs @@ -238,6 +238,58 @@ public void A_behavior_with_no_fixed_half_is_left_alone() behavior.UpdateCount.Should().Be(1); } + [Fact] + [Trait("GraphBehavior", "FixedUpdate")] + public void A_behavior_may_end_its_own_instance_from_a_fixed_update() + { + var entity = new TestEntity(_tagsManager, _cuesManager); + var behavior = new SelfEndingBehavior(endOnFixedUpdate: true); + + AbilityHandle handle = GrantAndActivate(entity, behavior); + handle.IsActive.Should().BeTrue(); + + // Ending an instance removes the behavior from the dictionary being walked. A graph completing on the fixed + // step does exactly this, so it is the ordinary case rather than an exotic one. + entity.Abilities.Invoking(x => x.FixedUpdateAbilities(1.0 / 60.0)).Should().NotThrow(); + + handle.IsActive.Should().BeFalse(); + } + + [Fact] + [Trait("GraphBehavior", "FixedUpdate")] + public void A_behavior_may_clear_its_own_ability_from_a_fixed_update() + { + var entity = new TestEntity(_tagsManager, _cuesManager); + var behavior = new SelfClearingBehavior(entity, clearOnFixedUpdate: true); + + AbilityHandle handle = GrantAndActivate(entity, behavior); + handle.IsActive.Should().BeTrue(); + entity.Abilities.GrantedAbilities.Should().ContainSingle(); + + // Clearing removes the handle from the granted set being walked. + entity.Abilities.Invoking(x => x.FixedUpdateAbilities(1.0 / 60.0)).Should().NotThrow(); + + entity.Abilities.GrantedAbilities.Should().BeEmpty("the clear has to actually remove, or this proves nothing"); + } + + [Fact] + [Trait("GraphBehavior", "FixedUpdate")] + public void A_behavior_may_grant_another_ability_from_a_fixed_update() + { + var entity = new TestEntity(_tagsManager, _cuesManager); + var granted = new AbilityData("GrantedDuringFixedUpdate"); + var behavior = new GrantingBehavior(entity, granted, grantOnFixedUpdate: true); + + AbilityHandle handle = GrantAndActivate(entity, behavior); + handle.IsActive.Should().BeTrue(); + + // Adding to the granted set is what still invalidates an enumerator - a graph node granting an ability is the + // ordinary way to reach it. + entity.Abilities.Invoking(x => x.FixedUpdateAbilities(1.0 / 60.0)).Should().NotThrow(); + + entity.Abilities.GrantedAbilities.Should().HaveCount(2); + } + private static GraphProcessor StartGraphWith(TrackingStateNode node) { var graph = new Graph(); diff --git a/Forge/Abilities/Ability.cs b/Forge/Abilities/Ability.cs index 979b7bf1..bf1f994b 100644 --- a/Forge/Abilities/Ability.cs +++ b/Forge/Abilities/Ability.cs @@ -41,6 +41,10 @@ private record struct BehaviorBinding(IAbilityBehavior Behavior, AbilityBehavior private readonly Dictionary _behaviors = []; + // Reused by both update rails so the per-frame walk allocates nothing. They never overlap: a host drives one and + // then the other, and neither re-enters itself. + private readonly List _behaviorBuffer = []; + private readonly Action? _tagChangedHandler; private readonly EventSubscriptionToken? _eventSubscriptionToken; @@ -341,9 +345,16 @@ internal void UpdateBehaviors(double deltaTime) internal void FixedUpdateBehaviors(double deltaTime) { - foreach (BehaviorBinding binding in _behaviors.Values) + BufferBehaviorInstances(); + + for (int i = 0; i < _behaviorBuffer.Count; i++) { - binding.Behavior.OnFixedUpdate(deltaTime); + // Looked up rather than snapshotted, so an instance ended earlier in this same walk - by itself or by + // another behavior - is skipped instead of being advanced after it finished. + if (_behaviors.TryGetValue(_behaviorBuffer[i], out BehaviorBinding binding)) + { + binding.Behavior.OnFixedUpdate(deltaTime); + } } } @@ -592,6 +603,15 @@ private EventSubscriptionToken SubscribeTypedEventCore(Tag tag, int pr return activeEffect ?? Owner.EffectsManager.FindActiveEffectByData(_cooldownEffects![index].EffectData); } + // Snapshots the instances before an update walks their behaviors. A behavior may start or end an instance from + // inside its own update - a graph completing on this rail ends its own - and starting one adds to the dictionary, + // which invalidates any enumerator open over it. + private void BufferBehaviorInstances() + { + _behaviorBuffer.Clear(); + _behaviorBuffer.AddRange(_behaviors.Keys); + } + private bool CanCommitCooldown() { if (_cooldownEffects is null) diff --git a/Forge/Core/EntityAbilities.cs b/Forge/Core/EntityAbilities.cs index 3920ef8a..a393e3bc 100644 --- a/Forge/Core/EntityAbilities.cs +++ b/Forge/Core/EntityAbilities.cs @@ -17,6 +17,11 @@ public class EntityAbilities(IForgeEntity owner) { private readonly Dictionary> _grantSources = []; private readonly HashSet _grantedAbilities = []; + + // Reused by both update rails so the per-frame walk allocates nothing. They never overlap: a host drives one and + // then the other, and neither re-enters itself. + private readonly List _updateBuffer = []; + private Action? _removeAbility; private Action? _inhibitAbility; @@ -87,8 +92,9 @@ public class EntityAbilities(IForgeEntity owner) /// Read-only: the manager keeps this set in step with the grant sources behind each ability, so grant and removal /// go through , TryGrantAbilityAndActivateOnce, /// , and the effect components rather than through this set. - /// The collection is live, so a handle removed while it is being enumerated invalidates the enumeration; copy it - /// first when the loop body can remove abilities. + /// The collection is live, so copy it first when the loop body can grant or clear abilities. Granting is the case + /// that throws: adding invalidates any open enumerator, while removing from a no longer + /// does. Both are worth copying for, since a loop that skips whatever a removal moved is wrong just as quietly. /// public IReadOnlyCollection GrantedAbilities => _grantedAbilities; @@ -503,9 +509,18 @@ public void UpdateAbilities(double deltaTime) /// The length of the fixed step, in seconds. public void FixedUpdateAbilities(double deltaTime) { - foreach (AbilityHandle handle in GrantedAbilities) + BufferGrantedAbilities(); + + for (int i = 0; i < _updateBuffer.Count; i++) { - handle.Ability?.FixedUpdateBehaviors(deltaTime); + AbilityHandle handle = _updateBuffer[i]; + + // Re-checked because an earlier behavior in this same walk may have cleared this ability, and an ability + // that is no longer granted should not be advanced. + if (_grantedAbilities.Contains(handle)) + { + handle.Ability?.FixedUpdateBehaviors(deltaTime); + } } } @@ -610,6 +625,15 @@ private static bool MatchesTags(Ability ability, TagContainer tagsToActivate) // Snapshots the granted abilities and seeds the per-ability failure flags for a tag-driven activation. The snapshot // keeps the indices stable while activations grant or remove other abilities. + // Snapshots the granted set before an update walks it. A behavior is free to grant, revoke or clear abilities from + // inside its own update - a graph node granting one is the ordinary way to reach it - and adding to the set + // invalidates any enumerator open over it. + private void BufferGrantedAbilities() + { + _updateBuffer.Clear(); + _updateBuffer.AddRange(_grantedAbilities); + } + private bool TryBeginActivationByTag( TagContainer tagsToActivate, out AbilityHandle[] handles, diff --git a/Forge/Statescript/GraphContext.cs b/Forge/Statescript/GraphContext.cs index 1c66563c..f168714d 100644 --- a/Forge/Statescript/GraphContext.cs +++ b/Forge/Statescript/GraphContext.cs @@ -58,11 +58,19 @@ public sealed class GraphContext /// exist to keep apart. One counter rather than one per rail for the same reason: a resolver cannot tell which /// rail it is being evaluated on, since nodes also run in cascades that begin on either or on neither, so a /// per-rail counter would be read unchanged across passes that are not the same pass. - /// This is a cache epoch, not a clock, and in particular it is not a network tick. It is local to one - /// graph execution, starts at zero every time a graph starts, and only ever counts up. A simulation tick shared - /// between peers is none of those things - it has to mean the same number on every machine and has to be settable, - /// because reconciliation rewinds it. A stamp that could go backwards would serve a value cached before the rewind - /// as though it were current. + /// It counts up for the whole life of this context and is never reset, including when a reusable + /// is started again - the processor keeps its context, so the count carries across + /// restarts. That is deliberate rather than an oversight: resetting to zero would let a resolver holding a cache + /// entry from the previous execution match a stamp from the new one and serve a value computed before the + /// restart. + /// It is a cache epoch, not a clock, and in particular it is not a network tick. It is local to one + /// context and only ever counts up. A simulation tick shared between peers is neither - it has to mean the same + /// number on every machine and has to be settable, because reconciliation rewinds it. A stamp that could go + /// backwards would serve a value cached before the rewind as though it were current, which is the same reason it + /// is not reset. + /// Because the counter is per context and resolver instances belong to the shared , a + /// resolver caching against it must key on the context as well: two processors over one graph reach the same + /// numeric stamp at different moments. /// It is also not a licence to memoize a property definition on. Every resolver bound to a node input /// is defined as a property, and nodes run in cascades between passes - an event listener firing, an /// ability activating - so a value held for a whole pass would be read after a Set Variable or a Set Position that diff --git a/docs/statescript/custom-resolvers.md b/docs/statescript/custom-resolvers.md index c149785b..424abe00 100644 --- a/docs/statescript/custom-resolvers.md +++ b/docs/statescript/custom-resolvers.md @@ -390,17 +390,19 @@ This means a graph variable with the same name as a property definition will sha `Resolve` runs every time a node reads the bound property, which for a resolver bound inside an array lambda means once per element. If a resolver is expensive — a spatial query, a scan of the scene — and you know its answer cannot change within a single update pass, you can cache it against `GraphContext.UpdateStamp`: ```csharp -private ulong _stamp; +private GraphContext? _cachedContext; +private ulong _cachedStamp; private Variant128 _cached; public Variant128 Resolve(GraphContext graphContext) { - if (graphContext.UpdateStamp == _stamp) + if (ReferenceEquals(_cachedContext, graphContext) && _cachedStamp == graphContext.UpdateStamp) { return _cached; } - _stamp = graphContext.UpdateStamp; + _cachedContext = graphContext; + _cachedStamp = graphContext.UpdateStamp; _cached = /* the expensive computation */; return _cached; } @@ -408,6 +410,8 @@ public Variant128 Resolve(GraphContext graphContext) `UpdateStamp` is a monotonic counter of update passes, advanced by the processor on both the frame and fixed rails so the two never share a value. -**Only the resolver can decide whether this is safe, and for most resolvers it is not.** Nodes also run in cascades *between* passes — an event listener firing, an ability activating, one node's message reaching the next — so a value held for a whole pass can be read after a Set Variable or a Set Position that changed what it was computed from. Anything reading graph variables, entity state, or world transforms is a poor candidate. This is why nothing in the framework caches property definitions on your behalf. +**The context is part of the key, and leaving it out is the mistake that looks like it works.** Resolver instances belong to the shared `Graph`, not to one execution of it, so every processor running that graph calls the *same* resolver object with its *own* context — and two contexts reach the same numeric stamp at different moments. Keyed on the stamp alone, one entity's answer is served to another. The context check is also what makes the first call compute: a stamp-only cache starts with `_cachedStamp` and `UpdateStamp` both at zero, so the very first resolve — during `StartGraph`, before any update pass has run — returns an uncomputed value. -`UpdateStamp` is **not a clock and not a network tick**. It is local to one graph execution, restarts at zero each time the graph starts, and only counts up — a shared simulation tick has to mean the same number on every peer and has to be settable, because reconciliation rewinds it. Read a network tick from your networking layer, never from here. +**Only the resolver can decide whether this is safe at all, and for most resolvers it is not.** Nodes also run in cascades *between* passes — an event listener firing, an ability activating, one node's message reaching the next — so a value held for a whole pass can be read after a Set Variable or a Set Position that changed what it was computed from. Anything reading graph variables, entity state, or world transforms is a poor candidate. This is why nothing in the framework caches property definitions on your behalf. + +`UpdateStamp` is **not a clock and not a network tick**. It is local to one `GraphContext`, counts up for that context's whole life and is never reset — not even when a reusable `GraphProcessor` is started again, since resetting would let a cache entry from the previous execution match a stamp from the new one. A shared simulation tick is the opposite on both counts: it has to mean the same number on every peer, and it has to be settable, because reconciliation rewinds it. Read a network tick from your networking layer, never from here. From c3bd130073c19f804200e277b3e2cf4b3e5bba09 Mon Sep 17 00:00:00 2001 From: Lex Date: Sun, 6 Sep 2026 01:15:01 -0300 Subject: [PATCH 4/6] fix(abilities): snapshot before dispatching frame updates too Pre-existing, and the same bug the fixed rail just had: a behavior granting an ability from OnUpdate threw on the open enumerator. Verified against the unfixed loop before fixing. --- Forge.Tests/Statescript/FixedUpdateTests.cs | 32 +++++++++++++++++++++ Forge/Abilities/Ability.cs | 9 ++++-- Forge/Core/EntityAbilities.cs | 11 +++++-- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/Forge.Tests/Statescript/FixedUpdateTests.cs b/Forge.Tests/Statescript/FixedUpdateTests.cs index b01f568b..448811ee 100644 --- a/Forge.Tests/Statescript/FixedUpdateTests.cs +++ b/Forge.Tests/Statescript/FixedUpdateTests.cs @@ -290,6 +290,38 @@ public void A_behavior_may_grant_another_ability_from_a_fixed_update() entity.Abilities.GrantedAbilities.Should().HaveCount(2); } + [Fact] + [Trait("GraphBehavior", "Update")] + public void A_behavior_may_grant_another_ability_from_a_frame_update() + { + var entity = new TestEntity(_tagsManager, _cuesManager); + var granted = new AbilityData("GrantedDuringFrameUpdate"); + var behavior = new GrantingBehavior(entity, granted, grantOnFixedUpdate: false); + + AbilityHandle handle = GrantAndActivate(entity, behavior); + handle.IsActive.Should().BeTrue(); + + // The frame rail has always had the same hazard; it was simply never covered. + entity.Abilities.Invoking(x => x.UpdateAbilities(0.016)).Should().NotThrow(); + + entity.Abilities.GrantedAbilities.Should().HaveCount(2); + } + + [Fact] + [Trait("GraphBehavior", "Update")] + public void A_behavior_may_end_its_own_instance_from_a_frame_update() + { + var entity = new TestEntity(_tagsManager, _cuesManager); + var behavior = new SelfEndingBehavior(endOnFixedUpdate: false); + + AbilityHandle handle = GrantAndActivate(entity, behavior); + handle.IsActive.Should().BeTrue(); + + entity.Abilities.Invoking(x => x.UpdateAbilities(0.016)).Should().NotThrow(); + + handle.IsActive.Should().BeFalse(); + } + private static GraphProcessor StartGraphWith(TrackingStateNode node) { var graph = new Graph(); diff --git a/Forge/Abilities/Ability.cs b/Forge/Abilities/Ability.cs index bf1f994b..08ccaa8a 100644 --- a/Forge/Abilities/Ability.cs +++ b/Forge/Abilities/Ability.cs @@ -337,9 +337,14 @@ internal void OnInstanceEnded(AbilityInstance instance, bool wasCanceled) internal void UpdateBehaviors(double deltaTime) { - foreach (BehaviorBinding binding in _behaviors.Values) + BufferBehaviorInstances(); + + for (int i = 0; i < _behaviorBuffer.Count; i++) { - binding.Behavior.OnUpdate(deltaTime); + if (_behaviors.TryGetValue(_behaviorBuffer[i], out BehaviorBinding binding)) + { + binding.Behavior.OnUpdate(deltaTime); + } } } diff --git a/Forge/Core/EntityAbilities.cs b/Forge/Core/EntityAbilities.cs index a393e3bc..c5e783d1 100644 --- a/Forge/Core/EntityAbilities.cs +++ b/Forge/Core/EntityAbilities.cs @@ -491,9 +491,16 @@ public void ClearAllAbilities() /// The time elapsed since the last update, in seconds. public void UpdateAbilities(double deltaTime) { - foreach (AbilityHandle handle in GrantedAbilities) + BufferGrantedAbilities(); + + for (int i = 0; i < _updateBuffer.Count; i++) { - handle.Ability?.UpdateBehaviors(deltaTime); + AbilityHandle handle = _updateBuffer[i]; + + if (_grantedAbilities.Contains(handle)) + { + handle.Ability?.UpdateBehaviors(deltaTime); + } } } From 9257bd7a87064a3246f0d59993c186fb416f0df3 Mon Sep 17 00:00:00 2001 From: Lex Date: Sun, 6 Sep 2026 01:17:29 -0300 Subject: [PATCH 5/6] docs(abilities): put the snapshot comments on the right methods Self review: the new buffer helper landed under TryBeginActivationByTag's comment, orphaning it, and the buffer comments claimed a non-re-entrancy the code only constrains callers to. --- Forge/Abilities/Ability.cs | 4 ++-- Forge/Core/EntityAbilities.cs | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Forge/Abilities/Ability.cs b/Forge/Abilities/Ability.cs index 08ccaa8a..1202fbfe 100644 --- a/Forge/Abilities/Ability.cs +++ b/Forge/Abilities/Ability.cs @@ -41,8 +41,8 @@ private record struct BehaviorBinding(IAbilityBehavior Behavior, AbilityBehavior private readonly Dictionary _behaviors = []; - // Reused by both update rails so the per-frame walk allocates nothing. They never overlap: a host drives one and - // then the other, and neither re-enters itself. + // Reused by both update rails so the per-frame walk allocates nothing, which makes the walk non-re-entrant. Nothing + // in the library re-enters it; a host must not drive an update of this same ability from inside a behavior. private readonly List _behaviorBuffer = []; private readonly Action? _tagChangedHandler; diff --git a/Forge/Core/EntityAbilities.cs b/Forge/Core/EntityAbilities.cs index c5e783d1..790bb995 100644 --- a/Forge/Core/EntityAbilities.cs +++ b/Forge/Core/EntityAbilities.cs @@ -18,8 +18,9 @@ public class EntityAbilities(IForgeEntity owner) private readonly Dictionary> _grantSources = []; private readonly HashSet _grantedAbilities = []; - // Reused by both update rails so the per-frame walk allocates nothing. They never overlap: a host drives one and - // then the other, and neither re-enters itself. + // Reused by both update rails so the per-frame walk allocates nothing. That makes the walk non-re-entrant, which is + // the one constraint on a callback: it may grant, clear or end abilities, but it must not drive an update of this + // same entity from inside one. private readonly List _updateBuffer = []; private Action? _removeAbility; @@ -630,8 +631,6 @@ private static bool MatchesTags(Ability ability, TagContainer tagsToActivate) return ability.AbilityData.AbilityTags?.HasAny(tagsToActivate) == true; } - // Snapshots the granted abilities and seeds the per-ability failure flags for a tag-driven activation. The snapshot - // keeps the indices stable while activations grant or remove other abilities. // Snapshots the granted set before an update walks it. A behavior is free to grant, revoke or clear abilities from // inside its own update - a graph node granting one is the ordinary way to reach it - and adding to the set // invalidates any enumerator open over it. @@ -641,6 +640,8 @@ private void BufferGrantedAbilities() _updateBuffer.AddRange(_grantedAbilities); } + // Snapshots the granted abilities and seeds the per-ability failure flags for a tag-driven activation. The snapshot + // keeps the indices stable while activations grant or remove other abilities. private bool TryBeginActivationByTag( TagContainer tagsToActivate, out AbilityHandle[] handles, From e98754bd6319b59280f22f4c15f747b3115297ad Mon Sep 17 00:00:00 2001 From: Lex Date: Sun, 6 Sep 2026 01:34:07 -0300 Subject: [PATCH 6/6] fix(abilities): refuse a re-entered update instead of corrupting the walk All three walks share one reused buffer, so a nested pass cleared and refilled the list the outer one was indexing - advancing some entries twice, skipping others, silently. Nesting is never useful here, so it is refused: Validation.Fail reports it in development, and the pass is dropped either way. --- Forge.Tests/Helpers/StatescriptTestHelpers.cs | 61 ++++++ .../Statescript/UpdateReentrancyTests.cs | 175 ++++++++++++++++++ Forge/Abilities/Ability.cs | 67 +++++-- Forge/Core/EntityAbilities.cs | 78 ++++++-- Forge/Statescript/GraphProcessor.cs | 43 ++++- 5 files changed, 388 insertions(+), 36 deletions(-) create mode 100644 Forge.Tests/Statescript/UpdateReentrancyTests.cs diff --git a/Forge.Tests/Helpers/StatescriptTestHelpers.cs b/Forge.Tests/Helpers/StatescriptTestHelpers.cs index ec6748d1..70b468eb 100644 --- a/Forge.Tests/Helpers/StatescriptTestHelpers.cs +++ b/Forge.Tests/Helpers/StatescriptTestHelpers.cs @@ -351,6 +351,67 @@ public void OnUpdate(double deltaTime) } } +/// +/// A state node that runs an arbitrary callback from its own update, so a test can re-enter the processor that is +/// walking it. +/// +/// What to run from the frame update. +internal sealed class CallbackStateNode(Action onUpdate) : StateNode +{ + private readonly Action _onUpdate = onUpdate; + + public int UpdateCount { get; private set; } + + protected override void OnActivate(GraphContext graphContext) + { + } + + protected override void OnDeactivate(GraphContext graphContext) + { + } + + protected override void OnUpdate(double deltaTime, GraphContext graphContext) + { + UpdateCount++; + _onUpdate(); + } +} + +/// +/// An ability behavior that drives another update of its own entity from inside one, which is the misuse the +/// re-entrancy guard exists to catch. +/// +/// The entity whose update is re-entered. +internal sealed class ReenteringBehavior(TestEntity entity) : IAbilityBehavior +{ + private readonly TestEntity _entity = entity; + + private bool _reentered; + + public int UpdateCount { get; private set; } + + public void OnStarted(AbilityBehaviorContext context) + { + } + + public void OnEnded(AbilityBehaviorContext context) + { + } + + public void OnUpdate(double deltaTime) + { + UpdateCount++; + + if (_reentered) + { + return; + } + + _reentered = true; + _entity.Abilities.UpdateAbilities(deltaTime); + } +} + /// /// An ability behavior that ends its own instance from inside an update, which is what a graph completing on that /// rail does. Ending removes the behavior from the dictionary the dispatch loop is walking. diff --git a/Forge.Tests/Statescript/UpdateReentrancyTests.cs b/Forge.Tests/Statescript/UpdateReentrancyTests.cs new file mode 100644 index 00000000..d6e9f251 --- /dev/null +++ b/Forge.Tests/Statescript/UpdateReentrancyTests.cs @@ -0,0 +1,175 @@ +// Copyright © Gamesmiths Guild. + +using FluentAssertions; +using Gamesmiths.Forge.Abilities; +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.Statescript; +using Gamesmiths.Forge.Statescript.Nodes; +using Gamesmiths.Forge.Tags; +using Gamesmiths.Forge.Tests.Helpers; + +namespace Gamesmiths.Forge.Tests.Statescript; + +/// +/// Covers the re-entrancy guard on the update walks. Each walk runs off one reused buffer, so a nested pass would clear +/// and refill the list the outer walk is still indexing - advancing some entries twice and skipping others, silently. +/// The nested pass is refused instead: loudly when validation is on, quietly when it is off. +/// +/// +/// Validation is a global switch, which is why the assembly disables test parallelization. Every test here restores it. +/// +/// The fixture providing tags and cues managers. +public sealed class UpdateReentrancyTests(TagsAndCuesFixture fixture) : IClassFixture, IDisposable +{ + private readonly TagsManager _tagsManager = fixture.TagsManager; + private readonly CuesManager _cuesManager = fixture.CuesManager; + + public void Dispose() + { + Validation.Enabled = false; + GC.SuppressFinalize(this); + } + + [Fact] + [Trait("Graph", "Reentrancy")] + public void Re_entering_a_graph_update_is_refused_and_reported() + { + Validation.Enabled = true; + + GraphProcessor? processor = null; + var node = new CallbackStateNode(() => processor!.UpdateGraph(0.016)); + + processor = StartGraphWith(node); + + processor.Invoking(x => x.UpdateGraph(0.016)) + .Should().Throw() + .WithMessage("*re-entered*"); + } + + [Fact] + [Trait("Graph", "Reentrancy")] + public void A_refused_graph_update_leaves_the_outer_walk_intact() + { + Validation.Enabled = false; + + GraphProcessor? processor = null; + var node = new CallbackStateNode(() => processor!.UpdateGraph(0.016)); + + processor = StartGraphWith(node); + + processor.Invoking(x => x.UpdateGraph(0.016)).Should().NotThrow(); + + // One update, not two: the nested pass was dropped rather than allowed to walk the same node again. + node.UpdateCount.Should().Be(1); + } + + [Fact] + [Trait("Graph", "Reentrancy")] + public void A_refused_graph_update_does_not_advance_the_stamp() + { + Validation.Enabled = false; + + GraphProcessor? processor = null; + var node = new CallbackStateNode(() => processor!.UpdateGraph(0.016)); + + processor = StartGraphWith(node); + + processor.UpdateGraph(0.016); + + // A refused pass is not a pass, so a resolver keyed on the stamp must not see it as a new one. + processor.GraphContext.UpdateStamp.Should().Be(1); + } + + [Fact] + [Trait("Graph", "Reentrancy")] + public void A_graph_update_can_run_again_after_one_was_refused() + { + Validation.Enabled = false; + + GraphProcessor? processor = null; + var node = new CallbackStateNode(() => processor!.UpdateGraph(0.016)); + + processor = StartGraphWith(node); + + processor.UpdateGraph(0.016); + processor.UpdateGraph(0.016); + + // The flag is cleared in a finally, so a refusal - or a node that throws - cannot wedge the graph shut. + node.UpdateCount.Should().Be(2); + } + + [Fact] + [Trait("GraphBehavior", "Reentrancy")] + public void Re_entering_an_ability_update_is_refused_and_reported() + { + Validation.Enabled = true; + + var entity = new TestEntity(_tagsManager, _cuesManager); + var behavior = new ReenteringBehavior(entity); + + GrantAndActivate(entity, behavior); + + entity.Abilities.Invoking(x => x.UpdateAbilities(0.016)) + .Should().Throw() + .WithMessage("*re-entered*"); + } + + [Fact] + [Trait("GraphBehavior", "Reentrancy")] + public void A_refused_ability_update_leaves_the_outer_walk_intact() + { + Validation.Enabled = false; + + var entity = new TestEntity(_tagsManager, _cuesManager); + var behavior = new ReenteringBehavior(entity); + + GrantAndActivate(entity, behavior); + + entity.Abilities.Invoking(x => x.UpdateAbilities(0.016)).Should().NotThrow(); + + behavior.UpdateCount.Should().Be(1); + } + + private static GraphProcessor StartGraphWith(CallbackStateNode node) + { + var graph = new Graph(); + graph.AddNode(node); + graph.AddConnection(new Connection( + graph.EntryNode.OutputPorts[EntryNode.OutputPort], + node.InputPorts[StateNode.InputPort])); + + var processor = new GraphProcessor(graph); + processor.StartGraph(); + + return processor; + } + + private static void GrantAndActivate(TestEntity entity, IAbilityBehavior behavior) + { + var abilityData = new AbilityData("ReentrantGraph", behaviorFactory: () => behavior); + + var grantConfig = new GrantAbilityConfig( + abilityData, + new ScalableInt(1), + AbilityDeactivationPolicy.CancelImmediately, + AbilityDeactivationPolicy.CancelImmediately, + false, + false, + LevelComparison.Higher); + + var effectData = new EffectData( + "GrantReentrantGraph", + new DurationData(DurationType.Infinite), + effectComponents: [new GrantAbilityEffectComponent([grantConfig])]); + + entity.EffectsManager.ApplyEffect(new Effect(effectData, new EffectOwnership(entity, entity))); + + AbilityHandle handle = entity.Abilities.GrantedAbilities.First(); + handle.TryActivate(out _).Should().BeTrue(); + } +} diff --git a/Forge/Abilities/Ability.cs b/Forge/Abilities/Ability.cs index 1202fbfe..31b82189 100644 --- a/Forge/Abilities/Ability.cs +++ b/Forge/Abilities/Ability.cs @@ -41,8 +41,8 @@ private record struct BehaviorBinding(IAbilityBehavior Behavior, AbilityBehavior private readonly Dictionary _behaviors = []; - // Reused by both update rails so the per-frame walk allocates nothing, which makes the walk non-re-entrant. Nothing - // in the library re-enters it; a host must not drive an update of this same ability from inside a behavior. + // Reused by both update rails so the per-frame walk allocates nothing. Sharing one list is what makes the walk + // non-re-entrant, which BufferBehaviorInstances enforces rather than leaves to trust. private readonly List _behaviorBuffer = []; private readonly Action? _tagChangedHandler; @@ -51,6 +51,8 @@ private record struct BehaviorBinding(IAbilityBehavior Behavior, AbilityBehavior private AbilityInstance? _persistentInstance; + private bool _updating; + internal event Action? OnAbilityDeactivated; /// @@ -337,30 +339,50 @@ internal void OnInstanceEnded(AbilityInstance instance, bool wasCanceled) internal void UpdateBehaviors(double deltaTime) { - BufferBehaviorInstances(); + if (!BufferBehaviorInstances()) + { + return; + } - for (int i = 0; i < _behaviorBuffer.Count; i++) + try { - if (_behaviors.TryGetValue(_behaviorBuffer[i], out BehaviorBinding binding)) + for (int i = 0; i < _behaviorBuffer.Count; i++) { - binding.Behavior.OnUpdate(deltaTime); + if (_behaviors.TryGetValue(_behaviorBuffer[i], out BehaviorBinding binding)) + { + binding.Behavior.OnUpdate(deltaTime); + } } } + finally + { + _updating = false; + } } internal void FixedUpdateBehaviors(double deltaTime) { - BufferBehaviorInstances(); + if (!BufferBehaviorInstances()) + { + return; + } - for (int i = 0; i < _behaviorBuffer.Count; i++) + try { - // Looked up rather than snapshotted, so an instance ended earlier in this same walk - by itself or by - // another behavior - is skipped instead of being advanced after it finished. - if (_behaviors.TryGetValue(_behaviorBuffer[i], out BehaviorBinding binding)) + for (int i = 0; i < _behaviorBuffer.Count; i++) { - binding.Behavior.OnFixedUpdate(deltaTime); + // Looked up rather than snapshotted, so an instance ended earlier in this same walk - by itself or by + // another behavior - is skipped instead of being advanced after it finished. + if (_behaviors.TryGetValue(_behaviorBuffer[i], out BehaviorBinding binding)) + { + binding.Behavior.OnFixedUpdate(deltaTime); + } } } + finally + { + _updating = false; + } } internal bool CanActivate(IForgeEntity? abilityTarget, out AbilityActivationFailures failureFlags) @@ -611,10 +633,29 @@ private EventSubscriptionToken SubscribeTypedEventCore(Tag tag, int pr // Snapshots the instances before an update walks their behaviors. A behavior may start or end an instance from // inside its own update - a graph completing on this rail ends its own - and starting one adds to the dictionary, // which invalidates any enumerator open over it. - private void BufferBehaviorInstances() + // + // What it may not do is drive another update of this same ability, because the buffer is one list reused per call: + // a nested pass would clear and refill the list the outer walk is still indexing, advancing some behaviors twice + // and skipping others. Refused rather than tolerated, so a validation-disabled build drops the nested pass instead + // of miscounting time. + private bool BufferBehaviorInstances() { + if (_updating) + { + Validation.Fail( + $"An update of ability '{AbilityData.Name}' was re-entered while one was already running, which " + + "would corrupt the walk in progress. Drive ability updates from the game loop, never from inside a " + + "behavior."); + + return false; + } + + _updating = true; + _behaviorBuffer.Clear(); _behaviorBuffer.AddRange(_behaviors.Keys); + + return true; } private bool CanCommitCooldown() diff --git a/Forge/Core/EntityAbilities.cs b/Forge/Core/EntityAbilities.cs index 790bb995..4201ab0c 100644 --- a/Forge/Core/EntityAbilities.cs +++ b/Forge/Core/EntityAbilities.cs @@ -18,11 +18,12 @@ public class EntityAbilities(IForgeEntity owner) private readonly Dictionary> _grantSources = []; private readonly HashSet _grantedAbilities = []; - // Reused by both update rails so the per-frame walk allocates nothing. That makes the walk non-re-entrant, which is - // the one constraint on a callback: it may grant, clear or end abilities, but it must not drive an update of this - // same entity from inside one. + // Reused by both update rails so the per-frame walk allocates nothing. Sharing one list is what makes the walk + // non-re-entrant, which BufferGrantedAbilities enforces rather than leaves to trust. private readonly List _updateBuffer = []; + private bool _updating; + private Action? _removeAbility; private Action? _inhibitAbility; @@ -492,17 +493,27 @@ public void ClearAllAbilities() /// The time elapsed since the last update, in seconds. public void UpdateAbilities(double deltaTime) { - BufferGrantedAbilities(); - - for (int i = 0; i < _updateBuffer.Count; i++) + if (!BufferGrantedAbilities()) { - AbilityHandle handle = _updateBuffer[i]; + return; + } - if (_grantedAbilities.Contains(handle)) + try + { + for (int i = 0; i < _updateBuffer.Count; i++) { - handle.Ability?.UpdateBehaviors(deltaTime); + AbilityHandle handle = _updateBuffer[i]; + + if (_grantedAbilities.Contains(handle)) + { + handle.Ability?.UpdateBehaviors(deltaTime); + } } } + finally + { + _updating = false; + } } /// @@ -517,19 +528,29 @@ public void UpdateAbilities(double deltaTime) /// The length of the fixed step, in seconds. public void FixedUpdateAbilities(double deltaTime) { - BufferGrantedAbilities(); - - for (int i = 0; i < _updateBuffer.Count; i++) + if (!BufferGrantedAbilities()) { - AbilityHandle handle = _updateBuffer[i]; + return; + } - // Re-checked because an earlier behavior in this same walk may have cleared this ability, and an ability - // that is no longer granted should not be advanced. - if (_grantedAbilities.Contains(handle)) + try + { + for (int i = 0; i < _updateBuffer.Count; i++) { - handle.Ability?.FixedUpdateBehaviors(deltaTime); + AbilityHandle handle = _updateBuffer[i]; + + // Re-checked because an earlier behavior in this same walk may have cleared this ability, and an + // ability that is no longer granted should not be advanced. + if (_grantedAbilities.Contains(handle)) + { + handle.Ability?.FixedUpdateBehaviors(deltaTime); + } } } + finally + { + _updating = false; + } } internal AbilityHandle GrantAbility( @@ -634,10 +655,29 @@ private static bool MatchesTags(Ability ability, TagContainer tagsToActivate) // Snapshots the granted set before an update walks it. A behavior is free to grant, revoke or clear abilities from // inside its own update - a graph node granting one is the ordinary way to reach it - and adding to the set // invalidates any enumerator open over it. - private void BufferGrantedAbilities() - { + // + // What it may not do is drive another update of this same entity, because the buffer is one list reused per call: + // a nested pass would clear and refill the list the outer walk is still indexing, advancing some abilities twice + // and skipping others. Refused rather than tolerated, so a validation-disabled build drops the nested pass instead + // of miscounting time. + private bool BufferGrantedAbilities() + { + if (_updating) + { + Validation.Fail( + "An ability update was re-entered while one was already running on this entity, which would corrupt " + + "the walk in progress. Drive UpdateAbilities and FixedUpdateAbilities from the game loop, never " + + "from inside an ability behavior."); + + return false; + } + + _updating = true; + _updateBuffer.Clear(); _updateBuffer.AddRange(_grantedAbilities); + + return true; } // Snapshots the granted abilities and seeds the per-ability failure flags for a tag-driven activation. The snapshot diff --git a/Forge/Statescript/GraphProcessor.cs b/Forge/Statescript/GraphProcessor.cs index f25f6dd9..f43fc50e 100644 --- a/Forge/Statescript/GraphProcessor.cs +++ b/Forge/Statescript/GraphProcessor.cs @@ -1,5 +1,7 @@ // Copyright © Gamesmiths Guild. +using Gamesmiths.Forge.Core; + namespace Gamesmiths.Forge.Statescript; /// @@ -17,6 +19,8 @@ public class GraphProcessor { private readonly List _updateBuffer = []; + private bool _updating; + /// /// Gets the graph that this processor is responsible for executing. /// @@ -86,9 +90,16 @@ public void UpdateGraph(double deltaTime) return; } - for (int i = 0; i < _updateBuffer.Count; i++) + try + { + for (int i = 0; i < _updateBuffer.Count; i++) + { + _updateBuffer[i].Update(deltaTime, GraphContext); + } + } + finally { - _updateBuffer[i].Update(deltaTime, GraphContext); + _updating = false; } } @@ -108,9 +119,16 @@ public void FixedUpdateGraph(double deltaTime) return; } - for (int i = 0; i < _updateBuffer.Count; i++) + try + { + for (int i = 0; i < _updateBuffer.Count; i++) + { + _updateBuffer[i].FixedUpdate(deltaTime, GraphContext); + } + } + finally { - _updateBuffer[i].FixedUpdate(deltaTime, GraphContext); + _updating = false; } } @@ -169,6 +187,23 @@ private bool BufferActiveNodes() return false; } + // The buffer is one list reused per call, so a nested pass would clear and refill the list the outer walk is + // still indexing: some nodes updated twice, others skipped, silently and depending on set order. Nesting is + // never useful here - updates cascade downward through messages, not by re-driving the graph - so it is + // refused outright rather than made to work. Refusing also keeps a validation-disabled build safe: it drops + // the nested pass instead of miscounting everyone's time. + if (_updating) + { + Validation.Fail( + "A graph update was re-entered while one was already running, which would corrupt the walk in " + + "progress. Drive UpdateGraph and FixedUpdateGraph from the host's own callbacks, never from inside " + + "a node or an ability behavior."); + + return false; + } + + _updating = true; + GraphContext.UpdateStamp++; _updateBuffer.Clear();