feat(terrainblending): restore enable setting#1755
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a runtime toggle for terrain blending and wires it through shader shared data, shader logic, feature serialization, deferred rendering, and dependent feature bindings so terrain blending work and resources are skipped when disabled. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Settings as TerrainBlending.Settings
participant FeatureBuf as FeatureBuffer
participant Runtime as Renderer/State
participant Shader as Lighting.hlsl
participant Deferred as DeferredPasses
UI->>Settings: Toggle Enabled
Settings->>FeatureBuf: Serialize settings
FeatureBuf->>Runtime: Provide feature buffer (includes Enabled)
Runtime->>Shader: Update SharedData::terrainBlendingSettings
Runtime->>Deferred: Bind SRVs (choose blendedDepth or depthSRV based on Enabled)
Deferred->>Shader: Execute shading path
alt Enabled == true
Shader->>Shader: sample blended depth, compute blendFactor
else Enabled == false
Shader->>Shader: skip terrain blend computations
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Features/TerrainBlending.h`:
- Around line 23-30: The TerrainBlending::Settings struct is 4 bytes in C++ but
HLSL expects a 16-byte layout (uint + padding); update the struct used by
Settings settings to include explicit padding so it becomes 16 bytes (e.g., add
three 4-byte padding members or a uint32_t pad[3]) and apply the same alignment
assertion (static_assert(sizeof(Settings) == 16) and/or alignas(16)) to
guarantee C++/HLSL layout match; update any code touching Settings (e.g.,
DrawSettings) if it reads/writes padding fields.
🧹 Nitpick comments (1)
package/Shaders/Lighting.hlsl (1)
993-1006: Redundantsaturatecall on line 1005.The value
blendFactorTerrainis already saturated on line 1000, and the only other assignment sets it to1(line 1003), which is within the [0,1] range. The finalsaturateon line 1005 has no effect.♻️ Suggested simplification
blendFactorTerrain = saturate((depthSampledLinear - depthPixelLinear) / 10.0); if (input.Position.z == depthSampled) blendFactorTerrain = 1; - - blendFactorTerrain = saturate(blendFactorTerrain); }
There was a problem hiding this comment.
Pull request overview
This PR reintroduces an explicit enable/disable setting for the Terrain Blending feature and threads that state through the C++ feature system and HLSL shaders so that disabling the feature cleanly restores vanilla behavior.
Changes:
- Add a
TerrainBlending::Settingsstruct with an ImGui checkbox, and expose it as part of the shared feature buffer so terrain blending can be toggled at runtime. - Gate all uses of the blended terrain depth buffers (in
State,Deferred,SubsurfaceScattering, and TerrainBlending hooks) on both the feature being loaded andsettings.Enabled. - Extend
SharedData::FeatureDataandLighting.hlslto carryterrainBlendingSettings.Enabledinto shaders and skip terrain blending mask computations and outputs when disabled.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/State.cpp | Uses terrainBlending.settings.Enabled to conditionally apply terrain shader hacks and to choose the appropriate depth SRV for shared data. |
| src/Features/TerrainBlending.h | Introduces the Settings struct and wires it into the TerrainBlending feature, enabling per-feature configuration. |
| src/Features/TerrainBlending.cpp | Defines JSON serialization for Settings, adds the ImGui checkbox UI, and gates depth redirection logic in hooks on settings.Enabled. |
| src/Features/SubsurfaceScattering.cpp | Switches SSS depth input to use blended terrain depth only when Terrain Blending is both loaded and enabled. |
| src/FeatureBuffer.cpp | Appends terrainBlending.settings to the feature buffer to match the new TerrainBlendingSettings entry in the shader constant buffer. |
| src/Deferred.cpp | Uses settings.Enabled to decide when to feed blended depth into deferred passes and when to run the terrain blending passes. |
| package/Shaders/Lighting.hlsl | Adds a runtime enable flag around terrain blending mask computation and writing of the blend factor into the G-buffer. |
| package/Shaders/Common/SharedData.hlsli | Adds TerrainBlendingSettings and extends the FeatureData cbuffer to carry the terrain blending enable flag into shaders. |
- Add 16-byte alignment and padding to Settings struct to match HLSL layout - Add STATIC_ASSERT_ALIGNAS_16 macro to verify struct size - Implement LoadSettings/SaveSettings for JSON persistence - Remove redundant saturate() call in shader Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
504a620 to
2c7f1cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Features/TerrainBlending.cpp`:
- Around line 11-16: In TerrainBlending::DrawSettings the call to
ImGui::Checkbox is passing a uint32_t (settings.Enabled) cast to bool* which
corrupts memory; change this to either use ImGui::CheckboxFlags with a uint32_t
mask referencing settings.Enabled or use a temporary bool (e.g., local bool
enabled = settings.Enabled != 0), pass &enabled to ImGui::Checkbox, then after
the UI call assign settings.Enabled = enabled ? 1u : 0u so the shader-aligned
uint32_t remains intact.
♻️ Duplicate comments (1)
src/Features/TerrainBlending.h (1)
23-27: Zero‑initialize Settings padding to avoid nondeterministic CBuffer bytes.Uninitialized padding can leak stack garbage into the feature buffer and cause noisy updates. Initialize it explicitly.
🛠️ Proposed fix
struct Settings { - uint32_t Enabled = true; - uint32_t pad[3]; + uint32_t Enabled = 1; + uint32_t pad[3] = { 0, 0, 0 }; };
|
✅ A pre-release build is available for this PR: |
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Performance
✏️ Tip: You can customize this high-level summary in your review settings.