feat(terrainblending): Add terrain blending strength and depth culling controls#1769
Conversation
📝 WalkthroughWalkthroughThis PR adds configurable terrain blending parameters by replacing hardcoded constants and padding fields in the TerrainBlendingSettings structure with two new user-adjustable float fields: BlendStrength and TerrainDepthCullingDistance. The changes span shader definitions, lighting calculations, and C++ serialization/UI exposure. Changes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Features/TerrainBlending.cpp`:
- Around line 20-33: The JSON-loaded settings must be clamped in LoadSettings():
after deserializing, call the same clamping logic as VR.h's ClampToValidRanges()
to enforce BlendStrength ∈ [0.125f, 1.25f] and TerrainDepthCullingDistance ∈
[256.0f, 8192.0f]; update LoadSettings() to directly clamp
settings.BlendStrength and settings.TerrainDepthCullingDistance to those exact
bounds (no isfinite checks required) so values from JSON cannot bypass the ImGui
slider ranges used in the UI.
🧹 Nitpick comments (4)
package/Shaders/Common/SharedData.hlsli (1)
241-244: Run hlslkit buffer scan after cbuffer layout changeTerrainBlendingSettings layout changed; please run an hlslkit buffer scan and recompile dependent shaders to confirm register packing and catch any conflicts.
As per coding guidelines, ...
package/Shaders/Lighting.hlsl (1)
992-1001: Blend strength parameterization looks good; verify shader compileThe guarded divisor is sensible. Since this shader now consumes the updated cbuffer layout, please run hlslkit compilation and buffer scanning to confirm no register conflicts.
As per coding guidelines, ...
src/Features/TerrainBlending.cpp (2)
7-11: Serialization update OK; PR title/issue reference suggestionIncluding the new fields in the JSON mapping makes the settings persist correctly. Separately, the PR title is a bit long; consider something like feat(terrainblending): add blend strength + depth culling (≤50 chars), and add an Implements #… reference if there’s a tracking issue.
As per coding guidelines, ...
291-292: Guard geometry access and avoid per-pass sqrtPlease confirm
a_pass->geometryis always valid for landscape passes; otherwise add a null guard before dereferencingworldBound. For perf, consider comparing squared distances to avoidGetDistance()’s sqrt in this hot path. If you ever need member-offset access across SE/AE/VR, preferREL::RelocateMember()patterns.As per coding guidelines, ...
| ImGui::SliderFloat("Blend Strength", &settings.BlendStrength, 0.125f, 1.25f, "%.3f"); | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { | ||
| ImGui::Text( | ||
| "Controls degree of blending.\n" | ||
| "Lower values create a tighter transition."); | ||
| } | ||
|
|
||
| ImGui::SeparatorText("Performance Option"); | ||
| ImGui::SliderFloat("Terrain Depth Culling Distance", &settings.TerrainDepthCullingDistance, 256.0f, 8192.0f, "%.0f units"); | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { | ||
| ImGui::Text( | ||
| "Skips terrain depth blending beyond this distance.\n" | ||
| "Decrease to just before blending stops to save performance."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get basic info
find . -name "TerrainBlending.cpp" -o -name "TerrainBlending.h" -o -name "TerrainBlending.hpp" | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 143
🏁 Script executed:
# Also search for any terrain blending related files
fd -e cpp -e h -e hpp | grep -i terrainRepository: doodlum/skyrim-community-shaders
Length of output: 327
🏁 Script executed:
# Check the actual code at lines 20-33 in TerrainBlending.cpp
cat -n src/Features/TerrainBlending.cpp | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 1910
🏁 Script executed:
# Also search for LoadSettings function
rg "LoadSettings" src/Features/TerrainBlending.cpp -A 15 -B 2Repository: doodlum/skyrim-community-shaders
Length of output: 501
🏁 Script executed:
# Check the header file for settings struct
cat -n src/Features/TerrainBlending.hRepository: doodlum/skyrim-community-shaders
Length of output: 4486
🏁 Script executed:
# Search for shader usage of BlendStrength and TerrainDepthCullingDistance
rg "BlendStrength|TerrainDepthCullingDistance" src/Features/ -t cpp -t hpp -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 103
🏁 Script executed:
# Check shader files for how these settings are used (divisor concerns)
rg "BlendStrength|TerrainDepthCullingDistance" src/ -t hlsl -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 104
🏁 Script executed:
# Look for other features that validate JSON settings to see common patterns
rg "LoadSettings" src/Features/ -A 8 | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 5668
🏁 Script executed:
# Search for shader files that might use TerrainBlending settings
find . -name "*.hlsl" -o -name "*.cs" | xargs grep -l "BlendStrength\|TerrainDepthCulling" 2>/dev/nullRepository: doodlum/skyrim-community-shaders
Length of output: 109
🏁 Script executed:
# Look for where settings are passed to shaders
rg "BlendStrength|TerrainDepthCulling" src/Features/ -B 3 -A 3Repository: doodlum/skyrim-community-shaders
Length of output: 2588
🏁 Script executed:
# Check VR.cpp to see the ClampToValidRanges pattern
cat -n src/Features/VR.cpp | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 3244
🏁 Script executed:
# Search for Constant Buffer or cbuffer usage in TerrainBlending
rg "cbuffer\|ConstantBuffer\|settings\." src/Features/TerrainBlending.cpp -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Find VR Settings struct to see ClampToValidRanges
rg "struct Settings" src/Features/VR.h -A 50 | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 2969
🏁 Script executed:
# Search for ClampToValidRanges implementation
rg "ClampToValidRanges" src/Features/ -B 2 -A 15Repository: doodlum/skyrim-community-shaders
Length of output: 1818
🏁 Script executed:
# Check if std::isfinite is used anywhere in codebase
rg "isfinite|std::clamp" src/Features/ -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 7955
🏁 Script executed:
# Look at how settings are used in compute shaders (constant buffer binding)
rg "constant buffer\|cbuffer.*Settings" src/ -i -A 5Repository: doodlum/skyrim-community-shaders
Length of output: 58
Validate and clamp JSON-loaded settings to safe ranges
Settings loaded from JSON bypass the ImGui slider bounds. Invalid or NaN values could cause logic errors in distance comparisons and culling. Add validation in LoadSettings().
Follow the established pattern used in VR.h's ClampToValidRanges(): directly clamp to the same ranges as the UI sliders without explicit isfinite checks.
void TerrainBlending::LoadSettings(json& o_json)
{
settings = o_json;
+ settings.BlendStrength = std::clamp(settings.BlendStrength, 0.125f, 1.25f);
+ settings.TerrainDepthCullingDistance = std::clamp(settings.TerrainDepthCullingDistance, 256.0f, 8192.0f);
}🤖 Prompt for AI Agents
In `@src/Features/TerrainBlending.cpp` around lines 20 - 33, The JSON-loaded
settings must be clamped in LoadSettings(): after deserializing, call the same
clamping logic as VR.h's ClampToValidRanges() to enforce BlendStrength ∈
[0.125f, 1.25f] and TerrainDepthCullingDistance ∈ [256.0f, 8192.0f]; update
LoadSettings() to directly clamp settings.BlendStrength and
settings.TerrainDepthCullingDistance to those exact bounds (no isfinite checks
required) so values from JSON cannot bypass the ImGui slider ranges used in the
UI.
|
Please review and address ai comments. They can be wrong. Also if that's in game units, we have a function that will convert to real-world metrics. Wetness uses it. |
|
Also, no magic numbers. Set up constants. |
|
✅ A pre-release build is available for this PR: |
|
Now obsolete with PR by doodlum: chore(terrain blending): performance tweaks #1771 |
2 participants
Adds Terrain Blending controls for Blend Strength and Terrain Depth Culling Distance, wired through settings → feature buffer → shader constants and used in blend falloff and depth‑pass culling.
Updates Terrain Blending UI with a Performance Option header, Blend Strength slider (0.125–1.25, default 1.0), and Depth Culling Distance slider (256–8192, default 2048) plus updated tooltips.
Note: The chosen defaults recapitulate previous default setting of TB. For VR I recomment Blend Strength = 0.5 and Depth Culling Distance = 1024 - but with the slider users can decide themselves - no need for clutter.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.