Client freeze on character select / loading screen fix#404
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the client would hang during the loading screen due to incorrect memory alignment of network packets. By explicitly enforcing packing on specific data structures, the client can now correctly interpret the data sent by the server, preventing silent failures in the protocol state machine and allowing the rendering process to resume as expected. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces memory alignment packing using #pragma pack(push, 1) and #pragma pack(pop) for network-related structs in WSclient.h. Feedback indicates that the packing block is incorrectly scoped: it should be expanded to include PRECEIVE_CHARACTER_LIST_EXTENDED to prevent buffer offset drift during data parsing, and it should also encompass later structs like PCREATE_CHARACTER_EXTENDED to ensure correct sizeof calculations for safe_cast operations, which are critical for player rendering.
Adds two defenses for the bug class behind PR sven-n#402 / fixed by sven-n#404: 1. static_assert(sizeof == N) on PRECEIVE_CREATE_CHARACTER (19), PRECEIVE_JOIN_MAP_SERVER_EXTENDED (92), and PRECEIVE_REVIVAL_EXTENDED (36). If the #pragma pack(push, 1) ever gets dropped or a field is added/reordered, the build now breaks at the offending struct with a pointer to check the pragma. Wire sizes match OpenMU's XML schema (CharacterInformationExtended, RespawnAfterDeathExtended). 2. Named overload safe_cast<T>(span, "TYPE_NAME") that logs an MCD_ERROR with packet type + received vs expected size on failure. Used at the join-map receive site. The dispatcher also logs the user-visible symptom ("loading screen will appear frozen") so a future drift turns a silent freeze into a one-line console diagnostic.
Root Cause: Missing
#pragma pack(push, 1)aroundPRECEIVE_JOIN_MAP_SERVER_EXTENDEDCommit:
b46d2cc1("fix pragma and DWORD to WORD")File:
src/source/Network/Server/WSclient.h(line ~344)What happened
The commit removed
#pragma pack(push, 1)and#pragma pack(pop)around thePRECEIVE_JOIN_MAP_SERVER_EXTENDEDstruct and changedDWORD ResetstoBYTE Spare; WORD Resets:Effect
Without
#pragma pack(push, 1), the struct uses default alignment (8-byte foruint64_t). This increasessizeof(PRECEIVE_JOIN_MAP_SERVER_EXTENDED)from 92 to ~96 bytes (tail padding to the next multiple of 8).Root cause chain
safe_cast<>inReceiveJoinMapServer()(WSclient.cpp:944) checksspan.size() < sizeof(T)→92 < 96= truesafe_castreturnsnullptrfalse, error is silently swallowed (line 13207-13210)CurrentProtocolStatestaysREQUEST_JOIN_MAP_SERVEREnableMainRendernever becomestrueSwapBuffersis never calledFix
Restore
#pragma pack(push, 1)/#pragma pack(pop)around the struct in WSclient.h. Since adjacent structs (PRECEIVE_CREATE_CHARACTER,PRECEIVE_REVIVAL_EXTENDED) also use raw casts with network data, it is safest to wrap all three in one packed block: