From 38ed71071e672dba0ce042fe53746dbea1106fe7 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:01:09 -0700 Subject: [PATCH 1/9] chore: pin ModernUO to 309fcfeb2 (SkillEvents.SkillUsed, #2636); test SDK 18.10.0 to match Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- ModernUO | 2 +- Projects/ModernSpawner.Tests/ModernSpawner.Tests.csproj | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ModernUO b/ModernUO index a52ce6e..309fcfe 160000 --- a/ModernUO +++ b/ModernUO @@ -1 +1 @@ -Subproject commit a52ce6ef705184117dbd008c42dcad577c99add4 +Subproject commit 309fcfeb27aa7cc943d44e8c6e17d2ae2e4d687a diff --git a/Projects/ModernSpawner.Tests/ModernSpawner.Tests.csproj b/Projects/ModernSpawner.Tests/ModernSpawner.Tests.csproj index 06cfe45..2fd7f68 100644 --- a/Projects/ModernSpawner.Tests/ModernSpawner.Tests.csproj +++ b/Projects/ModernSpawner.Tests/ModernSpawner.Tests.csproj @@ -5,7 +5,7 @@ - + all From 14819dd65c7189d33ec18d5fbd419709f41c2345 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:08:46 -0700 Subject: [PATCH 2/9] feat(triggers): skill trigger outcome, value window, and Any flag Replace SkillTrigger's (SkillName)(-1) sentinel with an explicit AnySkill flag, add SkillOutcome (Any/Success/Failure) and MaxSkillValue so the grammar becomes skill:[+|-]::[-]::. Add a pure MatchesContext helper for skill/outcome/window checks, and extend TriggerContext with SkillSuccess/SkillValue for Task 2 to populate. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- .../Triggers/TriggerParsingTests.cs | 86 +++++++++- Projects/ModernSpawner/Triggers/ITrigger.cs | 6 + .../ModernSpawner/Triggers/SkillTrigger.cs | 161 ++++++++++++------ 3 files changed, 202 insertions(+), 51 deletions(-) diff --git a/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs b/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs index 59c03ed..ac30c83 100644 --- a/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs +++ b/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs @@ -159,7 +159,7 @@ public void SkillTrigger_Parse_AnySkill() var trigger = SkillTrigger.Parse("skill:Any:8"); Assert.NotNull(trigger); - Assert.Equal((SkillName)(-1), trigger.TargetSkill); + Assert.True(trigger.AnySkill); Assert.Equal(8, trigger.Range); } @@ -234,5 +234,89 @@ public void SkillTrigger_Range_MinimumIsOne() Assert.Equal(1, trigger.Range); } + [Fact] + public void SkillTrigger_Parse_OutcomeSuffix() + { + var success = SkillTrigger.Parse("skill:Mining+:10"); + Assert.NotNull(success); + Assert.Equal(SkillName.Mining, success.TargetSkill); + Assert.Equal(SkillOutcome.Success, success.Outcome); + + var failure = SkillTrigger.Parse("skill:Mining-:10"); + Assert.NotNull(failure); + Assert.Equal(SkillOutcome.Failure, failure.Outcome); + + var any = SkillTrigger.Parse("skill:Mining:10"); + Assert.NotNull(any); + Assert.Equal(SkillOutcome.Any, any.Outcome); + } + + [Fact] + public void SkillTrigger_Parse_ValueWindow() + { + var window = SkillTrigger.Parse("skill:Magery:5:50-90"); + Assert.NotNull(window); + Assert.Equal(50.0, window.MinSkillValue); + Assert.Equal(90.0, window.MaxSkillValue); + + var minOnly = SkillTrigger.Parse("skill:Magery:5:50.0"); + Assert.NotNull(minOnly); + Assert.Equal(50.0, minOnly.MinSkillValue); + Assert.Equal(-1.0, minOnly.MaxSkillValue); + } + + [Fact] + public void SkillTrigger_Any_UsesFlagNotSentinel() + { + var any = SkillTrigger.Parse("skill:Any+:8"); + Assert.NotNull(any); + Assert.True(any.AnySkill); + Assert.True(any.MatchesSkill(SkillName.Alchemy)); + Assert.True(any.MatchesSkill(SkillName.Mining)); + Assert.Equal(SkillOutcome.Success, any.Outcome); + + var mining = SkillTrigger.Parse("skill:Mining:8"); + Assert.False(mining.AnySkill); + Assert.False(mining.MatchesSkill(SkillName.Alchemy)); + } + + [Theory] + [InlineData("skill:Mining:10")] + [InlineData("skill:Mining+:10:50-90:true:15")] + [InlineData("skill:Any-:8:0:false:5")] + [InlineData("skill:Blacksmith:15:80.0:true:10")] + public void SkillTrigger_Serialize_RoundTrips(string definition) + { + var first = SkillTrigger.Parse(definition); + Assert.NotNull(first); + var second = SkillTrigger.Parse(first.Serialize()); + Assert.NotNull(second); + Assert.Equal(first.AnySkill, second.AnySkill); + Assert.Equal(first.TargetSkill, second.TargetSkill); + Assert.Equal(first.Outcome, second.Outcome); + Assert.Equal(first.Range, second.Range); + Assert.Equal(first.MinSkillValue, second.MinSkillValue); + Assert.Equal(first.MaxSkillValue, second.MaxSkillValue); + Assert.Equal(first.RequireLOS, second.RequireLOS); + Assert.Equal(first.Cooldown, second.Cooldown); + } + + [Fact] + public void SkillTrigger_Evaluate_HonoursOutcomeAndWindow() + { + // Evaluate needs a spawner only for Running/Map/range; build the context without one and + // exercise the pure checks through a helper on the trigger (see Step 3: MatchesContext). + var success = SkillTrigger.Parse("skill:Mining+:10:50-90"); + Assert.True(success.MatchesContext(SkillName.Mining, 60.0, true)); + Assert.False(success.MatchesContext(SkillName.Mining, 60.0, false)); + Assert.False(success.MatchesContext(SkillName.Mining, 40.0, true)); + Assert.False(success.MatchesContext(SkillName.Mining, 95.0, true)); + Assert.False(success.MatchesContext(SkillName.Magery, 60.0, true)); + + var failure = SkillTrigger.Parse("skill:Any-:10"); + Assert.True(failure.MatchesContext(SkillName.Magery, 0.0, false)); + Assert.False(failure.MatchesContext(SkillName.Magery, 0.0, true)); + } + #endregion } diff --git a/Projects/ModernSpawner/Triggers/ITrigger.cs b/Projects/ModernSpawner/Triggers/ITrigger.cs index 3a5635e..2912789 100644 --- a/Projects/ModernSpawner/Triggers/ITrigger.cs +++ b/Projects/ModernSpawner/Triggers/ITrigger.cs @@ -63,6 +63,12 @@ public class TriggerContext /// public SkillName UsedSkill { get; set; } + /// Outcome of the skill attempt that raised a skill trigger. + public bool SkillSuccess { get; set; } + + /// Skill value of the user at the time of the attempt. + public double SkillValue { get; set; } + /// /// Custom data that can be passed by trigger sources. /// diff --git a/Projects/ModernSpawner/Triggers/SkillTrigger.cs b/Projects/ModernSpawner/Triggers/SkillTrigger.cs index 37e0733..9cbf454 100644 --- a/Projects/ModernSpawner/Triggers/SkillTrigger.cs +++ b/Projects/ModernSpawner/Triggers/SkillTrigger.cs @@ -2,55 +2,78 @@ namespace Server.Engines.ModernSpawner.Triggers; +/// Which attempt outcomes a skill trigger reacts to. +public enum SkillOutcome +{ + Any, + Success, + Failure +} + /// -/// Trigger that fires when a player uses a specific skill nearby. -/// Definition format: skill:SkillName:range:minSkillValue -/// Examples: -/// skill:Mining:10 - Triggers on Mining skill use within 10 tiles -/// skill:Magery:5:50.0 - Triggers on Magery use within 5 tiles if skill >= 50 -/// skill:Any:8 - Triggers on any skill use within 8 tiles +/// Fires when a player uses a skill near the spawner. +/// Definition: skill:<Skill>[+|-]:<range>:<min>[-<max>]:<los>:<cooldownSeconds>. +/// + reacts to successes only, - to failures only; Any matches every skill. +/// Examples: skill:Mining:10, skill:Magery+:5:50-90, skill:Any-:8. /// public class SkillTrigger : ITrigger { public string TriggerType => "skill"; - /// - /// The skill that triggers this (or SkillName.Invalid for any skill). - /// + + /// True when the trigger reacts to every skill; is then ignored. + public bool AnySkill { get; } + + /// The skill that triggers this when is false. public SkillName TargetSkill { get; } - /// - /// Range in tiles from spawner to detect skill use. - /// + /// Which outcomes react. + public SkillOutcome Outcome { get; } + + /// Range in tiles from the spawner. public int Range { get; } - /// - /// Minimum skill value required to trigger (0 = any level). - /// + /// Minimum skill value required; 0 means no lower bound. public double MinSkillValue { get; } - /// - /// Whether to require line of sight to the skill user. - /// + /// Maximum skill value allowed; -1 means no upper bound. + public double MaxSkillValue { get; } + + /// Whether the user must have line of sight to the spawner. public bool RequireLOS { get; } - /// - /// Cooldown between triggers. - /// + /// Minimum time between firings. public TimeSpan Cooldown { get; } private ModernSpawner _spawner; private DateTime _lastTriggered; public SkillTrigger(SkillName skill, int range = 10, double minSkillValue = 0, bool requireLOS = false) - : this(skill, range, minSkillValue, requireLOS, TimeSpan.FromSeconds(5)) + : this(false, skill, SkillOutcome.Any, range, minSkillValue, -1, requireLOS, TimeSpan.FromSeconds(5)) { } public SkillTrigger(SkillName skill, int range, double minSkillValue, bool requireLOS, TimeSpan cooldown) + : this(false, skill, SkillOutcome.Any, range, minSkillValue, -1, requireLOS, cooldown) + { + } + + public SkillTrigger( + bool anySkill, + SkillName skill, + SkillOutcome outcome, + int range, + double minSkillValue, + double maxSkillValue, + bool requireLOS, + TimeSpan cooldown + ) { + AnySkill = anySkill; TargetSkill = skill; + Outcome = outcome; Range = Math.Max(1, range); MinSkillValue = minSkillValue; + MaxSkillValue = maxSkillValue; RequireLOS = requireLOS; Cooldown = cooldown; } @@ -101,47 +124,55 @@ public bool Evaluate(TriggerContext context) return false; } - // Check skill value if required - if (MinSkillValue > 0 && context.UsedSkill != SkillName.Alchemy) // SkillName.Alchemy is used as "any" + if (!MatchesContext(context.UsedSkill, context.SkillValue, context.SkillSuccess)) { - var skill = mobile.Skills[context.UsedSkill]; - if (skill == null || skill.Value < MinSkillValue) - { - return false; - } + return false; } _lastTriggered = Core.Now; return true; } - public string Serialize() + /// Whether this trigger reacts to at all. + public bool MatchesSkill(SkillName skill) => AnySkill || skill == TargetSkill; + + /// The pure part of : skill, outcome and value window. + public bool MatchesContext(SkillName skill, double value, bool success) { - if ((int)TargetSkill == -1) + if (!MatchesSkill(skill)) { - return $"skill:Any:{Range}:{MinSkillValue}:{RequireLOS}:{(int)Cooldown.TotalSeconds}"; + return false; } - return $"skill:{TargetSkill}:{Range}:{MinSkillValue}:{RequireLOS}:{(int)Cooldown.TotalSeconds}"; - } + if (Outcome == SkillOutcome.Success && !success || Outcome == SkillOutcome.Failure && success) + { + return false; + } - /// - /// Checks if the skill matches this trigger. - /// - public bool MatchesSkill(SkillName skill) - { - // SkillName.Alchemy with value -1 means "any skill" (using a sentinel) - if ((int)TargetSkill == -1) + if (MinSkillValue > 0 && value < MinSkillValue) { - return true; + return false; } - return skill == TargetSkill; + return MaxSkillValue < 0 || value <= MaxSkillValue; + } + + public string Serialize() + { + var skill = AnySkill ? "Any" : TargetSkill.ToString(); + var suffix = Outcome switch + { + SkillOutcome.Success => "+", + SkillOutcome.Failure => "-", + _ => "" + }; + var window = MaxSkillValue < 0 ? $"{MinSkillValue}" : $"{MinSkillValue}-{MaxSkillValue}"; + return $"skill:{skill}{suffix}:{Range}:{window}:{RequireLOS}:{(int)Cooldown.TotalSeconds}"; } /// /// Parses a skill trigger definition string. - /// Format: skill:SkillName:range or skill:SkillName:range:minValue + /// Format: skill:<Skill>[+|-]:<range>:<min>[-<max>]:<los>:<cooldownSeconds>. /// public static SkillTrigger Parse(string definition) { @@ -164,13 +195,27 @@ public static SkillTrigger Parse(string definition) return null; } - // Parse skill name + // Parse skill name, optional +/- outcome suffix var skillName = parts[startIndex]; - SkillName skill; + var outcome = SkillOutcome.Any; + + if (skillName.EndsWith('+')) + { + outcome = SkillOutcome.Success; + skillName = skillName[..^1]; + } + else if (skillName.EndsWith('-')) + { + outcome = SkillOutcome.Failure; + skillName = skillName[..^1]; + } + + var anySkill = false; + var skill = default(SkillName); if (skillName.Equals("any", StringComparison.OrdinalIgnoreCase)) { - skill = (SkillName)(-1); // Sentinel for "any skill" + anySkill = true; } else if (!Enum.TryParse(skillName, true, out skill)) { @@ -184,11 +229,27 @@ public static SkillTrigger Parse(string definition) int.TryParse(parts[startIndex + 1], out range); } - // Parse min skill value (default: 0) + // Parse min/max skill value window (default: 0 / -1) var minValue = 0.0; + var maxValue = -1.0; if (parts.Length > startIndex + 2) { - double.TryParse(parts[startIndex + 2], out minValue); + var value = parts[startIndex + 2]; + var dashIndex = value.IndexOf('-', 1); + if (dashIndex > 0) + { + var minPart = value[..dashIndex]; + var maxPart = value[(dashIndex + 1)..]; + if (!double.TryParse(minPart, out minValue) || !double.TryParse(maxPart, out maxValue) || maxValue < minValue) + { + return null; + } + } + else + { + double.TryParse(value, out minValue); + maxValue = -1.0; + } } // Parse require LOS (default: false) @@ -205,6 +266,6 @@ public static SkillTrigger Parse(string definition) cooldown = TimeSpan.FromSeconds(cooldownSeconds); } - return new SkillTrigger(skill, range, minValue, requireLOS, cooldown); + return new SkillTrigger(anySkill, skill, outcome, range, minValue, maxValue, requireLOS, cooldown); } } From 1b30b6eb971d7e627bac60bf3dff013dc4a59bb3 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:17:40 -0700 Subject: [PATCH 3/9] feat(triggers): skill triggers fire from SkillEvents.SkillUsed; test host seeds the clock ModernSpawnerEvents becomes a one-way bridge: Configure() subscribes once to Server.Misc.SkillEvents.SkillUsed and forwards player attempts to the trigger system. The old C# event and the "call this from SkillCheck.cs" shim had no callers and are gone. ModernSpawnerConfiguration.Configure() now wires it. TriggerSystem.OnSkillUse takes (Mobile, Skill, bool) and fills the context with UsedSkill, SkillValue and SkillSuccess, so Task 1's outcome filter and value window are reachable end to end. Allocation is unchanged: the per-spawner TriggerContext that was already built is the only one. The test host seeds Core._now the way production does (Server.dll grants ModernSpawner.Tests InternalsVisibleTo) and gains AdvanceClock, so cooldowns and any absolute-clock comparison behave instead of reading as "never elapsed". SkillTriggerTests drives the real SkillCheck.Mobile_SkillCheckDirectTarget handler and covers range, outcome filtering, creatures, and the cooldown gate. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- .../Core/SkillTriggerTests.cs | 131 ++++++++++++++++++ .../Fixtures/ModernSpawnerTestServer.cs | 20 ++- .../ModernSpawnerConfiguration.cs | 7 +- .../Triggers/ModernSpawnerEvents.cs | 52 +++---- .../ModernSpawner/Triggers/TriggerSystem.cs | 20 ++- 5 files changed, 189 insertions(+), 41 deletions(-) create mode 100644 Projects/ModernSpawner.Tests/Core/SkillTriggerTests.cs diff --git a/Projects/ModernSpawner.Tests/Core/SkillTriggerTests.cs b/Projects/ModernSpawner.Tests/Core/SkillTriggerTests.cs new file mode 100644 index 0000000..3ca03bb --- /dev/null +++ b/Projects/ModernSpawner.Tests/Core/SkillTriggerTests.cs @@ -0,0 +1,131 @@ +using System; +using Server.Engines.ModernSpawner.Tests.Fixtures; +using Server.Engines.ModernSpawner.Triggers; +using Server.Misc; +using Server.Mobiles; +using Xunit; + +namespace Server.Engines.ModernSpawner.Tests; + +/// +/// End-to-end cover for skill triggers: a real handler raises +/// , forwards it, and the spawner +/// fires only for players, only in range, only for the configured outcome, and only once per cooldown. +/// +[Collection("Sequential ModernSpawner Tests")] +public class SkillTriggerTests +{ + private static ModernSpawner Place(string definition) + { + var spawner = new ModernSpawner(1, TimeSpan.FromMinutes(5), TimeSpan.FromMinutes(10), 0, default, "Rabbit"); + spawner.MoveToWorld(new Point3D(1500, 1500, 0), Map.Felucca); + spawner.AddToTriggerDefinitions(definition); + spawner.TriggerActivated = true; + return spawner; + } + + private static PlayerMobile PlacePlayer(Point3D at) + { + // Mobile.Player is not set by the PlayerMobile constructor (production sets it on login), and the + // bridge filters on it, so the test host sets it the way ModernUO's own mobile tests do. + var player = new PlayerMobile { Name = "Miner", Player = true }; + player.MoveToWorld(at, Map.Felucca); + return player; + } + + [Fact] + public void PlayerSkillUse_InRange_FiresTrigger() + { + var spawner = Place("skill:Mining:10:0:false:0"); + var player = PlacePlayer(new Point3D(1503, 1500, 0)); + try + { + Assert.False(spawner.Triggered); + SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0); + Assert.True(spawner.Triggered); + } + finally + { + player.Delete(); + spawner.Delete(); + } + } + + [Fact] + public void PlayerSkillUse_OutOfRange_DoesNotFire() + { + var spawner = Place("skill:Mining:5:0:false:0"); + var player = PlacePlayer(new Point3D(1520, 1500, 0)); + try + { + SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0); + Assert.False(spawner.Triggered); + } + finally + { + player.Delete(); + spawner.Delete(); + } + } + + [Fact] + public void FailureOnlyTrigger_IgnoresSuccess() + { + var spawner = Place("skill:Mining-:10:0:false:0"); + var player = PlacePlayer(new Point3D(1503, 1500, 0)); + try + { + SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0); + Assert.False(spawner.Triggered); + SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, -0.1); + Assert.True(spawner.Triggered); + } + finally + { + player.Delete(); + spawner.Delete(); + } + } + + [Fact] + public void CreatureSkillUse_DoesNotFire() + { + var spawner = Place("skill:Any:10:0:false:0"); + var rabbit = new Rabbit(); + rabbit.MoveToWorld(new Point3D(1503, 1500, 0), Map.Felucca); + try + { + SkillCheck.Mobile_SkillCheckDirectTarget(rabbit, SkillName.Mining, null, 1.0); + Assert.False(spawner.Triggered); + } + finally + { + rabbit.Delete(); + spawner.Delete(); + } + } + + [Fact] + public void Cooldown_SuppressesSecondFiringUntilElapsed() + { + var spawner = Place("skill:Mining:10:0:false:5"); + var player = PlacePlayer(new Point3D(1503, 1500, 0)); + try + { + SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0); + Assert.True(spawner.Triggered); + spawner.ResetTrigger(); + SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0); + Assert.False(spawner.Triggered); + + ModernSpawnerTestServer.AdvanceClock(TimeSpan.FromSeconds(6)); + SkillCheck.Mobile_SkillCheckDirectTarget(player, SkillName.Mining, null, 1.0); + Assert.True(spawner.Triggered); + } + finally + { + player.Delete(); + spawner.Delete(); + } + } +} diff --git a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs index 44f93dd..1757145 100644 --- a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs +++ b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs @@ -1,3 +1,4 @@ +using System; using System.Reflection; using System.Threading; using Server.Engines.ModernSpawner.Scripting; @@ -46,13 +47,11 @@ public static void Initialize() SkillsInfo.Configure(); - // NOTE: production (Main.cs) and ModernUO's own fixtures seed the loop clock here with - // `Core._now = DateTime.UtcNow`. That field is `internal` to Server.dll and its - // InternalsVisibleTo list only names Server.Tests and UOContent.Tests, so this assembly - // cannot set it and there is no public equivalent. Core.Now therefore stays - // DateTime.MinValue for this host. Nothing on the spawner lifecycle paths depends on an - // absolute wall clock (deadlines are relative, and the timer wheel runs on tick counts), - // but a future test that moves or reads the clock will need ModernUO to expose a seam. + // Production (Main.cs) and ModernUO's own fixtures seed the loop clock here; Server.dll + // grants this assembly InternalsVisibleTo, so the same seam is available. Without it + // Core.Now stays DateTime.MinValue and anything comparing against an absolute wall clock + // (cooldowns, time windows) reads as "never elapsed". + Core._now = DateTime.UtcNow; // The timer wheel must exist before NetState.Configure(), which schedules a recurring // sweep through Timer.DelayCall (production order in Main.cs: Timer.Init runs before @@ -91,4 +90,11 @@ public static void Initialize() _initialized = true; } } + + /// + /// Moves the engine clock forward. Only valid in this host, which never ticks the timer wheel, so + /// nothing schedules off the value being advanced. + /// + /// How far forward to move . + public static void AdvanceClock(TimeSpan by) => Core._now += by; } diff --git a/Projects/ModernSpawner/ModernSpawnerConfiguration.cs b/Projects/ModernSpawner/ModernSpawnerConfiguration.cs index ad51a7a..b61e567 100644 --- a/Projects/ModernSpawner/ModernSpawnerConfiguration.cs +++ b/Projects/ModernSpawner/ModernSpawnerConfiguration.cs @@ -1,3 +1,4 @@ +using Server.Engines.ModernSpawner.Triggers; using Server.Engines.Spawners; using Server.Mobiles; @@ -10,10 +11,14 @@ public static class ModernSpawnerConfiguration { /// /// Called during server startup to initialize ModernSpawner systems. - /// Pre-warms the property accessor cache for common types. + /// Subscribes the trigger bridge to engine events, then pre-warms the property accessor cache + /// for common types. /// public static void Configure() { + // Skill triggers listen on SkillEvents.SkillUsed; nothing else subscribes for them. + ModernSpawnerEvents.Configure(); + // Pre-warm the property accessor cache for common types. // This eliminates the first-access compilation cost during gameplay. // Based on benchmarks, each property compilation takes ~196μs. diff --git a/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs b/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs index 32d3a32..11a4817 100644 --- a/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs +++ b/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs @@ -1,48 +1,42 @@ -using System; +using Server.Misc; namespace Server.Engines.ModernSpawner.Triggers; /// -/// Provides custom event infrastructure for ModernSpawner triggers. -/// Server operators can call these methods from their skill/event implementations -/// to enable skill-based triggers. +/// Bridges engine events to the trigger system. /// public static class ModernSpawnerEvents { + private static bool _configured; + /// - /// Event fired when a skill is used. Subscribe to receive skill use notifications. + /// Subscribes to . Idempotent: repeated calls subscribe once. /// - public static event Action SkillUsed; + public static void Configure() + { + if (_configured) + { + return; + } + + SkillEvents.SkillUsed += OnSkillUsed; + _configured = true; + } /// - /// Call this method when a player uses a skill to notify the trigger system. - /// This should be called from SkillCheck handlers or individual skill implementations. - /// - /// Example integration in SkillCheck.cs: - /// - /// public static bool CheckSkill(Mobile from, Skill skill, object amObj, double chance) - /// { - /// // Notify ModernSpawner of skill use - /// Server.Engines.ModernSpawner.Triggers.ModernSpawnerEvents.OnSkillUsed(from, skill.SkillName); - /// - /// // ... rest of existing CheckSkill code - /// } - /// + /// Forwards a player's skill attempt to the trigger system; creatures are ignored. + /// Runs on every skill attempt server-wide, so it must stay allocation-free. /// - /// The mobile using the skill. - /// The skill being used. - public static void OnSkillUsed(Mobile mobile, SkillName skill) + /// The mobile that attempted the skill. + /// The skill attempted. + /// Whether the attempt succeeded. + public static void OnSkillUsed(Mobile mobile, Skill skill, bool success) { - if (mobile == null) + if (mobile?.Player != true || skill == null) { return; } - // Invoke any direct subscribers - SkillUsed?.Invoke(mobile, skill); - - // Notify the trigger system - TriggerSystem.Instance.OnSkillUse(mobile, skill); + TriggerSystem.Instance.OnSkillUse(mobile, skill, success); } - } diff --git a/Projects/ModernSpawner/Triggers/TriggerSystem.cs b/Projects/ModernSpawner/Triggers/TriggerSystem.cs index 9914d76..1bb39dc 100644 --- a/Projects/ModernSpawner/Triggers/TriggerSystem.cs +++ b/Projects/ModernSpawner/Triggers/TriggerSystem.cs @@ -339,13 +339,23 @@ public void UnregisterSkillTrigger(ModernSpawner spawner, SkillTrigger trigger) } } - public void OnSkillUse(Mobile mobile, SkillName skill) + /// + /// Dispatches a skill attempt to every registered skill trigger on the mobile's map. + /// Runs on every player skill attempt server-wide, so it allocates nothing beyond the + /// per-spawner it already builds. + /// + /// The mobile that attempted the skill. + /// The skill attempted. + /// Whether the attempt succeeded. + public void OnSkillUse(Mobile mobile, Skill skill, bool success) { - if (mobile == null || mobile.Map == null || mobile.Map == Map.Internal) + if (mobile == null || skill == null || mobile.Map == null || mobile.Map == Map.Internal) { return; } + var skillName = skill.SkillName; + // Check all registered skill triggers foreach (var (spawner, triggers) in _skillTriggers) { @@ -357,12 +367,14 @@ public void OnSkillUse(Mobile mobile, SkillName skill) var context = new TriggerContext(spawner) { TriggeringMobile = mobile, - UsedSkill = skill + UsedSkill = skillName, + SkillValue = skill.Value, + SkillSuccess = success }; foreach (var trigger in triggers) { - if (trigger.MatchesSkill(skill) && trigger.Evaluate(context)) + if (trigger.MatchesSkill(skillName) && trigger.Evaluate(context)) { spawner.Trigger(); break; // Only trigger once per spawner per skill use From c9cf809cd4366d913fca398e4d7c9924e554853b Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:28:14 -0700 Subject: [PATCH 4/9] perf(triggers): hoist skill value and pre-scan before allocating the context; OnSkillUse on ITriggerSystem Review round 1 on the skill-trigger dispatch path, which runs on every player skill attempt server-wide. Skill.Value was read once per map-matching spawner; each read re-derives the stat-scaled value through NonRacialValue and adds the racial bonus, so it is now hoisted alongside skillName and read once for the whole dispatch. Each map-matching spawner also allocated a TriggerContext before any trigger was consulted. A cheap indexed pre-scan for MatchesSkill(skillName) now runs first, so a spawner holding triggers for other skills allocates nothing. The pre-scan records the first matching index and the evaluation loop resumes there rather than rescanning from zero; both loops are indexed, with no enumerator and no closure. ITriggerSystem gains OnSkillUse(Mobile, Skill, bool) next to OnSpeech, so the interface matches what TriggerSystem exposes. ModernSpawnerEvents.OnSkillUsed uses `is not { Player: true }` instead of a lifted bool? comparison, and ModernSpawnerTestServer.AdvanceClock documents that the clock only moves forward and is shared, so tests must assert elapsed intervals rather than absolute deadlines. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- .../Fixtures/ModernSpawnerTestServer.cs | 8 ++++- .../ModernSpawner/Triggers/ITriggerSystem.cs | 9 ++++++ .../Triggers/ModernSpawnerEvents.cs | 2 +- .../ModernSpawner/Triggers/TriggerSystem.cs | 29 ++++++++++++++++--- 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs index 1757145..d6c565b 100644 --- a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs +++ b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs @@ -94,7 +94,13 @@ public static void Initialize() /// /// Moves the engine clock forward. Only valid in this host, which never ticks the timer wheel, so /// nothing schedules off the value being advanced. + /// + /// The clock only ever moves forward: it is seeded once per process, never rewound, and shared by + /// every test in the collection. A test must therefore assert on elapsed intervals rather than on + /// an absolute deadline, and must not assume the clock is where an earlier + /// test left it. + /// /// - /// How far forward to move . + /// How far forward to move . Must not be negative. public static void AdvanceClock(TimeSpan by) => Core._now += by; } diff --git a/Projects/ModernSpawner/Triggers/ITriggerSystem.cs b/Projects/ModernSpawner/Triggers/ITriggerSystem.cs index 7e68a02..5880e3b 100644 --- a/Projects/ModernSpawner/Triggers/ITriggerSystem.cs +++ b/Projects/ModernSpawner/Triggers/ITriggerSystem.cs @@ -40,6 +40,15 @@ public interface ITriggerSystem /// void OnSpeech(Mobile speaker, string text, Point3D location, Map map, ModernSpawner spawner); + /// + /// Called when a mobile attempts a skill. Dispatched from SkillEvents.SkillUsed through + /// , so it runs for every player skill attempt server-wide. + /// + /// The mobile that attempted the skill. + /// The skill attempted. + /// Whether the attempt succeeded. + void OnSkillUse(Mobile mobile, Skill skill, bool success); + /// /// Called when a spawned entity is killed. /// diff --git a/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs b/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs index 11a4817..e50dadf 100644 --- a/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs +++ b/Projects/ModernSpawner/Triggers/ModernSpawnerEvents.cs @@ -32,7 +32,7 @@ public static void Configure() /// Whether the attempt succeeded. public static void OnSkillUsed(Mobile mobile, Skill skill, bool success) { - if (mobile?.Player != true || skill == null) + if (mobile is not { Player: true } || skill == null) { return; } diff --git a/Projects/ModernSpawner/Triggers/TriggerSystem.cs b/Projects/ModernSpawner/Triggers/TriggerSystem.cs index 1bb39dc..0af4f53 100644 --- a/Projects/ModernSpawner/Triggers/TriggerSystem.cs +++ b/Projects/ModernSpawner/Triggers/TriggerSystem.cs @@ -341,8 +341,9 @@ public void UnregisterSkillTrigger(ModernSpawner spawner, SkillTrigger trigger) /// /// Dispatches a skill attempt to every registered skill trigger on the mobile's map. - /// Runs on every player skill attempt server-wide, so it allocates nothing beyond the - /// per-spawner it already builds. + /// Runs on every player skill attempt server-wide: is read once for the + /// whole dispatch (each read re-derives the stat-scaled value plus the racial bonus), and a + /// is allocated only for a spawner that holds a trigger for this skill. /// /// The mobile that attempted the skill. /// The skill attempted. @@ -355,6 +356,7 @@ public void OnSkillUse(Mobile mobile, Skill skill, bool success) } var skillName = skill.SkillName; + var skillValue = skill.Value; // Check all registered skill triggers foreach (var (spawner, triggers) in _skillTriggers) @@ -364,16 +366,35 @@ public void OnSkillUse(Mobile mobile, Skill skill, bool success) continue; } + // Cheap pre-scan: most spawners hold triggers for other skills, and those must not pay for a + // context. Indexed loops here so this path has no enumerator and no closure. + var firstMatch = -1; + for (var i = 0; i < triggers.Count; i++) + { + if (triggers[i].MatchesSkill(skillName)) + { + firstMatch = i; + break; + } + } + + if (firstMatch < 0) + { + continue; + } + var context = new TriggerContext(spawner) { TriggeringMobile = mobile, UsedSkill = skillName, - SkillValue = skill.Value, + SkillValue = skillValue, SkillSuccess = success }; - foreach (var trigger in triggers) + // Everything before firstMatch is already known not to match this skill. + for (var i = firstMatch; i < triggers.Count; i++) { + var trigger = triggers[i]; if (trigger.MatchesSkill(skillName) && trigger.Evaluate(context)) { spawner.Trigger(); From b22e4abab7d56b73db33e01e3044945efda040b7 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:34:23 -0700 Subject: [PATCH 5/9] feat(migration): map XmlSpawner SkillTrigger to the skill trigger grammar Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- .../ModernSpawnerTriggerRegistrationTests.cs | 18 +++++++++ .../Migration/XmlSpawnerMigrator.cs | 38 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs index 12a2198..8f04d92 100644 --- a/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs +++ b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs @@ -182,4 +182,22 @@ public void SpawnPointMigrator_RunningFalse_ProducesStoppedSpawner() Assert.True(running.Running); running.Delete(); } + + [Theory] + [InlineData("Mining", "skill:Mining:8:0:False:5")] + [InlineData("Mining+", "skill:Mining+:8:0:False:5")] + [InlineData("Magery-,50,90", "skill:Magery-:8:50-90:False:5")] + public void Migrator_MapsSkillTriggerAttribute(string xml, string expected) + { + var node = ParseNode($""); + var spawner = XmlSpawnerMigrator.ParseXmlSpawnerNode(node); + try + { + Assert.Contains(expected, spawner.TriggerDefinitions); + } + finally + { + spawner.Delete(); + } + } } diff --git a/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs b/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs index 139fab3..7f9aa2b 100644 --- a/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs +++ b/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs @@ -193,6 +193,17 @@ internal static ModernSpawner ParseXmlSpawnerNode(XmlNode node) spawner.AddToTriggerDefinitions($"speech:{encoded}:true:false:10:true:5"); } + var skillTrigger = GetAttribute(node, "SkillTrigger", null); + if (!string.IsNullOrWhiteSpace(skillTrigger)) + { + var definition = MapSkillTrigger(skillTrigger, proximityRange < 0 ? 10 : proximityRange); + if (definition != null) + { + spawner.AddToTriggerDefinitions(definition); + spawner.TriggerActivated = true; + } + } + // Parse spawn objects var objectsNode = node.SelectSingleNode("SpawnObjects") ?? node.SelectSingleNode("Objects"); if (objectsNode != null) @@ -322,6 +333,33 @@ internal static ModernSpawner ParseSpawnPointNode(XmlNode node) return spawner; } + /// + /// XmlSpawner SkillTrigger is SkillName[+|-][,min[,max]]; the modern grammar keeps the + /// suffix and folds min/max into the value window. Returns null when the skill name is unknown. + /// + internal static string MapSkillTrigger(string xml, int range) + { + var parts = xml.Split(','); + var name = parts[0].Trim(); + var suffix = ""; + if (name.EndsWith('+') || name.EndsWith('-')) + { + suffix = name[^1..]; + name = name[..^1]; + } + + if (!name.Equals("Any", StringComparison.OrdinalIgnoreCase) && !Enum.TryParse(name, true, out _)) + { + Logger.Warning("Skipping SkillTrigger with unknown skill name: {SkillName}", name); + return null; + } + + var min = parts.Length > 1 && double.TryParse(parts[1], out var parsedMin) && parsedMin > 0 ? parsedMin : 0; + var max = parts.Length > 2 && double.TryParse(parts[2], out var parsedMax) && parsedMax > 0 ? parsedMax : -1; + var window = max < 0 ? $"{min}" : $"{min}-{max}"; + return $"skill:{name}{suffix}:{range}:{window}:False:5"; + } + /// /// Converts XmlSpawner property format (prop/value/prop/value) to ModernSpawner format. /// From 00a24097ddf6632dc935779b0e961b04b73e3d5b Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:41:30 -0700 Subject: [PATCH 6/9] docs: skill triggers are wired to SkillEvents.SkillUsed; D3 ruled Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- CLAUDE.md | 3 ++- dev-docs/architecture.md | 17 ++++++++++------- dev-docs/modernuo-prerequisites.md | 5 ++--- dev-docs/product-spec.md | 4 ++-- dev-docs/xmlspawner-migration.md | 2 +- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 287ea2e..29dc380 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,7 +61,8 @@ ModernSpawner-specific: hooks (`OnStarted`, `OnSpawned`, `OnSpawnedDeath`, entry-aware `GetSpawnPosition`) carry the modern behaviour. Never add a parallel entry list or hide base members with `new`. - Triggers register through `TriggerSystem`; proximity uses `Item.HandlesOnMovement`/`OnMovement`, speech - uses `HandlesOnSpeech`. Extended (beyond 24-tile) proximity is stubbed pending a ModernUO area-movement API. + uses `HandlesOnSpeech`, skill uses `Server.Misc.SkillEvents.SkillUsed` (players only). Extended (beyond + 24-tile) proximity is stubbed pending a ModernUO area-movement API. - Trigger list changes go through the generated helpers (`AddToTriggerDefinitions`, `RemoveFromTriggerDefinitionsAt`, `ClearTriggerDefinitions`), then call `EnsureTriggersActive()`; the `TriggerActivated` setter does this for you. Never call `TriggerSystem.ActivateTriggers` directly — it is diff --git a/dev-docs/architecture.md b/dev-docs/architecture.md index 843b789..937c797 100644 --- a/dev-docs/architecture.md +++ b/dev-docs/architecture.md @@ -91,7 +91,7 @@ is tracked for serialization before triggers are re-registered. Deactivation is | proximity | `Item.OnMovement` (24-tile radius, engine-fixed) | yes | | speech | `Item.OnSpeech` (15/18-tile radius) | yes | | kill | `OnSpawnedDeath` via `BaseSpawner.NotifySpawnedDeath`, called from `BaseCreature.OnDeath` | yes, tested | -| skill | `ModernSpawnerEvents.OnSkillUsed` | no caller | +| skill | `SkillEvents.SkillUsed` → `ModernSpawnerEvents.OnSkillUsed` (players only) → `TriggerSystem.OnSkillUse` | yes | | timeofday | 2.5 s polling timer | yes | | game_time_window | one transition timer | yes (wrong clock constant) | | wall_time_window | `EventScheduler` + `BaseScheduledEvent` subclass | yes (close-edge filter bug) | @@ -296,9 +296,12 @@ differences from the original plan noted inline. on `BaseSpawner`, invoked from `BaseCreature.OnDeath` before base death while the link is intact. Death is distinct from removal (taming, pickup, delete). `RequireAllDead` is evaluated after removal against the entry's remaining live count. -- **Skill.** Needs a ModernUO PR: `SkillCheck` raises a generated `SkillEvents.SkillUsedEvent(Mobile, - SkillName, double value, bool success)`; ModernSpawner subscribes. Until merged, `skill:` definitions are - rejected at parse time with a visible error (never accepted as inert). +- **Skill.** `SkillCheck`'s four `Mobile_SkillCheck*` handlers raise `SkillEvents.SkillUsed(Mobile, Skill, + bool success)` once per attempt (short-circuited attempts included; not raised when the mobile lacks the + skill). `ModernSpawnerEvents.OnSkillUsed` forwards only players (`mobile is { Player: true }`) to + `TriggerSystem.OnSkillUse`, which pre-scans `SkillTrigger.MatchesSkill` before allocating a + `TriggerContext` so spawners with no matching trigger pay nothing. `SkillTrigger` adds an outcome filter + (any/success/failure) and a min/max skill-value window on top of range and line-of-sight. - **Grammar.** One definition grammar owned by each trigger's `Serialize()`. Gumps and importers construct trigger objects. `TriggerContext` becomes a `readonly record struct`. - **Extended proximity.** Clamp to `Core.GlobalMaxUpdateRange` with a warning; the sector-range @@ -400,9 +403,9 @@ carried across `Timer.DelayCall`; mutation-safe iteration and registration befor ## 11. ModernUO prerequisites created by this design Tracked in `modernuo-prerequisites.md`: DTO helper visibility (done), abstract entry ownership (§4.2), -`OnStarted/OnStopped` and `OnConfigureSpawned` virtuals, `OnSpawnedDeath` hook, `SkillUsedEvent`, test -initializer access, `InternalsVisibleTo("ModernSpawner.Tests")` on `Server.csproj` (so the test fixture -can seed `Core._now`), GUID-based replacement in `[ImportSpawners` (today it deletes co-located same-type +`OnStarted/OnStopped` and `OnConfigureSpawned` virtuals, `OnSpawnedDeath` hook, `SkillEvents.SkillUsed` +(done, #2636) with `InternalsVisibleTo("ModernSpawner.Tests")` on `Server.csproj` (so the test fixture can +seed `Core._now`), GUID-based replacement in `[ImportSpawners` (today it deletes co-located same-type spawners and calls `Respawn()` unconditionally, `ImportSpawnersCommand.cs:259`), sector-range movement subscription (deferred). diff --git a/dev-docs/modernuo-prerequisites.md b/dev-docs/modernuo-prerequisites.md index 09b0d13..898ee18 100644 --- a/dev-docs/modernuo-prerequisites.md +++ b/dev-docs/modernuo-prerequisites.md @@ -8,9 +8,7 @@ per-movement paths without a measurement, because shards run 12k+ spawners. ## Open -| PR | Change | Why ModernSpawner needs it | Submodule pin | -|---|---|---|---| -| [#2636](https://github.com/modernuo/ModernUO/pull/2636) | `SkillEvents.SkillChecked` (`Action`, `Server.Misc`) raised from `SkillCheck.CheckSkill`; `InternalsVisibleTo("ModernSpawner.Tests")` on `Server.csproj` | D3 skill triggers subscribe cross-assembly (generated events are static dispatch inside UOContent); the test fixture can seed `Core._now` | no pin needed until the ModernSpawner wiring PR consumes it; that PR pins the submodule to the PR head, or to `main` if it has merged by then | +(none) ## Merged @@ -18,6 +16,7 @@ per-movement paths without a measurement, because shards run 12k+ spawners. |---|---|---| | [#2619](https://github.com/modernuo/ModernUO/pull/2619) | `BaseSpawner.Dto.cs`: `private protected` DTO helpers → `protected` | `ModernSpawner.ToDto()` lives in another assembly and needs the `Dto*` helpers and `BoundsFromHomeRange` | | [#2621](https://github.com/modernuo/ModernUO/pull/2621) | Subclass-owned entries (`BaseSpawner` v13 / `Spawner` v2 owner contract), lifecycle hooks + `NotifySpawnedDeath`, `SpawnerEntry` v2 `Disabled`, DTO records own `entries`, save migration | D1/D11/D12: `ModernSpawner : Spawner` owns `List` with `ModernSpawnerEntry : SpawnerEntry`; kill trigger via the death hook. The ModernSpawner side is ported in [ModernSpawner #1](https://github.com/modernuo/ModernSpawner/pull/1); submodule at `a52ce6ef7` | +| [#2636](https://github.com/modernuo/ModernUO/pull/2636) | `SkillEvents.SkillUsed` (`Action`, `Server.Misc`) raised once per attempt from the four `Mobile_SkillCheck*` handlers; `InternalsVisibleTo("ModernSpawner.Tests")` on `Server.csproj` | D3 skill triggers subscribe cross-assembly; the test fixture seeds `Core._now`; submodule at `309fcfeb2` | ## Planned (see `architecture.md` §4–§5, §11; decisions D1, D2, D3, D11, D12) diff --git a/dev-docs/product-spec.md b/dev-docs/product-spec.md index 4506970..2e260c5 100644 --- a/dev-docs/product-spec.md +++ b/dev-docs/product-spec.md @@ -88,7 +88,7 @@ Status columns reflect the audit at `8935ca4`. "Target" is the v1 commitment. | Proximity beyond 24 tiles | Stubbed | Range clamped with a warning; wider ranges need a ModernUO area-subscription API (tracked in `modernuo-prerequisites.md`) | | Speech | Implemented | Kept; regex timeout; whether it may wake a stopped spawner is per-trigger (`wake:`) under **D2** | | Kill | Stubbed | Wired via a new upstream `BaseSpawner.OnSpawnedDeath` hook (the creature-death event fires after the spawner link is cleared) | -| Skill | Stubbed - today `skill:` definitions parse and register, but nothing calls `ModernSpawnerEvents.OnSkillUsed`, so they never fire | `skill:` definitions are rejected at parse time with a visible error until the wiring PR subscribes to ModernUO #2636's `SkillEvents.SkillChecked` (**D3**); a trigger that cannot fire must not look configured | +| Skill | Implemented | Wired to ModernUO's `SkillEvents.SkillUsed`, forwarded to players only (**D3**); outcome (any/success/failure) and min/max value-window semantics; grammar owned by `SkillTrigger.Serialize()` | | Game-time window | Partial | Constant derived from `Clock.SecondsPerUOMinute`; recomputed on map change | | Wall-clock window | Partial | Day/month filters apply to the open edge only; weekly/monthly recurrence exposed | | Legacy `timeofday` | Implemented | Retired in favour of `game_time_window` (importer maps to it) | @@ -207,7 +207,7 @@ stated conditions. | **D0** | Distribution: source submodule vs DLL | Source submodule for v1; DLL later | Open (default assumed) | | **D1** | Entry ownership | Change ModernUO: abstract entry ownership (`architecture.md` §4), including any streamlining of `BaseSpawner` that makes it more agnostic. **Condition:** no performance regression; trade-offs reported before merge | **Ruled** | | **D2** | Trigger semantics | State machine: gate set (windows) + bounded pending-cycle queue (events); `architecture.md` §5. **Condition:** no per-tick/per-movement cost growth at 12k+ spawners; implementation reviewed | **Ruled** | -| **D3** | Skill trigger source | Upstream `SkillCheck` hook via ModernUO PR | Open (default assumed); ModernUO #2636 open | +| **D3** | Skill trigger source | Upstream `SkillEvents.SkillUsed` hook (ModernUO #2636) | **Ruled** | | **D4** | Script language | Retire ModernSpawner's current `SET/Hits/100` command syntax (a copy of XmlSpawner's style, not XmlSpawner itself); add statements and actions on top of the existing, tested expression engine rather than writing a new engine | **Ruled** (retire); statement design pending review | | **D5** | Canonical export format | ModernUO `SpawnerDto`; own JSON and YAML removed; generalise upstream where needed | **Ruled** | | **D6** | Entry `Properties` syntax | ModernUO's `Name Value` pairs; ranges/expressions live in entry scripts | **Ruled** | diff --git a/dev-docs/xmlspawner-migration.md b/dev-docs/xmlspawner-migration.md index c703dec..54b5146 100644 --- a/dev-docs/xmlspawner-migration.md +++ b/dev-docs/xmlspawner-migration.md @@ -82,7 +82,7 @@ older RunUO XmlSpawner2 exports may differ and are reported, not silently accept | `ProximityTriggerSound`, `ProximityTriggerMessage` | trigger `onTriggered` feedback: `sound(id)`, `msg(trigMob, "text")` | fires on an *accepted* trigger with the triggering mobile (`XmlSpawner.cs:2324`), not on activate | | `TriggerProbability` (fraction) | spawner-level trigger `chance` | one roll per accepted trigger, not per trigger type | | `SpeechTrigger` | trigger `speech:text` | one case-insensitive substring match (`XmlSpawner.cs:2385`); do not split on commas | -| `SkillTrigger` | trigger `skill:name[:min[:max]][:success\|failure]` | XmlSpawner syntax `SkillName[+/-][,min,max]` (`+` success only, `-` failure only); extend the trigger to carry max and outcome | +| `SkillTrigger` | trigger `skill:[+\|-]::[-]::` | XmlSpawner syntax `SkillName[+/-][,min,max]` (`+` success only, `-` failure only) maps directly onto the outcome suffix and value window; `` is the node's `ProximityRange` (10 when absent); XmlSpawner's own skill trigger never fired in ServUO (its parser was unwired), so there is no runtime behaviour to preserve — only the intended semantics carry over | | `TODStart`, `TODEnd`, `TODMode` | `game_time_window` (mode 1) / `wall_time_window` (mode 0) | minutes → hour:minute | | `MinRefractory`, `MaxRefractory` | spawner-level trigger refractory `random(min,max)` | belongs to the spawner's accepted-trigger state, not to each translated trigger | | `KillReset` | kill trigger `resetAfterTicks` | count of spawn ticks without a kill before the kill counter resets (`XmlSpawner.cs:6735`) — add field or warn | From 8477ef182e538c41934901ecaf7c007d4bf8756c Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:59:28 -0700 Subject: [PATCH 7/9] fix(triggers): LOS means line of sight; parser hardening; dispatch loops iterate a snapshot; fixed test clock SkillTrigger.Parse threw ArgumentOutOfRangeException on an empty value segment ("skill:Mining:10::false:5"): IndexOf('-', 1) rejects startIndex 1 on a zero-length string. That took down the whole ActivateTriggers pass instead of skipping one definition. An empty window is now malformed and returns null. RequireLOS checked visibility, not line of sight. Mobile.CanSee(Item) ends in item.Visible and BaseSpawner sets Visible = false, so a RequireLOS skill or proximity trigger could never fire for a player. Both now use Mobile.InLOS. SkillTrigger.Evaluate runs MatchesContext immediately after the spawner/Running guard, so an attempt this trigger does not react to - the common case on a server-wide dispatch - pays nothing for the cooldown, map, range and LOS checks. OnSkillUse and CheckTimeOfDayTriggers called spawner.Trigger() while enumerating their dictionaries; Trigger() reaches Spawn() and any attached script, and a DESPAWN script or a spawned ModernSpawner can delete or register a spawner that carries a trigger, invalidating the enumerator. Both now copy their entries into an STArrayPool-rented KeyValuePair buffer, iterate it by index, skip entries whose spawner has since been deleted or unregistered, and return the buffer cleared in a finally. OnSkillUse also returns early when nothing is registered, which is the common case on a shard with no skill triggers. Parser hardening: Enum.TryParse accepts any numeric string, so both SkillTrigger.Parse and XmlSpawnerMigrator.MapSkillTrigger now require Enum.IsDefined; a failed range parse keeps the documented default of 10 rather than falling to 0 (and so to 1 through Math.Max); MapSkillTrigger rejects an inverted value window down the same warning path as an unknown skill name, and documents that an absent min reads as 0. TriggerSystem.ParseTrigger logs a warning when a registered factory returns null, which was silently swallowed, and its three Console.WriteLine calls become Logger calls. The test host seeds a fixed 2020-01-01T12:00:00Z rather than DateTime.UtcNow, so wall-clock-sensitive triggers take the same branch on every run, and AdvanceClock ignores a non-positive span per its documented forward-only contract. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- .../ModernSpawnerTriggerRegistrationTests.cs | 28 ++++ .../Fixtures/ModernSpawnerTestServer.cs | 24 ++- .../Triggers/TriggerParsingTests.cs | 29 ++++ .../Migration/XmlSpawnerMigrator.cs | 19 ++- .../Triggers/ProximityTrigger.cs | 5 +- .../ModernSpawner/Triggers/SkillTrigger.cs | 36 +++-- .../ModernSpawner/Triggers/TriggerSystem.cs | 149 +++++++++++++----- 7 files changed, 229 insertions(+), 61 deletions(-) diff --git a/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs index 8f04d92..243f13d 100644 --- a/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs +++ b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs @@ -187,6 +187,8 @@ public void SpawnPointMigrator_RunningFalse_ProducesStoppedSpawner() [InlineData("Mining", "skill:Mining:8:0:False:5")] [InlineData("Mining+", "skill:Mining+:8:0:False:5")] [InlineData("Magery-,50,90", "skill:Magery-:8:50-90:False:5")] + // An absent min is 0, the reading XmlSpawner itself gave "SkillName,,max". + [InlineData("Magery,,90", "skill:Magery:8:0-90:False:5")] public void Migrator_MapsSkillTriggerAttribute(string xml, string expected) { var node = ParseNode($""); @@ -200,4 +202,30 @@ public void Migrator_MapsSkillTriggerAttribute(string xml, string expected) spawner.Delete(); } } + + [Theory] + // An unknown skill name, and a window whose max is below its min: both are logged and dropped, so no + // skill definition reaches the spawner. TriggerActivated still comes from ProximityRange on the node, + // which proves the rejected attribute neither set it nor cleared it. + [InlineData("NotASkill")] + [InlineData("99")] + [InlineData("Magery,90,50")] + public void Migrator_RejectsMalformedSkillTriggerAttribute(string xml) + { + var node = ParseNode($""); + var spawner = XmlSpawnerMigrator.ParseXmlSpawnerNode(node); + try + { + Assert.DoesNotContain(spawner.TriggerDefinitions, d => d.StartsWith("skill:", StringComparison.Ordinal)); + + // The proximity definition from the same node is untouched, so this is a targeted rejection + // rather than the whole trigger block being lost. + Assert.Contains("proximity:8:true:false:5:0", spawner.TriggerDefinitions); + Assert.True(spawner.TriggerActivated); + } + finally + { + spawner.Delete(); + } + } } diff --git a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs index d6c565b..81d00cb 100644 --- a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs +++ b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs @@ -24,6 +24,12 @@ namespace Server.Engines.ModernSpawner.Tests.Fixtures; /// public static class ModernSpawnerTestServer { + /// + /// The instant is seeded to. Fixed so that wall-clock-sensitive triggers + /// take the same branch on every run; noon UTC is outside the night windows the trigger tests use. + /// + public static readonly DateTime FixedStartTime = new(2020, 1, 1, 12, 0, 0, DateTimeKind.Utc); + private static readonly Lock _lock = new(); private static bool _initialized; @@ -51,7 +57,11 @@ public static void Initialize() // grants this assembly InternalsVisibleTo, so the same seam is available. Without it // Core.Now stays DateTime.MinValue and anything comparing against an absolute wall clock // (cooldowns, time windows) reads as "never elapsed". - Core._now = DateTime.UtcNow; + // + // A fixed instant rather than DateTime.UtcNow: a real clock makes every wall-time window + // test depend on when the suite happens to run, so a run at 19:00 and a run at 09:00 would + // exercise different branches. Noon UTC sits outside the night windows the trigger tests use. + Core._now = FixedStartTime; // The timer wheel must exist before NetState.Configure(), which schedules a recurring // sweep through Timer.DelayCall (production order in Main.cs: Timer.Init runs before @@ -101,6 +111,14 @@ public static void Initialize() /// test left it. /// /// - /// How far forward to move . Must not be negative. - public static void AdvanceClock(TimeSpan by) => Core._now += by; + /// How far forward to move . A negative span is ignored. + public static void AdvanceClock(TimeSpan by) + { + if (by <= TimeSpan.Zero) + { + return; + } + + Core._now += by; + } } diff --git a/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs b/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs index ac30c83..60e57bb 100644 --- a/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs +++ b/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs @@ -318,5 +318,34 @@ public void SkillTrigger_Evaluate_HonoursOutcomeAndWindow() Assert.False(failure.MatchesContext(SkillName.Magery, 0.0, true)); } + [Theory] + // The empty window segment is the regression: IndexOf('-', 1) threw on "" because startIndex 1 is + // past the end of a zero-length string, so a hand-edited or round-tripped definition took down the + // whole ActivateTriggers pass rather than being skipped. + [InlineData("skill:Mining:10::false:5")] + [InlineData("skill:Mining:10:")] + // TryParse accepts any numeric string, so these must be rejected on the enum, not the parse. + [InlineData("skill:99:10")] + [InlineData("skill:-1:10")] + // An inverted window can never match. + [InlineData("skill:Magery:5:90-50")] + public void SkillTrigger_Parse_MalformedDefinition_ReturnsNullWithoutThrowing(string definition) + { + var trigger = SkillTrigger.Parse(definition); + + Assert.Null(trigger); + } + + [Fact] + public void SkillTrigger_Parse_UnparseableRange_KeepsTheDocumentedDefault() + { + // int.TryParse writes 0 on failure; the default is 10, and Math.Max(1, 0) would have silently + // made this a 1-tile trigger. + var trigger = SkillTrigger.Parse("skill:Mining:wide"); + + Assert.NotNull(trigger); + Assert.Equal(10, trigger.Range); + } + #endregion } diff --git a/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs b/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs index 7f9aa2b..683cb69 100644 --- a/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs +++ b/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs @@ -335,7 +335,10 @@ internal static ModernSpawner ParseSpawnPointNode(XmlNode node) /// /// XmlSpawner SkillTrigger is SkillName[+|-][,min[,max]]; the modern grammar keeps the - /// suffix and folds min/max into the value window. Returns null when the skill name is unknown. + /// suffix and folds min/max into the value window. An absent or unparseable bound is dropped rather + /// than rejected, so "Mining,,90" is min 0 / max 90 - the same reading XmlSpawner gave it. + /// Returns null, after a warning, when the skill name is unknown or the window is inverted + /// (max < min); the caller then adds no definition at all. /// internal static string MapSkillTrigger(string xml, int range) { @@ -348,7 +351,10 @@ internal static string MapSkillTrigger(string xml, int range) name = name[..^1]; } - if (!name.Equals("Any", StringComparison.OrdinalIgnoreCase) && !Enum.TryParse(name, true, out _)) + // TryParse accepts any numeric string ("99") as a SkillName, so the value has to be checked + // against the enum as well. + if (!name.Equals("Any", StringComparison.OrdinalIgnoreCase) && + (!Enum.TryParse(name, true, out var parsedSkill) || !Enum.IsDefined(parsedSkill))) { Logger.Warning("Skipping SkillTrigger with unknown skill name: {SkillName}", name); return null; @@ -356,6 +362,15 @@ internal static string MapSkillTrigger(string xml, int range) var min = parts.Length > 1 && double.TryParse(parts[1], out var parsedMin) && parsedMin > 0 ? parsedMin : 0; var max = parts.Length > 2 && double.TryParse(parts[2], out var parsedMax) && parsedMax > 0 ? parsedMax : -1; + + // An inverted window can never match, and SkillTrigger.Parse rejects it too; drop the whole + // trigger down the same path as an unknown name rather than emitting a definition that dies later. + if (max >= 0 && max < min) + { + Logger.Warning("Skipping SkillTrigger with inverted value window: {SkillTrigger}", xml); + return null; + } + var window = max < 0 ? $"{min}" : $"{min}-{max}"; return $"skill:{name}{suffix}:{range}:{window}:False:5"; } diff --git a/Projects/ModernSpawner/Triggers/ProximityTrigger.cs b/Projects/ModernSpawner/Triggers/ProximityTrigger.cs index 58bfeb2..db56906 100644 --- a/Projects/ModernSpawner/Triggers/ProximityTrigger.cs +++ b/Projects/ModernSpawner/Triggers/ProximityTrigger.cs @@ -81,8 +81,9 @@ public bool Evaluate(TriggerContext context) return false; } - // Check line of sight - if (RequireLineOfSight && !mobile.CanSee(_spawner)) + // Line of sight, not visibility: Mobile.CanSee(Item) ends in item.Visible, and a spawner is + // Visible = false, so CanSee could never pass here for a player. + if (RequireLineOfSight && !mobile.InLOS(_spawner)) { return false; } diff --git a/Projects/ModernSpawner/Triggers/SkillTrigger.cs b/Projects/ModernSpawner/Triggers/SkillTrigger.cs index 9cbf454..8c0fd0b 100644 --- a/Projects/ModernSpawner/Triggers/SkillTrigger.cs +++ b/Projects/ModernSpawner/Triggers/SkillTrigger.cs @@ -100,6 +100,13 @@ public bool Evaluate(TriggerContext context) return false; } + // Skill, outcome and value window first: a skill attempt that this trigger does not react to + // is the common case, and it must not pay for the cooldown, map, range and LOS checks below. + if (!MatchesContext(context.UsedSkill, context.SkillValue, context.SkillSuccess)) + { + return false; + } + // Check cooldown if (Core.Now - _lastTriggered < Cooldown) { @@ -118,13 +125,9 @@ public bool Evaluate(TriggerContext context) return false; } - // Check LOS if required - if (RequireLOS && !mobile.CanSee(_spawner)) - { - return false; - } - - if (!MatchesContext(context.UsedSkill, context.SkillValue, context.SkillSuccess)) + // Line of sight, not visibility: Mobile.CanSee(Item) ends in item.Visible, and a spawner is + // Visible = false, so CanSee could never pass here for a player. + if (RequireLOS && !mobile.InLOS(_spawner)) { return false; } @@ -217,16 +220,19 @@ public static SkillTrigger Parse(string definition) { anySkill = true; } - else if (!Enum.TryParse(skillName, true, out skill)) + // TryParse accepts any numeric string ("99") as a SkillName, so the value has to be checked + // against the enum as well. + else if (!Enum.TryParse(skillName, true, out skill) || !Enum.IsDefined(skill)) { return null; } - // Parse range (default: 10) + // Parse range (default: 10). int.TryParse writes 0 on failure, so only a successful parse + // may replace the default. var range = 10; - if (parts.Length > startIndex + 1) + if (parts.Length > startIndex + 1 && int.TryParse(parts[startIndex + 1], out var parsedRange)) { - int.TryParse(parts[startIndex + 1], out range); + range = parsedRange; } // Parse min/max skill value window (default: 0 / -1) @@ -235,6 +241,14 @@ public static SkillTrigger Parse(string definition) if (parts.Length > startIndex + 2) { var value = parts[startIndex + 2]; + + // An empty window segment ("skill:Mining:10::false:5") is malformed rather than a default: + // it is also what made IndexOf(char, 1) throw, since startIndex 1 is past the end of "". + if (value.Length == 0) + { + return null; + } + var dashIndex = value.IndexOf('-', 1); if (dashIndex > 0) { diff --git a/Projects/ModernSpawner/Triggers/TriggerSystem.cs b/Projects/ModernSpawner/Triggers/TriggerSystem.cs index 0af4f53..0909b22 100644 --- a/Projects/ModernSpawner/Triggers/TriggerSystem.cs +++ b/Projects/ModernSpawner/Triggers/TriggerSystem.cs @@ -1,5 +1,7 @@ using System; using System.Collections.Generic; +using Server.Buffers; +using Server.Logging; namespace Server.Engines.ModernSpawner.Triggers; @@ -14,6 +16,8 @@ public class TriggerSystem : ITriggerSystem /// public static TriggerSystem Instance { get; } = new(); + private static readonly ILogger Logger = LogFactory.GetLogger(typeof(TriggerSystem)); + private readonly Dictionary> _factories = new(StringComparer.OrdinalIgnoreCase); // Registered triggers by type for event routing @@ -62,15 +66,24 @@ public ITrigger ParseTrigger(string definition) { try { - return factory(definition); + var trigger = factory(definition); + if (trigger == null) + { + // A registered factory returning null means a malformed definition, which used to be + // swallowed: the spawner silently lost the trigger with nothing in the log. + Logger.Warning("Malformed {TriggerType} trigger definition: {Definition}", triggerType, definition); + } + + return trigger; } catch (Exception ex) { - Console.WriteLine($"Failed to parse trigger '{definition}': {ex.Message}"); + Logger.Warning(ex, "Failed to parse trigger definition: {Definition}", definition); + return null; } } - Console.WriteLine($"Unknown trigger type: {triggerType}"); + Logger.Warning("Unknown trigger type: {TriggerType}", triggerType); return null; } @@ -355,53 +368,82 @@ public void OnSkillUse(Mobile mobile, Skill skill, bool success) return; } + var registered = _skillTriggers.Count; + if (registered == 0) + { + return; + } + var skillName = skill.SkillName; var skillValue = skill.Value; - // Check all registered skill triggers - foreach (var (spawner, triggers) in _skillTriggers) + // spawner.Trigger() runs Spawn() and any attached script, and a DESPAWN script - or a spawned + // ModernSpawner - can delete or register a spawner that carries a skill trigger. Dispatching off + // a snapshot keeps that from invalidating the enumerator mid-loop. + var pool = STArrayPool>>.Shared; + var snapshot = pool.Rent(registered); + + try { - if (spawner.Map != mobile.Map) + var taken = 0; + foreach (var entry in _skillTriggers) { - continue; + snapshot[taken++] = entry; } - // Cheap pre-scan: most spawners hold triggers for other skills, and those must not pay for a - // context. Indexed loops here so this path has no enumerator and no closure. - var firstMatch = -1; - for (var i = 0; i < triggers.Count; i++) + for (var s = 0; s < taken; s++) { - if (triggers[i].MatchesSkill(skillName)) + var (spawner, triggers) = snapshot[s]; + + // The snapshot can name a spawner that an earlier iteration of this dispatch deleted or + // unregistered. + if (spawner.Deleted || spawner.Map != mobile.Map || !_skillTriggers.ContainsKey(spawner)) { - firstMatch = i; - break; + continue; } - } - if (firstMatch < 0) - { - continue; - } + // Cheap pre-scan: most spawners hold triggers for other skills, and those must not pay for + // a context. Indexed loops here so this path has no enumerator and no closure. + var firstMatch = -1; + for (var i = 0; i < triggers.Count; i++) + { + if (triggers[i].MatchesSkill(skillName)) + { + firstMatch = i; + break; + } + } - var context = new TriggerContext(spawner) - { - TriggeringMobile = mobile, - UsedSkill = skillName, - SkillValue = skillValue, - SkillSuccess = success - }; - - // Everything before firstMatch is already known not to match this skill. - for (var i = firstMatch; i < triggers.Count; i++) - { - var trigger = triggers[i]; - if (trigger.MatchesSkill(skillName) && trigger.Evaluate(context)) + if (firstMatch < 0) { - spawner.Trigger(); - break; // Only trigger once per spawner per skill use + continue; + } + + var context = new TriggerContext(spawner) + { + TriggeringMobile = mobile, + UsedSkill = skillName, + SkillValue = skillValue, + SkillSuccess = success + }; + + // Everything before firstMatch is already known not to match this skill. + for (var i = firstMatch; i < triggers.Count; i++) + { + var trigger = triggers[i]; + if (trigger.MatchesSkill(skillName) && trigger.Evaluate(context)) + { + spawner.Trigger(); + break; // Only trigger once per spawner per skill use + } } } } + finally + { + // Cleared: the buffer outlives this call inside the pool, and it holds spawner references. + pool.Return(snapshot, true); + } } private void StartTimeOfDayTimer() @@ -424,24 +466,45 @@ private void CheckTimeOfDayTriggers() return; } - foreach (var (spawner, triggers) in _timeOfDayTriggers) + // Same hazard as OnSkillUse: spawner.Trigger() runs Spawn() and any attached script, which can + // delete or register a spawner carrying a time-of-day trigger. Dispatch off a snapshot. + var pool = STArrayPool>>.Shared; + var snapshot = pool.Rent(_timeOfDayTriggers.Count); + + try { - if (spawner.Deleted || !spawner.Running) + var taken = 0; + foreach (var entry in _timeOfDayTriggers) { - continue; + snapshot[taken++] = entry; } - var context = new TriggerContext(spawner); - - foreach (var trigger in triggers) + for (var s = 0; s < taken; s++) { - if (trigger.Evaluate(context)) + var (spawner, triggers) = snapshot[s]; + + if (spawner.Deleted || !spawner.Running || !_timeOfDayTriggers.ContainsKey(spawner)) + { + continue; + } + + var context = new TriggerContext(spawner); + + for (var i = 0; i < triggers.Count; i++) { - spawner.Trigger(); - break; + if (triggers[i].Evaluate(context)) + { + spawner.Trigger(); + break; + } } } } + finally + { + // Cleared: the buffer outlives this call inside the pool, and it holds spawner references. + pool.Return(snapshot, true); + } } /// From 9615c8c80151c0c937a5f854a3de3be81bcaceea Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:00:15 -0700 Subject: [PATCH 8/9] docs: skill row audit status; skill triggers marked tested The audit-status column in product-spec.md records what the audit at the pinned commit found, not what this branch has since built. The skill row was flipped to "Implemented" when the wiring landed, which erases the finding the column exists to carry; it goes back to "Stubbed", with the as-built description staying in the v1-target cell, now noting that RequireLOS is line of sight rather than visibility. architecture.md marks the skill trigger "yes, tested" alongside kill, and its skill bullet says spawners with no matching trigger allocate nothing (they still pay one map compare and a linear scan of their trigger list) rather than "pay nothing"; the InLOS and snapshot-dispatch facts are recorded there too. xmlspawner-migration.md keeps the "never fired" claim but states it as what was verified: the XmlSpawner sources, ServUO and the ModernUO port alike, declare the parsed skill-trigger fields and read them, but never assign them. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- dev-docs/architecture.md | 9 ++++++--- dev-docs/product-spec.md | 2 +- dev-docs/xmlspawner-migration.md | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/dev-docs/architecture.md b/dev-docs/architecture.md index 937c797..acef6ad 100644 --- a/dev-docs/architecture.md +++ b/dev-docs/architecture.md @@ -91,7 +91,7 @@ is tracked for serialization before triggers are re-registered. Deactivation is | proximity | `Item.OnMovement` (24-tile radius, engine-fixed) | yes | | speech | `Item.OnSpeech` (15/18-tile radius) | yes | | kill | `OnSpawnedDeath` via `BaseSpawner.NotifySpawnedDeath`, called from `BaseCreature.OnDeath` | yes, tested | -| skill | `SkillEvents.SkillUsed` → `ModernSpawnerEvents.OnSkillUsed` (players only) → `TriggerSystem.OnSkillUse` | yes | +| skill | `SkillEvents.SkillUsed` → `ModernSpawnerEvents.OnSkillUsed` (players only) → `TriggerSystem.OnSkillUse` | yes, tested | | timeofday | 2.5 s polling timer | yes | | game_time_window | one transition timer | yes (wrong clock constant) | | wall_time_window | `EventScheduler` + `BaseScheduledEvent` subclass | yes (close-edge filter bug) | @@ -300,8 +300,11 @@ differences from the original plan noted inline. bool success)` once per attempt (short-circuited attempts included; not raised when the mobile lacks the skill). `ModernSpawnerEvents.OnSkillUsed` forwards only players (`mobile is { Player: true }`) to `TriggerSystem.OnSkillUse`, which pre-scans `SkillTrigger.MatchesSkill` before allocating a - `TriggerContext` so spawners with no matching trigger pay nothing. `SkillTrigger` adds an outcome filter - (any/success/failure) and a min/max skill-value window on top of range and line-of-sight. + `TriggerContext` so spawners with no matching trigger allocate nothing (one map compare and a linear scan + of their trigger list). `SkillTrigger` adds an outcome filter (any/success/failure) and a min/max + skill-value window on top of range and line-of-sight. Line of sight is `Mobile.InLOS`: `CanSee` ends in + `Item.Visible`, which a spawner never is. Dispatch iterates a pooled snapshot of the registration map, + because `Trigger()` reaches `Spawn()` and a script there can delete or register a spawner. - **Grammar.** One definition grammar owned by each trigger's `Serialize()`. Gumps and importers construct trigger objects. `TriggerContext` becomes a `readonly record struct`. - **Extended proximity.** Clamp to `Core.GlobalMaxUpdateRange` with a warning; the sector-range diff --git a/dev-docs/product-spec.md b/dev-docs/product-spec.md index 2e260c5..71b0978 100644 --- a/dev-docs/product-spec.md +++ b/dev-docs/product-spec.md @@ -88,7 +88,7 @@ Status columns reflect the audit at `8935ca4`. "Target" is the v1 commitment. | Proximity beyond 24 tiles | Stubbed | Range clamped with a warning; wider ranges need a ModernUO area-subscription API (tracked in `modernuo-prerequisites.md`) | | Speech | Implemented | Kept; regex timeout; whether it may wake a stopped spawner is per-trigger (`wake:`) under **D2** | | Kill | Stubbed | Wired via a new upstream `BaseSpawner.OnSpawnedDeath` hook (the creature-death event fires after the spawner link is cleared) | -| Skill | Implemented | Wired to ModernUO's `SkillEvents.SkillUsed`, forwarded to players only (**D3**); outcome (any/success/failure) and min/max value-window semantics; grammar owned by `SkillTrigger.Serialize()` | +| Skill | Stubbed | Wired to ModernUO's `SkillEvents.SkillUsed`, forwarded to players only (**D3**); outcome (any/success/failure) and min/max value-window semantics; `RequireLOS` is line of sight (`Mobile.InLOS`), not visibility; grammar owned by `SkillTrigger.Serialize()` | | Game-time window | Partial | Constant derived from `Clock.SecondsPerUOMinute`; recomputed on map change | | Wall-clock window | Partial | Day/month filters apply to the open edge only; weekly/monthly recurrence exposed | | Legacy `timeofday` | Implemented | Retired in favour of `game_time_window` (importer maps to it) | diff --git a/dev-docs/xmlspawner-migration.md b/dev-docs/xmlspawner-migration.md index 54b5146..9eee995 100644 --- a/dev-docs/xmlspawner-migration.md +++ b/dev-docs/xmlspawner-migration.md @@ -82,7 +82,7 @@ older RunUO XmlSpawner2 exports may differ and are reported, not silently accept | `ProximityTriggerSound`, `ProximityTriggerMessage` | trigger `onTriggered` feedback: `sound(id)`, `msg(trigMob, "text")` | fires on an *accepted* trigger with the triggering mobile (`XmlSpawner.cs:2324`), not on activate | | `TriggerProbability` (fraction) | spawner-level trigger `chance` | one roll per accepted trigger, not per trigger type | | `SpeechTrigger` | trigger `speech:text` | one case-insensitive substring match (`XmlSpawner.cs:2385`); do not split on commas | -| `SkillTrigger` | trigger `skill:[+\|-]::[-]::` | XmlSpawner syntax `SkillName[+/-][,min,max]` (`+` success only, `-` failure only) maps directly onto the outcome suffix and value window; `` is the node's `ProximityRange` (10 when absent); XmlSpawner's own skill trigger never fired in ServUO (its parser was unwired), so there is no runtime behaviour to preserve — only the intended semantics carry over | +| `SkillTrigger` | trigger `skill:[+\|-]::[-]::` | XmlSpawner syntax `SkillName[+/-][,min,max]` (`+` success only, `-` failure only) maps directly onto the outcome suffix and value window; `` is the node's `ProximityRange` (10 when absent); XmlSpawner's own skill trigger never fired (verified in both the ServUO sources and the ModernUO port: the parsed skill-trigger fields are declared and read, but never assigned), so there is no runtime behaviour to preserve — only the intended semantics carry over | | `TODStart`, `TODEnd`, `TODMode` | `game_time_window` (mode 1) / `wall_time_window` (mode 0) | minutes → hour:minute | | `MinRefractory`, `MaxRefractory` | spawner-level trigger refractory `random(min,max)` | belongs to the spawner's accepted-trigger state, not to each translated trigger | | `KillReset` | kill trigger `resetAfterTicks` | count of spawn ticks without a kill before the kill counter resets (`XmlSpawner.cs:6735`) — add field or warn | From 6a23144587b1a98a75037cbedd09c850430a4565 Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:10:31 -0700 Subject: [PATCH 9/9] perf(triggers): clear only the written snapshot entries on return Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr --- Projects/ModernSpawner/Triggers/TriggerSystem.cs | 16 ++++++++++------ dev-docs/architecture.md | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/Projects/ModernSpawner/Triggers/TriggerSystem.cs b/Projects/ModernSpawner/Triggers/TriggerSystem.cs index 0909b22..b0c5750 100644 --- a/Projects/ModernSpawner/Triggers/TriggerSystem.cs +++ b/Projects/ModernSpawner/Triggers/TriggerSystem.cs @@ -383,9 +383,9 @@ public void OnSkillUse(Mobile mobile, Skill skill, bool success) var pool = STArrayPool>>.Shared; var snapshot = pool.Rent(registered); + var taken = 0; try { - var taken = 0; foreach (var entry in _skillTriggers) { snapshot[taken++] = entry; @@ -441,8 +441,10 @@ public void OnSkillUse(Mobile mobile, Skill skill, bool success) } finally { - // Cleared: the buffer outlives this call inside the pool, and it holds spawner references. - pool.Return(snapshot, true); + // Clear only the entries written: the buffer outlives this call inside the pool and holds + // spawner references, but the bucket-sized array can be far larger than `taken`. + snapshot.AsSpan(0, taken).Clear(); + pool.Return(snapshot); } } @@ -471,9 +473,9 @@ private void CheckTimeOfDayTriggers() var pool = STArrayPool>>.Shared; var snapshot = pool.Rent(_timeOfDayTriggers.Count); + var taken = 0; try { - var taken = 0; foreach (var entry in _timeOfDayTriggers) { snapshot[taken++] = entry; @@ -502,8 +504,10 @@ private void CheckTimeOfDayTriggers() } finally { - // Cleared: the buffer outlives this call inside the pool, and it holds spawner references. - pool.Return(snapshot, true); + // Clear only the entries written: the buffer outlives this call inside the pool and holds + // spawner references, but the bucket-sized array can be far larger than `taken`. + snapshot.AsSpan(0, taken).Clear(); + pool.Return(snapshot); } } diff --git a/dev-docs/architecture.md b/dev-docs/architecture.md index acef6ad..7d1db80 100644 --- a/dev-docs/architecture.md +++ b/dev-docs/architecture.md @@ -300,8 +300,8 @@ differences from the original plan noted inline. bool success)` once per attempt (short-circuited attempts included; not raised when the mobile lacks the skill). `ModernSpawnerEvents.OnSkillUsed` forwards only players (`mobile is { Player: true }`) to `TriggerSystem.OnSkillUse`, which pre-scans `SkillTrigger.MatchesSkill` before allocating a - `TriggerContext` so spawners with no matching trigger allocate nothing (one map compare and a linear scan - of their trigger list). `SkillTrigger` adds an outcome filter (any/success/failure) and a min/max + `TriggerContext` so spawners with no matching trigger allocate nothing (a deleted check, a map compare, a + registry lookup and a linear scan of their trigger list). `SkillTrigger` adds an outcome filter (any/success/failure) and a min/max skill-value window on top of range and line-of-sight. Line of sight is `Mobile.InLOS`: `CanSee` ends in `Item.Visible`, which a spawner never is. Dispatch iterates a pooled snapshot of the registration map, because `Trigger()` reaches `Spawn()` and a script there can delete or register a spawner.