[sailro]

Problem

With the default settings (DrawEntireTerrain=No) the terrain is only drawn within a window of tiles around the view center. At high camera zoom this window no longer covers the visible ground, so any map water renders behind it. Many maps that have no real water still keep a map-sized "Default Water" polygon, which then makes the whole map look flooded when the camera is zoomed out.

The legacy workaround was DrawEntireTerrain=Yes, but that always draws the entire map and is very slow even at normal zoom (#2743).

Fix

Size the normal draw window to the actual visible footprint instead of a fixed tile count:

• Project the four view corners onto the ground plane (the same method updateCenter() already uses to position the window).
• Grow the draw size just enough to span them, bounded by the map extent and snapped to whole vertex-buffer tiles, so the terrain only reallocates when a zoom threshold is crossed.
• Use a single square size so the window stays stable as the camera rotates (no reallocation on yaw).
• Near-horizontal / parallel corner rays (which meet the ground far away, behind the camera, or not at all) are clamped to the map extent, so they cannot produce a degenerate or NaN draw size.

At normal zoom the footprint is smaller than the normal window, so this is a no-op and behavior is unchanged; the window only grows as the camera zooms out. It works for all maps.

This change lives in the default DrawEntireTerrain=No path. It does not modify the DrawEntireTerrain path, so it does not by itself resolve #2743 — it removes the need for that slow workaround in the common case.

Before / After

Before — zoomed out on a no-water map, the terrain window ends and the map's default water polygon fills the rest of the view:

After — the draw window covers the visible ground, so the sand renders all the way out (same scene type, patched build):

After on a stock map (Whiteout) — the real lake still renders correctly and the terrain fills the view (the black wedge top-left is genuinely off-map, past the terrain border at extreme zoom):

Testing

• Built Release / Win32 and tested in skirmish (Zero Hour).
• At maximum camera zoom-out: no water bleed-through on no-water maps, and the real water on stock maps still renders correctly.
• Performance stays smooth at all zoom levels across multiple maps (no DrawEntireTerrain=Yes-style degradation).

AI disclosure

Per the contribution guidelines: the code in this PR was written with the help of an LLM (GitHub Copilot CLI) and then reviewed, iterated on, and verified by a human. The approach — projecting the view footprint the same way updateCenter() does, tile snapping, and map-extent / NaN bounding — was chosen deliberately, and the change was validated in-game across several maps and zoom levels. The diff is intentionally small and self-contained: one file, only the draw-size calculation in updateTerrain().

cc @xezon

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; the previous missing-ground issue is fixed.

Summary

The PR grows and centers the terrain window to cover the visible ground at high zoom.

• The terrain window grows to cover the ground visible at high zoom.

_{Reviews (11) · Last reviewed commit: "fix(terrain): Keep horizon fallback cons..." · Reviewed by Greptile}

[Skyaero42]

@Mauller is this something you were addressing in #1711 as well, or is it a separate issue?

[sailro]

Tidied the comments in updateTerrain() to match the style we landed on in #2786:

• Dropped references to other functions (updateCenter()) so the comments can't go stale if that code is renamed or moved.
• Removed the issue id from the comments.
• Condensed the header block from ~13 lines down to 5, keeping just the what/why/how. The finer implementation details (map-extent bounding, NaN clamping, tile snapping, rotation stability) are already documented by the inline comments right next to the relevant lines, so nothing was lost.

No functional change — comments only.

[sailro]

@Mauller is this something you were addressing in #1711 as well, or is it a separate issue?

Hi @Skyaero42, From the #2785 side, I think these are separate, complementary fixes rather than the same one:

• fix(view): Adjust default camera height to compensate for screen aspect ratio #1711 is about camera height vs. aspect ratio (Camera height does not scale properly along with aspect ratio of game resolution #78): it scales m_maxHeightAboveGround/m_minHeightAboveGround by an aspect-ratio factor in setDefaultView() (and re-applies it on resolution change / new game) so the camera isn't too low on wide screens. It doesn't change terrain rendering.
• fix(terrain): Cover the visible ground at high camera zoom #2785 is about terrain coverage: in updateTerrain() it sizes the terrain draw window to the visible footprint so the ground — and not the map's water plane — is drawn when the view is zoomed out. It doesn't change the camera.

Same file, but different functions (setDefaultView/setZoomToDefault vs updateTerrain), so no logical overlap or expected conflict.

They're actually complementary: #1711 raises the effective camera height on wide aspect ratios, which is exactly the zoomed-out condition where an un-sized terrain window stops covering the view and the water bleeds through. #2785 derives the window from the projected view corners, so it adapts automatically to whatever camera height #1711 produces. So #1711 can make the symptom more visible on wide screens, and #2785 fixes the rendering side of it.

@Mauller can confirm the #1711 specifics/intent — I'm only speaking to how #2785 relates.

Thanks!

[sailro]

Hello @xezon @Skyaero42

I'd be happy to discuss or modify anything in this PR that you think is worth changing.

I'm just trying to fix a display issue that I think is crucial given today's high resolutions.

Thanks!

[sailro]

I was able to test the fix during a 4h session (Generals Online fork with this fix backported), on various maps. All good !

[Skyaero42]

There is a major deficit in reviewing capacity. You just gonna have to be patient.

[sailro]

@Skyaero42 @xezon Update now that #2846 has landed.

I rebased this PR on top of it, and the fix now lives inside the new W3DView::getDesiredTerrainDrawSize(): the footprint-based growth is applied to the "desired" draw size there, so it composes with your model instead of fighting it.

Why it's still worth having alongside #2846:

• bugfix(heightmap): Fix full terrain rebuild on every frame when DrawEntireTerrain is enabled #2846 targets the m_drawEntireTerrain / showAllTerrain path. It makes DrawEntireTerrain=Yes cheap by no longer rebuilding the whole terrain every frame.
• This PR targets the default path (DrawEntireTerrain=No). When you zoom far out on a normal map, terrain is only drawn in the fixed NORMAL/LOW_ANGLE window around the view center. Past that window the map's default water plane shows through, so maps with no real water look flooded. The only prior workaround was DrawEntireTerrain=Yes, i.e. exactly the slow path bugfix(heightmap): Fix full terrain rebuild on every frame when DrawEntireTerrain is enabled #2846 had to rescue.

So the two are complementary: #2846 makes the "draw everything" workaround fast; this PR removes the need for that workaround in the common case by sizing the draw window to the actual visible footprint. It's a no-op at normal zoom and grows only as you zoom out, bounded by the map extent and snapped to tile blocks so it doesn't reallocate every frame.

The latest commit also extends the coverage to the scripted camera (for example the main-menu shell map), which can likewise see past the regular window.

Happy to tweak anything.

[Caball009]

I did some quick performance comparisons:

1. Comparing with DrawEntireTerrain=No, there's still a noticeable drop in performance, even at camera heights where all visible terrain is rendered.
2. Comparing with DrawEntireTerrain=Yes, there's a very noticeable improvement in performance.

@sailro Can you verify these findings?

I'd be hesitant to add this code for DrawEntireTerrain=No, but perhaps it's worth having for DrawEntireTerrain=Yes.

[sailro]

@Caball009 Thanks a lot for profiling this - your finding was real and pointed at an actual inefficiency in my code, which I've now fixed (latest commit).

Root cause of the drop you saw with DrawEntireTerrain=No:
The normal draw window is 129 vertices = 128 tiles wide. My snapping was adding a full spare 32-tile block before rounding up, so the window grew to 161 as soon as the visible footprint passed ~95 tiles - roughly a whole block earlier than geometrically necessary. So for footprints of 96-128 tiles, which the normal 129 window still fully covers, I was drawing a 161 window = 25 blocks instead of 16 (+56%). Depending on the viewport aspect ratio, the footprint can already sit in that band at fairly normal camera heights - which is exactly the "drop even at camera heights where all visible terrain is rendered" you observed. Good catch.

The fix:
Compute the required block count with a ceiling division to the smallest size that actually covers the footprint, plus a small centering margin (sized to updateCenter()'s CENTER_LIMIT re-centering tolerance) instead of a whole spare block. Result:

visible footprint	before	after
up to ~124 tiles	161 (25 blocks)	129 (16 blocks) - no-op
~125-128 tiles	193	161
larger	grows	grows one block later

So at any camera height where the visible terrain fits the normal window, this is now a genuine no-op - only the four corner-ray projections run, and setTerrainDrawSize()'s guard early-returns with no rebuild.

On the two cost sources when actually zoomed out:

• (a) unavoidable: once the visible ground genuinely exceeds the normal window, more terrain has to be drawn to cover it - any fix that covers the ground pays this. But it scales with what's on screen, unlike DrawEntireTerrain=Yes which draws the whole map regardless of zoom.
• (b) avoidable: the extra ring of blocks from the early growth. That was the bug; it's gone now.

Comparing against the DrawEntireTerrain=No baseline is only apples-to-apples in the range where 129 still covers everything (now a no-op). Past that, the baseline is necessarily cheaper because it draws less - and shows the water bug this PR fixes.

On your suggestion to apply it for DrawEntireTerrain=Yes instead:
I looked into it and it isn't a clean swap. DrawEntireTerrain=Yes also (1) forces the camera far-clip plane to 100000 in updateCameraClipPlanes() and (2) takes a whole-map early-return in updateCenter() (no streaming). Repurposing the flag to mean "footprint-sized" would change its documented semantics and remove the conservative "just draw the whole map" escape hatch. Footprint sizing for the =Yes path could be a separate follow-up, but I think this PR is best kept on the default path, which is where the flooding bug actually bites players.

Happy to share raw numbers or a capture if useful.

[sailro]

@Caball009 happy to discuss or modify anything in this PR that you think is worth changing. Thanks !

[sailro]

@Skyaero42 thank you for this review.

So I think I addressed both comments in my last fixing commit.

• I replaced the manual bounds logic with clamp(), while preserving an explicit NaN fallback.
• I removed unnecessary ray normalization: indeed line-plane intersection is scale-invariant.
• I reused one intersection scale for both X and Y instead of solving them separately.

This removes four inverse-square-root operations and four divisions per draw-size recalculation. The calculation only runs when the camera transform changes, not while stationary.

[sailro]

@Skyaero42 @Caball009 I've been testing this for weeks with private builds / private online lobbies with friends on Generals Online. Works well from my point of view.

Happy to discuss or modify anything in this PR that you think is worth changing. Thanks !

[Caball009]

Yeah, I still need to do another round of reviewing. I'll try to get to it.

[Caball009]

@sailro Try rotating the map at very high camera heights. It's fine with m_drawEntireTerrain but very laggy with your code. The lagging gets worse as the camera height increases and rotation speed increases. It starts to become noticeable with camera heights >= 850.0 (m_maxCameraHeight).

[xezon]

The original height map update is inefficient, not necessarily caused by this Pull

[OmarAglan]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57b230fec5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[sailro]

Thanks @Caball009, @xezon, @OmarAglan and @Skyaero42. I reproduced the rotation regression and pushed a follow-up commit. My earlier square-window claim was wrong: its world-axis spans still changed with yaw, triggering buffer rebuilds.

Idle-machine comparison: previous PR state vs this fix; Win32 Release, 1280x720 windowed, uncapped, verified camera heights, two 20-second runs per case with continuous rotation.

Height	Average FPS: before / after	p99 frame time: before / after
850	929 / 1062	2.13 / 1.33 ms
1500	351 / 648	44.01 / 2.11 ms

The patched draw size stayed constant; the old revision had 260/460 observed size changes at 850/1500 across 40 seconds each.

Why eight files now: sizing and centering must agree. W3DView.cpp computes both; HeightMap.cpp reuses the center, avoiding the existing expensive scan and low-angle offset. The other six files are signature/forwarding plumbing. The new argument defaults to nullptr, preserving the legacy path for existing callers such as WorldBuilder. This is not a general renderer refactor.

[Caball009]

I don't know how much of a problem this will be realistically, but there's a very noticeable lag that happens when zooming in / out at extremely high camera heights >= 1200. Try with 2500 for instance. You can use numpad 4 & 8 for smoother zooming than using the scroll button.

[sailro]

I don't know how much of a problem this will be realistically, but there's a very noticeable lag that happens when zooming in / out at extremely high camera heights >= 1200. Try with 2500 for instance. You can use numpad 4 & 8 for smoother zooming than using the scroll button.

Yes it is probably quite an edge case at those zoom-levels but the culprit is the full terrain rebuild whenever zoom crosses a 32-tile size threshold.

setTerrainDrawSize() calls initHeightData(), which releases and recreates all terrain buffers, repopulates the mesh, and schedules another full update. Both growing and shrinking the window pay this cost.

The minimal approach would retain the larger terrain window during zooming and add modest growth headroom, rather than shrinking and rebuilding at every threshold. That reduces repeated stalls, but temporarily draws more terrain and won’t eliminate first-time growth stalls.

But the complete fix would reuse existing buffers and update only newly needed regions. That touches more renderer internals and needs broader testing. Perhaps for another follow-up PR ?

I think what we have now is quite nice: solve the issue and is now quite performant thanks to the reviews.

[sailro]

Using this latest version for weeks without any issue.

[Caball009]

It's still on my todo list to look at again. I don't know enough about the code specifically to review it properly, though, but I can test gameplay and performance.

[tintinhamans]

When the top of the view shows sky the corner rays miss the ground, and this falls back to the full map. Cutscenes and low pitch cameras then draw the whole map. Can it use the normal or low angle size instead?

[sailro]

Fixed in 1f6460baf. Horizon-crossing corner rays now use the normal or low-angle minimum draw size and leave centering to the existing calcVis/low-pitch path instead of resizing to the full map.

[sailro]

Follow-up after runtime testing: using the normal/low-angle minimum here is not safe, so I reverted that part in 95f3f4d92. On Whiteout at height 1500 and 10° pitch, the minimum fallback selected 257x257 and left large black terrain gaps with floating objects; the conservative map fallback selected 540x540 and rendered correctly. The separate change to use the projected center only after the window expands remains in place.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 70f4e001-1130-4d0d-b8c4-aab548999cfe

Commits

Reviewing files that changed from the base of the PR and between 1f6460b and 95f3f4d.

Files selected for processing: 1

• Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp

Files with no reviewable changes: 1

• Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.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.

Walkthrough

W3DView now calculates terrain draw dimensions from the projected camera footprint and can pass a derived draw center to height-map update methods. When provided, the center determines the terrain origin.

Changes

Terrain draw sizing and centering

Layer / File(s)	Summary
Camera-footprint sizing Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DView.h, Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp	getDesiredTerrainDrawSize calculates dimensions from projected view bounds, map limits, and pitch-dependent minimums. It supplies a draw center when the dimensions exceed the minimum.
Centered terrain updates Core/GameEngineDevice/Include/W3DDevice/GameClient/*HeightMap.h, Core/GameEngineDevice/Source/W3DDevice/GameClient/*HeightMap.cpp, Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp	Height-map update methods accept and forward an optional draw center. When supplied, HeightMapRenderObjClass uses it to calculate the terrain origin and skips the alternating X-update delay.

Priority: Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant W3DView
  participant HeightMapRenderObjClass
  participant BaseHeightMapRenderObjClass
  W3DView->>HeightMapRenderObjClass: pass optional drawCenter to updateCenter
  HeightMapRenderObjClass->>BaseHeightMapRenderObjClass: forward drawCenter
  HeightMapRenderObjClass->>HeightMapRenderObjClass: compute origin from drawCenter when provided

Loading

Suggested reviewers: xezon

Merge Risk: Minimal · up to 95f3f

No unresolved terrain-coverage issue is established by this review; the change is mergeable after normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: fixing terrain coverage at high camera zoom.
Description check		The description directly explains the terrain coverage problem, the footprint-based sizing fix, scope, behavior, testing, and known limitations.
Linked Issues check		The only directly linked issue is #2743. It is closed and provides historical context only. No active directly linked issue remains, so this pull request has no linked-issue coding requirements.
Out of Scope Changes check		The reported changes stay within terrain draw-window sizing and centering. W3DView computes the visible footprint and passes the selected draw center through the HeightMap update path. Header and impl…

Full details: Out of Scope Changes check

Explanation

The reported changes stay within terrain draw-window sizing and centering. W3DView computes the visible footprint and passes the selected draw center through the HeightMap update path. Header and implementation signature changes support that behavior. No unrelated change is identified.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

• X
• Mastodon
• Reddit
• LinkedIn

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