Update from task cff1fd13-280a-450c-9ac0-bd11ae94a1ea#23
Open
tronpis wants to merge 11 commits into
Open
Conversation
• Implemented ChunkSystem.cpp with full chunk management including 32x32 tile division, dynamic loading/unloading based on player position, and mesh generation with exposed face detection • Added ChunkManager singleton handling chunk lifecycle, world-to-chunk coordinate conversion, and streaming operations around player location • Integrated LODSystem with configurable distance levels for terrain and vegetation detail reduction based on camera distance • Developed CullingSystem implementing both frustum culling using extracted view-projection matrix planes and distance-based culling for performance optimization • Updated ChunkSystem.h with complete interface definitions for chunk operations, LOD configuration, and culling parameters • Modified .gitignore to standardize compressed file exclusions without editor-specific entries This implementation provides a comprehensive world management system with efficient memory usage through chunked streaming, visual quality scaling via LOD, and performance optimization through culling techniques.
Review Summary by QodoImplement world chunk system with LOD and culling
WalkthroughsDescription• Implemented comprehensive chunk system with 32x32 tile division and dynamic streaming • Added LOD system with configurable distance levels for terrain and vegetation detail reduction • Developed frustum and distance-based culling for performance optimization • Integrated ChunkManager singleton for chunk lifecycle and world-to-chunk coordinate conversion Diagramflowchart LR
ChunkSystem["Chunk System<br/>32x32 tiles"]
ChunkMgr["ChunkManager<br/>Singleton"]
LODSys["LOD System<br/>Distance-based detail"]
CullSys["Culling System<br/>Frustum & Distance"]
Terrain["Terrain Generation<br/>Heightmap & Biomes"]
Renderer["Renderer<br/>Mesh rendering"]
ChunkSystem -->|manages| ChunkMgr
ChunkMgr -->|generates| Terrain
ChunkMgr -->|streams| ChunkSystem
LODSys -->|adjusts detail| ChunkSystem
CullSys -->|filters visible| ChunkMgr
ChunkMgr -->|sends meshes| Renderer
File Changes1. FarmEngine/world/chunks/ChunkSystem.cpp
|
Code Review by Qodo
1.
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR adds chunk system optimization with LOD and culling, but contains critical bugs that block merge:
Critical Issues (Must Fix):
- Logic Errors - Three instances where
chunkCoords.yis incorrectly used instead ofchunkCoords.z, causing chunks to be accessed from wrong coordinates (lines 346, 439, 451) - Mesh Corruption - Vertex stride calculation divides by 3 instead of 6, generating incorrect indices for all face types (lines 189, 204, 219, 234, 249, 264)
- Race Conditions - Missing mutex protection in
getChunk()andrequestChunkGeneration(), creating data races in multi-threaded access (lines 333-342, 349-356) - Naming Inconsistency - Mixed PascalCase/camelCase in
CullingConfigboolean fields breaks conventions (lines 199-201)
Summary:
The coordinate system bugs will cause the entire chunk loading system to malfunction. The mesh generation errors will corrupt all rendered geometry. The threading issues create undefined behavior. All must be fixed before merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Comment on lines
+204
to
+264
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 3) - 4; | ||
| opaqueMesh.indices.insert(opaqueMesh.indices.end(), { | ||
| base, base+1, base+2, | ||
| base, base+2, base+3 | ||
| }); | ||
| } | ||
|
|
||
| // Top face (+Y) | ||
| if (getBlock(x, y+1, z) == 0) { | ||
| opaqueMesh.vertices.insert(opaqueMesh.vertices.end(), { | ||
| fx, fy+1, fz, 0, 1, 0, | ||
| fx, fy+1, fz+1, 0, 1, 0, | ||
| fx+1, fy+1, fz+1, 0, 1, 0, | ||
| fx+1, fy+1, fz, 0, 1, 0 | ||
| }); | ||
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 3) - 4; | ||
| opaqueMesh.indices.insert(opaqueMesh.indices.end(), { | ||
| base, base+1, base+2, | ||
| base, base+2, base+3 | ||
| }); | ||
| } | ||
|
|
||
| // Bottom face (-Y) | ||
| if (getBlock(x, y-1, z) == 0) { | ||
| opaqueMesh.vertices.insert(opaqueMesh.vertices.end(), { | ||
| fx, fy, fz+1, 0, -1, 0, | ||
| fx, fy, fz, 0, -1, 0, | ||
| fx+1, fy, fz, 0, -1, 0, | ||
| fx+1, fy, fz+1, 0, -1, 0 | ||
| }); | ||
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 3) - 4; | ||
| opaqueMesh.indices.insert(opaqueMesh.indices.end(), { | ||
| base, base+1, base+2, | ||
| base, base+2, base+3 | ||
| }); | ||
| } | ||
|
|
||
| // Front face (+Z) | ||
| if (getBlock(x, y, z+1) == 0) { | ||
| opaqueMesh.vertices.insert(opaqueMesh.vertices.end(), { | ||
| fx, fy, fz+1, 0, 0, 1, | ||
| fx, fy+1, fz+1, 0, 0, 1, | ||
| fx+1, fy+1, fz+1, 0, 0, 1, | ||
| fx+1, fy, fz+1, 0, 0, 1 | ||
| }); | ||
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 3) - 4; | ||
| opaqueMesh.indices.insert(opaqueMesh.indices.end(), { | ||
| base, base+1, base+2, | ||
| base, base+2, base+3 | ||
| }); | ||
| } | ||
|
|
||
| // Back face (-Z) | ||
| if (getBlock(x, y, z-1) == 0) { | ||
| opaqueMesh.vertices.insert(opaqueMesh.vertices.end(), { | ||
| fx+1, fy, fz, 0, 0, -1, | ||
| fx+1, fy+1, fz, 0, 0, -1, | ||
| fx, fy+1, fz, 0, 0, -1, | ||
| fx, fy, fz, 0, 0, -1 | ||
| }); | ||
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 3) - 4; |
Contributor
There was a problem hiding this comment.
Same vertex stride error exists for all face generations (Left, Top, Bottom, Front, Back faces). All base index calculations must divide by 6, not 3, to match the vertex format of 6 floats per vertex.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a chunk-based world system featuring generation, management, Level of Detail (LOD), and culling mechanisms. Feedback identifies critical bugs in mesh generation regarding vertex normals and index calculations, as well as a missing resize operation in the vegetation culling logic. Additionally, improvements were suggested to replace hardcoded world height values with configuration settings and to optimize the performance of chunk sorting.
| fx+1, fy+1, fz+1, 0, 1, 0, | ||
| fx+1, fy, fz+1, 0, 1, 0 | ||
| }); | ||
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 3) - 4; |
Contributor
There was a problem hiding this comment.
The calculation of the base index is incorrect. Since each vertex consists of 6 floats (3 for position and 3 for normal), the number of vertices is vertices.size() / 6. Using / 3 will result in incorrect indices, leading to rendering artifacts or potential crashes. This issue is repeated for all faces in this function.
Suggested change
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 3) - 4; | |
| uint32_t base = static_cast<uint32_t>(opaqueMesh.vertices.size() / 6) - 4; |
| , config(config) | ||
| { | ||
| const int32_t size = config.chunkSize; | ||
| const int32_t height = 64; // Default chunk height |
Contributor
There was a problem hiding this comment.
The chunk height is hardcoded to 64 here and in several other places throughout the file (e.g., lines 75, 117, 132, 159, 765, 768). This should be moved to ChunkConfig to improve maintainability and allow for flexible world heights.
Comment on lines
+552
to
+561
| std::sort(visibleChunks.begin(), visibleChunks.end(), | ||
| [cameraPosition](Chunk* a, Chunk* b) { | ||
| glm::vec3 posA = a->getPosition(); | ||
| glm::vec3 posB = b->getPosition(); | ||
|
|
||
| float distA = glm::length(posA - cameraPosition); | ||
| float distB = glm::length(posB - cameraPosition); | ||
|
|
||
| return distA < distB; | ||
| }); |
Contributor
There was a problem hiding this comment.
Sorting the visible chunks every frame is a performance bottleneck. Additionally, using glm::length inside the comparator is inefficient as it involves a square root. Consider using squared distance for sorting, and only re-sorting when the camera moves significantly.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+333
to
+336
| Chunk* ChunkManager::getChunk(int32_t chunkX, int32_t chunkZ) { | ||
| int64_t key = (static_cast<int64_t>(chunkX) << 32) | | ||
| (static_cast<uint32_t>(chunkZ) & 0xFFFFFFFF); | ||
|
|
There was a problem hiding this comment.
3. Chunk key shift is ub 🐞 Bug ≡ Correctness
ChunkManager builds the chunk-map key by left-shifting a signed value derived from chunkX; when chunkX is negative (valid for negative world coords), the shift is undefined behavior in C++. This can produce inconsistent keys and chunk lookup failures depending on compiler/optimization.
Agent Prompt
### Issue description
Left-shifting a negative signed integer is undefined behavior in C++. The current key pack does exactly that when `chunkX < 0`.
### Issue Context
The chunk system supports negative coordinates (via `floor()`), so this is a real runtime condition.
### Fix Focus Areas
- FarmEngine/world/chunks/ChunkSystem.cpp[333-336]
- FarmEngine/world/chunks/ChunkSystem.cpp[496-498]
- FarmEngine/world/chunks/ChunkSystem.h[118-120]
### Expected change
- Pack using unsigned arithmetic, e.g.:
```cpp
uint64_t key = (static_cast<uint64_t>(static_cast<uint32_t>(chunkX)) << 32) |
static_cast<uint32_t>(chunkZ);
```
- Consider changing the map key type to `uint64_t` to match.
- Apply the same packing logic everywhere the key is computed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR was created by qwen-chat coder for task cff1fd13-280a-450c-9ac0-bd11ae94a1ea.