Implement Castle Siege Mini-Map & Guild Commands (#730)

[didiconcs]

Summary

Implements Castle Siege Phase 11 (#730): the mini-map broadcast and guild-command relay.

• CastleSiegeMiniMap gathers own-side player positions and alive gate/statue positions, and pushes them every 3 seconds to every online alliance master of a participating guild, while the siege is in the Start state.
• CastleSiegeGuildCommandAction lets an alliance master issue a directional command (type + coordinates) to every player on their own side.
• New ICastleSiegeMiniMapPlugIn / ICastleSiegeCommandPlugIn view interfaces and matching GameServer/RemoteView + GameServer/MessageHandler plug-ins, following the existing Castle Siege conventions.

Design decisions (spec doesn't fully cover the current codebase shape)

The original implementation plan for this phase predates phases 4-10, so a few things needed a concrete call rather than a literal reading, flagging these for review:

• Push, not subscribe. There's no existing "open mini-map" request packet, so every online alliance master of a participating guild is treated as an implicit recipient of the 3-second push, rather than gating on a per-player "wants updates" request. Happy to add a request packet + flag if a stricter model is preferred.
• CastleGuildCommand.Team is never trusted for authorization or audience selection. The issuer's actual side is always re-derived server-side from CastleSiegeContext, so a spoofed Team byte can't mis-target orders or leak to the wrong side.
• Alliance master = the issuing guild's GuildMaster, where that guild's CastleSiegeGuildParticipant.IsAllianceMaster is true in context.FinalGuildList.
• Both mini-map and guild commands are restricted to CastleSiegeState.Start. The spec says so explicitly for the mini-map; for guild commands this is inferred (commands are meaningless outside battle) rather than spec-mandated, worth confirming that's the intended behavior.
• Only alive gates/statues appear on the NPC layer (CastleSiegeMiniMapNpcType only defines Gate/GuardianStatue, matching the wire format).

Verification

• dotnet build src/Startup/MUnique.OpenMU.Startup.csproj -c Debug: 0 errors.
• dotnet test tests/MUnique.OpenMU.Tests --filter FullyQualifiedName~CastleSiege: 106/106 passing (existing suite, confirms no regression, this PR doesn't add new automated tests yet, since the mini-map/command flow is push-based and harder to unit-test without a fuller integration harness; happy to add coverage if there's a preferred pattern from the recent Castle Siege PRs).

Known minor items

A couple of small things worth a look, not blocking:

• Client-supplied command coordinates are relayed without validation against actual map bounds.
• The Start-state check is read once before acquiring the context lock and isn't re-checked once held, so a command could in theory land in the narrow window right as the state transitions away from Start.

🤖 Generated with Claude Code

[sven-n]

Review

Overall this is well-executed and fits the existing Castle Siege conventions closely: the handler mirrors CastleSiegeGateOperateHandlerPlugIn, the two remote views use the exact GetRequiredSize / GetSpan(size)[..size] / SendAsync(Write) idiom of CastleSiegeGuildListPlugIn, and splitting a lock-free GetDefenseStructures out of GetDefenseStructureSnapshotAsync (with the remark explaining why) is the right call — GetRuntimeSnapshot copies under _runtimeLock and releases before returning, so there is no lock-ordering hazard with ExecutionLock.

I checked the wire format against the client (sven-n/MuMain). The offsets line up exactly: PWHEADER_DEFAULT_WORD2 = 4-byte C2 header + INT Value → count at index 4, entries at 8 (CastleSiegeMiniMapPlayerPositions), and PWHEADER_DEFAULT_WORD = header + BYTE Value → count at 4, entries at 5 (CastleSiegeMiniMapNpcPositions). NpcType 0/1 maps to the client's m_byType + 1 (1 = gate, 2 = statue). Good.

Two things came out of that client comparison that I think need to change, plus a few smaller items.

---

1. Team is a command-group index, not a side — overwriting it breaks the feature

This is the main finding, and it inverts the design decision in the PR description.

In the client, Team is not an audience or authorization field. It is the number of the command group (squad) the commander picked in the UI:

• NewUISiegeWarCommander.cpp:82-88 — SelectCmd.byTeam = m_iCurSelectBtnGroup; then SendCastleGuildCommand(...). m_iCurSelectBtnGroup is the index of one of MAX_COMMANDGROUP (= 7, NewUISiegeWarBase.h:10) group buttons.
• On receive, NewUISiegeWarBase.cpp:539 — SetMapInfo stores into m_CmdBuffer[data.byTeam], one slot per group.
• NewUISiegeWarBase.cpp:417-460 — RenderCmdIconInMiniMap renders all 7 slots simultaneously and draws the label byTeam + 1 ("1".."7") next to each icon.

So the audience is already fully determined server-side by which players receive the packet — Team only selects the display slot and the number shown on the mini map. Replacing it with (byte)CastleSiegeJoinSide (1..4) means:

• the commander's group selection is silently discarded — an order for group 5 renders as "2".."5" depending on the issuer's side;
• all 7 independent markers collapse into a single slot per side, so a new order always overwrites the previous one instead of standing alongside it;
• issue Castle Siege Mini-Map & Guild Commands #730's packet spec ("team : byte — Team identifier") and the packet definition's own "Team number from 0 to 7" are not honoured.

Recommendation: relay request.Team unchanged, but validate it — and this part matters, because SetMapInfo indexes m_CmdBuffer[data.byTeam] with no bounds check (GuildCommander m_CmdBuffer[7], NewUISiegeWarBase.h:95). A client sending Team = 200 would currently be an out-of-bounds write in every same-side player's client. So the handler should drop the packet (or clamp) when Team > 6. The rendering path does guard with byTeam >= 0 && byTeam <= 6, but SetMapInfo writes before that check runs.

That also flips the two items in "Known minor items":

• Coordinate validation is a non-issue — PositionX/PositionY are bytes and the siege map is 256×256, so every possible value is in bounds. Nothing to add there.
• The Team byte you deliberately dropped is the one that actually needs validation. Worth swapping the emphasis in the code comment too.

Sanity check on the enum mapping while relaying: _ => Wait turns any out-of-range Command byte into a valid "wait" marker. The client's RenderCmdIconInMiniMap treats byCmd == 3 as "empty slot" and leaves iWidth/iHeight uninitialized for anything above 2, so rejecting values > 2 outright is safer than mapping them to Wait.

2. Re-check the Start state inside the lock

You flagged this yourself, and it's a two-line fix with a direct precedent: CastleSiegeGateOperateAction.IsAuthorizedAsync reads context.CurrentState while holding ExecutionLock. Moving the CurrentState: Start check from the pre-lock pattern match into the try block closes the window and matches the neighbouring action. Keep the cheap Configuration.Enabled check outside if you like.

While you're in there: the issuer's side comes from participant.Side but the recipient filter uses context.GetPlayerJoinSide(candidate). Those are two different sources of truth — GetTrackedPlayerJoinSide prefers the per-character PlayerJoinSides entry and only falls back to the guild list. They can diverge briefly after CastleSiegeCrownMechanics swaps guild.Side on a crown capture (the resync via SetPlayerJoinSideAsync happens right after, but not atomically). Using context.GetPlayerJoinSide(player) for the issuer as well makes the fan-out self-consistent — the GetSiegePlayers().Contains(player) check already guarantees it won't return None for map reasons. Same applies to entry.Participant!.Side in CastleSiegeMiniMap.BroadcastAsync.

3. Heads-up: the current client discards the mini-map packets entirely

Not caused by this PR, but it means the mini-map half can't be verified end-to-end today, so it's worth knowing before this is called done.

WSclient.cpp:12028 and 12048 both guard with:

if (g_pSiegeWarfare->GetCurSiegeWarType() != TYPE_GUILD_COMMANDER)
    return;

GetCurSiegeWarType() returns m_iCurSiegeWarType, which is assigned from CNewUISiegeWarfare::SIEGEWAR_TYPE_* (NewUISeigeWarfare.h:20-26: NONE = -1, OBSERVER = 0, COMMANDER = 1, SOLDIER = 2). But TYPE_GUILD_COMMANDER is from the unrelated csmapinterface enum (_enum.h:3601-3607: TYPE_OBSERVER = 0, TYPE_GUILD_SOLDIER = 1, TYPE_GUILD_COMMANDER = 2). So the guard admits only soldiers (2) and rejects commanders (1) — and CNewUISiegeWarfare::SetGuildMemberLocation then no-ops for anyone who isn't SIEGEWAR_TYPE_COMMANDER (1). Net effect: both B6 and BB are always dropped. The two constants need to be unified on the client side (a sven-n/MuMain change) before any of this is observable.

4. Mini-map request handling vs. the spec

The push model is a defensible call and there is genuinely no client→server mini-map request packet in ClientToServerPackets.xml today. Two things to square up though:

• Castle Siege Mini-Map & Guild Commands #730's acceptance criterion is "Only alliance masters of participating guilds can request the mini-map", with CastleSiegeMiniMapRequestHandlerPlugIn and a tracking set in the context. If push is the accepted model, that criterion should be struck from the issue rather than left open.
• The XML descriptions ("sends ... to a Castle Siege mini-map requester") and the new doc comments ("broadcasts ... to requesting alliance masters", CastleSiegeMiniMap.cs) still describe a request model that doesn't exist. Worth rewording to match the implementation.

One ordering detail is load-bearing and undocumented: ReceiveGuildMemberLocation calls ClearGuildMemberLocation() for the whole buffer, and ReceiveGuildNpcLocation only appends. So the player packet must be sent before the NPC packet in every cycle, and the player packet must be sent even when the list is empty — which SendAsync does correctly today. A short comment there would stop a future refactor from swapping or short-circuiting them.

Also missing from the spec: the "up to 1000 points per guild" cap (the client reserves 1600 in m_vGuildMemberLocationBuffer). It's a std::vector, so there's no overflow risk, and the C2 length field won't wrap until ~32k players — but a .Take(1000) is free and keeps the packet honest.

5. Tests

tests/MUnique.OpenMU.Tests already has the pattern this needs: CastleSiegeNpcRemoteViewTests, CastleSiegeCrownRemoteViewTests, CastleSiegeMachineRemoteViewTests etc., all built on CastleSiegeRemoteViewTestHelper.CreatePlayer() which hands you a player plus the raw output. Serializing ShowPlayerPositionsAsync / ShowNpcPositionsAsync / ShowGuildCommandAsync and asserting the bytes is a dozen lines each and requires none of the integration harness you mentioned — it would also have caught the Team semantics above. That's the gap I'd most like closed before merge; the push/broadcast loop itself is fine to leave uncovered for now.

Smaller items

• No rate limit on the command handler. Each accepted packet fans out to every same-side player on the map, with no cooldown, and takes ExecutionLock on the way. The client's byLifeTime = 100 means rapid re-issues are pointless for gameplay, so a ~1 s per-player cooldown in the action would cost nothing and removes the amplification vector.
• Tick placement. Task.WhenAll in BroadcastAsync surfaces exceptions, and the mini-map block sits before the NextNpcSaveUtc block in OnTickAsync. A single failing send would skip the periodic NPC state save for that tick. Moving the block after the save (or catching around the fan-out) makes that independent.
• PlugInResources.Designer.cs — the diff also strips trailing whitespace from two unrelated lines (617, 1031). Harmless, but it makes the file diverge from what the ResX generator emits; worth reverting to keep the generated file byte-clean.
• The spec listed ICastleSiegeMiniMapNpcPlugIn / CastleSiegeMiniMapNpcPlugIn as separate files. Merging both sends into one interface is cleaner and I'd keep it — just noting it as a deliberate deviation from the file list in Castle Siege Mini-Map & Guild Commands #730.
• GetSiegePlayers() is re-enumerated a few times per broadcast (once for recipients, once per distinct side). Irrelevant at real player counts; only worth folding into a single grouped pass if you're touching the method anyway.

CI is green and the packet definitions were already in master, so nothing else needs regenerating. With the Team relay fixed and the state check moved under the lock, this looks good to me.

---

Generated by Claude Code

[didiconcs]

Thanks for the thorough review, especially for checking the wire format against the client, that caught something real.

Addressed everything:

1. Team semantics fixed. Team is now relayed unchanged from the request instead of being overwritten with (byte)side - the handler drops the packet if Team > 6 (bounds check for the client's GuildCommander[7]) or if Command doesn't map to a known value (previously defaulted to Wait, now rejected instead).
2. Start-state check moved inside ExecutionLock in CastleSiegeGuildCommandAction, matching CastleSiegeGateOperateAction's pattern.
3. Issuer side now resolved via context.GetPlayerJoinSide(player) in both the guild-command action and CastleSiegeMiniMap.BroadcastAsync, same source as the recipient filter.
4. Tests added (CastleSiegeMiniMapRemoteViewTests): mini-map player/NPC packet serialization, the empty-list-still-sent case, and specifically a regression test asserting ShowGuildCommandAsync relays Team unchanged - this one would have caught the original bug.
5. Reworded the mini-map XML/doc comments away from "requester" language to match the push model that's actually implemented.
6. Added the 1s per-player cooldown and the .Take(1000) cap you flagged under "smaller items", and moved the mini-map broadcast to run after the periodic NPC save in OnTickAsync so a failing send can't skip it.
7. Reverted the incidental whitespace diff in PlugInResources.Designer.cs.

Left #730's "only alliance masters can request the mini-map" acceptance criterion as-is since I can't edit that issue myself, flagging here that it should be struck or reworded given the push model - happy to leave that to a maintainer.

On your heads-up about the client dropping these packets today (TYPE_GUILD_COMMANDER/SIEGEWAR_TYPE_COMMANDER mismatch): noted, that's tracked as a sven-n/MuMain issue, not something for this PR.

123/123 tests passing, clean build.