Guild role assignment for assistants and battle masters - #955
eduardosmaniotto wants to merge 4 commits into
Conversation
Implements the previously unhandled C1 E1 GuildRoleAssignRequest: only the guild master may promote/demote members to AssistantMaster, BattleMaster or NormalMember (no leadership transfer). The guild server now publishes position changes so members update without relog. Also fixes a NullReferenceException in GuildRelationshipChangeAction where failed validations crashed instead of returning the result, and sorts the guild list by rank (master, assistant, battle master, normal) then name.
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Code review of the guild role assignment changes: 2 findings, both in GuildServer.ChangeGuildMemberPositionAsync. The alliance null-deconstruction fix and the guild list ordering look correct.
Generated by Claude Code
| await guild.DatabaseContext.SaveChangesAsync().ConfigureAwait(false); | ||
| var listEntry = guild.Members[characterId]; | ||
| listEntry.PlayerPosition = role; | ||
| await this._changePublisher.AssignGuildToPlayerAsync(listEntry.ServerId, listEntry.PlayerName, new GuildMemberStatus(guildId, role)).ConfigureAwait(false); |
There was a problem hiding this comment.
Reusing AssignGuildToPlayerAsync for a position change registers the member a second time in the game server's guild list.
GameServer.AssignGuildToPlayerAsync ends with Context.RegisterGuildMemberAsync(player), and GameServerContext.RegisterGuildMemberAsync does a plain guildList.Add(guildMember) on a LockableList<Player> : List<Player> — no duplicate check. The member was already registered when they entered the world (PlayerEnteredGameAsync → AssignGuildToPlayerAsync).
Scenario: guild master promotes an online member to battle master. From then on ForEachGuildPlayerAsync iterates that player twice, so every guild chat message (GameServer.cs:227) and every guild-wide action (/guildmove, /guilddisconnect, guild war messages) is delivered to them twice. On logout UnregisterGuildMemberAsync → List.Remove removes only the first occurrence, so a stale Player reference stays in the list and keeps receiving guild broadcasts (and leaks). Each further promotion/demotion adds another duplicate.
A dedicated publisher member (or an idempotent registration / a status-only update path that doesn't re-register) is needed here.
Generated by Claude Code
| if (guildMember != null) | ||
| listEntry.PlayerPosition = role; | ||
| if (listEntry.PlayerName is not null) | ||
| { |
There was a problem hiding this comment.
The guard only checks PlayerName is not null, but PlayerName is kept after logout while SetServerId(..., OfflineServerId) sets ServerId to 0xFF (GuildMemberLeftGameAsync). So for an offline member whose name was cached the publish still happens with serverId == 0xFF: GuildChangeToGameServerPublisher silently drops it, and the Dapr GuildChangePublisher invokes gameServer256 and logs an error on every call. Reachable when the target logs out between the action's online check and the guild server processing the change (and for any other future caller of this public API).
| { | |
| if (listEntry.PlayerName is not null && listEntry.ServerId != OfflineServerId) |
Generated by Claude Code
2 participants
Summary
C1 E1 GuildRoleAssignRequest.GuildRoleAssignHandlerPlugInmaps the wire role toGuildPosition(leadership transfer and unknown roles rejected, undocumented
Typebyte ignored) and
GuildRoleAssignActionenforces master-only accesswith same-guild online target resolution.
GuildServer.ChangeGuildMemberPositionAsyncnow publishes via the existing
AssignGuildToPlayerAsync, so no new packetsor publisher members.
GuildRelationshipChangeActiondeconstructed a null
GuildDataon every failed validation, throwingNullReferenceExceptioninstead of sending the result. Both callsites now check success/null before deconstructing.
GuildListRequestActionsorts members byrank (master, assistant, battle master, normal) then name, since the
client renders them in received order.