Implement Castle Siege registration and mark system

[Zylkien]

Summary

Implements the Castle Siege guild registration and Sign of Lord registration system.

• Adds guild and alliance registration with the required validation and result codes.
• Adds guild unregistration during the registration period.
• Adds Sign of Lord submission, inventory consumption, and persistent mark counting.
• Preserves the Sign of Lord when registration persistence fails.
• Adds registration-state queries for guild and alliance members, including when the alliance master is offline.
• Adds the B2 registration request handlers, view interfaces, and remote-view implementations.
• Restores persistent registrations after a server restart.
• Adds configurable Sign of Lord item definition and level settings.
• Adds an EF migration and an update plug-in for existing Season 6 databases.
• Adds focused handler, serialization, persistence, alliance, and initialization tests.

The 30-minute registration notifications are provided by the state-machine implementation merged in #861.

Packet dependency

The complete Castle Siege packet definitions were merged into master through #863 as part of #731. This PR consumes those definitions and does not add or modify packet XML, generated packet artifacts, or packet documentation.

Sign of Lord availability

The default Season 6 configuration uses item 14/21 at level 3 as the Sign of Lord and permits that item level.

This PR intentionally does not add a drop source. Obtaining the Sign of Lord remains a server-configuration, administrator-tooling, or follow-up gameplay concern.

Validation

• Castle Siege registration result codes match protocol values 0–8.
• Sign of Lord result codes match protocol values 0–3.
• Alliance masters and standalone guild masters can register.
• Alliance members can query their master guild’s registration when the master is offline.
• Invalid registration scenarios are rejected.
• Unregistration removes the persistent registration.
• Sign of Lord submission consumes the configured inventory item.
• A Sign of Lord is preserved if mark persistence fails.
• Mark counts persist and survive context reloads.
• Existing customized Sign of Lord configuration is preserved.
• Configurations without the default item initialize without throwing.
• Focused registration, alliance, serialization, and initialization tests pass.
• The complete solution builds without errors.
• Changed-file formatting and analyzer checks pass.

Closes #723

[sven-n]

Review — Castle Siege registration & mark system

I verified the four new S→C packet layouts against the MuMain client (WSclient.h / WSclient.cpp) and reviewed the game-logic, handler and view code.

What's good

• Packet layouts are correct. PMSG_ANS_REGCASTLESIEGE (13), PMSG_ANS_GIVEUPCASTLESIEGE (14), PMSG_ANS_GUILDREGINFO (19) and PMSG_ANS_REGGUILDMARK (17) match the XML field offsets 1:1, including the 8-byte guild name and the mark count: the client assembles btGuildMark4..1 into a DWORD LSB-first, so IntegerBigEndian is right.
• Result codes match the client. CastleSiegeRegistrationResult 0–8 maps exactly onto ReceiveBCReg, and CastleSiegeUnregistrationResult 0–3 onto ReceiveBCGiveUp (incl. NotRegistered = 2, WrongState = 3).
• Clean separation action / view interface / remote view, everything guarded by ExecutionLock, and good test coverage (validation matrix, persistence round-trip after a simulated restart, subcodes, backpack-index translation, serialization).

Issues worth addressing

1. Mark registration throws away two client-supported error codes. ReceiveBCRegMark handles 0 = failed, 1 = ok, 2 = "This guild has not participated in castle siege", 3 = "Incorrect item was registered". The view interface only carries bool success, so both specific messages can never be shown — the guild-not-registered case and the wrong-item case both surface as the generic failure. Consider a CastleSiegeMarkRegistrationResult enum mirroring 0–3, the same way registration/unregistration already do.

2. The Emblem can be destroyed without the mark being counted. In CastleSiegeRegisterMarkAction, DestroyInventoryItemAsync runs first and IncrementMarksAsync throws InvalidOperationException if the registration row disappeared meanwhile — the item is gone, the mark is not persisted, and the exception escapes into the packet handler (no view response is sent either). Increment first, or catch and restore/report.

3. Full Guild table load per B2 request, inside the siege lock. CastleSiegeContext.GetPersistentGuildIdAsync does context.GetAsync<Guild>() and a client-side FirstOrDefault on every register/unregister/state/mark packet, and it runs while holding ExecutionLock — the same semaphore the state-machine timer uses. Any player can spam B2-03 and serialize a full-table scan against the siege loop. GuildServer already maintains the Guid→uint mapping (_guildIdMapping); exposing the persistent id on Interfaces.Guild / IGuildServer (or adding a by-name query next to GuildWithNameExistsAsync) would remove the scan entirely.

4. Multi-game-server duplicate registrations. RegisteredGuilds and ExecutionLock are per-process. A guild master connected to server B doesn't see server A's cached registration, so the AlreadyRegistered / RegistrationOrder logic can produce a second row for the same guild. If that's an accepted limitation inherited from #861, a note in the class docs would help.

5. The mark item likely can't exist in the shipped configuration. EmblemGroup = 14, EmblemNumber = 21, EmblemLevel = 3 — in VersionSeasonSix/Items/Misc.cs, 14/21 is Rena, created without a MaximumItemLevel, so a level-3 instance can't be produced by the initialization data. Either seed the Sign/Emblem of Lord definition or (preferably) make it a configurable ItemDefinition reference on CastleSiegeConfiguration, next to RegisterMinLevel/RegisterMinMembers. Also worth unifying the naming: the PR body says "Sign of Lord", the code says "Emblem of Lord", the packet docs say "guild mark".

Smaller points

• CastleSiegeMarkRegistrationHandlerPlugIn.HandlePacketAsync reads ItemIndex (index 4) with no packet.Length < CastleSiegeMarkRegistration.Length guard; the repo's convention (e.g. AnimationHandlerPlugIn, CharacterMoveBaseHandlerPlugIn) is to check first, otherwise a short B2 04 packet throws out of the handler.
• The three new RemoteView/CastleSiege plugins lack the [Display(Name = ..., Description = ..., ResourceType = typeof(PlugInResources))] attribute that every other remote-view plugin (and the message handlers in this very PR) carries — the resx additions only cover the handlers.
• checked((byte)Math.Min(registration.RegistrationOrder, byte.MaxValue)): after the Math.Min the checked can never trip; a plain cast reads better (and the silent cap at 255 deserves a comment).
• CastleSiegeGuildResolver.ResolveRegistrationGuildAsync falls back to GetGuildIdByNameAsync, which only searches guilds currently loaded in memory and returns 0 when not found — the resulting RuntimeId can be a meaningless 0. Nothing consumes it on that path today; consider dropping it from the reference for the read-only query.
• CastleSiegeUnregisterGuildAction discards the resolver's result and reports the generic Failed where the register path distinguishes NoGuild/InvalidGuild. That's forced by the client's 0–3 range, but a one-line comment would prevent a future "bug fix".
• RegistrationOrder is max + 1 over the in-memory set, so ranks are not compacted after an unregistration — presumably intended, just flagging.

Status

GitHub reports the PR as conflicting with master (mergeable_state: dirty) — it needs a rebase/merge before it can be evaluated by CI.

---

Generated by Claude Code

[sven-n]

Second pass — commit 875352d ("Address Castle Siege registration review")

Thanks, all five main points and every smaller note from the previous round are addressed, most of them with a regression test:

• CastleSiegeMarkRegistrationResult (0–3) now mirrors ReceiveBCRegMark exactly — verified again against the client, incl. the "not participated" and "incorrect item" cases.
• The Sign of Lord is no longer destroyed before the mark is persisted; IncrementMarksAsync returns int? and evicts the stale cache entry, covered by MarkRegistrationPreservesItemWhenRegistrationDisappearedAsync.
• The full Guild table scan is gone: IGuildServer.GetPersistentGuildIdAsync(uint) reads the guild server's in-memory dictionary, with Dapr host/client wiring. Nice.
• The item is configuration-driven (SignOfLordItemDefinition / SignOfLordItemLevel), with initializer, update plug-in (UpdateVersion 102) and EF migration.
• Packet-length guard, [Display] attributes on the three remote views, the dead checked, the multi-server remark and the RegistrationOrder comment are all in.

New points from this revision

1. Alliance members may now be reported as not registered. ResolveRegistrationGuildIdAsync resolves the alliance via GetGuildIdByNameAsync, which only searches GuildServer._guildDictionary — guilds that are loaded because a member is online. If the alliance master guild has nobody online, that returns 0, the method returns null, and a member of a registered alliance gets NotRegistered / rank 0. The previous name-based persistence lookup did not have this hole. The guild container already holds the DataModel.Entities.Guild (which has the Guid), so an IGuildServer overload that resolves the alliance master's persistent id directly — or a by-name persistence lookup as fallback — would close it. Worth a test with an offline alliance master.

2. InitializeRegistration overwrites customised configuration. It's now called unconditionally from CreateCastleSiegeConfiguration, including the existingConfiguration branch, so an admin's own SignOfLordItemDefinition / SignOfLordItemLevel is reset on every initialization run. Consider only filling it when SignOfLordItemDefinition is null. Related: this.GameConfiguration.Items.Single(...) throws on any configuration that has no 14/21 item — SingleOrDefault plus an early return would be friendlier, especially since the update plug-in is IsMandatory.

3. The Sign of Lord is still not obtainable from the shipped data. Raising Rena's MaximumItemLevel to 3 only permits a level-3 instance to exist — I checked DefaultDropGenerator.GenerateItemDrop, which leaves the level at 0 for a plain DropItemGroup, so the Rena drop wiring is not affected (good), but nothing creates a level-3 item either. Is that intentionally left to GM item creation / a later PR? A line in the PR description would help.

4. Item destruction is now the last step, so the mirror-image failure exists: if DestroyInventoryItemAsync throws, the mark is already persisted and the item is kept. Much less severe than the original ordering, but a short comment (or a compensating decrement) would document the choice.

5. Scope has grown well past the description. This revision pulls in the complete Castle Siege server-to-client packet set (~10k generated lines, 30+ new docs/Packets files, ClientToServerPackets.xml changes, the EF migration and CastleSiegePacketDefinitionTests). The PR body still says it deliberately contains "only the four response definitions required by this registration phase, so it can be reviewed independently of #736" — that's no longer accurate, and the overlap with #736 now needs an explicit decision on which PR owns those definitions. Please update the description.

6. Still conflicting with master; after the rebase the new migration's timestamp/ordering and UpdateVersion = 102 should be re-checked against whatever landed meanwhile.

---

Generated by Claude Code

[sven-n]

Third pass — commit efda253 ("Refine Castle Siege registration edge cases")

Everything from the second round is resolved, and the PR is now in good shape:

• Offline alliance master is fixed properly: IGuildServer.GetPersistentAllianceMasterGuildIdAsync resolves the master's persistent id straight from the guild container (with Dapr host/client wiring), so GetGuildIdByNameAsync is out of the path entirely. The GuildAllianceTest case that takes the master's last member offline is exactly the right regression test, and asserting GetGuildIdByNameAsync is never called in the game-logic test is a nice touch. The registration path stores under the alliance master's own persistent id, so writer and reader agree.
• Initializer no longer overwrites a customised SignOfLordItemDefinition, and SingleOrDefault + early return means a configuration without item 14/21 initializes instead of throwing — both covered in TestInitializationWithEfCore.
• The persist-before-destroy ordering is now documented with its trade-off.
• Scope is clean again: with the packet set merged through #863, the diff is only game logic, guild-server API, configuration/migration and tests. Description updated accordingly, the Sign of Lord availability question is answered explicitly, base is clean, and both checks (Azure build, Codacy) are green.

Remaining nits (non-blocking)

• GuildServer.GetPersistentAllianceMasterGuildIdAsync uses guild.Guild.AllianceGuild is Guild allianceMaster ? allianceMaster.Id : guild.Guild.Id. Since AllianceGuild is typed as Interfaces.Guild, an instance that isn't a DataModel.Entities.Guild silently falls back to the guild's own id — i.e. it would quietly bucket an alliance member under the wrong registration rather than failing. A is null check plus an explicit cast would make that case loud instead of subtly wrong.
• In TestInitializationWithEfCore, the trailing restore block (re-adding signOfLord and resetting the configuration) has no assertions after it — if it's only there so later parts of the fixture stay valid, a one-line comment saying so would help; otherwise it can go.
• GuildAllianceTest fully-qualifies MUnique.OpenMU.GuildServer.GuildServer.OfflineServerId; a using alias would read better.

Nothing here blocks a merge from my side — I'd leave the formal approval to you.

---

Generated by Claude Code

[sven-n]

Approving — all review findings from the three rounds are addressed, the packet layouts and result codes are verified against the MuMain client, the scope is back to registration logic plus configuration/migration and tests, and CI is green. The remaining nits are non-blocking and can be picked up in a follow-up.

---

Generated by Claude Code