Repository navigation
Conversation
GameMapDefinition.Discriminator tells maps with the same number apart: the stages of Devil Square are seven definitions on maps 9 (1 to 4) and 32 (5 to 7). The extended client needs it to know the stage it is in. MapChangedExtended (C3 1C 0F) is MapChanged with the discriminator appended at index 15, after the 15 bytes of the original, which stays as it was. A client can tell by the length whether a server sends it: an older server sends the 15-byte MapChanged, whose bytes 10 to 14 are not initialized. The structs, the connection extension and the packet tests come from the XSLT transformations of the project, the markdown documentation from GenerateMarkdownDocs.xslt run with Saxon (the build runs it with npx xslt3).
…he extended client MuMain (the extended client, 106.3) needs to know which stage of Devil Square a player is in: its monster data can make a monster attack differently on one stage, and the map number alone does not tell the stages of a map apart. MapChangeExtendedPlugIn and TeleportExtendedPlugIn send MapChangedExtended with the discriminator of the player's map. Other clients keep MapChangePlugIn and TeleportPlugIn, and the original MapChanged. A discriminator which doesn't fit into a byte is sent as 0, instead of being cut to the one of another map. Tests: the extended client gets the discriminator with a map change, a teleport and a failed map change, a Season 6 client the original packet. PlayerTestHelper can now log in any player, so the remote player of the tests enters the world the usual way instead of getting its character set by reflection.
…inator A character which loses its connection during Devil Square can log in again and rejoin the game on its stage. That needs no map change: the mini game puts the character on the map of its stage before the character information is sent, so MapChangedExtended never tells the client the stage then. CharacterInformationExtended (C3 F3 03) gets the discriminator at index 92, which makes it 93 bytes long, and UpdateCharacterStatsExtendedPlugIn sends the one of the character's map. As with MapChangedExtended, a client can tell by the length whether a server sends it. The structs, the connection extension and the packet tests come from the XSLT transformations of the project, the markdown documentation from GenerateMarkdownDocs.xslt run with Saxon. Test: the extended client gets the discriminator when it enters the world on a stage of Devil Square.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 31 |
| Duplication | 6 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
The for loop moved its counter in the body (Sonar S127, reported by Codacy on MUnique#1346). Same packets, same failure without a header.
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Changes requested: src/Network/Packets/ServerToClient/ServerToClientPackets.xml defines MapChangedExtended with five reserved bytes (offsets 10–14), while the generated sender in src/Network/Packets/ServerToClient/ConnectionExtensions.cs:1412-1423 does not initialize them. GetSpan() need not return zero-filled memory, so a reused output buffer can leak stale bytes. Please explicitly initialize reserved bytes and test with a prefilled nonzero buffer. Please also verify compatibility with released MuMain and the companion extended client for map changes, teleports, and direct login to Devil Square.
The generated senders write only the fields of a packet, so its unused bytes kept what an earlier packet left in the reused output buffer: bytes 10 to 14 of MapChangedExtended and byte 89 of CharacterInformationExtended (review of MUnique#1346). The map change, the teleport and the character information of the extended client write into a span which is cleared first, as OpenNpcWindowPlugIn does. The remote view tests write into memory which is filled with 0xFF, and a test checks the unused bytes.
|
Thanks for the review. Both points are addressed. Unused bytes (d75043b): the extended client's map change, teleport and character information are now written into a span that is cleared first, as The remote view tests now write into a The original Compatibility, tested live against a server built from d75043b with a fresh database. Both clients report version 106.3, so both get the extended packets:
The packets are from the admin panel's network analyzer, and for both logins the server logged Unrelated to this PR, seen with both clients: going from Devil Square to character selection through the menu warps the character out, and the client's |
CancelPendingFlush and Complete had empty bodies (Sonar S1186) and FlushAsync ignored its cancellation token (Sonar S1172), reported by Codacy on MUnique#1346. The empty methods now say why nothing happens, and a flush with a cancelled token throws, as it does on a real pipe.
MuMain needs to know which stage of Devil Square a player is in: its monster data can make a monster attack differently on one stage (map variants, MuMain's monsters design D67), and the map number alone does not tell the stages apart. OpenMU already does: the seven Devil Square stages are map definitions on number 9 (stages 1–4) and 32 (stages 5–7) with
Discriminator1 to 7. This sends that discriminator to the extended client.The original
MapChangedpacket stays as it was (coding rules, rule 2): the extended client gets new or longer…Extendedpackets.Commits
MapChangedExtended(C3 1C 0F, 16 bytes) isMapChangedwithMapDiscriminatorappended at index 15, after the 15 bytes of the original. A client can tell by the length whether a server sends it: an older server sends the 15-byteMapChanged, whose bytes 10 to 14 are not initialized.MapChangeExtendedPlugInandTeleportExtendedPlugIn([MinimumClient(106, 3)]) sendMapChangedExtendedwith the discriminator of the player's map; other clients keepMapChangePlugIn/TeleportPlugInand the original packet. A discriminator which doesn't fit into a byte is sent as 0, not cut to the one of another map. Plugin names and descriptions in en, de and zh-CN.CharacterInformationExtended(C3 F3 03) getsMapDiscriminatorat index 92 and is 93 bytes long. A character which loses its connection in Devil Square can log in again and rejoin its stage without a map change; the mini game puts it on the stage's map before the character information is sent, so this is how the client learns the stage then.forcounter in its body (Sonar S127, reported by Codacy here).The structs, connection extensions and packet tests come from the project's XSLT transformations. The markdown documentation comes from
GenerateMarkdownDocs.xslt, run with Saxon (the build runs it withnpx xslt3); on master's XML it reproduces all 336 committed packet documents byte for byte.Testing
MapDiscriminatorRemoteViewTests: a remote player logs in throughPlayerTestHelperand warps to a Devil Square stage. The extended client getsMapChangedExtendedwith the stage for a map change, a teleport and a failed map change (also for discriminators 257 and -1, which it gets as 0), andCharacterInformationExtendedwith the stage when it enters the world on a stage. A Season 6 client gets the 15-byteMapChanged.MUnique.OpenMU.Network.Packets.Tests: 650/650.MUnique.OpenMU.Tests: 2507 passed, 2 skipped, 3 failed. The 3 are Discord text tests (GuildShowsMasterAndOnlineMembersAsync,WhoShowsCharacterAsync,SetupReportListsCreatedAdoptedAndFailed), which expect\nand get\r\non Windows; they don't touch this change.MapDiscriminatorRemoteViewTestspasses again (5/5).Clients
ReceiveJoinMapServer'ssafe_castrefuses only a packet shorter than its struct, andReceiveTeleportreads its fields without a length check.The fork PR is Mosch0512#7.