Fixed cue magnitude reentracy.

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;
+
+/// <summary>
+/// An <see cref="CueMagnitudeType.AttributeValueChange"/> 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.
+/// </summary>
+/// <param name="tagsAndCuesFixture">The fixture providing the <see cref="TagsManager"/> and <see cref="CuesManager"/>.
+/// </param>
+public class CueMagnitudeReentrancyTests(TagsAndCuesFixture tagsAndCuesFixture) : IClassFixture<TagsAndCuesFixture>
+{
+	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<AttributeAccumulatorEffectComponent>()!.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");
+	}
+
+	/// <summary>
+	/// A cue handler that takes its effect off the target from the update, the way a "dispel on this cue" handler
+	/// would.
+	/// </summary>
+	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);
+		}
+	}
+}

Forge/Cues/CuesManager.cs

@@ -11,6 +11,11 @@ namespace Gamesmiths.Forge.Cues;
 /// </summary>
 public sealed class CuesManager
 {
+	/// <summary>
+	/// The most cues one effect can have before the magnitudes held across its hooks move from the stack to the heap.
+	/// </summary>
+	internal const int MaxStackCueMagnitudes = 8;
+
 	private readonly Dictionary<Tag, HashSet<ICueHandler>> _registeredCues = [];
 
 	/// <summary>
@@ -125,6 +130,44 @@ public void UpdateCue(Tag cueTag, IForgeEntity? target, CueParameters? parameter
 		}
 	}
 
+	/// <summary>
+	/// Decides whether an effect's cues fire for a trigger and, when they do, reads the magnitude of each one into
+	/// <paramref name="magnitudes"/>, one slot per entry of the effect's cues.
+	/// </summary>
+	/// <remarks>
+	/// 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 <see cref="ExecuteCues"/> or <see cref="UpdateCues(in EffectEvaluatedData, ReadOnlySpan{int})"/>
+	/// 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.
+	/// </remarks>
+	/// <param name="effectEvaluatedData">The evaluated data of the effect whose cues are being read.</param>
+	/// <param name="triggerRequirement">The trigger the cues are being read for.</param>
+	/// <param name="magnitudes">Receives one magnitude per cue; at least as long as the effect's cues.</param>
+	/// <returns><see langword="true"/> if the cues fire and <paramref name="magnitudes"/> was filled; otherwise,
+	/// <see langword="false"/>.</returns>
+	internal static bool TryCaptureCueMagnitudes(
+		in EffectEvaluatedData effectEvaluatedData,
+		CueTriggerRequirement triggerRequirement,
+		Span<int> 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<int> 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<int> 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<int> 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));
 			}