bugfix(heightmap): Fix full terrain rebuild on every frame when DrawEntireTerrain is enabled

[xezon]

Merge with Rebase

• Fixes Massive performance degradation with DrawEntireTerrain=Yes #2743
• Fixes Severe in-game performance degradation in certain mods #2797
• Closes fix(heightmap): Prevent per-frame full terrain rebuild with DrawEntireTerrain #2786
• Follow up for fix(heightmap): Fix up view and heightmap to work better with low camera pitch #2677

This change fixes the terrain update for DrawEntireTerrain.

It has 2 commits:

The first commit removes the legacy and unused code for stretched terrain and half height map.

The second commit fixes the terrain update for DrawEntireTerrain. W3DView tells the HeightMapRenderObjClass which size to use. The terrain oversize logic for the scripted camera was adapted to indirectly account for DrawEntireTerrain as well. All DrawEntireTerrain specific knowledge was removed from height map.

TODO

• Add pull id to commit titles
• Replicate in Generals

[greptile-apps]

Greptile Summary

This PR fixes a regression where DrawEntireTerrain mode caused a full terrain rebuild on every frame. The draw-size responsibility was moved from WorldHeightMap (constructor + createDrawArea) into W3DView::getDesiredTerrainDrawSize, which now passes the correct extents to HeightMapRenderObjClass::setTerrainDrawSize. The PR also removes the legacy stretched/half-height-map terrain LOD modes and their associated dead code.

• Terrain draw-size refactor: W3DView::getDesiredTerrainDrawSize centralises logic that previously existed redundantly across WorldHeightMap's constructor, createDrawArea, and updateTerrain; scripted cameras now always use NORMAL_DRAW_WIDTH/HEIGHT and rely on oversizeTerrain for larger extents.
• DrawEntireTerrain rebuild fix: updateCenter now sets m_updating = true before the early-return path that handles full-map coverage, and a m_needFullUpdate guard ensures the block update fires only when truly needed rather than every frame.
• Legacy LOD removal: TERRAIN_LOD_STRETCH_NO_CLOUDS, TERRAIN_LOD_HALF_CLOUDS, and TERRAIN_LOD_STRETCH_CLOUDS are removed along with their supporting fields and dead commented-out code in W3DTreeBuffer.cpp.

Confidence Score: 5/5

Safe to merge; the changes are well-scoped, the root cause is correctly addressed, and the removed LOD modes were fully unused.

The fix correctly moves terrain draw-size control to W3DView, removes the redundant per-frame overrides in WorldHeightMap, and the m_updating/m_needFullUpdate guard in updateCenter directly resolves the unnecessary rebuild. Dead code and legacy LOD removals are clean with no remaining callers. The only open point (stale m_desiredDrawWidth/Height in reset()) was already flagged in a prior review.

HeightMap.cpp — specifically reset() and oversizeTerrain, where the interaction between m_desiredDrawWidth/Height and m_oversizeDrawWidth/Height under a map reload warrants a follow-up to initialise the desired sizes in reset().

Important Files Changed

Filename	Overview
Core/GameEngineDevice/Source/W3DDevice/GameClient/HeightMap.cpp	Core of the fix: m_updating correctly moved before the full-map early-return, m_needFullUpdate guard added, oversizeTerrain updated to pass (0,0) to preserve desired size, and setTerrainDrawSize refactored with m_desiredDrawWidth/Height. reset() still does not reinitialise m_desiredDrawWidth/Height (noted in previous review).
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp	New getDesiredTerrainDrawSize correctly separates DrawEntireTerrain from pitch-based logic and adds a scripted-camera (!m_isUserControlled) guard; setTerrainDrawSize is now only called when a size is actually determined.
Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp	Correctly removes DrawEntireTerrain and stretchTerrain overrides from the constructor and createDrawArea; createDrawArea now simply clamps to map width/height.
Core/GameEngineDevice/Source/W3DDevice/GameClient/BaseHeightMap.cpp	Replaces hardcoded 7 with TERRAIN_LOD_MAX and removes the stale == 5 null-assignment for the now-deleted TERRAIN_LOD_STRETCH_CLOUDS.
Core/GameEngine/Include/GameClient/TerrainVisual.h	Removes three legacy LOD enum entries (STRETCH_NO_CLOUDS, HALF_CLOUDS, STRETCH_CLOUDS) and their name-table strings; remaining values renumber automatically.
Core/GameEngineDevice/Include/W3DDevice/GameClient/HeightMap.h	Adds m_desiredDrawWidth / m_desiredDrawHeight member declarations alongside the existing oversize fields.
Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DView.h	Declares the new private helper getDesiredTerrainDrawSize returning a bool and populating an ICoord2D.
Core/GameEngineDevice/Include/W3DDevice/GameClient/WorldHeightMap.h	Removes STRETCH_DRAW_WIDTH / STRETCH_DRAW_HEIGHT constants; NORMAL_DRAW_* and LOW_ANGLE_DRAW_* constants unchanged.
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTreeBuffer.cpp	Removes a large block of commented-out panel-tree code that relied on the now-deleted m_useHalfHeightMap / m_stretchTerrain flags.
GeneralsMD/Code/GameEngine/Include/Common/GlobalData.h	Marks m_stretchTerrain and m_useHalfHeightMap as Legacy, unused without removing them, preserving binary layout while communicating intent.
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp	Removes the TERRAIN_LOD_HALF_CLOUDS step in calculateTerrainLOD, skipping directly from NO_WATER to DISABLE.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[W3DView::updateTerrain called each frame] --> B[getDesiredTerrainDrawSize]
    B --> C{DrawEntireTerrain?}
    C -- Yes --> D{heightMap available?}
    D -- Yes --> E[dimensions = map XExtent x YExtent]
    D -- No --> F[return false - setTerrainDrawSize skipped]
    C -- No --> G{scripted camera or high pitch?}
    G -- Yes --> H[dimensions = NORMAL_DRAW_WIDTH x HEIGHT]
    G -- No --> I[dimensions = LOW_ANGLE_DRAW_WIDTH x HEIGHT]
    E --> J[return true]
    H --> J
    I --> J
    J --> K[setTerrainDrawSize width height]
    K --> L{width or height > 0?}
    L -- Yes --> M[update m_desiredDrawWidth/Height]
    L -- No --> N[keep existing m_desiredDrawWidth/Height]
    M --> O[effective = max oversizeW desiredW - clamped to map extent]
    N --> O
    O --> P[updateCenter]
    P --> Q{m_x >= XExtent AND m_y >= YExtent?}
    Q -- Yes - full map covered --> R{m_needFullUpdate?}
    R -- Yes --> S[updateBlock full terrain - m_needFullUpdate = false]
    R -- No --> T[skip rebuild - m_updating = false - return early]
    S --> T
    Q -- No - partial coverage --> U[standard origin tracking and partial block updates]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[W3DView::updateTerrain called each frame] --> B[getDesiredTerrainDrawSize]
    B --> C{DrawEntireTerrain?}
    C -- Yes --> D{heightMap available?}
    D -- Yes --> E[dimensions = map XExtent x YExtent]
    D -- No --> F[return false - setTerrainDrawSize skipped]
    C -- No --> G{scripted camera or high pitch?}
    G -- Yes --> H[dimensions = NORMAL_DRAW_WIDTH x HEIGHT]
    G -- No --> I[dimensions = LOW_ANGLE_DRAW_WIDTH x HEIGHT]
    E --> J[return true]
    H --> J
    I --> J
    J --> K[setTerrainDrawSize width height]
    K --> L{width or height > 0?}
    L -- Yes --> M[update m_desiredDrawWidth/Height]
    L -- No --> N[keep existing m_desiredDrawWidth/Height]
    M --> O[effective = max oversizeW desiredW - clamped to map extent]
    N --> O
    O --> P[updateCenter]
    P --> Q{m_x >= XExtent AND m_y >= YExtent?}
    Q -- Yes - full map covered --> R{m_needFullUpdate?}
    R -- Yes --> S[updateBlock full terrain - m_needFullUpdate = false]
    R -- No --> T[skip rebuild - m_updating = false - return early]
    S --> T
    Q -- No - partial coverage --> U[standard origin tracking and partial block updates]

Loading

_{Reviews (3): Last reviewed commit: "bugfix(heightmap): Prevent full terrain ..." | Re-trigger Greptile}

[xezon]

I am not sure if it should be removed or left. The settings still exist in the game INI files, and this allows to see that it does nothing if someone searched for it.

[xezon]

7 previously referred to TERRAIN_LOD_MAX

[xezon]

5 previously referred to enum value TERRAIN_LOD_STRETCH_CLOUDS, which was removed.

It set newROBJ to null which meant it would have leaked here. Very strange code.

[sailro]

Thanks for taking this on, @xezon - I reviewed the branch locally and I'm happy to see it land in place of #2786. The approach here is cleaner than mine: making W3DView::getDesiredTerrainDrawSize() the single source of truth and stripping all DrawEntireTerrain knowledge out of HeightMap removes the "two sources of truth" that caused the per-frame rebuild in the first place, rather than just reconciling them like I did.

A few things I checked while reviewing:

•  updateCenter rework - the earlier m_updating = true is safe; every early  return  after it resets the flag (or falls through), so no risk of the update flag getting stuck.
• _TerrainLOD  renumbering - no compatibility concerns: the only hardcoded integer comparisons are replaced with TERRAIN_LOD_MAX , LOD is parsed from INI by name via TerrainLODNames, and m_terrainLOD isn't serialized into saves/replays.
• The oversize/desired size math checks out - clamping the final max(oversize, desired) to the map extent is equivalent to the old per-source clamp.

I also confirmed it carries over the updateCenter m_needFullUpdate flush from #2786, which was the second half of that fix (the shell-map / scripted-camera regression), so nothing is lost in the supersede.

---

the branch doesn't compile for the base Generals target (Zero Hour /  GeneralsMD  builds fine).

The  chore  commit ( ffe9ce6 ) removes  TERRAIN_LOD_HALF_CLOUDS  from the shared enum in  Core/GameEngine/Include/GameClient/TerrainVisual.h , and you correctly updated  calculateTerrainLOD()  in GeneralsMD/.../W3DDevice/GameClient/W3DDisplay.cpp  - but the mirrored  Generals/ copy was missed. It still references the now-deleted enumerator:

Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp:1646
    case TERRAIN_LOD_HALF_CLOUDS: curLOD = TERRAIN_LOD_DISABLE; break;
    case TERRAIN_LOD_NO_WATER:    curLOD = TERRAIN_LOD_HALF_CLOUDS; break;

So the base-game compile fails with C2065: 'TERRAIN_LOD_HALF_CLOUDS': undeclared identifier  (×2).

The fix is just to mirror the GeneralsMD change — collapse those two lines to:

    case TERRAIN_LOD_NO_WATER: curLOD = TERRAIN_LOD_DISABLE; break;

That's the only dangling reference I found - grepping the tree, the removed  TERRAIN_LOD_HALF_CLOUDS  /  TERRAIN_LOD_STRETCH_*  /  STRETCH_DRAW_*  symbols are clean everywhere else; this is the one spot where the Generals/GeneralsMD mirror got out of sync. Everything else in the PR looks good.

---

LGTM from my side. Thanks for the cleanup commit too — good to see the dead stretch-terrain / half-height-map paths finally gone !

[Caball009]

There's an issue with building placement that I noticed with this branch & DrawEntireTerrain enabled. The placement is not smooth but severely lagging sometimes, possibly related to the world terrain:

gen_zh_building_placement.mp4

[xezon]

Very odd. Looks to be related to ray cast. Mouse Position says world: none. Mouse cursor apparently does not hit terrain.

[stephanmeesters]

I ran the new chat "5.6-sol" model over this and it had this to say:

[P2] Clamp terrain draw dimensions before the initial buffer build

Location: Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp:529

After parsing, the constructor can leave the initial draw area at 129x129 even when the stored heightmap is smaller.

Before W3DView can correct the dimensions, W3DTerrainVisual::load() passes them to initHeightData(). This immediately calls updateBlock() at Core/GameEngineDevice/Source/ W3DDevice/GameClient/HeightMap.cpp:1338.

For small maps, the initial buffer build requests coordinates outside the heightmap. Because getDisplayHeight() performs an unchecked array access, this can result in out-of-
bounds reads.

The first view update clamps the dimensions and rebuilds the terrain before rendering, so affected maps may load and display without visible problems. No shipped map has a dimension
below 129, but WorldBuilder permits sufficiently small custom maps.

Preserve the unconditional min(drawSize, extent) clamps after parsing while removing only the DrawEntireTerrain override.

and indeed it does appear to read the map height array out of bounds for very small maps after the changes made in this PR, i tested with a 30x30 map, but also maps like "[NoMoney] 1v1 Survival" would be affected, still pretty niche though,

Could be verified using this at WorldHeightMap.h:260

UnsignedByte getDisplayHeight(Int x, Int y)
  {
  	const Int mapX = x + m_drawOriginX;
  	const Int mapY = y + m_drawOriginY;
  	const Int ndx = mapX + m_width * mapY;

  	DEBUG_ASSERTCRASH(
  		//mapX >= 0 && mapX < m_width &&
  		//mapY >= 0 && mapY < m_height &&
  		ndx >= 0 && ndx < m_dataSize,
  		("Height sample (%d, %d) is outside %dx%d heightmap; index %d, size %d",
  			mapX, mapY, m_width, m_height, ndx, m_dataSize));

  	return m_data[ndx];
  }

[xezon]

There's an issue with building placement that I noticed with this branch & DrawEntireTerrain enabled. The placement is not smooth but severely lagging sometimes, possibly related to the world terrain:

The branch was outdated. Is fixed after Rebase.