refactor(core): port ModernSpawner to the ModernUO entry-ownership contract - #1
Merged
Merged
Conversation
The build is red on this commit by design; the port that follows makes it green. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…ntract Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
ChurnTick enumerated BaseSpawner.Spawned while killing from it. Deleting a spawned entity routes through BaseSpawner.Remove, which removes it from that same dictionary, so the first kill threw "Collection was modified". Snapshot the candidates into a PooledRefList<ISpawnable> first, then kill from the snapshot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…ts for the ported spawner Boots a real ModernUO world in the test host (ModernSpawnerTestServer, guarded to run once, behind a non-parallel collection fixture) and exercises a live ModernSpawner end to end over the entry-ownership contract: entry construction, spawn/kill/respawn against the single Spawned registry, stop/start, Respawn idempotence, Dupe, the DTO round trip and the binary round trip that proves [AfterDeserialization] rebuilds Spawned over the modern entries. The fixture reuses ModernUO's own Server.Tests.Maps.TestMapDefinitions rather than copying the map table, so ModernSpawner.Tests now references Server.Tests.csproj and copies Distribution/Data to its output. Booting the fixture surfaced a startup defect in PropertyAccessorCache: PrewarmCache compiles a getter for every property of Mobile, and ref-returning properties such as Mobile.DamageEntries (ref ValueLinkList<DamageEntry>) cannot be expressed as Func<object, object?>, so Expression.Lambda threw and ModernSpawnerConfiguration.Configure crashed. FindProperty now reports ref, pointer and ref-struct properties as "not found", the way indexers are already skipped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Both tests passed with the behaviour they name deleted, so they guarded nothing. Kill_DispatchesOnDespawnScriptAndKillTrigger now gives each half a real observable. SETVAR wrote to a per-ScriptContext dictionary discarded at the end of execution, so the despawn script left no trace; it now uses SET, which writes through PropertyAccessorCache to the ScriptContext target that OnSpawnedDeath binds to the dying creature, and the test asserts the creature's Name changed. The kill trigger half registers a KillTrigger definition, sets TriggerActivated and cycles Stop/Start so OnStarted runs ActivateTriggers, then asserts the spawner's Triggered flag was false before the kill and true after -- a flag only TriggerSystem's Trigger() call can set. Dupe_ClonesModernFields asserted 2 of the 12 fields the clone path copies. It now sets a distinct non-default value for all eleven ModernSpawner.CloneEntry copies plus the base clone's Disabled, and asserts each on the copy. Verified by mutation: gutting OnSpawnedDeath, removing only its TriggerSystem.OnEntityKilled call, and dropping three CloneEntry lines each fail the covering tests; the engine was restored afterwards. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…trigger definitions Trigger registration now 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 has definitions, so it is idempotent even though ActivateTriggers appends rather than replaces. BaseSpawner.Start() only reaches OnStarted when Running actually flips, and every construction path builds an already-running spawner, so migrated and imported spawners never registered the triggers they had just been given. ToSpawner(), both XmlSpawnerMigrator paths, XmlSpawnerImporter and both SpawnerJsonImporter entry points now end in EnsureTriggersActive(); OnStarted and [AfterDeserialization] use it too. [dupe also dropped triggers: _triggerDefinitions is [SerializedIgnoreDupe] with no OnAfterDuped override, so a copy came back TriggerActivated with an empty list. The new override copies the list through the generated setter and registers it. Also: OnStopped skips the deactivate script once the item is flagged deleted (the trigger deactivation still runs) and documents that deletion reaches it through BaseSpawner.OnDelete -> Stop(); AdoptEntries returns early when handed its own list; the after-spawn script doc records Spawn()'s early returns. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
§2.2 describes trigger registration as it is: one guarded EnsureTriggersActive() called from OnStarted, [AfterDeserialization] and every construction path, with deactivation in OnStopped/OnDelete. The kill row is wired and tested - OnSpawnedDeath via BaseSpawner.NotifySpawnedDeath from BaseCreature.OnDeath. §2.4 drops SpawnArea, which no longer exists; Spawner.SpawnBounds is the one bounds and HomeRange is a computed view over it. §2.5 applies loot and the entry spawn script in OnSpawned, not SpawnFromEntry. §2.6 records that the DTO carries modern entries, triggers and cycle state and that both round trips are tested. §3's hot-path bullet drops the closure and temp-entry allocations, which the port removed. A living doc must not point at a branch: §4.3 says "implemented" instead of naming port/entry-contract and its commits, and the retired "support branch" model is gone from §5, product-spec.md §6 and the prerequisites table. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
…Deleted guard could never fire Item.Delete() calls OnDelete() before it sets the Deleted flag, so on the deletion path - Item.Delete -> BaseSpawner.OnDelete -> Stop() -> OnStopped - Deleted is still false and the guard added in a8e5981 was dead code. Removed it and kept the doc sentence: deleting a running spawner does run the OnDeactivate script, and nothing in OnStopped's state distinguishes a stop from a deletion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXJwRhwvHQXV2ABJHicwsr
3 tasks
kamronbatman
added a commit
that referenced
this pull request
Sep 12, 2026
…tor honours Running=false; no nullable value types (#2) ## Summary Closes the trigger-registration follow-ups parked in #1, and removes the last nullable value types from the engine. - **Registration follows every flag and list change.** `TriggerActivated` is now a property whose setter calls `EnsureTriggersActive()` (serialized slot unchanged, layout byte-identical). `TriggerConfigGump` adds and removes through the generated helpers (`RemoveFromTriggerDefinitionsAt` for the clicked row) and re-registers after each edit; `SpawnerJsonImporter` clears through `ClearTriggerDefinitions()` so dirty tracking holds. `TriggerSystem.ActivateTriggers` still has exactly one caller. - **Deactivation no longer gated on the flag.** `OnStopped` and `OnDelete` call `DeactivateTriggers` unconditionally (a no-op when nothing is registered), so clearing `TriggerActivated` after registration can no longer leave a deleted spawner in the trigger system. - **XmlSpawner migrator honours `Running="false"`** on both node forms: the spawner is stopped (it is constructed running) and its triggers registered or not accordingly. Behaviour note: a `Running="true"` node with zero parsed entries now imports stopped (before, it was left in its constructed running state), which matches D9's "unreproducible encounters import stopped". - **Perf seed and gump fixes found by review.** `[ModernSpawnerPerfSeed` now activates the proximity trigger it adds, so the proximity-dispatch benchmark measures something. `TriggerConfigGump` writes the time-window definitions with the trigger type names `TriggerSystem` actually registers (it wrote `walltime:`/`gametime:` before, which parsed as unknown while the gump reported success), and clamps the page after deleting the last trigger on a page. - **No nullable value types:** `XmlSpawnerImporter` delays use the `TimeSpan.Zero` sentinel `AddModernEntry` already understands; `SkillTrigger` gets a `TimeSpan cooldown` overload; `ScriptContext.LocationOverride` becomes `HasLocationOverride` + `Point3D`. The remaining `Type?` members live only in the legacy YAML/JSON export models (`ScriptExportModels`, `SpawnerExportModels`), which the SpawnerDto-only format decision (D5/D6) retires; they are left for that work. - Docs: `CLAUDE.md` rule for trigger mutations; `dev-docs/architecture.md` §2.2; `dev-docs/modernuo-prerequisites.md` records ModernUO #2636 (`SkillEvents.SkillChecked` + test-clock IVT) as open; `dev-docs/product-spec.md` corrects the D3 row: `skill:` definitions parse and register today but never fire, because nothing calls `ModernSpawnerEvents.OnSkillUsed` until the wiring PR subscribes to #2636's event. ## Test plan - [x] New world-backed tests in the sequential collection: toggling `TriggerActivated` registers/unregisters; adding a definition to an activated running spawner registers immediately; deleting an activated spawner leaves nothing registered; stopping with a stale registration unregisters; deleting a stopped spawner with a stale registration unregisters; migrator `Running="false"` on both node forms. Each mutation-checked against its reverted behaviour. - [x] `dotnet build ModernSpawner.slnx` clean; full suite green (438). The test fixture now runs `EventScheduler.Configure()` like production, since window triggers schedule through it. - [ ] CI ## Follow-ups - `TriggerSystem.OnMobileProximity` allocates a `TriggerContext` class per movement event per spawner; the D2 trigger-state-machine work should make it a `readonly struct` passed by `in`. - Legacy export models keep their nullable members until the SpawnerDto-only format (D5/D6) retires them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports ModernSpawner onto the entry-ownership contract that merged in ModernUO #2621, and bumps the submodule to
a52ce6ef7.ModernSpawner : Spawnernow ownsList<ModernSpawnerEntry>and implements the base contract (Entries,EntrySpan,CreateEntry,AddEntryCore,RemoveEntryCore,ClearEntriesCore,AdoptEntries,CloneEntry). The parallel_modernSpawnedmap, the temp-entry spawn path, and thenewhides ofAddEntry/Start/Stopare gone.ModernSpawnerEntry : SpawnerEntrykeeps only its 11 extra fields and declares[DirtyTrackingEntity] private BaseSpawner Owner => Parent;for cross-assembly dirty tracking.OnStarted/OnStopped(activate/deactivate scripts, trigger (de)activation), entry-awareGetSpawnPosition(positioning rule, spawn-area offset),OnSpawned(loot template, OnSpawn script),OnSpawnedDeath(kill triggers, OnDespawn script).ModernSpawnerDtoownsentries(typed) plus triggers and cycle-mode state;SpawnAreais replaced bySpawner.SpawnBounds.Respawn()was a no-op;Disabledwas ignored by the modern spawn path.EnsureTriggersActive), so spawners built by the XmlSpawner migrator, the JSON/XML importers,[ImportSpawners, and[duperegister their triggers immediately instead of on the next restart;[dupenow deep-copies trigger definitions.PropertyAccessorCacheno longer tries to compile accessors for ref-returning properties, which madeModernSpawnerConfiguration.Configure()throw on a real server.Test plan
dotnet build ModernSpawner.slnxclean,TreatWarningsAsErrorsondotnet test Projects/ModernSpawner.Tests: 428 passed (419 existing + 9 lifecycle)OnSpawnedDeath, lines fromCloneEntry, or the import-pathEnsureTriggersActive()call fails the corresponding lifecycle testFollow-ups (tracked in
dev-docs/modernuo-prerequisites.md)InternalsVisibleTo("ModernSpawner.Tests")so the fixture can seed the test clock (Core.NowisDateTime.MinValuein the test host today).TriggerConfigGumptogglesTriggerActivatedand adds definitions without callingEnsureTriggersActive, so gump-added triggers stay inert until the next stop/start (one-line follow-up).OnDelete/OnStoppedgateDeactivateTriggersonTriggerActivated; clearing the flag after registration can leave a deleted spawner registered inTriggerSystem.XmlSpawnerMigratorignoresrunning="false"in the source file (spawners are constructed running and never stopped);XmlSpawnerImporterhandles it correctly.XmlSpawnerImporter,SkillTrigger,ScriptContext) to be removed in a follow-up.dotnet build -c Analyzeis red inside the ModernUO submodule (CA1307/CA1310), so the analyzer gate gives no signal for this repo yet.