Implement Castle Siege Crown and switch mechanics

[Zylkien]

Summary

Implements the Castle Siege Crown capture and switch mechanics described in #726.

Changes

• Validate that the Crown and both switches are occupied by alive, guilded players from the same attacking side.
• Accumulate Crown capture time and preserve capped progress when an attempt is interrupted.
• Send attempt, failure, and success notifications to the Crown user.
• Swap the capturing and defending sides after a successful capture.
• Reassign participant sides and respawn non-defending players.
• Persist the intermediate guild-side changes for restart recovery.
• Broadcast switch occupants, Crown availability, and ownership changes.
• Persist the final castle owner when the battle ends.
• Reset taxes and tribute money when ownership changes.
• Add null-safe remote views for the required Castle Siege packets.
• Add regression tests for the mechanics, persistence, restart recovery, and packet serialization.

Testing

• GameLogic build succeeded.
• GameServer build succeeded.
• 9 focused Crown and switch tests passed.
• 89 complete Castle Siege tests passed.
• Formatting and diff checks passed.

Scope note

Life Stone destruction during side reassignment depends on the runtime Life Stone implementation from #727. The current change performs the side reassignment and respawn flow without introducing temporary or unused Life Stone code.

[sven-n]

Review

I went through the diff against a2b1a65 (18 files, +1478/-3) and the surrounding Castle Siege code. Overall this is a clean, well-factored addition: the new mechanics classes mirror the existing CastleSiegeParticipantTracker style, the view plug-ins are null-safe, resource entries are complete, and the enum casts to the network enums are actually value-compatible (CastleSiegeCrownAccessState 0/1/2 vs CastleSiegeCrownAccessStateType.Started/Succeeded/Failed, CastleSiegeJoinSide 0-4). Test coverage of the state machine is good. CI is green.

A few things I'd like to see addressed before merge.

1. The switch packet carries the object id where the client expects the switch index

CastleSiegeSwitchMechanics.cs:71 fills CastleSiegeSwitchInfo.ObjectId from siegeSwitch.Id, and CastleSiegeSwitchInfoPlugIn forwards it into SendCastleSiegeSwitchInfoAsync(switchIndex, ...). The packet field is defined as SwitchIndex (ServerToClientPackets.xml, B2/20), and CastleSiegeSwitch already exposes the zero-based SwitchIndex (0/1) which is the value used for context.SwitchUsers[...] right above. Sending the map object id there will make the client attribute the occupancy to a non-existent switch slot.

CastleSiegeCrownRemoteViewTests.SerializeCrownAndSwitchNotificationsAsync asserts switchInfo.SwitchIndex, Is.EqualTo(0x1234), so the test currently locks the behaviour in rather than catching it. If the object id really is what the client wants here, could you add a note explaining it — the field name says otherwise.

2. Crown progress is counted in fixed 1-second steps instead of elapsed time

CastleSiegeCrownMechanics.cs:16,50 adds a hardcoded TickInterval of one second per invocation. That silently depends on GameContext's periodic timer (new Timer(this.ExecutePeriodicTasks, null, 1000, 1000)), and CastleSiegePlugIn.ExecuteTaskAsync deliberately drops ticks when the previous one is still running (ExecutionLock.WaitAsync(0)). A tick that overruns — e.g. one that also runs SaveNpcStatesAsync or SetPlayerJoinSideAsync over a full map — makes the Crown take measurably longer than CrownHoldTimeSeconds in wall-clock terms, and the value reported to the client in AccumulatedTimeMs drifts away from what the player sees. OnTickAsync already has utcNow; passing it in and accumulating the delta since the last Crown tick would remove both the drift and the hidden coupling to the timer period.

3. Switch and Crown state are re-broadcast to every player every second

SendSwitchInfoAsync is called unconditionally from OnTickAsync while the battle runs and sends 2 switch packets + 1 Crown-state packet to every player on the siege map, whether or not anything changed. On a full siege map that is a constant packet stream for state that changes rarely. The context already has the "notify only on change" pattern in _notifiedPlayerJoinSides / SynchronizePlayerJoinSideAsync; the same approach here (diff the per-switch occupant and the Crown availability, broadcast only on transition, and send the current state to a player when they enter the map) would cut this down a lot.

4. CheckResultAsync throwing aborts the whole End transition

CastleSiegeCrownMechanics.cs:138 throws when MiddleOwnerGuildId is no longer in FinalGuildList. It is called first in case CastleSiegeState.End:, so if it ever throws, AwardRewardsAsync, ParticipantTracking.Clear(), SaveNpcStatesAsync and DespawnMachinesAsync are all skipped, and the exception only reaches the logger.LogError in ExecuteTaskAsync — the siege silently ends without rewards. FinalGuildList is not immutable during the battle: LoadFinalGuildListAsync drops entries whose guild isn't loaded (runtimeGuildId == 0), and ResolvePlayerJoinSideAsync removes and re-adds entries under a new runtime key. I'd log a warning and treat it as "no winner" instead of throwing, so the rest of the End handling always runs.

5. The intermediate owner isn't persisted, so restart recovery can hand the castle to a different guild

ChangeWinnerGuildAsync persists the swapped sides (SaveFinalGuildListAsync) but MiddleOwnerGuildId is runtime-only. After a restart mid-battle, LoadFinalGuildListAsync calls InitializeBattleOwner(), which picks the Defense-side guild with IsAllianceMaster. The capture swaps Side but leaves IsAllianceMaster untouched, so:

• if the capturing guild is not its alliance's master, the recovered owner is the master guild of the capturing alliance, not the guild that took the Crown;
• if no guild on the new Defense side carries the flag, MiddleOwnerGuildId becomes null and the capture is lost entirely.

Persisting the middle owner on CastleSiegeData (or moving the master flag along with the side) would make the recovery match what the PR description claims.

6. Questions on the intended behaviour

• Progress carry-over across players/sides. CapAccumulatedTime keeps CrownHoldTimeSeconds - 1, and ChangedCrownUserContinuesCappedProgressAsync pins that down, so a player from a different attacking alliance can walk up and finish a capture in a single tick using the 9 seconds someone else invested. If that's the original behaviour, fine — worth a comment in CapAccumulatedTime. If not, the accumulated time needs to be tracked per side and reset when the side changes.
• Defenders blocking the Crown. CastleSiegeCrownIntelligence picks MinBy(player => player.Id) among all alive players in range, so a defender standing on the Crown makes GetCaptureSide return null and quietly stalls every attacker. The client-facing enum already has OccupiedByOtherPlayer (3) and OccupiedByOtherSide (4) for exactly this, and nothing in the PR ever sends them. Was that left for a later phase?

Minor

• ChangeWinnerGuildAsync is public and throws on inputs that CheckMiddleWinnerAsync has already validated; internal (or a private helper) would keep the invariant local.
• Array.Clear(context.SwitchUsers) after a capture is overwritten by CastleSiegeSwitchIntelligence on its next tick and doesn't clear CastleSiegeSwitch.Occupant, so the two representations disagree for up to a second.
• crown.State is now written from both CastleSiegeCrownIntelligence.TickAsync and SendSwitchInfoAsync on independent timers. They agree today because both derive it from IsCrownAvailable, but it's worth having a single owner of that field.

---

Generated by Claude Code

[sven-n]

Second pass — reviewed 739c0d0 ("Address Castle Siege Crown review findings")

Thanks, this addresses the substance of everything I raised. Walking through the previous points:

• Decoupling ChatServer from DataModel #2 elapsed time — CheckMiddleWinnerAsync now takes utcNow and accumulates the real delta, and the Attempt packet is suppressed for zero-length ticks. ✅
• Item stacking in Inventory #3 broadcast volume — change-diffing via LastBroadcastSwitchInfos/LastBroadcastCrownAvailability, plus SynchronizePlayerAsync for players entering mid-battle, is exactly what I was after (and the value-record Equals makes the diff work). ✅
• Implement usage of ammunition (Arrows, Bolts) for Bows/Crossbows #4 End-state resilience — warning instead of throw, with FinalResultRetainsPersistedOwnerWhenIntermediateOwnerIsMissingAsync covering it. ✅
• Implement Launcher with auto-update feature #5 restart recovery — ApplyOwner + SaveOwnerAsync at capture time, and InitializeBattleOwner preferring the Defense guild that matches SiegeData.OwnerGuildId before falling back to the alliance master, closes the hole. The IsAllianceMaster = false line in the capture test is a good regression guard. ✅
• Minor items — ChangeWinnerGuildAsync/CheckResultAsync are internal now, CastleSiegeSwitch.Occupant is cleared on capture, and crown.State has a single writer since CastleSiegeCrownIntelligence stopped setting it (the Locked assignment on the non-Start path still covers the post-battle case). ✅
• Bug at player appearance - weird wing is shown #1 switch index — accepted with the comment at CastleSiegeSwitchMechanics.cs:106. I can't verify the MuMain behaviour myself, so I'll take your word for it. Since the packet field is still called SwitchIndex in ServerToClientPackets.xml while the record property is ObjectId, would you mind adding a <Description> to that field (or renaming it) so the next reader doesn't "fix" it?
• Pet system (Raven, Dark Horse) is missing #6 progress carry-over — documented as intended at CastleSiegeCrownMechanics.cs:213. ✅

Three small things from this commit:

1. Unbounded elapsed time can skip the hold requirement

CastleSiegeCrownMechanics.cs:25 credits the full gap since the last update. LastCrownUpdateUtc only advances inside CheckMiddleWinnerAsync, which only runs on ticks that actually acquire ExecutionLock, so any dropped or long-running tick (a slow SetPlayerJoinSideAsync over a full map, a GC pause, an NTP step) lands as one large elapsed on the next tick. A player who stepped onto the Crown a moment before such a stall gets credited for the whole gap and can complete a capture without ever holding it — the old fixed 1s step was accidentally protecting against that. Clamping to a small maximum (say two periodic intervals) keeps the wall-clock accuracy while bounding the jump.

2. LastBroadcastSwitchInfos is indexed by list position, not by SwitchIndex

CastleSiegeSwitchMechanics.cs:26 and :53 index the 2-element array with the position in the ordered switchInfos list. Today CastleSiegeNpcController maps monster 217/218 to indices 0/1, so this matches — but only as long as exactly one switch of each number is spawned. A configuration with a duplicate switch spawn area produces a third entry and throws IndexOutOfRangeException inside the periodic task, which would silently stop all Crown processing for the rest of the battle (the exception only reaches the logger.LogError in ExecuteTaskAsync). Indexing by siegeSwitch.SwitchIndex instead — the value already used one line up for SwitchUsers — removes the coupling entirely.

3. Taxes and tribute are now reset mid-battle on every Crown flip

Moving ApplyOwner into ChangeWinnerGuildAsync (:115) means the tax rates and TributeMoney are zeroed the moment the Crown changes hands, not when the siege ends. If the original owner recaptures the Crown before the End state, they finish the siege still owning the castle but with their tax rates and accumulated tribute wiped by the intermediate flip — ApplyOwner won't restore them because the owner is unchanged by then. Was that the intent, or should the economy reset stay in CheckResultAsync (comparing against the owner at the start of the battle) while the mid-battle write only persists the ownership for recovery?

Nothing here blocks; #1 and #2 are cheap and I'd take both. Note that the Azure build for this head was still queued when I looked and there's no .NET SDK in my environment, so I haven't seen the new tests run — worth a glance once it finishes.

---

Generated by Claude Code

[sven-n]

Third pass — reviewed 73326a5 ("Harden Castle Siege Crown state handling")

All three points from the previous round are handled, each with a regression test:

• Clamped progress — MaximumProgressInterval caps the credited delta at 2s, and CrownProgressClampsDelayedUpdatesAsync proves a 30s gap can't satisfy the hold in one update. ✅
• Snapshot keyed by object id — LastBroadcastSwitchInfos is now a Dictionary<ushort, …> rebuilt per broadcast, and DuplicateSwitchSpawnsAreTrackedByObjectIdAsync covers the duplicate-spawn case that used to throw. ✅
• Packet field documented — the <Description> is in the XML and propagated consistently through ServerToClientPackets.cs, …Ref.cs, ConnectionExtensions.cs and both docs/Packets/*.md files, so the generated output is in sync. ✅
• Economy question answered — the comment in ApplyOwner states the intent (a seal ends the previous tenure, so its economy doesn't survive the handover) and RecaptureDoesNotRestorePreviousOwnershipEconomyAsync pins it down. That answers what I asked; no objection. ✅

The Azure build is green on this head (build 4545), so the new tests have run.

One thing I missed earlier, and one nit:

SynchronizePlayerAsync enumerates ActiveNpcs off the tick thread

CastleSiegeContext.ActiveNpcs is a plain List<T>; every mutation in CastleSiegeNpcController (:208, :438, :575) is under _runtimeLock, and readers elsewhere go through GetRuntimeSnapshot(), which copies under that same lock. The new code reads the list directly in four places — CastleSiegeSwitchMechanics.cs:84 and :120, CastleSiegeCrownMechanics.cs:127 and :138.

On the periodic-task path that's harmless, since PrepareAsync/DespawnAllAsync run from the same tick. But CastleSiegeSwitchMechanics.SynchronizePlayerAsync is called from CastleSiegePlugIn.SynchronizePlayerAsync, i.e. from ObjectAddedToMapAsync/PlayerStateChangedAsync on a player's own thread, outside ExecutionLock — so a player entering the siege map while a tick is spawning or despawning NPCs can hit InvalidOperationException: Collection was modified. It also writes context.IsCrownAvailable and crown.State from that thread via UpdateCrownState; the computed value is the same either way, so that part is benign, but a read-only variant for the sync path would be tidier. Switching the four reads to context.NpcController.GetRuntimeSnapshot() follows the existing convention and costs nothing.

Nit

MaximumProgressInterval (2s) is implicitly "two periodic ticks" and only holds while GameContext runs its task timer at 1000ms. A one-line comment saying so would save the next reader the trip to GameContext.

With the snapshot change I'd consider this ready from my side.

---

Generated by Claude Code