Repository navigation
Conversation
|
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
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 Review
Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the boundary lookup null-handling fix and related object-editing changes. |
| Description check | |
The description is related to the reported WorldBuilder null-handling and object-editing changes, and it includes validation details. |
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 help to get the list of available commands.
![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.
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.
41e4e15 to
16fe4d2
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| m_blendedTiles[m_blendTileNdxes[ndx]].inverted |= FLIPPED_MASK; | ||
| { | ||
| //Find a new tile so as not to affect other cells using the base one. | ||
| TBlendTileInfo tempBlendTileInfo=m_blendedTiles[m_blendTileNdxes[ndx]]; |
There was a problem hiding this comment.
🟠 High src/WHeightMapEdit.cpp:1192
When findOrCreateBlendTile returns -1 because the blend-tile table is full or the source tile is unavailable, this path overwrites the valid base index in m_blendTileNdxes[ndx] with -1; subsequent blend/render lookups then index out of bounds and can crash WorldBuilder. Check the allocation result before remapping the cell.
🤖 Copy this AI Prompt to have your agent fix this:
In file @Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp around line 1192:
When `findOrCreateBlendTile` returns `-1` because the blend-tile table is full or the source tile is unavailable, this path overwrites the valid base index in `m_blendTileNdxes[ndx]` with `-1`; subsequent blend/render lookups then index out of bounds and can crash WorldBuilder. Check the allocation result before remapping the cell.
There was a problem hiding this comment.
Fixed separately in #3384. Both required blend records are obtained before changing the cell. If allocation fails, the original cell remains unchanged and any newly allocated unused record is reclaimed by restoring the blend-table count. #3384 now contains only that fix on current main; this PR remains focused on the nullable boundary output.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
|
Needs rebase. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| else if (motion == -1) | ||
| { | ||
| // add a boundary | ||
| m_addingNewBorder = true; | ||
|
|
||
| ICoord2D initialBoundary = { 1, 1 }; | ||
| pDoc->addBoundary(&initialBoundary); |
There was a problem hiding this comment.
{1,1} boundary, even if the user releases without dragging. Mouse-up does not register an undo action or discard that boundary, so an accidental miss leaves an unwanted boundary in the map. Make creation undoable or discard an uncompleted boundary.
16fe4d2 to
1bb7eff
Compare
1bb7eff to
bf1b9b6
Compare
Rebased |
xezon
changed the title
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
Is not practical issue because no caller passes null, but ok to fix. Will remove complaints from bots. Are there maybe some more untested nulls in this file or related files that want to be tested? I am asking because it is better to bunch 20 together than create 20 commits for 20 null tests.
xezon
added
Minor
@xezon i did expanded audit across both WorldBuilder versions and the shared heightmap code
did a test and both reproduce the extracted production functions for gen and zh there is also two defensive candidates
i can expand this pr if you want or make a follow up pr group these findings |
|
You can append here and then we take a look at them. |
done |
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
Change title and description need update.
3c6a603 to
467e43c
Compare
OmarAglan
changed the title
| m_objectToModify->setThingTemplate(m_oldThing); | ||
| } | ||
| if (m_newName != m_oldName) { | ||
| if (m_newName != m_oldName || m_newThing != m_oldThing) { |
There was a problem hiding this comment.
You wrote:
"UndoMove()" still needs it, as the restoring of the old template can (tested) overwrite the saved object name even when "m_newName == m_oldName"
It is confusing. I think what makes this complicated is there are 2 sources for a name. Can we maybe simplify this logic so that there is only ever one source of a name?
There was a problem hiding this comment.
We could make the names stored in MoveInfo the only source. SetThingTemplate already keeps m_newName in sync and the constructor captures m_oldName, so DoMove and UndoMove can call setName unconditionally after the template call. Claude came up with this:
if (m_newThing != m_oldThing) {
m_objectToModify->setThingTemplate(m_newThing);
}
m_objectToModify->setName(m_newName);
There was a problem hiding this comment.
i applied @bobtista suggestion, so right now the names stored in MoveInfo are now the only source for the object name. DoMove and UndoMove change the template only if it differs, then always call setName with m_newName / m_oldName. The || m_newThing != m_oldThing condition is gone.
MoveInfo::SetThingTemplate still copies the new template name into m_newName, so a replace picks up the new name and undo restores the original one, including for objects that had no template
MapObject::setThingTemplate still writes the name as a side effect
the other callers already call setName themselves, so that could be removed, but it is shared Core code, so I left it out of this PR. I can do it as a follow up if we want that
… names MoveInfo::DoMove and MoveInfo::UndoMove now always apply the name stored in MoveInfo after the optional template change, instead of deciding with a compound condition whether the name written by MapObject::setThingTemplate needs to be overwritten.
72126d7 to
8c7652b
Compare
3 participants
Follow-up to #3368.
Fixes null handling and object editing paths in Generals and Zero Hour WorldBuilder:
findBoundaryNear()to receive a null output handle when no boundary matches.removeFirstObject()on an empty list.The Generals and Zero Hour implementations remain aligned.
Validation:
RETAIL_COMPATIBLE_DATAenabled and disabled.Code changes and local regression fixtures were generated with Codex.