diff --git a/CLAUDE.md b/CLAUDE.md index 9415134..287ea2e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,6 +62,11 @@ ModernSpawner-specific: 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. +- 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 + not idempotent, and within `Projects/ModernSpawner` `EnsureTriggersActive` is its only caller (tests call + it deliberately, to build the stale registrations teardown has to survive). ## ModernUO changes diff --git a/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs new file mode 100644 index 0000000..12a2198 --- /dev/null +++ b/Projects/ModernSpawner.Tests/Core/ModernSpawnerTriggerRegistrationTests.cs @@ -0,0 +1,185 @@ +using System; +using System.Xml; +using Server.Engines.ModernSpawner.Migration; +using Server.Engines.ModernSpawner.Triggers; +using Xunit; + +namespace Server.Engines.ModernSpawner.Tests; + +/// +/// Trigger registration follows every flag and list change: the +/// setter registers and unregisters immediately, and stopping or deleting a spawner tears the registration +/// down regardless of what the flag says at that moment. +/// +[Collection("Sequential ModernSpawner Tests")] +public class ModernSpawnerTriggerRegistrationTests +{ + private const string Proximity = "proximity:8:true"; + + private static ModernSpawner Place() + { + var spawner = new ModernSpawner(1, TimeSpan.FromMinutes(5), TimeSpan.FromMinutes(10), 0, default, "Rabbit"); + spawner.MoveToWorld(new Point3D(1500, 1500, 0), Map.Felucca); + return spawner; + } + + [Fact] + public void TogglingTriggerActivated_RegistersAndUnregisters() + { + var spawner = Place(); + spawner.AddToTriggerDefinitions(Proximity); + Assert.False(spawner.HandlesOnMovement); + + spawner.TriggerActivated = true; + Assert.True(spawner.HandlesOnMovement); + + spawner.TriggerActivated = false; + Assert.False(spawner.HandlesOnMovement); + spawner.Delete(); + } + + [Fact] + public void AddingDefinitionToActivatedRunningSpawner_RegistersImmediately() + { + var spawner = Place(); + spawner.TriggerActivated = true; + Assert.False(spawner.HandlesOnMovement); + + spawner.AddToTriggerDefinitions(Proximity); + spawner.EnsureTriggersActive(); + Assert.True(spawner.HandlesOnMovement); + + spawner.RemoveFromTriggerDefinitions(Proximity); + spawner.EnsureTriggersActive(); + Assert.False(spawner.HandlesOnMovement); + spawner.Delete(); + } + + [Fact] + public void DeletingAnActivatedSpawner_LeavesNothingRegistered() + { + var spawner = Place(); + spawner.AddToTriggerDefinitions(Proximity); + spawner.TriggerActivated = true; + Assert.True(spawner.HandlesOnMovement); + + spawner.TriggerActivated = false; + spawner.TriggerActivated = true; + spawner.Delete(); + + Assert.False(TriggerSystem.Instance.IsRegistered(spawner)); + } + + [Fact] + public void Stop_UnregistersEvenWhenFlagWasClearedAfterRegistration() + { + var spawner = Place(); + spawner.AddToTriggerDefinitions(Proximity); + spawner.TriggerActivated = true; + spawner.Stop(); + Assert.False(TriggerSystem.Instance.IsRegistered(spawner)); + + // A registration that outlived its flag - the state a raw field write or a pre-fix gump edit + // could leave behind. Teardown does not consult the flag, so it still has to be cleaned up. + spawner.TriggerActivated = false; + spawner.Start(); + Assert.False(TriggerSystem.Instance.IsRegistered(spawner)); + + TriggerSystem.Instance.ActivateTriggers(spawner); + Assert.True(TriggerSystem.Instance.IsRegistered(spawner)); + + spawner.Stop(); + Assert.False(TriggerSystem.Instance.IsRegistered(spawner)); + spawner.Delete(); + } + + [Fact] + public void GumpTimeTriggerDefinitions_ParseAndRegister() + { + // Built exactly as TriggerConfigGump builds them from its hour fields: the registered type + // names, in the argument format each Parse accepts. + const int startHour = 18; + const int endHour = 6; + var wallTime = $"wall_time_window:{startHour}:0:{endHour}:0"; + const string gameTime = "game_time_window:21:5:true"; + + // ParseTrigger returns null for an unrecognised type (it only logs), so this pins the branch. + Assert.NotNull(TriggerSystem.Instance.ParseTrigger(wallTime)); + Assert.NotNull(TriggerSystem.Instance.ParseTrigger(gameTime)); + + var spawner = Place(); + spawner.AddToTriggerDefinitions(wallTime); + spawner.AddToTriggerDefinitions(gameTime); + spawner.TriggerActivated = true; + + Assert.True(TriggerSystem.Instance.IsRegistered(spawner)); + spawner.Delete(); + } + + [Fact] + public void DeletingAStoppedSpawner_WithStaleRegistration_Unregisters() + { + var spawner = Place(); + spawner.AddToTriggerDefinitions(Proximity); + spawner.Stop(); // Running false: OnStopped is out of the picture + TriggerSystem.Instance.ActivateTriggers(spawner); // stale registration behind a false flag + Assert.True(TriggerSystem.Instance.IsRegistered(spawner)); + + spawner.Delete(); // only OnDelete can clean this up + Assert.False(TriggerSystem.Instance.IsRegistered(spawner)); + } + + private static XmlNode ParseNode(string xml) + { + var doc = new XmlDocument(); + doc.LoadXml(xml); + return doc.DocumentElement; + } + + // ProximityRange is the attribute ParseXmlSpawnerNode maps to a proximity trigger definition plus + // TriggerActivated = true, so this form exercises trigger (de)registration alongside Running. + private static string XmlSpawnerNode(string running) + { + var runningAttribute = running != null ? $" Running=\"{running}\"" : string.Empty; + return ""; + } + + // The SpawnPoint form has no trigger-mapped attribute, so these tests assert Running only. + private static string SpawnPointNode(string running) + { + var runningAttribute = running != null ? $" Running=\"{running}\"" : string.Empty; + return ""; + } + + [Fact] + public void Migrator_RunningFalse_ProducesStoppedSpawnerWithNoRegisteredTriggers() + { + var stopped = XmlSpawnerMigrator.ParseXmlSpawnerNode(ParseNode(XmlSpawnerNode("false"))); + Assert.False(stopped.Running); + Assert.False(TriggerSystem.Instance.IsRegistered(stopped)); + stopped.Delete(); + + // Running="true" (the same construction path) must still register and run, so the fix for the + // false case did not just make everything stop. + var running = XmlSpawnerMigrator.ParseXmlSpawnerNode(ParseNode(XmlSpawnerNode("true"))); + Assert.True(running.Running); + Assert.True(TriggerSystem.Instance.IsRegistered(running)); + running.Delete(); + } + + [Fact] + public void SpawnPointMigrator_RunningFalse_ProducesStoppedSpawner() + { + var stopped = XmlSpawnerMigrator.ParseSpawnPointNode(ParseNode(SpawnPointNode("false"))); + Assert.False(stopped.Running); + stopped.Delete(); + + // Running absent defaults to true - the historical "always start" behavior for a form that had + // no Running attribute before this fix. + var running = XmlSpawnerMigrator.ParseSpawnPointNode(ParseNode(SpawnPointNode(null))); + Assert.True(running.Running); + running.Delete(); + } +} diff --git a/Projects/ModernSpawner.Tests/Core/XmlSpawnerImporterEntryDelayTests.cs b/Projects/ModernSpawner.Tests/Core/XmlSpawnerImporterEntryDelayTests.cs new file mode 100644 index 0000000..b21a038 --- /dev/null +++ b/Projects/ModernSpawner.Tests/Core/XmlSpawnerImporterEntryDelayTests.cs @@ -0,0 +1,108 @@ +using System; +using System.IO; +using Server.Engines.ModernSpawner.Serialization; +using Xunit; + +namespace Server.Engines.ModernSpawner.Tests; + +/// +/// Covers the sentinel-based min/max delay handling in +/// 's entry parsing (Task 3: no nullable value types). This needs a +/// resolvable , so it runs against the real world in the sequential collection +/// rather than as a pure unit test. +/// +[Collection("Sequential ModernSpawner Tests")] +public class XmlSpawnerImporterEntryDelayTests +{ + private static string BuildXml(string name, string objects2) => $""" + + + {name} + Felucca + 1500 + 1500 + 0 + 1500 + 1500 + 0 + 0 + 4 + 5 + 5 + 10 + False + -1 + 0 + False + False + False + {objects2} + + + """; + + private static ModernSpawner FindByName(string name) + { + foreach (var item in World.Items.Values) + { + if (item is ModernSpawner spawner && spawner.Name == name) + { + return spawner; + } + } + + Assert.Fail($"No imported ModernSpawner named '{name}' was found in the world."); + return null; + } + + [Fact] + public void ParseEntry_NoDelayTokens_LeavesEntryAtSpawnerDefault() + { + var name = "ImporterDelayTest-" + Guid.NewGuid(); + var tempFile = Path.GetTempFileName(); + try + { + File.WriteAllText(tempFile, BuildXml(name, "Rabbit:MX=3")); + + var result = XmlSpawnerImporter.ImportFromFile(tempFile, respawn: false); + Assert.Equal(1, result.Imported); + + var spawner = FindByName(name); + var entry = Assert.Single(spawner.ModernEntries); + + Assert.Equal(TimeSpan.Zero, entry.MinDelay); + Assert.Equal(spawner.MinDelay, entry.EffectiveMinDelay); + + spawner.Delete(); + } + finally + { + File.Delete(tempFile); + } + } + + [Fact] + public void ParseEntry_DnToken_SetsMinDelayInMinutes() + { + var name = "ImporterDelayTest-" + Guid.NewGuid(); + var tempFile = Path.GetTempFileName(); + try + { + File.WriteAllText(tempFile, BuildXml(name, "Rabbit:MX=3:DN=2")); + + var result = XmlSpawnerImporter.ImportFromFile(tempFile, respawn: false); + Assert.Equal(1, result.Imported); + + var spawner = FindByName(name); + var entry = Assert.Single(spawner.ModernEntries); + + Assert.Equal(TimeSpan.FromMinutes(2), entry.MinDelay); + + spawner.Delete(); + } + finally + { + File.Delete(tempFile); + } + } +} diff --git a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs index 6f0b7b3..44f93dd 100644 --- a/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs +++ b/Projects/ModernSpawner.Tests/Fixtures/ModernSpawnerTestServer.cs @@ -81,6 +81,9 @@ public static void Initialize() World.ExitSerializationThreads(); DecayScheduler.Configure(); + // WallTimeWindowTrigger.Activate schedules through EventScheduler.Shared, which is null + // until this runs (production reaches it through UOContent's Configure pass). + Server.Engines.Events.EventScheduler.Configure(); // Without npc-speeds.json every BaseCreature constructor throws. Server.Mobiles.NPCSpeeds.Configure(); Server.Engines.Spawners.SpawnerJsonSerializer.Configure(); diff --git a/Projects/ModernSpawner/Core/ModernSpawner.cs b/Projects/ModernSpawner/Core/ModernSpawner.cs index ff7cdc2..1e4bde2 100644 --- a/Projects/ModernSpawner/Core/ModernSpawner.cs +++ b/Projects/ModernSpawner/Core/ModernSpawner.cs @@ -81,11 +81,31 @@ public partial class ModernSpawner : Spawner private List _triggerDefinitions = []; /// - /// Whether this spawner is trigger-activated (vs. timer-based). + /// Whether this spawner is trigger-activated (vs. timer-based). Master switch for this spawner's + /// trigger definitions: setting it registers or unregisters the triggers immediately through + /// , so there is no window where the flag and the trigger + /// registry disagree. The backing field is generated; serialization order 9 is unchanged. /// - [SerializableField(9)] - [SerializedCommandProperty(AccessLevel.Developer)] - private bool _triggerActivated; + // Hand-written [SerializableProperty] rather than [SerializableField(9, fieldChanged:)] so the + // registration call sits at the mutation point with this doc comment; the generated pipeline + // (equality check -> assign -> MarkDirty -> callback) is equivalent. + [SerializableProperty(9)] + [CommandProperty(AccessLevel.Developer)] + public bool TriggerActivated + { + get => _triggerActivated; + set + { + if (_triggerActivated == value) + { + return; + } + + _triggerActivated = value; + this.MarkDirty(); + EnsureTriggersActive(); + } + } /// /// External trigger state - set by trigger system. @@ -578,7 +598,8 @@ private void MaybeAutoResetSequence() /// /// Brings this spawner's trigger registrations in line with its current state, and is the only /// caller of outside the trigger system itself. - /// appends rather than replaces, so this + /// replaces the spawner's batch in the registry but + /// appends to the per-type dispatch lists, so a second call would duplicate dispatch; this /// deactivates first and is therefore safe to call any number of times. Every construction path /// that can leave a spawner running with triggers already set - start, deserialization, dupe, /// import, migration - ends here, because only reaches @@ -622,10 +643,9 @@ protected override void OnStopped() ScriptEngine.Instance.Execute(deactivateScript, new ScriptContext(null, this)); } - if (_triggerActivated) - { - TriggerSystem.Instance.DeactivateTriggers(this); - } + // DeactivateTriggers is a no-op when nothing is registered, so no flag check: the flag can be + // cleared after registration and must not leave a stale entry behind. + TriggerSystem.Instance.DeactivateTriggers(this); } /// @@ -1036,11 +1056,9 @@ public override void OnDelete() // Unsubscribe from extended area movement before deletion UnsubscribeFromExtendedAreaMovement(); - // Deactivate triggers before deletion - if (_triggerActivated) - { - TriggerSystem.Instance.DeactivateTriggers(this); - } + // Deactivate triggers before deletion. DeactivateTriggers is a no-op when nothing is registered, + // so no flag check: the flag can be cleared after registration and must not leave a stale entry behind. + TriggerSystem.Instance.DeactivateTriggers(this); base.OnDelete(); } diff --git a/Projects/ModernSpawner/Gumps/TriggerConfigGump.cs b/Projects/ModernSpawner/Gumps/TriggerConfigGump.cs index 29ed3c5..a093f79 100644 --- a/Projects/ModernSpawner/Gumps/TriggerConfigGump.cs +++ b/Projects/ModernSpawner/Gumps/TriggerConfigGump.cs @@ -175,6 +175,17 @@ protected override void BuildLayout(ref DynamicGumpBuilder builder) private List GetTriggerList() => _spawner.TriggerDefinitions ?? []; + /// Appends a minute field, zero-padding single digits so 18:0 renders as 18:00. + private static void AppendMinutes(scoped ref ValueStringBuilder sb, ReadOnlySpan minutes) + { + if (minutes.Length == 1) + { + sb.Append('0'); + } + + sb.Append(minutes); + } + private static void FormatTriggerDisplay(string definition, scoped ref ValueStringBuilder sb) { if (string.IsNullOrEmpty(definition)) @@ -203,24 +214,50 @@ private static void FormatTriggerDisplay(string definition, scoped ref ValueStri return; } - if (triggerType.InsensitiveEquals("walltime")) + if (triggerType.InsensitiveEquals("wall_time_window")) { - // Format: walltime:startHour:endHour - Span parts = stackalloc Range[4]; + // Format: wall_time_window:startHour:startMin:endHour:endMin:allowedDays:allowedMonths:timezone + Span parts = stackalloc Range[8]; var count = span.Split(parts, ':'); var startHour = count > 1 ? span[parts[1]] : "0"; - var endHour = count > 2 ? span[parts[2]] : "24"; + var startMinute = count > 2 ? span[parts[2]] : "00"; + var endHour = count > 3 ? span[parts[3]] : "23"; + var endMinute = count > 4 ? span[parts[4]] : "59"; sb.Append("Real Time: "); sb.Append(startHour); - sb.Append(":00 - "); + sb.Append(':'); + AppendMinutes(ref sb, startMinute); + sb.Append(" - "); sb.Append(endHour); - sb.Append(":00"); + sb.Append(':'); + AppendMinutes(ref sb, endMinute); return; } - if (triggerType.InsensitiveEquals("gametime")) + if (triggerType.InsensitiveEquals("game_time_window")) { - sb.Append("Game Time: Night hours"); + // Format: game_time_window:startHour:endHour:nightOnly:dayOnly + Span parts = stackalloc Range[5]; + var count = span.Split(parts, ':'); + if (count > 3 && span[parts[3]].InsensitiveEquals("true")) + { + sb.Append("Game Time: Night hours"); + return; + } + + if (count > 4 && span[parts[4]].InsensitiveEquals("true")) + { + sb.Append("Game Time: Day hours"); + return; + } + + var startHour = count > 1 ? span[parts[1]] : "0"; + var endHour = count > 2 ? span[parts[2]] : "23"; + sb.Append("Game Time: "); + sb.Append(startHour); + sb.Append(":00 - "); + sb.Append(endHour); + sb.Append(":00"); return; } @@ -280,6 +317,7 @@ public override void OnResponse(NetState state, in RelayInfo info) range = Math.Max(1, parsedRange); } _spawner.AddToTriggerDefinitions($"proximity:{range}:true"); + _spawner.EnsureTriggersActive(); from.SendMessage($"Added proximity trigger with {range} tile range."); break; } @@ -298,13 +336,19 @@ public override void OnResponse(NetState state, in RelayInfo info) { endHour = Math.Clamp(parsedEnd, 0, 23); } - _spawner.AddToTriggerDefinitions($"walltime:{startHour}:{endHour}"); + // WallTimeWindowTrigger.Parse reads wall_time_window:startHour:startMin:endHour:endMin; + // the gump only offers whole hours, so the minute fields are zero. + _spawner.AddToTriggerDefinitions($"wall_time_window:{startHour}:0:{endHour}:0"); + _spawner.EnsureTriggersActive(); from.SendMessage($"Added time window trigger: {startHour}:00 - {endHour}:00."); break; } case ButtonId_AddGameTime: - _spawner.AddToTriggerDefinitions("gametime:night"); + // GameTimeWindowTrigger.Parse reads game_time_window:startHour:endHour:nightOnly; + // NightOnly is the parser's night preset and overrides the hours it is given. + _spawner.AddToTriggerDefinitions("game_time_window:21:5:true"); + _spawner.EnsureTriggersActive(); from.SendMessage("Added game time trigger for night hours."); break; @@ -315,13 +359,20 @@ public override void OnResponse(NetState state, in RelayInfo info) var deleteIndex = info.ButtonID - ButtonId_DeleteBase; if (deleteIndex >= 0 && deleteIndex < triggers.Count) { - triggers.RemoveAt(deleteIndex); + // triggers is the live list: remove through the generated index helper so the + // spawner is marked dirty and duplicate definitions still delete by position. + _spawner.RemoveFromTriggerDefinitionsAt(deleteIndex); + _spawner.EnsureTriggersActive(); from.SendMessage("Trigger removed."); } } break; } - from.SendGump(new TriggerConfigGump(_spawner, _page)); + // A delete can empty the page that was being viewed, so clamp before re-sending: the list is + // re-read because the switch above may have added to or removed from it. + var remaining = GetTriggerList(); + var lastPage = Math.Max(0, (remaining.Count - 1) / TriggersPerPage); + from.SendGump(new TriggerConfigGump(_spawner, Math.Min(_page, lastPage))); } } diff --git a/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs b/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs index 5cf0834..139fab3 100644 --- a/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs +++ b/Projects/ModernSpawner/Migration/XmlSpawnerMigrator.cs @@ -128,7 +128,7 @@ public static (int success, int failed) ImportFromFile(string path) /// /// Parses an XmlSpawner node from the save format. /// - private static ModernSpawner ParseXmlSpawnerNode(XmlNode node) + internal static ModernSpawner ParseXmlSpawnerNode(XmlNode node) { // Parse location var x = GetIntAttribute(node, "X", 0); @@ -247,15 +247,20 @@ private static ModernSpawner ParseXmlSpawnerNode(XmlNode node) // Place the spawner spawner.MoveToWorld(new Point3D(x, y, z), map); - // Start if it was running + // Start if it was running, stop otherwise - the spawner is constructed already running, so + // "Running=false" (or no entries to run with) has to be applied explicitly. var running = GetBoolAttribute(node, "Running", true); if (running && spawner.Entries.Count > 0) { spawner.Start(); } + else + { + spawner.Stop(); + } - // Start() is a no-op on a spawner that was constructed running, so OnStarted never registers - // the triggers this migration just set. Register them here. + // Start()/Stop() only reach OnStarted/OnStopped when Running flips; the spawner was constructed + // running, so register (or unregister) explicitly for the state the file asked for. spawner.EnsureTriggersActive(); return spawner; @@ -264,7 +269,7 @@ private static ModernSpawner ParseXmlSpawnerNode(XmlNode node) /// /// Parses a SpawnPoint node (alternative XmlSpawner export format). /// - private static ModernSpawner ParseSpawnPointNode(XmlNode node) + internal static ModernSpawner ParseSpawnPointNode(XmlNode node) { var x = GetIntAttribute(node, "X", 0); var y = GetIntAttribute(node, "Y", 0); @@ -298,13 +303,20 @@ private static ModernSpawner ParseSpawnPointNode(XmlNode node) spawner.MoveToWorld(new Point3D(x, y, z), map); - if (spawner.Entries.Count > 0) + // This node form has no dedicated attribute for stopped spawners in the wild, but honour one if + // present; default true preserves the historical "always start" behavior when it is absent. + var running = GetBoolAttribute(node, "Running", true); + if (running && spawner.Entries.Count > 0) { spawner.Start(); } + else + { + spawner.Stop(); + } - // Start() is a no-op on a spawner that was constructed running, so OnStarted never registers - // any triggers this path set. Register them here. + // Start()/Stop() only reach OnStarted/OnStopped when Running flips; the spawner was constructed + // running, so register (or unregister) explicitly for the state the file asked for. spawner.EnsureTriggersActive(); return spawner; diff --git a/Projects/ModernSpawner/Perf/SpawnerPerfCommands.cs b/Projects/ModernSpawner/Perf/SpawnerPerfCommands.cs index 8204f0b..236bc66 100644 --- a/Projects/ModernSpawner/Perf/SpawnerPerfCommands.cs +++ b/Projects/ModernSpawner/Perf/SpawnerPerfCommands.cs @@ -118,8 +118,10 @@ private static void PerfSeed_OnCommand(CommandEventArgs e) maxCount: 1, dotimer: false); - // Add a proximity trigger so player sweeps exercise the dispatch path. + // Add a proximity trigger so player sweeps exercise the dispatch path. The flag is set + // after the definition exists: its setter registers whatever is in the list at that moment. spawner.AddToTriggerDefinitions("proximity:8:true"); + spawner.TriggerActivated = true; _seeded.Add(spawner); created++; diff --git a/Projects/ModernSpawner/Scripting/ScriptContext.cs b/Projects/ModernSpawner/Scripting/ScriptContext.cs index b8170ae..2714adb 100644 --- a/Projects/ModernSpawner/Scripting/ScriptContext.cs +++ b/Projects/ModernSpawner/Scripting/ScriptContext.cs @@ -53,11 +53,23 @@ public class ScriptContext /// public bool CancelSpawn { get; set; } + /// True when a script set . + public bool HasLocationOverride { get; private set; } + + private Point3D _locationOverride; + /// - /// Gets or sets custom spawn location override. - /// If set, the entity will be placed at this location instead of the calculated position. + /// Custom spawn location a script may set; consult before reading. /// - public Point3D? LocationOverride { get; set; } + public Point3D LocationOverride + { + get => _locationOverride; + set + { + _locationOverride = value; + HasLocationOverride = true; + } + } public ScriptContext(ModernSpawner spawner, ModernSpawnerEntry entry = null, IEntity target = null) { diff --git a/Projects/ModernSpawner/Serialization/SpawnerJsonImporter.cs b/Projects/ModernSpawner/Serialization/SpawnerJsonImporter.cs index 44452c7..22ea7f5 100644 --- a/Projects/ModernSpawner/Serialization/SpawnerJsonImporter.cs +++ b/Projects/ModernSpawner/Serialization/SpawnerJsonImporter.cs @@ -202,7 +202,13 @@ public static void ConfigureSpawner(ModernSpawner spawner, SpawnerExportData dat // Import triggers (clears existing) if (data.Triggers != null) { - spawner.TriggerDefinitions?.Clear(); + // Through the generated helper so the spawner is marked dirty; it dereferences the list, + // so the null check stays. + if (spawner.TriggerDefinitions != null) + { + spawner.ClearTriggerDefinitions(); + } + ImportTriggers(spawner, data.Triggers); } diff --git a/Projects/ModernSpawner/Serialization/XmlSpawnerImporter.cs b/Projects/ModernSpawner/Serialization/XmlSpawnerImporter.cs index 0b14501..1d4bd8c 100644 --- a/Projects/ModernSpawner/Serialization/XmlSpawnerImporter.cs +++ b/Projects/ModernSpawner/Serialization/XmlSpawnerImporter.cs @@ -291,8 +291,8 @@ private static void ParseEntry(ModernSpawner spawner, string entryStr, int defau var maxCount = defaultMaxCount; var probability = 100; var subgroup = 0; - TimeSpan? minDelay = null; - TimeSpan? maxDelay = null; + var minDelay = TimeSpan.Zero; + var maxDelay = TimeSpan.Zero; string properties = null; for (var i = 1; i < parts.Length; i++) @@ -371,8 +371,8 @@ private static void ParseEntry(ModernSpawner spawner, string entryStr, int defau maxCount: maxCount, properties: properties, parameters: parameters, - minDelay: minDelay ?? TimeSpan.Zero, - maxDelay: maxDelay ?? TimeSpan.Zero, + minDelay: minDelay, + maxDelay: maxDelay, dotimer: false ); entry.Subgroup = subgroup; diff --git a/Projects/ModernSpawner/Triggers/SkillTrigger.cs b/Projects/ModernSpawner/Triggers/SkillTrigger.cs index af84396..37e0733 100644 --- a/Projects/ModernSpawner/Triggers/SkillTrigger.cs +++ b/Projects/ModernSpawner/Triggers/SkillTrigger.cs @@ -41,13 +41,18 @@ public class SkillTrigger : ITrigger private ModernSpawner _spawner; private DateTime _lastTriggered; - public SkillTrigger(SkillName skill, int range = 10, double minSkillValue = 0, bool requireLOS = false, TimeSpan? cooldown = null) + public SkillTrigger(SkillName skill, int range = 10, double minSkillValue = 0, bool requireLOS = false) + : this(skill, range, minSkillValue, requireLOS, TimeSpan.FromSeconds(5)) + { + } + + public SkillTrigger(SkillName skill, int range, double minSkillValue, bool requireLOS, TimeSpan cooldown) { TargetSkill = skill; Range = Math.Max(1, range); MinSkillValue = minSkillValue; RequireLOS = requireLOS; - Cooldown = cooldown ?? TimeSpan.FromSeconds(5); + Cooldown = cooldown; } public void Activate(ModernSpawner spawner) diff --git a/Projects/ModernSpawner/Triggers/TriggerSystem.cs b/Projects/ModernSpawner/Triggers/TriggerSystem.cs index 5527095..9914d76 100644 --- a/Projects/ModernSpawner/Triggers/TriggerSystem.cs +++ b/Projects/ModernSpawner/Triggers/TriggerSystem.cs @@ -101,6 +101,12 @@ public void ActivateTriggers(ModernSpawner spawner) } } + /// + /// Whether currently has triggers registered with this system, i.e. whether + /// has run for it without a matching . + /// + internal bool IsRegistered(ModernSpawner spawner) => spawner != null && _allTriggers.ContainsKey(spawner); + public void DeactivateTriggers(ModernSpawner spawner) { if (spawner == null) diff --git a/dev-docs/architecture.md b/dev-docs/architecture.md index cc5f53a..843b789 100644 --- a/dev-docs/architecture.md +++ b/dev-docs/architecture.md @@ -69,14 +69,22 @@ triggers and runs the entry's `OnDespawnScript`. `TriggerSystem` is a singleton registry keyed by spawner with per-type lists. Triggers are parsed from `type:field:field` strings stored on the spawner (`_triggerDefinitions`). Registration goes through one guarded helper, `ModernSpawner.EnsureTriggersActive()`, the only caller of -`TriggerSystem.ActivateTriggers` outside the trigger system: it deactivates first and re-registers only -when the spawner is running, is `TriggerActivated` and actually has definitions, which makes it idempotent -(`ActivateTriggers` itself appends rather than replaces). `OnStarted` and `[AfterDeserialization]` call it, -and so does every construction path that hands back an already-running spawner — `OnAfterDuped`, -`ModernSpawnerDto.ToSpawner`, both JSON importer entry points, `XmlSpawnerImporter` and -`XmlSpawnerMigrator` — because `BaseSpawner.Start()` only reaches `OnStarted` when `Running` actually -flips. Deactivation is in `OnStopped` (reached by `Stop()` and, through `BaseSpawner.OnDelete`, by -deletion) and in `OnDelete`. Wiring: +`TriggerSystem.ActivateTriggers` outside the trigger system, within the engine project: it deactivates +first and re-registers only when the spawner is running, is `TriggerActivated` and actually has +definitions, which makes it idempotent. `ActivateTriggers` on its own is not: it *replaces* +`_allTriggers[spawner]` with the batch it just parsed while the per-type lists it feeds +(`_proximityTriggers`, `_speechTriggers`, …) *append*, so calling it twice duplicates dispatch and orphans +the first batch — those triggers are no longer reachable for `Deactivate()`. `OnStarted` and +`[AfterDeserialization]` call it, and so does every construction path that hands back an already-running +spawner — `OnAfterDuped`, `ModernSpawnerDto.ToSpawner`, both JSON importer entry points, +`XmlSpawnerImporter` and `XmlSpawnerMigrator` — because `BaseSpawner.Start()` only reaches `OnStarted` +when `Running` actually flips. The same helper is the mandatory follow-up for every other list or flag change: the +`TriggerActivated` setter calls it, and so do `TriggerConfigGump`'s add/remove handlers and the JSON +importer's clear path. Those list mutations go only through the generated +`AddToTriggerDefinitions`/`RemoveFromTriggerDefinitionsAt`/`ClearTriggerDefinitions` helpers, so the change +is tracked for serialization before triggers are re-registered. Deactivation is unconditional in +`OnStopped` (reached by `Stop()` and, through `BaseSpawner.OnDelete`, by deletion) and in `OnDelete`, and +`XmlSpawnerMigrator` honours an explicit `Running="false"` on both node forms it reads. Wiring: | Trigger | Source event | Wired | |---|---|---| diff --git a/dev-docs/modernuo-prerequisites.md b/dev-docs/modernuo-prerequisites.md index be8d78d..09b0d13 100644 --- a/dev-docs/modernuo-prerequisites.md +++ b/dev-docs/modernuo-prerequisites.md @@ -10,7 +10,7 @@ per-movement paths without a measurement, because shards run 12k+ spawners. | PR | Change | Why ModernSpawner needs it | Submodule pin | |---|---|---|---| -(none) +| [#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 | ## Merged @@ -21,10 +21,6 @@ per-movement paths without a measurement, because shards run 12k+ spawners. ## Planned (see `architecture.md` §4–§5, §11; decisions D1, D2, D3, D11, D12) -- `SkillEvents.SkillUsedEvent` raised from `SkillCheck` (D3). -- `TestServerInitializer` usable from an external test assembly (or a public variant that takes an assembly - list), and `InternalsVisibleTo("ModernSpawner.Tests")` on `Server.csproj` so the fixture can seed - `Core._now` (today the test clock is `DateTime.MinValue`). - `[ImportSpawners`: GUID-based replacement, preserve `running`, no unconditional `Respawn()`. - Deferred: sector-range movement subscription for proximity triggers wider than 24 tiles. diff --git a/dev-docs/product-spec.md b/dev-docs/product-spec.md index db2fef4..4506970 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 | Wired via an upstream ModernUO hook; until then `skill:` definitions are rejected at parse time with a visible error (**D3**) | +| 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 | | 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) | +| **D3** | Skill trigger source | Upstream `SkillCheck` hook via ModernUO PR | Open (default assumed); ModernUO #2636 open | | **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** |