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/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/Core/ModernSpawnerTriggerRegistrationTests.cs b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs index 12a2198..243f13d 100644 --- a/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs +++ b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs @@ -182,4 +182,50 @@ 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")] + // 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($""); + var spawner = XmlSpawnerMigrator.ParseXmlSpawnerNode(node); + try + { + Assert.Contains(expected, spawner.TriggerDefinitions); + } + finally + { + 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/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..81d00cb 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; @@ -23,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; @@ -46,13 +53,15 @@ 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". + // + // 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 @@ -91,4 +100,25 @@ 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. + /// + /// 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 . 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/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 diff --git a/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs b/Projects/ModernSpawner.Tests/Triggers/TriggerParsingTests.cs index 59c03ed..60e57bb 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,118 @@ 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)); + } + + [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 139fab3..683cb69 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,48 @@ 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. 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) + { + var parts = xml.Split(','); + var name = parts[0].Trim(); + var suffix = ""; + if (name.EndsWith('+') || name.EndsWith('-')) + { + suffix = name[^1..]; + name = name[..^1]; + } + + // 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; + } + + 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"; + } + /// /// Converts XmlSpawner property format (prop/value/prop/value) to ModernSpawner format. /// 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/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/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 32d3a32..e50dadf 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 is not { 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/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 37e0733..8c0fd0b 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; } @@ -77,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) { @@ -95,53 +125,57 @@ public bool Evaluate(TriggerContext context) return false; } - // Check LOS if required - if (RequireLOS && !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 (RequireLOS && !mobile.InLOS(_spawner)) { return false; } - // Check skill value if required - if (MinSkillValue > 0 && context.UsedSkill != SkillName.Alchemy) // SkillName.Alchemy is used as "any" - { - var skill = mobile.Skills[context.UsedSkill]; - if (skill == null || skill.Value < MinSkillValue) - { - 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,31 +198,72 @@ 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)) + // 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 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]; + + // 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) + { + 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 +280,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); } } diff --git a/Projects/ModernSpawner/Triggers/TriggerSystem.cs b/Projects/ModernSpawner/Triggers/TriggerSystem.cs index 9914d76..b0c5750 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; } @@ -339,36 +352,100 @@ 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: 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. + /// 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 registered = _skillTriggers.Count; + if (registered == 0) { return; } - // Check all registered skill triggers - foreach (var (spawner, triggers) in _skillTriggers) + var skillName = skill.SkillName; + var skillValue = skill.Value; + + // 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); + + var taken = 0; + try { - if (spawner.Map != mobile.Map) + foreach (var entry in _skillTriggers) { - continue; + snapshot[taken++] = entry; } - var context = new TriggerContext(spawner) + for (var s = 0; s < taken; s++) { - TriggeringMobile = mobile, - UsedSkill = skill - }; + var (spawner, triggers) = snapshot[s]; - foreach (var trigger in triggers) - { - if (trigger.MatchesSkill(skill) && trigger.Evaluate(context)) + // 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)) + { + 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) { - 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 + { + // 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); + } } private void StartTimeOfDayTimer() @@ -391,24 +468,47 @@ 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); + + var taken = 0; + try { - if (spawner.Deleted || !spawner.Running) + 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)) { - spawner.Trigger(); - break; + continue; + } + + var context = new TriggerContext(spawner); + + for (var i = 0; i < triggers.Count; i++) + { + if (triggers[i].Evaluate(context)) + { + spawner.Trigger(); + break; + } } } } + finally + { + // 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 843b789..7d1db80 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, 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) | @@ -296,9 +296,15 @@ 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 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. - **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 +406,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..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 | 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 | 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) | @@ -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..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: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 (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 |