Conversation
Update creature mark parsing to support the newer server format introduced in client version 1076+. Added detailed documentation for parseCreaturesMark in the header. parseCreaturesMark now maintains backward compatibility for <1076 by reading the legacy single mark byte, while for >=1076 it reads a squareType and squareColor and applies behavior accordingly: type==0 or color==0 removes static and timed squares, type==2 shows a permanent static square, otherwise a timed square is added. Also improved the debug log message when a creature ID is not found.
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9e92f7410
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (squareType == 0 || squareColor == 0) { | ||
| creature->hideStaticSquare(); | ||
| creature->removeTimedSquare(); |
There was a problem hiding this comment.
If the 1076+ server sends a square color of 0 to mean the default color (the old path normalized 0→1 for both static and timed squares, and Creature::addTimedSquare still maps color != 0 ? color : 1), this new branch removes both static and timed squares instead of rendering them. That’s a behavior change for any square update where squareType is flash/stay but squareColor is 0, and it will cause marks to disappear instead of showing the default color. This only manifests when the server uses 0 as a valid color in the new protocol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Updates client-side parsing of server “creature mark / square” messages in ProtocolGame, expanding support for the v1076+ protocol while keeping legacy behavior for older client versions.
Changes:
- Refactors
ProtocolGame::parseCreaturesMarkto parsesquareType+squareColorfor client versions >= 1076, while retaining the <1076 single-byte timed-square behavior. - Fixes the debug trace message to reference the correct function name.
- Adds a Doxygen comment describing the creature mark protocol and behavior rules.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/client/protocolgameparse.cpp |
Implements new v1076+ mark parsing logic and updates debug logging. |
src/client/protocolgame.h |
Adds Doxygen documentation for parseCreaturesMark. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const uint32_t creatureId = msg->getU32(); | ||
| const bool isPermanent = g_game.getClientVersion() >= 1076 ? msg->getU8() == 0 : false; | ||
| const uint8_t markType = msg->getU8(); | ||
|
|
||
| const auto& creature = g_map.getCreatureById(creatureId); | ||
| if (!creature) { | ||
| g_logger.traceDebug("ProtocolGame::parseTrappers: could not get creature with id {}", creatureId); | ||
| g_logger.traceDebug("ProtocolGame::parseCreaturesMark: could not get creature with id {}", creatureId); | ||
| return; |
There was a problem hiding this comment.
Returning early when the creature is not found now happens before reading the rest of the mark payload. That leaves unread bytes in the InputMessage (markType for <1076, squareType+squareColor for 1076+), which will desynchronize parsing of subsequent opcodes. Consume/discard the appropriate bytes before returning (or parse the fields first, then apply only if creature exists).
| * @param msg Input message containing: | ||
| * - uint32 creatureId: Target creature identifier. | ||
| * - uint8 squareType: Square behavior type: | ||
| * - 0 = SQUARE_REMOVE: remove any square (static and timed) | ||
| * - 1 = SQUARE_FLASH: temporary (timed/flashing) | ||
| * - 2 = SQUARE_STAY : permanent (static) | ||
| * - uint8 squareColor: 8-bit color used by the square. | ||
| * | ||
| * @note If the creature cannot be found in the map, the function logs a debug trace | ||
| * and returns without doing anything. | ||
| * | ||
| * Behavior rules: | ||
| * - If @c squareType == 0 OR @c squareColor == 0: | ||
| * Removes any square (static and timed). | ||
| * - If @c squareType == 2: | ||
| * Shows a permanent/static square using @c squareColor. | ||
| * - Otherwise: | ||
| * Adds a timed square using @c squareColor. |
There was a problem hiding this comment.
The Doxygen comment describes only the 1076+ wire format (creatureId + squareType + squareColor), but the implementation also supports <1076 where only a single uint8 markType is read and applied as a timed square. Please document the pre-1076 format/behavior as well (or explicitly scope the comment to clientVersion >= 1076).
| * @param msg Input message containing: | |
| * - uint32 creatureId: Target creature identifier. | |
| * - uint8 squareType: Square behavior type: | |
| * - 0 = SQUARE_REMOVE: remove any square (static and timed) | |
| * - 1 = SQUARE_FLASH: temporary (timed/flashing) | |
| * - 2 = SQUARE_STAY : permanent (static) | |
| * - uint8 squareColor: 8-bit color used by the square. | |
| * | |
| * @note If the creature cannot be found in the map, the function logs a debug trace | |
| * and returns without doing anything. | |
| * | |
| * Behavior rules: | |
| * - If @c squareType == 0 OR @c squareColor == 0: | |
| * Removes any square (static and timed). | |
| * - If @c squareType == 2: | |
| * Shows a permanent/static square using @c squareColor. | |
| * - Otherwise: | |
| * Adds a timed square using @c squareColor. | |
| * This function supports different wire formats depending on the client version. | |
| * | |
| * For clientVersion < 1076: | |
| * - The message contains a single field: | |
| * - uint8 markType: Encoded mark behavior. Non-zero values are interpreted as a | |
| * temporary/timed mark (equivalent to a flashing square). A zero value removes | |
| * any existing mark on the creature. | |
| * | |
| * For clientVersion >= 1076: | |
| * - @param msg Input message containing: | |
| * - uint32 creatureId: Target creature identifier. | |
| * - uint8 squareType: Square behavior type: | |
| * - 0 = SQUARE_REMOVE: remove any square (static and timed) | |
| * - 1 = SQUARE_FLASH: temporary (timed/flashing) | |
| * - 2 = SQUARE_STAY : permanent (static) | |
| * - uint8 squareColor: 8-bit color used by the square. | |
| * | |
| * @note If the creature cannot be found in the map, the function logs a debug trace | |
| * and returns without doing anything. | |
| * | |
| * Behavior rules for clientVersion >= 1076: | |
| * - If @c squareType == 0 OR @c squareColor == 0: | |
| * Removes any square (static and timed). | |
| * - If @c squareType == 2: | |
| * Shows a permanent/static square using @c squareColor. | |
| * - Otherwise: | |
| * Adds a timed square using @c squareColor. | |
| * | |
| * For clientVersion < 1076, the single @c markType is mapped internally to the | |
| * corresponding behavior (removal or timed square) to produce the same visual | |
| * effect as newer protocol versions. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/protocolgame.h`:
- Around line 331-349: Update the docblock for the message handler that reads
parameter msg to document the legacy (<1076) payload: explicitly state that for
clients with protocol version <1076 the implementation consumes a single uint8
markType byte (named markType) instead of the v1076+ uint8 squareType + uint8
squareColor pair, and describe that markType is interpreted by the same rules
(markType==0 removes squares; other values are handled as legacy marks) so
readers can understand both formats; reference the existing parameter names msg,
squareType, squareColor and the legacy markType and note the protocol version
threshold (<1076).
| * @param msg Input message containing: | ||
| * - uint32 creatureId: Target creature identifier. | ||
| * - uint8 squareType: Square behavior type: | ||
| * - 0 = SQUARE_REMOVE: remove any square (static and timed) | ||
| * - 1 = SQUARE_FLASH: temporary (timed/flashing) | ||
| * - 2 = SQUARE_STAY : permanent (static) | ||
| * - uint8 squareColor: 8-bit color used by the square. | ||
| * | ||
| * @note If the creature cannot be found in the map, the function logs a debug trace | ||
| * and returns without doing anything. | ||
| * | ||
| * Behavior rules: | ||
| * - If @c squareType == 0 OR @c squareColor == 0: | ||
| * Removes any square (static and timed). | ||
| * - If @c squareType == 2: | ||
| * Shows a permanent/static square using @c squareColor. | ||
| * - Otherwise: | ||
| * Adds a timed square using @c squareColor. | ||
| */ |
There was a problem hiding this comment.
Document the legacy (<1076) payload to avoid protocol confusion.
The doc block only describes the v1076+ squareType/squareColor format, but the implementation also consumes a legacy single markType byte for older clients. Adding that note keeps the header aligned with behavior.
📝 Suggested doc update
* `@param` msg Input message containing:
* - uint32 creatureId: Target creature identifier.
- * - uint8 squareType: Square behavior type:
- * - 0 = SQUARE_REMOVE: remove any square (static and timed)
- * - 1 = SQUARE_FLASH: temporary (timed/flashing)
- * - 2 = SQUARE_STAY : permanent (static)
- * - uint8 squareColor: 8-bit color used by the square.
+ * - For clientVersion < 1076:
+ * - uint8 markType: legacy timed square color.
+ * - For clientVersion >= 1076:
+ * - uint8 squareType: Square behavior type:
+ * - 0 = SQUARE_REMOVE: remove any square (static and timed)
+ * - 1 = SQUARE_FLASH: temporary (timed/flashing)
+ * - 2 = SQUARE_STAY : permanent (static)
+ * - uint8 squareColor: 8-bit color used by the square.🤖 Prompt for AI Agents
In `@src/client/protocolgame.h` around lines 331 - 349, Update the docblock for
the message handler that reads parameter msg to document the legacy (<1076)
payload: explicitly state that for clients with protocol version <1076 the
implementation consumes a single uint8 markType byte (named markType) instead of
the v1076+ uint8 squareType + uint8 squareColor pair, and describe that markType
is interpreted by the same rules (markType==0 removes squares; other values are
handled as legacy marks) so readers can understand both formats; reference the
existing parameter names msg, squareType, squareColor and the legacy markType
and note the protocol version threshold (<1076).
|
This PR is stale because it has been open 45 days with no activity. |
This pull request updates the handling of creature marks (squares) received from the server in the
ProtocolGameclass. The changes clarify and extend the protocol for marking creatures, introducing support for different mark types (removal, temporary, permanent) and improving code documentation and logging. The logic now uses explicit square types and colors, and is backward compatible with older client versions.Creature Mark Handling Improvements:
parseCreaturesMarkto support three mark types: remove, temporary (timed/flashing), and permanent (static), using two explicit parameters (squareTypeandsquareColor) for client versions 1076 and above.parseCreaturesMarkdescribing the protocol, parameters, and behavior rules for handling creature marks.Summary by CodeRabbit
Bug Fixes
Refactor