feat: Terrain conforming particles - #3245
Conversation
stephanmeesters
added
Enhancement
b0d2d9b to
4b2af0a
Compare
9491602 to
11b2e6b
Compare
11b2e6b to
60d17b0
Compare
PR Summary by QodoAdd terrain-conforming particle rendering
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
|
| Filename | Overview |
|---|---|
| Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp | Implements terrain mesh generation and rendering, but unchecked lookup indexing can corrupt memory and unchecked sizes can generate invalid UVs. |
| Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DTerrainParticle.h | Defines the new renderer and fixed-capacity mesh storage whose lookup capacity is not tied to terrain-bound area. |
| Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DParticleSys.cpp | Integrates conforming particles into existing batching while preserving texture, shader, and alignment batch boundaries. |
| Core/GameEngine/Include/GameClient/ParticleSys.h | Adds the conforming alignment value, INI name, and alignment accessors consistently. |
| Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp | Adds a bounded terrain-flatness query used to coarsen generated particle meshes. |
| Core/Libraries/Source/WWVegas/WW3D2/statistics.cpp | Adds terrain-particle counters to the existing per-frame statistics lifecycle. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Particle systems] --> B[Compatible particle batch]
B --> C{Alignment}
C -->|Billboard or planar| D[PointGroup renderer]
C -->|Conforming| E[Terrain bounds calculation]
E --> F[Flatness test and recursive subdivision]
F --> G[Vertex lookup and index generation]
G --> H[Dynamic DX8 buffers]
H --> I[Terrain-conforming overlay]
Prompt To Fix All With AI
### Issue 1
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp:203-205
**Lookup Buffer Overflow**
When a conforming particle covers more than 32,768 terrain grid points, `gridLocation` can exceed the fixed 32,768-entry `m_vertexLookup`. Particle size has no upper bound, and the bounds are clipped only to the map and visible area, so a sufficiently large particle can access memory outside the lookup buffer, corrupt memory, or crash the renderer.
### Issue 2
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp:221-222
**Invalid Particle Size Division**
A zero-sized conforming particle can reach these UV calculations because particle parsing and runtime size updates do not require a positive value. If its bounds still form a quad, dividing by `2.0f * particle.size` produces infinite or NaN texture coordinates, resulting in malformed or unpredictable rendering. Non-positive and non-finite sizes should be skipped before generating geometry.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(terrainparticle): Add define guard ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| const Int gridLocation = (y - particle.bounds.lo.y) * particle.bounds.width() + x - particle.bounds.lo.x; | ||
| UnsignedShort& index = m_vertexLookup[gridLocation]; |
There was a problem hiding this comment.
When a conforming particle covers more than 32,768 terrain grid points, gridLocation can exceed the fixed 32,768-entry m_vertexLookup. Particle size has no upper bound, and the bounds are clipped only to the map and visible area, so a sufficiently large particle can access memory outside the lookup buffer, corrupt memory, or crash the renderer.
Knowledge Base Used: Rendering and video devices
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp
Line: 203-205
Comment:
**Lookup Buffer Overflow**
When a conforming particle covers more than 32,768 terrain grid points, `gridLocation` can exceed the fixed 32,768-entry `m_vertexLookup`. Particle size has no upper bound, and the bounds are clipped only to the map and visible area, so a sufficiently large particle can access memory outside the lookup buffer, corrupt memory, or crash the renderer.
**Knowledge Base Used:** [Rendering and video devices](https://app.greptile.com/thesuperhackers/-/custom-context/knowledge-base/thesuperhackers/generalsgamecode/-/docs/rendering-and-video-devices.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| vertex.u1 = 0.5f - localX / (2.0f * particle.size); | ||
| vertex.v1 = 0.5f - localY / (2.0f * particle.size); |
There was a problem hiding this comment.
A zero-sized conforming particle can reach these UV calculations because particle parsing and runtime size updates do not require a positive value. If its bounds still form a quad, dividing by 2.0f * particle.size produces infinite or NaN texture coordinates, resulting in malformed or unpredictable rendering. Non-positive and non-finite sizes should be skipped before generating geometry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp
Line: 221-222
Comment:
**Invalid Particle Size Division**
A zero-sized conforming particle can reach these UV calculations because particle parsing and runtime size updates do not require a positive value. If its bounds still form a quad, dividing by `2.0f * particle.size` produces infinite or NaN texture coordinates, resulting in malformed or unpredictable rendering. Non-positive and non-finite sizes should be skipped before generating geometry.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Code Review by Qodo
1. Field effects vanish near view edges
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| m_terrainInViewBounds.hi.x = REAL_TO_INT_FLOOR((worldBoundingBox.Center.X + worldBoundingBox.Extent.X) / MAP_XY_FACTOR); | ||
| m_terrainInViewBounds.lo.y = REAL_TO_INT_FLOOR((worldBoundingBox.Center.Y - worldBoundingBox.Extent.Y) / MAP_XY_FACTOR); | ||
| m_terrainInViewBounds.hi.y = REAL_TO_INT_FLOOR((worldBoundingBox.Center.Y + worldBoundingBox.Extent.Y) / MAP_XY_FACTOR); |
There was a problem hiding this comment.
1. Field effects vanish near view edges 📎 Requirement gap ≡ Correctness
W3DTerrainParticle::setBoundingBox stores floor(worldMaximum / MAP_XY_FACTOR) as the exclusive IRegion2D::hi endpoint, while mesh generation only addresses vertices through hi - 1. When a conforming radiation or toxin field reaches the positive X or Y edge of the visible box, intersection drops the final terrain row or column and can leave fewer than two vertices to render, including active portions on adjacent high terrain.
Agent Prompt
## Issue description
The terrain-view maximum is converted with `floor` and then used as an exclusive `IRegion2D` endpoint, causing conforming particle meshes to omit terrain cells at the positive visible boundaries.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[277-288]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[369-375]
## Recommended Fix
Convert each world-space maximum to a conservative exclusive terrain-vertex endpoint, following the ceiling-and-increment convention used by `calcTerrainBounds`: use `REAL_TO_INT_CEIL(max / MAP_XY_FACTOR) + 1` for both `hi.x` and `hi.y`. Keep the lower bounds floored, clamp the resulting region to the map's logical bounds, and verify that particles crossing either positive view edge retain all visible terrain quads.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| vertex.u1 = 0.5f - localX / (2.0f * particle.size); | ||
| vertex.v1 = 0.5f - localY / (2.0f * particle.size); |
There was a problem hiding this comment.
2. Terrain particles render twice as large 🐞 Bug ≡ Correctness
W3DTerrainParticle treats Particle::getSize() as a half-width even though PointGroupClass treats the same value as the quad's full width. Every conforming system therefore covers twice the intended width and height, changing the appearance and area of existing particle definitions when they select the new alignment.
Agent Prompt
## Issue description
Terrain-conforming particles interpret the existing particle size as a half-width, making them twice as wide and tall as equivalent point-group particles.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[119-124]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[217-223]
## Recommended Fix
Treat size as the full quad width. Compute the rotated projected radius from `size * 0.5f`, and map texture coordinates so offsets of `-size/2` and `+size/2` correspond to the texture edges.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| DX8Wrapper::Set_DX8_Texture_Stage_State(0, D3DTSS_ADDRESSU, D3DTADDRESS_BORDER); | ||
| DX8Wrapper::Set_DX8_Texture_Stage_State(0, D3DTSS_ADDRESSV, D3DTADDRESS_BORDER); | ||
| DX8Wrapper::Set_DX8_Texture_Stage_State(0, D3DTSS_BORDERCOLOR, 0x00000000); |
There was a problem hiding this comment.
4. Later textures gain transparent edges 🐞 Bug ≡ Correctness
updateSettings() changes the global texture-stage U and V address modes to border sampling with a transparent border, but render() only reapplies the texture's filtering afterward. A regular particle batch or another renderer following a conforming batch can therefore inherit border sampling and display transparent or missing texture regions outside the normalized coordinate range.
Agent Prompt
## Issue description
Terrain-particle rendering installs persistent border-addressing states and fails to restore them, allowing the state to affect later draws.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[101-147]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[329-334]
## Recommended Fix
Save the previous U and V addressing modes and border color before changing them, then restore all three states after the terrain batch finishes. If the wrapper cannot query state, use an established scoped render-state mechanism or explicitly restore the engine's documented defaults.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| vertex.u1 = 0.5f - localX / (2.0f * particle.size); | ||
| vertex.v1 = 0.5f - localY / (2.0f * particle.size); | ||
| m_outcodes[index] = getUVOutcode(vertex.u1, vertex.v1); |
There was a problem hiding this comment.
5. Zero-size particles render unpredictably 🐞 Bug ☼ Reliability
addVertex() divides texture-coordinate offsets by 2.0f * particle.size without rejecting a zero size. Particle definitions and size-rate updates can produce zero without validation, so an off-grid zero-size conforming particle can submit infinite or undefined texture coordinates to the graphics device.
Agent Prompt
## Issue description
The conforming renderer divides by particle size even though the particle system permits that value to be zero.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[112-126]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[217-223]
## Recommended Fix
Reject particles whose size is zero or nonpositive before calculating bounds or texture coordinates. Keep the check local to the conforming renderer so existing point-group behavior remains unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const Int gridLocation = (y - particle.bounds.lo.y) * particle.bounds.width() + x - particle.bounds.lo.x; | ||
| UnsignedShort& index = m_vertexLookup[gridLocation]; |
There was a problem hiding this comment.
3. Large particle fields corrupt memory 🐞 Bug ☼ Reliability
W3DTerrainParticle::addVertex uses a row-major offset within the particle's complete terrain-region bounds to index m_vertexLookup, even though that vector has only MAX_VERTICES entries and batch flushing limits generated vertices rather than covered grid positions. A configuration-driven particle spanning more than 32,768 terrain positions can reach its far-corner offset on a flat region that bypasses recursive subdivision and emits only a four-vertex quad, accessing beyond the allocation before flushing can intervene.
Agent Prompt
## Issue description
The fixed 32,768-entry vertex lookup is indexed by positions within an individual particle's complete terrain region rather than by generated vertex count. A particle region larger than `MAX_VERTICES` can therefore access beyond the lookup allocation even when flat-terrain coarsening emits only one quad, before vertex or index batch flushing can intervene.
## Fix Focus Areas
- Core/GameEngineDevice/Include/W3DDevice/GameClient/W3DTerrainParticle.h[76-81]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[124-137]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[202-224]
## Recommended Fix
Before drawing each particle, safely calculate its terrain-region area and resize the lookup to that area, guarding against integer overflow and excessive allocation, or replace it with a bounds-safe sparse lookup. Alternatively, subdivide oversized regions and give each subdivision an independently bounded lookup; retain the separate vertex/index batch limits and flushing logic.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (m_batchParticleAlignment == ParticleSystemInfo::PARTICLE_ALIGNMENT_CONFORMING) | ||
| { | ||
| m_terrainParticles->setTexture(m_batchTexture.Peek()); | ||
| m_terrainParticles->setShader( shader ); | ||
| m_terrainParticles->setArrays( m_posBuffer, m_RGBABuffer, m_sizeBuffer, m_angleBuffer, pointCount ); | ||
| m_terrainParticles->setBoundingBox( m_batchBoundingBox ); | ||
| m_terrainParticles->render(); |
There was a problem hiding this comment.
6. Rotated particle edges disappear 🐞 Bug ≡ Correctness
The conforming rendering path accepts particles only after the existing visibility prepass expands the visible box by psize on each axis. Its projected radius is instead `size * (abs(cosine) + abs(sine))`, so at 45 degrees geometry can extend about 1.414 times the particle size and is never submitted when its center lies in that omitted band.
Agent Prompt
## Issue description
Conforming particles use a rotated-square terrain footprint, but the shared prepass culls based on an unrotated `psize` extent. Particles whose rotated footprint overlaps visible terrain can be rejected before the terrain renderer can clip and draw them.
## Fix Focus Areas
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DParticleSys.cpp[171-189]
- Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainParticle.cpp[112-125]
## Recommended Fix
For conforming particle systems, expand the X/Y prepass culling radius by the rotated footprint radius calculated from the particle angle, or conservatively use `psize * sqrt(2)`. Leave the existing radius unchanged for other alignment modes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| { | ||
| PARTICLE_ALIGNMENT_BILLBOARD = 0, | ||
| PARTICLE_ALIGNMENT_XYPLANAR, | ||
| PARTICLE_ALIGNMENT_CONFORMING, |
There was a problem hiding this comment.
7. Editing effects removes terrain alignment 🐞 Bug ⚙ Maintainability
DebugWindowDialog::getSwitchFromSystem recognizes only XY-planar alignment and updateSwitchToSystem maps an unchecked alignment control to billboard mode. Once the new conforming enum value is loaded into the Particle Editor, editing any switches-dialog setting writes that unchecked state back and replaces conforming alignment with billboard.
Agent Prompt
## Issue description
The Particle Editor represents alignment as a boolean XY-plane checkbox. It cannot represent the new conforming enum value and writes the unchecked value back as billboard alignment during unrelated switch edits.
## Fix Focus Areas
- Core/GameEngine/Include/GameClient/ParticleSys.h[437-443]
- Core/Tools/ParticleEditor/ParticleEditorDialog.cpp[1085-1114]
- Core/Tools/ParticleEditor/CSwitchesDialog.cpp[74-127]
## Recommended Fix
Make the editor expose all alignment enum values, preferably with a three-value control, and write the selected enum value directly. At minimum, preserve conforming alignment when the legacy XY-plane switch is not changed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Merge by rebase
Summary
Makes it possible to render particles correctly on uneven terrain.
IsGroundAligned = CONFORMING.ENABLE_TERRAIN_CONFORMING_PARTICLES.Method
W3DTerrainParticleis a combination of (an evolved version of)W3DScorchandPointGroupClass.Testing
How-to
Using a test build
See this test build.
Using data patch
Building Patch2 with a modification to particle systems see the data PR.
Using code
Alternatively, you can cherry-pick this commit which force-enables the right particles:
Performance impact
The test build was used to test a moderately heavy scene (3 nuke cannons + 3 nukes shooting at a hill in Alpine Assault) using a Macbook Pro 2013 base spec aka Potato, which showed a 5.5% average FPS loss.
Image gallery
More
Todo