Repository navigation
feat(physics): wire Jolt HeightFieldShape into terrain collision (#428) - #468
Conversation
Terrain had no collision — players fell through it, NPCs/vehicles couldn't traverse it, raycasts missed it. Build a static JPH::HeightFieldShape body from the terrain's CPU height field so characters, vehicles, and raycasts interact with the surface. - JoltShapes::CreateTerrainHeightFieldShape maps worldSize/heightScale/entity scale onto Jolt's offset + scale*(x, sample, z) surface; pads odd resolutions up to the block-size multiple; adaptive bit-depth (~2cm). - ShapeType::HeightField + GetShapeType round-trip (was defaulting to Box). - JoltScene static terrain-body lifecycle on the NON_MOVING layer; UserData = entity UUID so queries resolve the terrain entity; torn down on stop/shutdown. - TerrainComponent.m_CollisionEnabled (serialized, default on) round-trips through scene YAML and save-game (format v5 -> v6). - Built GPU-free from CPU heights at OnPhysics3DStart (works headless / on the dedicated server), re-synced on runtime Regenerate. - Functional tests: body rests on flat + procedural terrain; raycast hits the surface and resolves to the terrain entity. Streaming per-tile collision and sculpt dirty-region updates are scoped to a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds static Jolt physics height-field collision bodies for ChangesTerrain Height-Field Collision Integration
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEngine/src/OloEngine/Physics3D/JoltScene.cpp`:
- Around line 331-409: Terrain bodies are not being torn down when a terrain
entity or TerrainComponent is removed during runtime, so the Jolt body and
m_BodyIDToEntity mapping linger until shutdown. Update the terrain
removal/destruction path in JoltScene to call
DestroyTerrainBody(entity.GetUUID()) when a terrain entity is deleted or its
TerrainComponent is detached, and ensure the cleanup stays idempotent by reusing
DestroyTerrainBody and the existing CreateTerrainBody bookkeeping.
In `@OloEngine/src/OloEngine/Physics3D/JoltShapes.cpp`:
- Around line 458-522: CreateTerrainHeightFieldShape currently forwards
unvalidated floating-point inputs into Jolt shape setup; add finite checks
before constructing JPH::HeightFieldShapeSettings. In
JoltShapes::CreateTerrainHeightFieldShape, validate all values in heights as
well as worldSizeX, worldSizeZ, heightScale, and each component of scale with
std::isfinite, and return nullptr with an error log if any NaN/Inf is found.
Make sure the checks happen before computing joltScale,
CalculateBitsPerSampleForError, or settings.Create() so the method fails closed.
In `@OloEngine/src/OloEngine/Scene/Scene.cpp`:
- Around line 2717-2766: The terrain collision setup in CreateTerrainBody leaves
an old static body active on every skip/failure path; make sure any existing
runtime terrain body is destroyed before returning when collision is disabled,
terrain is streaming, CPU heights are unavailable, or
JoltShapes::CreateTerrainHeightFieldShape fails. Update the cleanup in
Scene::CreateTerrainBody so these early exits clear the previous body/token
state consistently, not only the success path that replaces it with a new shape.
- Around line 2848-2856: Mirror terrain body cleanup when terrain is destroyed
or its component is removed. Update DestroyEntity() and the TerrainComponent
removal path to explicitly destroy the raw Jolt static body created by
BuildTerrainCollisionBody and remove its BodyID→entity mapping, since terrain is
not handled by the Rigidbody3DComponent/vehicle/character cleanup. Use the
existing terrain-body bookkeeping in Scene::BuildTerrainCollisionBody and the
entity cleanup flow in Scene::DestroyEntity as the main touchpoints.
In `@OloEngine/tests/Functional/AnimationPhysics/TerrainCollisionTest.cpp`:
- Around line 54-63: The expected procedural height-field params in
MakeProcParams() are missing TerrainComponent-authored inputs that
BuildTerrainCollisionBody copies at runtime. Update MakeProcParams() to populate
the same TerrainGenerator::HeightParams fields used by the runtime path,
including m_HeightShaping and m_ProceduralErosionIterations, so the test builds
the expected field from the same terrain values and stays aligned with
BuildTerrainCollisionBody.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bbffe2b4-9dca-4b07-86f0-f18f974ce734
📒 Files selected for processing (12)
OloEngine/src/OloEngine/Physics3D/JoltScene.cppOloEngine/src/OloEngine/Physics3D/JoltScene.hOloEngine/src/OloEngine/Physics3D/JoltShapes.cppOloEngine/src/OloEngine/Physics3D/JoltShapes.hOloEngine/src/OloEngine/Physics3D/Physics3DTypes.hOloEngine/src/OloEngine/SaveGame/SaveGameComponentSerializer.cppOloEngine/src/OloEngine/SaveGame/SaveGameTypes.hOloEngine/src/OloEngine/Scene/Components.hOloEngine/src/OloEngine/Scene/Scene.cppOloEngine/src/OloEngine/Scene/SceneSerializer.cppOloEngine/tests/CMakeLists.txtOloEngine/tests/Functional/AnimationPhysics/TerrainCollisionTest.cpp
| TerrainGenerator::HeightParams MakeProcParams() | ||
| { | ||
| TerrainGenerator::HeightParams params; | ||
| params.Resolution = kProcResolution; | ||
| params.Seed = kProcSeed; | ||
| params.Octaves = kProcOctaves; | ||
| params.Frequency = kProcFrequency; | ||
| params.Lacunarity = kProcLacunarity; | ||
| params.Persistence = kProcPersistence; | ||
| return params; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Mirror every runtime terrain-generation input in the expected height-field.
BuildTerrainCollisionBody also copies terrain.m_HeightShaping and terrain.m_ProceduralErosionIterations into TerrainGenerator::HeightParams, but MakeProcParams() leaves those implicit. That means the “exact CPU field” claim here stops being true as soon as either default diverges, and this test starts failing for the wrong reason. Build the expected params from the same authored TerrainComponent values the runtime path uses.
Also applies to: 188-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@OloEngine/tests/Functional/AnimationPhysics/TerrainCollisionTest.cpp` around
lines 54 - 63, The expected procedural height-field params in MakeProcParams()
are missing TerrainComponent-authored inputs that BuildTerrainCollisionBody
copies at runtime. Update MakeProcParams() to populate the same
TerrainGenerator::HeightParams fields used by the runtime path, including
m_HeightShaping and m_ProceduralErosionIterations, so the test builds the
expected field from the same terrain values and stays aligned with
BuildTerrainCollisionBody.
…uts (review) Address code-review findings on the #428 terrain heightfield collision: - JoltShapes::CreateTerrainHeightFieldShape fails closed on non-finite inputs (any NaN/Inf height sample, world size, height scale, or entity scale) instead of forwarding them into Jolt's quantization math. - Scene::BuildTerrainCollisionBody destroys any prior terrain body up front, so every skip/failure path (collision disabled, streaming, no CPU heights, shape build failure) leaves no stale body on the old surface — not just the success path that replaced it. - The static terrain body is now torn down when terrain is removed at runtime: Scene::DestroyEntity drops it on entity destruction (m_Registry.destroy doesn't fire OnComponentRemoved), and a new hand-written OnComponentRemoved<Terrain Component> covers component detach. TerrainComponent added to kComponentsCustomOnRemove (OloHeaderTool + ComponentHandlerCoverageTest mirror); the generated no-op list is regenerated. - Test: MakeProcParams mirrors every field BuildTerrainCollisionBody copies (shaping + erosion) so the reconstructed field can't silently drift; new DestroyingTerrainEntityRemovesCollision test pins the destroy-cleanup path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
What & why
Closes the #428 Tier-1 gap: terrain had no collision — players fell through it, NPCs/vehicles couldn't traverse it, and AI/weapon raycasts missed it. This builds a static
JPH::HeightFieldShapebody from the terrain's CPU height field so characters, vehicles, and raycasts interact with the surface, matching the rendered mesh exactly.How
JoltShapes::CreateTerrainHeightFieldShape— maps the terrain'sworldSize/heightScale/ entity scale onto Jolt'soffset + scale·(x, sample, z)surface formula. Jolt's sample layout (index = z·N + x, normalized heights) matchesTerrainData::GetHeightData()directly. Odd resolutions are padded up to the block-size multiple (Jolt requiressampleCount % blockSize == 0); bit-depth is chosen adaptively for ~2 cm precision; min/max is auto-derived.ShapeType::HeightField+ stringifier +GetShapeTyperound-trip fix (it was silently defaulting toBox).JoltScenestatic terrain-body lifecycle (CreateTerrainBody/DestroyTerrainBody/DestroyAllTerrainBodies) on theNON_MOVINGlayer; bodyUserData= entity UUID so raycasts/contacts resolve back to the terrain entity; torn down inShutdown/OnRuntimeStop/OnPhysics3DStop.TerrainComponent.m_CollisionEnabled(serialized, default on) + runtime body token. Round-trips through scene YAML and save-game (formatv5 → v6).OnPhysics3DStart(readsTerrainDataif already built, elseTerrainGenerator::GenerateHeightField— the identical field the render path uploads), so it works headless and on the dedicated server. Re-synced on runtimeRegenerate.Tests (104/104 green, incl. all cross-binding coverage guards)
New
TerrainCollisionTest.cpp(Functional, headless — realScene::OnPhysics3DStart+ Jolt step):Coverage guards (ComponentTuple/Handler/Serializer + SaveGame), save-game round-trips,
SceneSerializer,LuaDrivesTerrainRegeneration, andJoltShapesall still pass.Scoped to a follow-up (filed separately)
Streamed per-tile collision (
TerrainStreamer) and sculpt/erosion dirty-region partial updates (HeightFieldShape::SetHeights).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes