Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughWorldBuilder changes flip-state sizing and blend-tile serialization, adjusts image and blend handling, removes the raw tile data API, and updates boundary editing and lookup. ChangesHeight-Map Data Handling
Boundary Editing and Lookup
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: 🟡 Moderate · up to Terrain blending can leave an invalid index when the blend table fills, risking unsafe memory access during later editing. Guard the failed remap before merging; the API-removal concern is resolved. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The terrain-blending change can store a failed allocation result as a valid blend index, exposing later editor operations to out-of-bounds memory access. The demonstrated scope is local WorldBuilder editing. The shared map loader repairs invalid indices, which limits propagation through saved maps. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b7d8f3e0-b520-425e-9af7-399518604f30
📒 Files selected for processing (13)
Core/Tools/CMakeLists.txtCore/Tools/WorldBuilder/CMakeLists.txtCore/Tools/WorldBuilder/include/BorderTool.hCore/Tools/WorldBuilder/include/WHeightMapEdit.hCore/Tools/WorldBuilder/src/BorderTool.cppCore/Tools/WorldBuilder/src/WHeightMapEdit.cppGenerals/Code/Tools/WorldBuilder/CMakeLists.txtGenerals/Code/Tools/WorldBuilder/include/BorderTool.hGenerals/Code/Tools/WorldBuilder/include/WHeightMapEdit.hGenerals/Code/Tools/WorldBuilder/src/BorderTool.cppGenerals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppGeneralsMD/Code/Tools/WorldBuilder/CMakeLists.txtscripts/cpp/unify_move_files.py
💤 Files with no reviewable changes (3)
- Generals/Code/Tools/WorldBuilder/include/BorderTool.h
- Generals/Code/Tools/WorldBuilder/include/WHeightMapEdit.h
- Generals/Code/Tools/WorldBuilder/src/BorderTool.cpp
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| if (blendTileVersion <= K_BLEND_TILE_VERSION_7) { | ||
| // Version 7 uses the legacy row width, even when the in-memory rows are wider. | ||
| Int byteWidth = (m_width+1)/8; | ||
| for (Int j=0; j<m_height; j++) { | ||
| chunkWriter.writeArrayOfBytes((char*)m_cellCliffState + j*m_flipStateWidth, byteWidth); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'setWidth|setDimensions|m_width[[:space:]]*=|mapWidth|heightMapWidth|resize|newMap|createMap|RETAIL_COMPATIBLE_DATA' Core/Tools/WorldBuilder Core/GameEngineDevice/Source/W3DDevice Generals/Code/Tools/WorldBuilder
rg -n 'width.*(multiple|%|/|64|128)|m_width.*(8|16|32|64|128)' Core/Tools/WorldBuilder Core/GameEngineDevice/Source/W3DDevice/GameClientRepository: TheSuperHackers/GeneralsGameCode
Length of output: 26448
🏁 Script executed:
set -e
printf '%s\n' '--- WHeightMapEdit declarations and construction ---'
sed -n '80,235p' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp
sed -n '110,175p' Core/Tools/WorldBuilder/include/WHeightMapEdit.h
printf '%s\n' '--- resize implementation ---'
sed -n '1760,1910p' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- WorldBuilderDoc map creation/loading entrypoints ---'
sed -n '960,1070p' Core/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
rg -n -C 5 'new WorldHeightMapEdit|WorldHeightMapEdit\(|resize\(|initialHeight|xExtent|yExtent|borderWidth' Core/Tools/WorldBuilder/src Core/Tools/WorldBuilder/include
printf '%s\n' '--- dimension validation and format readers ---'
rg -n -C 6 'xSize|ySize|xExtent|yExtent|width.*8|width.*64|MAP_SIZE|map size|MapSize|newWidth|newHeight' Core/Tools/WorldBuilder/src Core/Tools/WorldBuilder/include Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 11905
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- WorldBuilder document creation/loading and resize callers ---'
rg -n -C 8 'WorldHeightMapEdit|resize\(|xExtent|yExtent|initialHeight|borderWidth|NewHeightMap|load' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp Generals/Code/Tools/WorldBuilder/src Generals/Code/Tools/WorldBuilder/include
printf '%s\n' '--- NewHeightMap declarations and implementation ---'
fd -i 'NewHeightMap' Generals/Code/Tools/WorldBuilder
printf '%s\n' '--- WorldHeightMap loading dimension handling ---'
sed -n '820,985p' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp
sed -n '1025,1090p' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp
printf '%s\n' '--- all dimension-related guards in relevant WorldBuilder files ---'
rg -n -C 4 'BAD_VALUE|<= *0|< *0|% *8|/ *8|multiple|extent|size' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp Generals/Code/Tools/WorldBuilder/src Generals/Code/Tools/WorldBuilder/include Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp Core/Tools/WorldBuilder/include/WHeightMapEdit.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 45686
🏁 Script executed:
set -e
printf '%s\n' '--- NewHeightMap files ---'
fd -i 'NewHeightMap' Generals/Code/Tools/WorldBuilder Core/Tools/WorldBuilder
printf '%s\n' '--- NewHeightMap validation ---'
rg -n -C 10 'OnInitDialog|OnOK|DDX|xExtent|yExtent|borderWidth|forResize|SetLimitText|GetDlgItem|BAD_VALUE|<=|< *0|% *8|/ *8' Generals/Code/Tools/WorldBuilder/src/NewHeightMap.cpp Generals/Code/Tools/WorldBuilder/include/NewHeightMap.h Core/Tools/WorldBuilder/src/NewHeightMap.cpp Core/Tools/WorldBuilder/include/NewHeightMap.h 2>/dev/null || true
printf '%s\n' '--- WorldHeightMap constructors and readers ---'
rg -n -C 10 'WorldHeightMap::WorldHeightMap|readInt\(\)|K_BLEND_TILE_VERSION|numBytesX|byteWidth|m_width|m_height' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp Core/GameEngineDevice/Include/W3DDevice/GameClient/WorldHeightMap.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 41736
🏁 Script executed:
set -e
printf '%s\n' '--- save function and changed hunk ---'
sed -n '560,635p' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp
rg -n -C 10 'saveToFile\(|SaveMap|OnSaveDocument|OnFileSave|write.*height|heightMap.*save' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp Generals/Code/Tools/WorldBuilder/src/SaveMap.cpp Generals/Code/Tools/WorldBuilder/include/SaveMap.h 2>/dev/null || true
printf '%s\n' '--- exact parsing and flag readers ---'
rg -n -C 12 'ParseHeightMapData|ParseSizeOnly|m_dataSize.*m_width|byteWidth|numBytesX' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 41363
🏁 Script executed:
set -e
sed -n '250,320p' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
sed -n '780,855p' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
rg -n -C 6 'throw ERROR|ERROR_CORRUPT_FILE_FORMAT|height map file write failed|saveToFile' Core/Tools/WorldBuilder/src Generals/Code/Tools/WorldBuilder/srcRepository: TheSuperHackers/GeneralsGameCode
Length of output: 12421
🏁 Script executed:
set -e
rg -n -C 12 'setCliffState|m_cellCliffState\[|setCellCliffFlagFromHeights|initCliffFlagsFromHeights' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp Core/GameEngineDevice/Include/W3DDevice/GameClient/WorldHeightMap.h Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp Core/Tools/WorldBuilder/include/WHeightMapEdit.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 28808
Reject version-7 saves when the omitted cliff-state byte is non-zero.
WorldBuilder can resize maps to widths where version 7 writes fewer cliff-state bytes than the in-memory row. Normal cliff recalculation can set bits in that omitted byte. The retail version-7 reader does not read it, so reopening the saved map clears those bits.
Version 8 preserves the bits but violates the retail compatibility contract. Check the omitted byte before writing any chunks and reject the save when it contains data.
Suggested fix
void WorldHeightMapEdit::saveToFile(DataChunkOutput &chunkWriter)
{
+ #if RTS_GENERALS && RETAIL_COMPATIBLE_DATA
+ Int legacyByteWidth = (m_width+1)/8;
+ if (legacyByteWidth < m_flipStateWidth) {
+ for (Int j=0; j<m_height; j++) {
+ if (m_cellCliffState[j*m_flipStateWidth + legacyByteWidth] != 0) {
+ throw(ERROR_CORRUPT_FILE_FORMAT);
+ }
+ }
+ }
+ #endif
+
// This is the chunk writer stuff.
int i;
16019dd to
41e4e15
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: e8f4db5c-e63f-4bae-add6-3392b599ac21
📒 Files selected for processing (4)
Generals/Code/Tools/WorldBuilder/include/WHeightMapEdit.hGenerals/Code/Tools/WorldBuilder/src/BorderTool.cppGenerals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppGeneralsMD/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| Short newNdx = findOrCreateBlendTile(&tempBlendTileInfo); | ||
| m_blendTileNdxes[ndx] = newNdx; //remap this tile to use a new one. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
ast-grep outline Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp \
--match findOrCreateBlendTile --view expanded
ast-grep run --lang cpp \
--pattern 'Int WorldHeightMapEdit::findOrCreateBlendTile(TBlendTileInfo *$INFO) { $$$BODY }' \
Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 306
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- helper ---'
sed -n '1040,1115p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- changed blend block ---'
sed -n '1145,1215p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- consumers ---'
rg -n -C 5 'm_blendTileNdxes|autoBlendOut|optimizeTiles' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- requested diff ---'
git diff --unified=35 e70f26db9f0dec4e23f2443921b5341e04958a5f 41e4e15b934fd0d37a60f3bb82c80a161349e1d0 -- Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 41942
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- autoBlendOut relevant range ---'
sed -n '1210,1360p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- optimizeTiles relevant range ---'
sed -n '1660,1765p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 8401
Handle failure before replacing the primary blend index.
findOrCreateBlendTile can return -1 when the blend table is full. The second allocation stores that value in m_blendTileNdxes[ndx] without a check. optimizeTiles and later blend operations can then use -1 as an array index.
Allocate the flipped description before committing the cell indices. If it fails, preserve the previous cell state.
Suggested fix
if (newNdx >= 0) {
Int ndx = (yIndex*m_width)+xIndex;
- m_tileNdxes[ndx] = curTileNdx;
- if (TheGlobalData->m_use3WayTerrainBlends && m_blendTileNdxes[ndx] != 0)
+ Bool hasBaseBlend = TheGlobalData->m_use3WayTerrainBlends &&
+ m_blendTileNdxes[ndx] != 0;
+ Short primaryNdx = hasBaseBlend ? m_blendTileNdxes[ndx] : newNdx;
+ if (hasBaseBlend)
{
//this tile already has a blend applied to it. So we put the new blend into the
//secondary layer.
- m_extraBlendTileNdxes[ndx]=newNdx;
//force the primary layer to flip if the extra blend layer needs flip.
//we only do this on vertical/horizontal base blends because they work in either flip cases.
if (flipped && !baseIsDiagonal)
{
//Find a new tile so as not to affect other cells using the base one.
TBlendTileInfo tempBlendTileInfo=m_blendedTiles[m_blendTileNdxes[ndx]];
tempBlendTileInfo.inverted |= FLIPPED_MASK;
- Short newNdx = findOrCreateBlendTile(&tempBlendTileInfo);
- m_blendTileNdxes[ndx] = newNdx; //remap this tile to use a new one.
+ Short flippedNdx = findOrCreateBlendTile(&tempBlendTileInfo);
+ if (flippedNdx < 0)
+ return;
+ primaryNdx = flippedNdx;
}
- }
- else
- m_blendTileNdxes[ndx] = newNdx;
+ }
+ m_tileNdxes[ndx] = curTileNdx;
+ if (hasBaseBlend)
+ m_extraBlendTileNdxes[ndx] = newNdx;
+ m_blendTileNdxes[ndx] = primaryNdx;
}
Align WorldHeightMapEdit and BorderTool with Zero Hour. Preserve Generals BlendTileData v7 row packing under RETAIL_COMPATIBLE_DATA, adopt the wider in-memory flag rows, and inherit the shared raw-tile accessor.
41e4e15 to
16fe4d2
Compare
1 participant
Applies the same fix to the Generals and Zero Hour copies of
WHeightMapEdit.cppin their existing directories.findBoundaryNear()permits a nulloutHandle, but its no-match path dereferences it unconditionally. This can crash a lookup when no nearby boundary exists.Guard the final handle assignment, matching the existing checks on successful lookups. The function still sets
outNdxto-1when no boundary matches.Validation:
This branch contains one fix commit above #3368 and should be rebased onto main after that PR merges.
Codex generated the fix and local regression fixtures