perf(gamelogic): Bound terrain deformation scans by bobtista · Pull Request #3258 · TheSuperHackers/GeneralsGameCode

bobtista · 2026-09-04T22:14:12Z

Fixes Terrain deformation loops scan from row 0 wastefully #3257

createCraterInTerrain and flattenTerrain calculate iMin.y, but their inner loops start at row 0. The shape checks reject rows outside the affected area, so this wastes more work as the Y position increases.

Now starts the loops at iMin.y. Retail compatible builds clamp iMin.y to 0 before the loops to keep the original lower bound and CRC. Without retail compatibility, rows below 0 are processed the same way columns below 0 already are.

Todo:

Testing

coderabbitai · 2026-09-23T19:09:19Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: 19fc1b9f-d3e0-475b-934a-21773d2a60c7

📥 Commits

Reviewing files that changed from the base of the PR and between 0995c38 and 0f18622.

📒 Files selected for processing (1)

Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp

💤 Files with no reviewable changes (1)

Core/GameEngine/Source/GameLogic/Map/TerrainLogic.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.

Walkthrough

Five loops in flattenTerrain and createCraterInTerrain now start at max(0, iMin.y). Their upper bounds and terrain calculations are unchanged.

Changes

Terrain deformation loop bounds

Layer / File(s)	Summary
Bounded terrain loops `Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp`	The box, sphere/cylinder, and crater loops start at `max(0, iMin.y)` instead of zero. Their upper bounds and terrain calculations are unchanged.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor · Severity of issue fixed: Low

Suggested reviewers: caball009

Merge Risk: ⚪ Minimal · up to 0f186

The reduced terrain scans preserve the inspected deformation and map-edge behavior. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR satisfies issue `#3257`. `createCraterInTerrain` and all four `flattenTerrain` loops now start at `std::max(0, iMin.y)`. The upper bounds and terrain calculations remain unchanged. This removes s…
Out of Scope Changes check	✅ Passed	The changes remain within issue `#3257`. The include cleanup supports the implementation and no unrelated source, API, configuration, or behavior changes are identified.
Title check	✅ Passed	The title clearly and concisely describes the main change: reducing unnecessary terrain deformation scans.
Description check	✅ Passed	The description explains the affected functions, the performance issue, the compatibility behavior, and the testing status. It is directly related to the changeset.

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: f09a5a66-dc2a-43e9-bde7-23ea259246d2

📥 Commits

Reviewing files that changed from the base of the PR and between 56733ad and 0995c38.

📒 Files selected for processing (1)

Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

greptile-apps · 2026-09-26T17:14:34Z

Confidence Score: 4/5

[Medium risk] Bounds terrain deformation scan loops to valid map range.

The PR should not merge until the non-retail lower-edge deformation behavior is addressed.

Findings

Negative rows enter terrain scans ▶

Summary

The PR starts terrain-flattening and crater scans at their calculated minimum row, avoiding scans from row zero for footprints farther up the map. It adds a retail-compatibility-only clamp at the lower edge.

Non-retail builds no longer retain the previous lower-edge bound.

_{Reviews (2) · Last reviewed commit: "refactor(gamelogic): Clamp deformation r..."}

xezon · 2026-09-27T14:45:53Z

 			Int numSamples = 0;
 			for (i=iMin.x; i<=iMax.x; i++) {
-				for (j=0; j<=iMax.y; j++) {
+				for (j=std::max(0, iMin.y); j<=iMax.y; ++j) {


Are the clamps for zero really needed?

Yes I think so. The original loops started at row 0, but iMin.y can be negative when the shape crosses y=0. In a VC6 retail-compatible build, I tested a Sneak Attack tunnel near that edge; removing the clamp changed the terrain height, the tunnel's z and the game CRC on the next frame. The clamp preserves the original lower bound.

That said, normal player placement appears to prevent it. Scripted or directly spawned objects can still reach it. Seems good to keep it imho

Ok. Can we move the clamping out of the loop and then put it behind RETAIL_COMPATIBLE_CRC?

Moved the clamp before the loops and put it behind RETAIL_COMPATIBLE_CRC.

I moved the clamp, but greptile has a complaint. Are we ok with that for non-retail behavior?

There are 2 possibilities for non-retail:

clamp min and max coords at map bounds

Do not clamp min and max coords at map bounds

Retail only clamps min, but not max.

I suggest test how it looks in game with clamping and not clamping at the map edges.

I tested structures centred on the playable edges. Removing the clamp gets rid of the bottom-edge wall; clamping all bounds also introduces walls at the left and right edges. I suggest keeping the current non-retail behavior.

Bottom-edge comparison below, top to bottom: retail, no clamp, clamp all.

No clamp looks best

…il compatibility only

greptile-apps · 2026-09-29T20:32:36Z

 	for (Int i = iMin.x; i <= iMax.x; i++ )
  {
-		for ( Int j=0; j <= iMax.y; j++ )
+		for ( Int j=iMin.y; j <= iMax.y; ++j )


Negative rows enter terrain scans

If RETAIL_COMPATIBLE_CRC is disabled, a crater or structure whose footprint crosses the lower map edge now starts scanning at a negative row. The same change affects the flattening loops. Those rows were previously skipped; now they can change the height samples used for flattening and, on maps with a height-map border, allow terrain writes into border cells.

bobtista self-assigned this Sep 4, 2026

bobtista added Minor Severity: Minor < Major < Critical < Blocker Performance Is a performance concern labels Sep 4, 2026

bobtista force-pushed the bobtista/perf/terrain-deformation-row-bounds branch from 3bd2141 to 4d81449 Compare September 14, 2026 21:08

OmarAglan mentioned this pull request Sep 19, 2026

unify(map): Merge and move GameLogic map headers and implementations to Core #3189

Merged

3 tasks done

perf(gamelogic): Bound terrain deformation scans

56733ad

bobtista force-pushed the bobtista/perf/terrain-deformation-row-bounds branch from 4d81449 to 56733ad Compare September 23, 2026 19:09

Caball009 reviewed Sep 24, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp Outdated

refactor(terrain): Use std::max for deformation bounds

0995c38

coderabbitai Bot reviewed Sep 24, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp Outdated

Caball009 reviewed Sep 24, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp Outdated

refactor(gamelogic): Remove the redundant algorithm include

0f18622

bobtista marked this pull request as ready for review September 26, 2026 17:11

greptile-apps Bot reviewed Sep 26, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp Outdated

xezon reviewed Sep 27, 2026

View reviewed changes

refactor(gamelogic): Clamp deformation rows before the loops for reta…

f30f6f7

…il compatibility only

greptile-apps Bot reviewed Sep 29, 2026

View reviewed changes

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Confidence Score: 4/5

Findings

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bobtista commented Sep 4, 2026 •

edited

Loading

coderabbitai Bot commented Sep 23, 2026 •

edited

Loading

greptile-apps Bot commented Sep 26, 2026 •

edited

Loading

bobtista Sep 30, 2026 •

edited

Loading