[c++] Fix Moghouse entry in WoTG + SoA#9729
Conversation
|
|
||
| auto isMogHouseExit = std::memcmp(&this->RectID, "zmrq", 4) == 0; // zmrq is the universal Mog House exit zoneline | ||
| auto isMogHouseEntrance = std::memcmp(&this->RectID, "zmr", 3) == 0; // zmr* are zone-specific Mog House entry zonelines | ||
| auto isMogHouseEntrance = std::memcmp(&this->RectID, "zmr", 3) == 0 || std::memcmp(&this->RectID, "zms", 3) == 0; // zmr* are zone-specific Mog House entry zonelines, zms* = WoTG [S] city and Adoulin Mog Houses |
There was a problem hiding this comment.
I really don't like the chaining of these two memcpy's, using them with ||, and then possibly short-circuiting. Instead, can you check some condition, and do one memcpy or the other
memcpy docs for reference:
https://en.cppreference.com/w/cpp/string/byte/memcmp
There was a problem hiding this comment.
Throwing this code snippet, my comment, and the definition of RectID into AI, it's given the following suggestions:
Since RectID is a uint32_t, you can replace memcmp with direct integer comparisons. This satisfies the reviewer's request to avoid chained function calls and short-circuiting logic while being more performant.
Recommended Fix: Hexadecimal Literals
Using hex literals is the standard way to handle 4-byte ID checks in systems programming. Note that for little-endian systems (like x86 or modern consoles), the byte order is reversed.
// Byte values: z=0x7A, m=0x6D, r=0x72, q=0x71, s=0x73
// "zmrq" -> 0x71726D7A
// "zmr" -> 0x00726D7A (using a 3-byte mask)
uint32_t val = this->RectID;
uint32_t prefix = val & 0x00FFFFFF; // Mask out the 4th byte
auto isMogHouseExit = (val == 0x71726D7A);
auto isMogHouseEntrance = (prefix == 0x726D7A || prefix == 0x736D7A);Alternative: Using std::string_view
If you prefer to keep the code readable with string literals, use std::bit_cast or a pointer cast to treat the integer as a character array.
#include <string_view>
auto bytes = reinterpret_cast<const char*>(&this->RectID);
std::string_view view(bytes, 4);
auto isMogHouseExit = (view == "zmrq");
// Check the prefix only once
std::string_view prefix = view.substr(0, 3);
auto isMogHouseEntrance = (prefix == "zmr" || prefix == "zms");I'd probably opt for the second one, adding a comment above the reinterpret_cast to say we're going to treat this like a string temporarily, but we're capping it at 4 characters - which is fine because RectID is a uint32 (4 bytes). Then we can do checks and use substrings and everything is a lot more readable, without spooky looking usage of memcpy.
I affirm:
What does this pull request do?
Tried zoning into Moghouses in WoTG and SoA zones and would just get sent back to entry zoneline (all zones enabled on my test server), fixes the issue.
Steps to test these changes
Zone into WoTG and SoA moghouses before and after change.