fix(terrain): Cover the visible ground at high camera zoom

[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]

Greptile Summary

The PR dynamically sizes and centers the normal terrain rendering window from the camera’s projected ground footprint, preventing water behind the terrain from becoming visible at high zoom while retaining bounded fallback behavior.

• Projects view-plane corners onto terrain height bounds and derives a yaw-stable square footprint.
• Snaps terrain dimensions to vertex-buffer tile boundaries and caps them at map extents.
• Passes the calculated footprint center through the terrain renderer hierarchy.
• The latest revision keeps CENTER_LIMIT private to HeightMap.cpp while preserving the five-cell sizing margin.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable new defect or outstanding rule violation identified.

The latest change only moves an otherwise repository-unused centering constant into its implementation file and preserves the draw-window margin; the complete terrain-footprint path remains bounded by finite projection checks and map extents.

Important Files Changed

Filename	Overview
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp	Calculates a finite, map-bounded camera footprint and uses it to size and center the terrain window.
Core/GameEngineDevice/Source/W3DDevice/GameClient/HeightMap.cpp	Accepts the calculated draw center, applies it to terrain-origin updates, and keeps the origin-drift constant implementation-local.
Core/GameEngineDevice/Include/W3DDevice/GameClient/BaseHeightMap.h	Extends the terrain centering interface with an optional draw-center argument.
Core/GameEngineDevice/Source/W3DDevice/GameClient/FlatHeightMap.cpp	Propagates the optional draw center through the flat-height-map renderer.
Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DView.h	Updates the terrain sizing helper contract to return the projected draw center.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Camera[Camera view plane] --> Projection[Project corners onto terrain height planes]
    Projection -->|Invalid or horizon crossing| Fallback[Use map-sized fallback]
    Projection --> Footprint[Compute bounded XY footprint]
    Footprint --> Snap[Add centering margin and snap to buffer tiles]
    Snap --> Resize[Set terrain draw size]
    Footprint --> Center[Calculate footprint center]
    Center --> Update[Update terrain draw origin]
    Resize --> Update

Loading

_{Reviews (9): Last reviewed commit: "refactor(terrain): Keep the centering co..." | Re-trigger 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.