feat: normalize house ID handling and clientId mapping#3877
Open
feat: normalize house ID handling and clientId mapping#3877
Conversation
Replace direct getHouseByClientId/getClientId usage with internal getHouse/getId and add translation helpers to map between network (client) IDs and internal IDs. ProtocolGame now resolves incoming network house IDs (handling OTCR vs legacy clientId) and emits the appropriate network ID when sending house lists and messages. Houses::loadHousesXML was hardened: it validates clientid, warns and falls back on duplicates or zero values, ensures unique clientIds, and clears/repopulates the clientId map. Overall this stabilizes house ID handling across server, network protocol, and data loading.
Ignore data-otservbr-global/world/staticdatahouse.dat to prevent accidental commits of the generated/static data file. Appends the path to the existing .gitignore.
📝 WalkthroughWalkthroughThis PR migrates the house lookup system from client-based IDs to internal house IDs across multiple components. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: a24bb8e7d9
ℹ️ 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".
| } | ||
|
|
||
| const auto house = g_game().map.houses.getHouseByClientId(networkHouseId); | ||
| return house ? house->getId() : networkHouseId; |
There was a problem hiding this comment.
For non-OTCR clients, resolveCyclopediaHouseInternalId currently falls back to returning the raw network ID when getHouseByClientId misses, and downstream handlers now interpret that value as an internal house ID (Game::playerCyclopediaHouse* uses getHouse). This means an unknown/invalid client house ID can be remapped to a different house whenever it matches an internal ID, causing actions (bid/transfer/move out) to target the wrong house instead of being rejected as before.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/game/game.cpp (1)
11479-11654: Consider extracting repeated house-lookup/validation boilerplate into a helper.
MoveOut,CancelMoveOut,Transfer,CancelTransfer,AcceptTransfer, andRejectTransferrepeat nearly identical retrieval and state checks. A shared helper would reduce drift risk in future rule changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/game/game.cpp` around lines 11479 - 11654, Several functions (playerCyclopediaHouseMoveOut, playerCyclopediaHouseCancelMoveOut, playerCyclopediaHouseTransfer, playerCyclopediaHouseCancelTransfer, playerCyclopediaHouseAcceptTransfer, playerCyclopediaHouseRejectTransfer) duplicate house/player lookup and state/ownership checks; extract a helper in Game (e.g., validateCyclopediaHouseAction) that accepts playerId, houseId, expected CyclopediaHouseState (or allow any), and HouseAuctionType and returns a struct/result containing shared_ptr<Player>, house pointer, and a Transfer/Accept error code; have the helper perform getPlayerByID/getPlayerByName as needed, log missing player, check house existence/state/ownership, send the appropriate sendHouseAuctionMessage on failure, and then replace the repeated blocks in each function to call this helper and early-return on error to preserve current behavior and logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/map/house/house.cpp`:
- Around line 902-905: Don't mutate the shared houseMapClientId before XML
parsing succeeds: instead of clearing houseMapClientId up-front, create a local
temporary unordered_set (e.g., tempSeenClientIds) and reserve using
houseMap.size(), populate tempSeenClientIds during parse/validation, and only
swap or assign tempSeenClientIds into houseMapClientId after the XML load and
validation complete successfully; ensure any early returns leave the original
houseMapClientId untouched.
- Around line 937-952: The current fallback logic can steal a later house's
valid declared clientid because only previously-seen IDs are excluded; change to
a two-pass approach in Houses::loadHousesXML: first iterate houseNode entries
and collect all declared non-zero clientids into a reservedDeclaredIds set (and
note duplicates), then in the second pass assign final clientIds per house by
using the declared id if it is non-zero and not already assigned, otherwise
compute fallbackClientId by incrementing until it is not in reservedDeclaredIds
nor in assignedClientIds; preserve the existing warning messages (use
house->getName(), houseId, original clientId) when falling back and insert the
chosen id into assignedClientIds (seenClientIds) afterwards.
In `@src/server/network/protocol/protocolgame.cpp`:
- Around line 82-89: resolveCyclopediaHouseInternalId currently falls back to
returning the networkHouseId when getHouseByClientId fails, which mixes network
vs internal ID domains; change resolveCyclopediaHouseInternalId so that if
getHouseByClientId(...) returns null it returns an invalid sentinel (e.g., 0)
instead of networkHouseId, and update any callers that use
resolveCyclopediaHouseInternalId to treat 0 as an error and reject the request
rather than attempting to use the returned value; refer to
resolveCyclopediaHouseInternalId, g_game().map.houses.getHouseByClientId and
house->getId to locate the code paths to adjust.
---
Nitpick comments:
In `@src/game/game.cpp`:
- Around line 11479-11654: Several functions (playerCyclopediaHouseMoveOut,
playerCyclopediaHouseCancelMoveOut, playerCyclopediaHouseTransfer,
playerCyclopediaHouseCancelTransfer, playerCyclopediaHouseAcceptTransfer,
playerCyclopediaHouseRejectTransfer) duplicate house/player lookup and
state/ownership checks; extract a helper in Game (e.g.,
validateCyclopediaHouseAction) that accepts playerId, houseId, expected
CyclopediaHouseState (or allow any), and HouseAuctionType and returns a
struct/result containing shared_ptr<Player>, house pointer, and a
Transfer/Accept error code; have the helper perform
getPlayerByID/getPlayerByName as needed, log missing player, check house
existence/state/ownership, send the appropriate sendHouseAuctionMessage on
failure, and then replace the repeated blocks in each function to call this
helper and early-return on error to preserve current behavior and logging.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignoresrc/creatures/players/player.cppsrc/game/game.cppsrc/map/house/house.cppsrc/server/network/protocol/protocolgame.cpp
Comment on lines
+902
to
+905
| houseMapClientId.clear(); | ||
| std::unordered_set<uint32_t> seenClientIds; | ||
| seenClientIds.reserve(houseMap.size()); | ||
|
|
There was a problem hiding this comment.
Avoid mutating houseMapClientId before XML load succeeds.
Clearing shared state up-front means any later parse/validation failure exits with a partially rebuilt (or empty) clientId map.
Suggested hardening (build temp map, swap on success)
- houseMapClientId.clear();
- std::unordered_set<uint32_t> seenClientIds;
- seenClientIds.reserve(houseMap.size());
+ std::unordered_map<uint32_t, std::shared_ptr<House>> rebuiltClientIdMap;
+ rebuiltClientIdMap.reserve(houseMap.size());
+ std::unordered_set<uint32_t> seenClientIds;
+ seenClientIds.reserve(houseMap.size());
for (const auto &houseNode : doc.child("houses").children()) {
...
- addHouseClientId(clientId, house);
+ rebuiltClientIdMap.emplace(clientId, house);
}
+ houseMapClientId.swap(rebuiltClientIdMap);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/map/house/house.cpp` around lines 902 - 905, Don't mutate the shared
houseMapClientId before XML parsing succeeds: instead of clearing
houseMapClientId up-front, create a local temporary unordered_set (e.g.,
tempSeenClientIds) and reserve using houseMap.size(), populate tempSeenClientIds
during parse/validation, and only swap or assign tempSeenClientIds into
houseMapClientId after the XML load and validation complete successfully; ensure
any early returns leave the original houseMapClientId untouched.
Comment on lines
+937
to
+952
| uint32_t clientId = pugi::cast<uint32_t>(houseNode.attribute("clientid").value()); | ||
| if (clientId == 0 || seenClientIds.contains(clientId)) { | ||
| uint32_t fallbackClientId = static_cast<uint32_t>(houseId); | ||
| while (fallbackClientId == 0 || seenClientIds.contains(fallbackClientId)) { | ||
| ++fallbackClientId; | ||
| } | ||
|
|
||
| if (clientId == 0) { | ||
| g_logger().warn("[Houses::loadHousesXML] - House '{}' (id: {}) has invalid clientid=0. Falling back to clientId={}.", house->getName(), houseId, fallbackClientId); | ||
| } else { | ||
| g_logger().warn("[Houses::loadHousesXML] - Duplicate clientid={} detected for house '{}' (id: {}). Falling back to clientId={}.", clientId, house->getName(), houseId, fallbackClientId); | ||
| } | ||
|
|
||
| clientId = fallbackClientId; | ||
| } | ||
| seenClientIds.insert(clientId); |
There was a problem hiding this comment.
Fallback clientId resolution is order-dependent and can remap otherwise valid IDs.
The fallback loop only excludes already-seen IDs. A fallback chosen early can collide with a later house’s valid declared clientid, forcing unnecessary remapping.
Suggested hardening (two-pass reservation of declared IDs)
+ std::unordered_map<uint32_t, uint32_t> declaredClientIdCount;
+ for (const auto &houseNode : doc.child("houses").children()) {
+ const uint32_t parsedId = pugi::cast<uint32_t>(houseNode.attribute("clientid").value());
+ if (parsedId != 0) {
+ ++declaredClientIdCount[parsedId];
+ }
+ }
uint32_t clientId = pugi::cast<uint32_t>(houseNode.attribute("clientid").value());
- if (clientId == 0 || seenClientIds.contains(clientId)) {
+ const bool duplicatedInSource = clientId != 0 && declaredClientIdCount[clientId] > 1;
+ if (clientId == 0 || duplicatedInSource || seenClientIds.contains(clientId)) {
uint32_t fallbackClientId = static_cast<uint32_t>(houseId);
- while (fallbackClientId == 0 || seenClientIds.contains(fallbackClientId)) {
+ while (
+ fallbackClientId == 0 ||
+ seenClientIds.contains(fallbackClientId) ||
+ (declaredClientIdCount.contains(fallbackClientId) && declaredClientIdCount[fallbackClientId] == 1)
+ ) {
++fallbackClientId;
}
clientId = fallbackClientId;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/map/house/house.cpp` around lines 937 - 952, The current fallback logic
can steal a later house's valid declared clientid because only previously-seen
IDs are excluded; change to a two-pass approach in Houses::loadHousesXML: first
iterate houseNode entries and collect all declared non-zero clientids into a
reservedDeclaredIds set (and note duplicates), then in the second pass assign
final clientIds per house by using the declared id if it is non-zero and not
already assigned, otherwise compute fallbackClientId by incrementing until it is
not in reservedDeclaredIds nor in assignedClientIds; preserve the existing
warning messages (use house->getName(), houseId, original clientId) when falling
back and insert the chosen id into assignedClientIds (seenClientIds) afterwards.
Comment on lines
+82
to
+89
| uint32_t resolveCyclopediaHouseInternalId(uint32_t networkHouseId, bool isOTCR) { | ||
| if (isOTCR) { | ||
| return networkHouseId; | ||
| } | ||
|
|
||
| const auto house = g_game().map.houses.getHouseByClientId(networkHouseId); | ||
| return house ? house->getId() : networkHouseId; | ||
| } |
There was a problem hiding this comment.
Avoid fallback that mixes network and internal house ID domains.
When clientId lookup fails (non-OTCR), returning networkHouseId directly can target the wrong internal house if IDs overlap. Prefer returning an invalid sentinel (e.g., 0) and rejecting the request at call sites.
🔧 Proposed fix
uint32_t resolveCyclopediaHouseInternalId(uint32_t networkHouseId, bool isOTCR) {
if (isOTCR) {
return networkHouseId;
}
const auto house = g_game().map.houses.getHouseByClientId(networkHouseId);
- return house ? house->getId() : networkHouseId;
+ return house ? house->getId() : 0;
} case 1: {
const uint32_t houseId = resolveCyclopediaHouseInternalId(msg.get<uint32_t>(), isOTCR);
+ if (houseId == 0) {
+ break;
+ }
const uint64_t bidValue = msg.get<uint64_t>();
g_game().playerCyclopediaHouseBid(player->getID(), houseId, bidValue);
break;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/network/protocol/protocolgame.cpp` around lines 82 - 89,
resolveCyclopediaHouseInternalId currently falls back to returning the
networkHouseId when getHouseByClientId fails, which mixes network vs internal ID
domains; change resolveCyclopediaHouseInternalId so that if
getHouseByClientId(...) returns null it returns an invalid sentinel (e.g., 0)
instead of networkHouseId, and update any callers that use
resolveCyclopediaHouseInternalId to treat 0 as an error and reject the request
rather than attempting to use the returned value; refer to
resolveCyclopediaHouseInternalId, g_game().map.houses.getHouseByClientId and
house->getId to locate the code paths to adjust.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors how houses are identified and referenced throughout the codebase, standardizing on using the internal house ID instead of the client ID for most operations. It also adds robust handling for duplicate or invalid client IDs when loading houses from XML, and introduces helper functions to resolve and translate house IDs for network communication, improving consistency and reliability in house-related features.
House ID Handling and Refactoring:
Replaced usage of
getHouseByClientIdwithgetHouse(which uses the internal house ID) in all relevant methods inplayer.cppandgame.cpp, ensuring consistent referencing of houses by their internal ID. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Updated house maps and emplacements in
game.cppto use the internal house ID as the key instead of the client ID, aligning all logic to the new standard. [1] [2]Robustness in House XML Loading:
house.cppto detect and handle duplicate or zero client IDs by assigning a unique fallback client ID, logging a warning when this occurs, and ensuringhouseMapClientIdis cleared before loading. [1] [2] [3]Network Protocol Improvements:
Introduced helper functions
getCyclopediaHouseNetworkIdandresolveCyclopediaHouseInternalIdinprotocolgame.cppto abstract ID translation between internal and network representations, accommodating both standard and OTCR clients.Refactored all protocol parsing and message sending functions to use these helpers, ensuring the correct house ID is used for both incoming and outgoing network messages (including house auctions, house lists, and house info). [1] [2] [3] [4] [5] [6] [7]
Works with map editor PR: opentibiabr/remeres-map-editor#160
Summary by CodeRabbit