Skip to content

refactor(baseheightmap): Extract scorch rendering from BaseHeightMap into new W3DScorch - #3017

Open
stephanmeesters wants to merge 1 commit into
TheSuperHackers:mainfrom
stephanmeesters:refactor/scorches
Open

refactor(baseheightmap): Extract scorch rendering from BaseHeightMap into new W3DScorch#3017
stephanmeesters wants to merge 1 commit into
TheSuperHackers:mainfrom
stephanmeesters:refactor/scorches

Conversation

@stephanmeesters

Copy link
Copy Markdown
  • Extracts the scorch rendering code into its own class W3DScorch
  • Usages of the macro DO_SCORCH have mostly been removed
  • Disabling DO_SCORCH before didn't compile but this has been fixed. Scorches are now disabled using W3DScorchDummy when using DO_SCORCH=0
  • W3DScorchDummy could be used in headless mode (not part of this PR)

N.B. some code was in DO_SCORCH that didn't actually belong inside the macro, for example Real m_curImpassableSlope;

@stephanmeesters stephanmeesters added the Refactor Edits the code with insignificant behavior changes, is never user facing label Jul 27, 2026
@greptile-apps

greptile-apps Bot commented Jul 27, 2026

Copy link
Copy Markdown

Greptile Summary

This PR extracts terrain-scorch ownership and rendering from BaseHeightMap into W3DScorch.

  • Adds W3DScorch and its no-op W3DScorchDummy implementation.
  • Delegates scorch allocation, invalidation, storage, and drawing from BaseHeightMap.
  • Updates flat and regular height-map render paths to use the delegated renderer.
  • Registers the new source and header in the GameEngineDevice target.

Confidence Score: 4/5

The PR appears safe to merge, with only the previously reported unreachable disabled-scorch branch remaining.

The rendering refactor preserves the active scorch path, while the no-op implementation remains inaccessible because the feature macro is still hardcoded on.

Files Needing Attention: Core/GameEngineDevice/Include/W3DDevice/GameClient/BaseHeightMap.h; Core/GameEngineDevice/Source/W3DDevice/GameClient/BaseHeightMap.cpp

Important Files Changed

Filename Overview
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScorch.cpp Implements the extracted scorch lifecycle, geometry generation, and draw submission.
Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DScorch.h Defines the real and no-op scorch-rendering implementations.
Core/GameEngineDevice/Source/W3DDevice/GameClient/BaseHeightMap.cpp Replaces embedded scorch state and behavior with delegation to the new renderer.
Core/GameEngineDevice/Include/W3DDevice/GameClient/BaseHeightMap.h Removes embedded scorch data and exposes the delegated draw entry point.
Core/GameEngineDevice/Source/W3DDevice/GameClient/FlatHeightMap.cpp Routes flat-terrain scorch drawing through BaseHeightMap.
Core/GameEngineDevice/Source/W3DDevice/GameClient/HeightMap.cpp Routes regular-terrain scorch drawing through BaseHeightMap.
Core/GameEngineDevice/CMakeLists.txt Adds the extracted scorch implementation to the device-library sources.

Sequence Diagram

sequenceDiagram
    participant Terrain as HeightMap Render
    participant Base as BaseHeightMap
    participant Scorch as W3DScorch
    participant Map as WorldHeightMap
    participant DX8 as DX8Wrapper
    Terrain->>Base: drawScorches()
    Base->>Scorch: drawScorches(map)
    Scorch->>Map: read terrain heights and flip state
    Scorch->>Scorch: rebuild vertex/index buffers when invalidated
    Scorch->>DX8: bind buffers, shader, and texture
    Scorch->>DX8: Draw_Triangles()
Loading

Reviews (2): Last reviewed commit: "refactor(baseheightmap): Extract scorch ..." | Re-trigger Greptile

Comment thread Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DScorch.cpp
Comment on lines +274 to 278
#if DO_SCORCH
m_scorches = NEW W3DScorch;
#else
m_scorches = NEW W3DScorchDummy;
#endif

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Disabled scorch branch stays enabled

When the build configures DO_SCORCH=0, the unconditional #define DO_SCORCH 1 in BaseHeightMap.h replaces that value before this branch is evaluated, so the real renderer is constructed instead of W3DScorchDummy and the advertised disabled-scorch configuration is ineffective.

Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/BaseHeightMap.cpp
Line: 274-278

Comment:
**Disabled scorch branch stays enabled**

When the build configures `DO_SCORCH=0`, the unconditional `#define DO_SCORCH 1` in `BaseHeightMap.h` replaces that value before this branch is evaluated, so the real renderer is constructed instead of `W3DScorchDummy` and the advertised disabled-scorch configuration is ineffective.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine for now, DO_SCORCH is not defined in the cmake

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. That said, #define DO_SCORCH 1 is still unconditionally present in BaseHeightMap.h (line ~72), which means the #else / W3DScorchDummy branch in the constructor is dead code right now — not just unused via CMake, but literally unreachable.

That's not a blocker, but if the intent is to wire up the toggle in the future, the #define DO_SCORCH 1 in the header would need to be removed so the value can come from the build system. You may want to leave a // TODO: comment there or just remove the define now to make the branch live. Either way, happy to drop the original comment if you're treating W3DScorchDummy as forward-looking scaffolding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant