bugfix(worldbuilder): Roll back failed blend allocations by OmarAglan · Pull Request #3384 · TheSuperHackers/GeneralsGameCode

OmarAglan · 2026-09-28T16:28:29Z

Follow-up to merged unify(worldbuilder): Merge WHeightMapEdit #3368.

Applies the same fix to the Generals and Zero Hour copies of WHeightMapEdit.cpp in their existing directories.

A three-way terrain blend can require both a secondary blend record and a flipped primary copy. If the secondary consumes the last available slot, the primary allocation fails and the current code stores -1 as the cell’s primary blend index.

Obtain both required records before changing the cell indices or cliff mapping. If the primary allocation fails, restore the previous blend-table count to reclaim any secondary record created by the failed attempt.

Existing records remain reusable when the table is full, and capacity warnings are preserved.

Validation:

Reproduced the original invalid primary index.
Passed 56 regression scenarios for each game copy, covering either allocation failing, full and nearly full tables, record reuse, missing source tiles and subsequent use of reclaimed slots.
Compared successful results against the original production functions.
Built both WorldBuilder targets with VC6 and passed debug/release compilation checks with data compatibility enabled and disabled.

This branch contains one fix commit directly above main and is independent of the boundary-handle fix in #3383.

Codex generated the fix and local regression fixtures.

coderabbitai · 2026-09-28T16:28:57Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 390885c1-c113-4b28-b935-2e2815a7e45b

📥 Commits

Reviewing files that changed from the base of the PR and between 01c7d68 and 8faff9a.

📒 Files selected for processing (1)

Generals/Code/Tools/WorldBuilder/include/WHeightMapEdit.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

Walkthrough

The changes update height-map flag-row sizing, BlendTileData serialization, blend allocation handling, and boundary selection. They also remove getRawTileData, change directory image filename handling, and reset the boundary handle when no boundary is found.

Changes

Height-Map Editing

Layer / File(s)	Summary
Height-map data handling `Generals/Code/Tools/WorldBuilder/include/WHeightMapEdit.h`, `Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp`	Flag-row sizing now uses ceiling division by eight in construction and resize paths. Directory image loading copies each filename into the file buffer without adding the directory path. The `getRawTileData` declaration and implementation are removed. Comments for `selectDuplicates` and `selectSimilar` are corrected.
Blend serialization and allocation `Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp`, `GeneralsMD/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp`	BlendTileData serialization writes version 7 and legacy row widths when both build conditions are defined; otherwise, it writes version 8 and the full in-memory state. Blend allocation and cliff updates now occur within successful allocation paths.
Boundary selection and lookup `Generals/Code/Tools/WorldBuilder/src/BorderTool.cpp`, `Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp`	`BorderTool::mouseDown` handles `motion == -1` as boundary creation and keeps the bottom-left boundary modification check. `findBoundaryNear` sets the output handle to `-1` when no boundary is found.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk

Merge Risk: 🟡 Moderate · up to 8faff

Guard the optional boundary handle before merging: a boundary lookup with a null handle can crash when no boundary matches. The inspected boundary-tool caller is unaffected, and blend persistence matches the reader.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 8faff

The change improves containment of failed terrain edits, and the supported saved-map formats match their reader. No new security boundary violation was established. Concurrent access and interruption recovery remain incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — In the inspected blend-tool path, editor coordinates and texture selection drive mutation of a duplicated working map's blend and cliff tables. The demonstrated scope is terrain state in the current editor document; this path does not show a tenant, service, credential, or privilege transition.

Trust Boundaries and Controls

inferred — For synchronous edits of an unpublished map, delayed cell publication and count restoration prevent failed blend allocation from introducing a dangling primary record reference. Duplicate-map ownership is counterevidence to concurrent observation in the inspected caller; it is not a global thread-confinement guarantee.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: rolling back failed blend allocations.
Description check	✅ Passed	The description explains the blend-allocation failure, the rollback fix, its application to both game copies, and reported validation.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: ec896c24-cf3c-4218-bd92-4499ae1811e7

📥 Commits

Reviewing files that changed from the base of the PR and between e6d72ec and 01c7d68.

📒 Files selected for processing (4)

Generals/Code/Tools/WorldBuilder/include/WHeightMapEdit.h
Generals/Code/Tools/WorldBuilder/src/BorderTool.cpp
Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
GeneralsMD/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; 3 remain after this review.

macroscopeapp · 2026-10-04T08:25:30Z

 	}

 	(*outNdx) = -1;
+	(*outHandle) = -1;


🟠 High src/WHeightMapEdit.cpp:3452

When no boundary is near and outHandle is null, findBoundaryNear dereferences it and crashes instead of returning -1 through outNdx. Guard this cleanup assignment with if (outHandle), as done for successful matches.

🤖 Copy this AI Prompt to have your agent fix this:

In file @Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp around line 3452: When no boundary is near and `outHandle` is null, `findBoundaryNear` dereferences it and crashes instead of returning `-1` through `outNdx`. Guard this cleanup assignment with `if (outHandle)`, as done for successful matches.

Fixed separately in #3383. The no-match assignment now checks outHandle before writing through it, matching the successful lookup paths. The unrelated invalid-input behavior is unchanged: a null pt still resets outNdx and returns. #3384 remains focused on blend allocation.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

greptile-apps · 2026-10-04T08:27:06Z

Confidence Score: 4/5

[Medium impact] Terrain blending logic adds rollback for failed allocations.

The PR is not yet safe to merge because the previously reported boundary-placement defect remains unresolved.

Findings

Click creates misplaced boundary ▶

Summary

The PR updates both WorldBuilder variants to acquire the secondary and flipped primary blend records before changing a cell, and restores the blend-table count if the primary allocation fails.

The only change since the previous review removes a code comment; it introduces no new behavioral issue.

_{Reviews (3) · Last reviewed commit: "bugfix(worldbuilder): Roll back failed b..." · Reviewed by Greptile}

greptile-apps · 2026-10-04T08:27:11Z

+	else if (motion == -1)
+	{
+		// add a boundary
+		m_addingNewBorder = true;
+
+		ICoord2D initialBoundary = { 1, 1 };
+		pDoc->addBoundary(&initialBoundary);


Click creates misplaced boundary

Clicking and releasing on empty space without dragging now adds a boundary at {1,1}, not at the clicked location. Only mouse movement repositions the new boundary; mouse-up leaves it there. An ordinary click can therefore save an unintended boundary in the map.

will push a pr fixing this

xezon · 2026-10-04T08:27:49Z

This needs a rebase.

OmarAglan · 2026-10-09T16:05:53Z

This needs a rebase.

Rebased

OmarAglan force-pushed the bugfix/worldbuilder-blend-allocation branch from e6d72ec to 01c7d68 Compare September 30, 2026 08:25

coderabbitai Bot reviewed Sep 30, 2026

View reviewed changes

Comment thread Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp

OmarAglan force-pushed the bugfix/worldbuilder-blend-allocation branch from 01c7d68 to 8faff9a Compare September 30, 2026 08:36

OmarAglan marked this pull request as ready for review October 4, 2026 08:24

macroscopeapp Bot reviewed Oct 4, 2026

View reviewed changes

greptile-apps Bot reviewed Oct 4, 2026

View reviewed changes

OmarAglan mentioned this pull request Oct 4, 2026

fix(worldbuilder): Handle nulls in boundary lookup and object editing #3383

Open

OmarAglan force-pushed the bugfix/worldbuilder-blend-allocation branch from 8faff9a to 1b87fd5 Compare October 4, 2026 08:37

OmarAglan mentioned this pull request Oct 4, 2026

chore: Add static asserts to verify the size of arrays #3389

Open

2 tasks

bugfix(worldbuilder): Roll back failed blend allocations

62e8815

OmarAglan force-pushed the bugfix/worldbuilder-blend-allocation branch from 1b87fd5 to 62e8815 Compare October 9, 2026 15:47

This was referenced Oct 9, 2026

bugfix(worldbuilder): Reject lossy retail cliff saves #3454

Draft

bugfix(worldbuilder): Create boundaries only when dragging #3455

Draft

Conversation

OmarAglan commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Merge Risk

Security Architecture Review

Security review details

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

macroscopeapp Bot Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

OmarAglan Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

macroscopeapp Bot Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

Uh oh!

greptile-apps Bot Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

OmarAglan Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

xezon commented Oct 4, 2026

Uh oh!

OmarAglan commented Oct 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

OmarAglan commented Sep 28, 2026 •

edited

Loading

coderabbitai Bot commented Sep 28, 2026 •

edited

Loading

greptile-apps Bot commented Oct 4, 2026 •

edited

Loading