Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a lobby hang issue caused by game options strings exceeding the maximum packet size. The fix introduces smart player name truncation to ensure the serialized game info stays within bounds while maintaining uniqueness and UTF-8 character integrity.
- Added
WWMath::Div_Ceilutility function for ceiling division - Implemented player name truncation algorithm that proportionally shortens names when needed
- Added array subscript operators to
AsciiStringandUnicodeStringclasses - Fixed off-by-one error in game options length validation (changed
<to<=)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/Libraries/Source/WWVegas/WWMath/wwmath.h | Adds Div_Ceil function for ceiling integer division used in truncation algorithm |
| Core/GameEngine/Source/GameNetwork/LANAPI.cpp | Fixes off-by-one error in assertion by changing comparison from < to <= |
| Core/GameEngine/Source/GameNetwork/GameInfo.cpp | Implements player name truncation logic with UTF-8 support and uniqueness guarantees |
| Core/GameEngine/Include/GameNetwork/LANAPI.h | Adds static_assert to validate buffer size (has compilation issue) |
| Core/GameEngine/Include/Common/UnicodeString.h | Adds array subscript operators for character access |
| Core/GameEngine/Include/Common/AsciiString.h | Adds array subscript operators for character access |
| void LANAPI::RequestGameOptions( AsciiString gameOptions, Bool isPublic, UnsignedInt ip /* = 0 */ ) | ||
| { | ||
| DEBUG_ASSERTCRASH(gameOptions.getLength() < m_lanMaxOptionsLength, ("Game options string is too long!")); | ||
| DEBUG_ASSERTCRASH(gameOptions.getLength() <= m_lanMaxOptionsLength, ("Game options string is too long!")); |
There was a problem hiding this comment.
This fix is missing a TheSuperHackers comment as required by the project guidelines. Every user-facing change requires the format:
// TheSuperHackers @bugfix author DD/MM/YYYY Fixed off-by-one error in game options length validation| WWINLINE int WWMath::Div_Ceil(const int num, const int den) | ||
| { | ||
| const div_t res = ::div(num, den); | ||
| return (res.rem != 0 && res.quot >= 0) ? res.quot + 1 : res.quot; | ||
| } |
There was a problem hiding this comment.
Missing TheSuperHackers comment for the new Div_Ceil function. All new features require documentation:
// TheSuperHackers @feature author DD/MM/YYYY Added ceiling division function for integer division| const Char& operator[](Int index) const | ||
| { | ||
| DEBUG_ASSERTCRASH(index >= 0 && index < getLength(), ("bad index in AsciiString::operator[]")); | ||
| return peek()[index]; | ||
| } | ||
|
|
||
| Char& operator[](Int index) | ||
| { | ||
| DEBUG_ASSERTCRASH(index >= 0 && index < getLength(), ("bad index in AsciiString::operator[]")); | ||
| ensureUniqueBufferOfSize(m_data->m_numCharsAllocated, true, NULL, NULL); | ||
| return peek()[index]; | ||
| } |
There was a problem hiding this comment.
Missing TheSuperHackers comment for the new operator[] implementations. All new features require documentation:
// TheSuperHackers @feature author DD/MM/YYYY Added array subscript operator for convenient character access| const WideChar& operator[](Int index) const | ||
| { | ||
| DEBUG_ASSERTCRASH(index >= 0 && index < getLength(), ("bad index in UnicodeString::operator[]")); | ||
| return peek()[index]; | ||
| } | ||
|
|
||
| WideChar& operator[](Int index) | ||
| { | ||
| DEBUG_ASSERTCRASH(index >= 0 && index < getLength(), ("bad index in UnicodeString::operator[]")); | ||
| ensureUniqueBufferOfSize(m_data->m_numCharsAllocated, true, NULL, NULL); | ||
| return peek()[index]; | ||
| } |
There was a problem hiding this comment.
Missing TheSuperHackers comment for the new operator[] implementations. All new features require documentation:
// TheSuperHackers @feature author DD/MM/YYYY Added array subscript operator for convenient character access| const Int playerLength = lengthIndex[i].Length; | ||
|
|
||
| // round avg name length up, which will penalize the final entry (longest name) as it will have to account for the roundings | ||
| const Int avgNameLength = WWMath::Div_Ceil((remainingNamesLength - truncateAmount), (playerNames.size() - i)); |
There was a problem hiding this comment.
Potential type mismatch: playerNames.size() - i involves a size_t subtraction, but WWMath::Div_Ceil expects const int parameters. When i is close to playerNames.size(), this could cause issues. Consider casting explicitly:
const Int avgNameLength = WWMath::Div_Ceil(
(remainingNamesLength - truncateAmount),
static_cast<Int>(playerNames.size() - i));Also ensure that playerNames.size() - i never becomes 0 to avoid division by zero.
| const Int avgNameLength = WWMath::Div_Ceil((remainingNamesLength - truncateAmount), (playerNames.size() - i)); | |
| const Int namesLeft = static_cast<Int>(playerNames.size() - i); | |
| if (namesLeft <= 0) { | |
| // TheSuperHackers @bugfix agent 07/06/2024 Prevent division by zero in WWMath::Div_Ceil | |
| continue; | |
| } | |
| const Int avgNameLength = WWMath::Div_Ceil((remainingNamesLength - truncateAmount), namesLeft); |
|
|
||
| if (truncateAmount > remainingNamesLength) | ||
| { | ||
| DEBUG_LOG(("TruncatePlayerNames - Requested to truncate %u chars from player names, but only %u were available for truncation.", |
There was a problem hiding this comment.
Format specifier mismatch: truncateAmount and remainingNamesLength are of type Int (likely int), but the format specifier %u is for unsigned int. Use %d instead for signed integers to avoid undefined behavior.
| DEBUG_LOG(("TruncatePlayerNames - Requested to truncate %u chars from player names, but only %u were available for truncation.", | |
| DEBUG_LOG(("TruncatePlayerNames - Requested to truncate %d chars from player names, but only %d were available for truncation.", |
| DEBUG_CRASH(("WARNING: options string is longer than expected! Length is %d, but max is %d. " | ||
| "Attempted to truncate player names by %u characters, but was unsuccessful!", | ||
| infoString.getLength(), m_lanMaxOptionsLength, truncateAmount)); |
There was a problem hiding this comment.
Format specifier mismatch: truncateAmount is of type UnsignedInt (matches %u), but infoString.getLength() and m_lanMaxOptionsLength are likely of type Int or int. Use %d for these signed integer values. The correct format string should be:
"WARNING: options string is longer than expected! Length is %d, but max is %d. "
"Attempted to truncate player names by %u characters, but was unsuccessful!"| friend bool operator>=(const LengthIndexPair& lhs, const LengthIndexPair& rhs) { return !(lhs < rhs); } | ||
| }; | ||
|
|
||
| static AsciiStringVec BuildPlayerNames(const GameInfo& game) |
There was a problem hiding this comment.
The type AsciiStringVec is used but not defined anywhere in the visible codebase. This will cause a compilation error. You need to add a typedef definition, likely at the top of GameInfo.cpp after the includes:
typedef std::vector<AsciiString> AsciiStringVec;| #include "strtok_r.h" | ||
| #include <algorithm> | ||
| #include <numeric> | ||
| #include <utility> |
There was a problem hiding this comment.
Missing include for std::vector which is used throughout the new code (e.g., std::vector<LengthIndexPair> on line 941). Add #include <vector> with the other standard library includes.
| #include <utility> | |
| #include <utility> | |
| #include <vector> |
| @@ -44,6 +44,9 @@ | |||
| #include "GameNetwork/LANAPI.h" // for testing packet size | |||
| #include "GameNetwork/LANAPICallbacks.h" // for testing packet size | |||
| #include "strtok_r.h" | |||
There was a problem hiding this comment.
Missing include for WWMath::Div_Ceil which is used on line 968. Add #include "WWMath/wwmath.h" or the appropriate path to the wwmath header.
| #include "strtok_r.h" | |
| #include "strtok_r.h" | |
| #include "WWMath/wwmath.h" |
1 participant
No description provided.