perf(Terrain Variation): feature rework, miplevel & sample optimisation#1113
perf(Terrain Variation): feature rework, miplevel & sample optimisation#1113davo0411 wants to merge 61 commits into
Conversation
![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
🧹 Nitpick comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
11-27: Document the derivation and rationale for magic number constants.Several constants lack documentation about their derivation:
WORLD_SCALE = 332.54- document how this value was chosen and its relationship to world unitsDISTANCE_LOD_THRESHOLD = 1024.0andDISTANCE_LOD_TRANSITION = 300.0- explain the rationale for these specific distance valuesHASH_MULTIPLIERandHASH_SINE_MULTIPLIERvalues - document why these specific multipliers provide good hash distributionThis will improve maintainability and make it easier to adjust these values for different scenes or performance requirements.
80-100: Optimize LOD weights structure for 2-sample blending.Since
ComputeStochasticOffsetsLODonly uses 2 samples, storing afloat3with the third component always 0.0 is wasteful. Consider using afloat2for LOD weights or create a separate structure for 2-sample offsets.struct StochasticOffsets { float2 offset1; float2 offset2; float2 offset3; float3 weights; }; +// Specialized structure for LOD sampling (2 samples only) +struct StochasticOffsetsLOD +{ + float2 offset1; + float2 offset2; + float2 weights; // Only need 2 weights +}; -inline StochasticOffsets ComputeStochasticOffsetsLOD(float2 landscapeUV) +inline StochasticOffsetsLOD ComputeStochasticOffsetsLOD(float2 landscapeUV) { // ... existing code ... - offsetsLOD.weights = float3(0.65, 0.35, 0.0); + offsetsLOD.weights = float2(0.65, 0.35); }
104-127: Remove commented-out code for cleaner implementation.The
StochasticSample3function contains extensive commented-out distance-based optimization code that should be removed since this function is designed to be simple without distance considerations.inline float4 StochasticSample3(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsets, float2 dx, float2 dy) { - // // Distance-based LOD with smooth transition - // float distanceFactor = saturate((distance - DISTANCE_LOD_THRESHOLD) / DISTANCE_LOD_TRANSITION); - // Take 3 samples always float4 sample1 = tex.SampleLevel(samp, uv + offsets.offset1, mipLevel); float4 sample2 = tex.SampleLevel(samp, uv + offsets.offset2, mipLevel); float4 sample3 = tex.SampleLevel(samp, uv + offsets.offset3, mipLevel); - - // // Early exit for distant terrain - return single sample with cheap offset - // if (distanceFactor >= 0.9) - // { - // return sample1; - // } - - // Blend using the barycentric weights - float4 result = sample1 * offsets.weights.x + - sample2 * offsets.weights.y + - sample3 * offsets.weights.z; - - return result; + // Blend using the barycentric weights + return sample1 * offsets.weights.x + + sample2 * offsets.weights.y + + sample3 * offsets.weights.z; }
129-148: Extract magic numbers as named constants.The
StochasticSampleLODfunction uses magic numbers that should be defined as named constants for better maintainability.+// LOD sampling constants +static const float LOD_OFFSET_SCALE = 0.01; +static const float LOD_SAMPLE_WEIGHT = 0.65; + inline float4 StochasticSampleLOD(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsetsLOD, float2 dx, float2 dy) { - float offsetScale = 0.01; + float offsetScale = LOD_OFFSET_SCALE; // ... existing code ... // Simple 2-sample blend weighted toward first sample - return lerp(sample2, sample1, 0.65); + return lerp(sample2, sample1, LOD_SAMPLE_WEIGHT); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🔇 Additional comments (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
182-186: LGTM: Proper weight normalization implemented.The weight normalization using
rcp()and proper scaling correctly addresses the previous review concern about missing normalization. The implementation efficiently handles the case where weights don't sum to 1.0.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
39-50: Hash functions look correct but could benefit from documentation.The hash implementations are mathematically sound. The
hashLODfunction uses a simplified approach appropriate for LOD scenarios.Consider adding brief comments explaining the mathematical basis:
+// Fast hash function using sine-based pseudo-random generation inline float2 hash2D2D(float2 s) { s = s * HASH_MULTIPLIER; return frac(sin(s.x + s.y) * HASH_SINE_MULTIPLIER); } +// Simplified hash for LOD terrain where quality can be reduced inline float2 hashLOD(float2 p)
151-162: Early exit optimization could be further improved.While the distance-based early exit has been improved from the previous version, it could be more optimal by checking distance before any texture sampling, as suggested in the past review.
Based on the past review recommendation, consider moving the distance check before sampling:
inline float4 StochasticEffect(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsets, float2 dx, float2 dy, float distance) { // Distance-based LOD with smooth transition float distanceFactor = saturate((distance - DISTANCE_LOD_THRESHOLD) / DISTANCE_LOD_TRANSITION); + // Early exit for distant terrain - avoid expensive computation entirely + if (distanceFactor >= 0.7) + { + return tex.SampleLevel(samp, uv + offsets.offset1, mipLevel); + } + // Take first sample (always needed) float4 sample1 = tex.SampleLevel(samp, uv + offsets.offset1, mipLevel); - // Early exit for distant terrain - avoid expensive computation - if (distanceFactor >= 0.7) - { - return sample1; - }This eliminates one texture sample for distant terrain where detail isn't critical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🔇 Additional comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
11-27: Well-structured constants section with good optimization approach.The pre-computed constants are a solid optimization strategy, replacing runtime calculations with compile-time values. The reduction of
HEIGHT_BLEND_CONTRASTfrom 16.0 to 12.0 for performance is a reasonable trade-off.
194-196: Weight calculation and normalization look correct.The combined weight calculation properly incorporates height influence and the normalization using
rcp()is mathematically sound and GPU-optimized.
105-127: StochasticSample3 implementation is efficient and correct.The simplified function appropriately removes height/luminescence calculations for performance-critical scenarios where quality can be sacrificed. The barycentric blending is mathematically correct.
82-84:Details
✅ Verification successful
Magic number needs explanation or verification.
The
LOD_SCALEconstant uses a very specific value (255437.0) with a comment about precomputed scaling, but the calculation should be verified for accuracy.
🏁 Script executed:
#!/bin/bash # Verify the LOD_SCALE calculation python3 -c " WORLD_SCALE = 332.54 result = (WORLD_SCALE / 0.010416667) * 8.0 print(f'Calculated LOD_SCALE: {result}') print(f'Hardcoded LOD_SCALE: 255437.0') print(f'Difference: {abs(result - 255437.0)}') print(f'Relative error: {abs(result - 255437.0) / result * 100:.4f}%') "Length of output: 428
LOD_SCALE constant verified – no change needed
Calculated LOD_SCALE = 255390.7 vs. hardcoded 255437.0 (difference ≈ 46.3, 0.018% error), which is negligible. Keeping the existing value is acceptable.
| float contrastFactor = HEIGHT_BLEND_CONTRAST * (1.0 - HEIGHT_INFLUENCE); | ||
| float3 blendWeights = pow(saturate(offsets.weights), contrastFactor); |
There was a problem hiding this comment.
Potential issue with height blend contrast calculation.
The contrast factor calculation appears mathematically incorrect. When HEIGHT_INFLUENCE is higher (more height influence desired), the contrast is actually reduced due to the (1.0 - HEIGHT_INFLUENCE) term.
- float contrastFactor = HEIGHT_BLEND_CONTRAST * (1.0 - HEIGHT_INFLUENCE);
+ float contrastFactor = HEIGHT_BLEND_CONTRAST * (1.0 + HEIGHT_INFLUENCE);This ensures that higher height influence results in sharper height-based transitions, which is the expected behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| float contrastFactor = HEIGHT_BLEND_CONTRAST * (1.0 - HEIGHT_INFLUENCE); | |
| float3 blendWeights = pow(saturate(offsets.weights), contrastFactor); | |
| float contrastFactor = HEIGHT_BLEND_CONTRAST * (1.0 + HEIGHT_INFLUENCE); | |
| float3 blendWeights = pow(saturate(offsets.weights), contrastFactor); |
🤖 Prompt for AI Agents
In features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
around lines 180 to 181, the contrastFactor calculation uses (1.0 -
HEIGHT_INFLUENCE), which reduces contrast as height influence increases,
contrary to the intended effect. To fix this, change the formula to multiply
HEIGHT_BLEND_CONTRAST by HEIGHT_INFLUENCE directly, so that higher height
influence results in a higher contrast factor and sharper transitions.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (1)
148-149: Height-blend contrast formula still inverted
Previous feedback not applied – using(1 - HEIGHT_INFLUENCE)reduces contrast when influence increases.See prior comment; please switch to
(1 + HEIGHT_INFLUENCE).features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
552-642: Duplication of non-stochastic overloads persistsThe two new “WITHOUT StochasticOffsets” overloads copy ~90 % of the stochastic versions, repeating long chains of texture look-ups and weight processing.
Refactor by extracting sampling logic into a helper and sharing the common weight/blend path to cut maintenance burden and shader size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🔇 Additional comments (1)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
60-64: Verify use ofmininstead of standardmaxin mip computationThe comment itself says “Standard mipmapping uses max here” yet the code now uses
min.
This biases towards lower mip-levels (sharper sampling) and can defeat the intended optimisation.Please confirm this is deliberate; otherwise revert to
max.
|
@coderabbitai give me an in depth plan of approach for refactoring Extendedmaterials.hlsli to reduce code repetition. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@davo0411 below is a step-by-step refactor roadmap that keeps performance-critical shader code fast while collapsing the largest pockets of duplication in Goal: Reduce repetition in • Phase 0 Safety net & metrics • Phase 1 Factor out shared math utilities
• Phase 2 Unify TRUE_PBR vs non-TRUE_PBR branches • Phase 3 Merge stochastic vs non-stochastic overloads • Phase 4 Shrink the monster parameter lists
• Phase 5 Leverage compile-time loops to remove 6-layer unrolling heights[0] = Sample(…layer0…);
…
heights[5] = Sample(…layer5…);into [unroll] for (uint i = 0; i < 6; ++i)
{
heights[i] = SampleDisplacement(i, params);
}saves 5 duplicated blocks while producing identical assembly when • Phase 6 House-keeping & docs Deliverables
This phased plan lets us land refactors incrementally: helpers first, then PBR merge, then sampling merge, minimising merge-conflicts with your ongoing Terrain Variation tweaks. Let me know which phases you’d like fleshed out with concrete diff snippets next, and I’ll queue them up. 🏁 Script executed: Length of output: 2080 @davo0411 here’s what the quick inventory told us and how it fits into the phased roadmap I sent earlier. ──────────────────────────────── Duplication snapshot ──────────────────────────────── That confirms our earlier hypothesis: once we collapse those four axes of variation we delete ~450 lines without touching algorithms. ────────────────────────────── Immediate next move (Phase 1) ───────────────────────── • Add a new helper section (just above the first big • Replace the six copy-pasted sample blocks in both PBR and non-PBR variants with: [unroll] for (uint i = 0; i < 6; ++i)
{
heights[i] = SampleDisplacementLayer(i, coords, displacementTex[i], samp, p, dx, dy,
SAMPLING_MODE == Stochastic, offsets);
}(Those edits come in Phase 5, but the helper above is required first.) • Nothing else changes yet—build should stay green. Let me know when you’re ready and I’ll queue the follow-up requirements to:
This staged approach keeps every intermediate commit self-contained and bisectable while chipping away at the repetition. ⏳ Preparing changes... |
|
A summary of the edits CodeRabbit can apply:
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets): 📄 scripts/duplication_inventory.shAdd a new executable script at scripts/duplication_inventory.sh with the following contents: Mark the script as executable: Run the script locally and redirect its full output to docs/duplication_inventory.md: This creates the inventory report with counts and unique signatures. Add and commit both the new script and docs/duplication_inventory.md so reviewers have a clear baseline for the refactor work (addresses requirement 0-a / collect_stats).
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
345-363: RedundantGetTerrainHeightcalls inside the parallax loop
Within each iteration you recomputeweightsfour times – yet the returned array is never used in this loop. This is pure overhead (texture fetch + logic) on every step of the while-loop.Consider sampling only the scalar heights here and compute / normalise weights once after the final parallax coordinate has been resolved, or make the
weightsparameter optional to avoid the extra work when the caller does not need it.
[performance]
♻️ Duplicate comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
45-49: Hash still returns highly-correlated offsets
Thehash2D2Dimplementation is unchanged from the previously-flagged version – a scalarsin()result is broadcast to both components, soxandyremain identical and 2-D randomness is lost.-inline float2 hash2D2D(float2 s) -{ - s = s * HASH_MULTIPLIER; - return frac(sin(s.x + s.y) * HASH_SINE_MULTIPLIER); // identical x & y -}Please adopt the decorrelated version suggested earlier (independent dot products per component).
51-55: Return type / implementation mismatch inhashLOD
hashLODis declared to return afloat2yet produces a single scalar that will be replicated, again killing entropy in the second channel. Either change the return type tofloator compute an independent second component.
109-113: Division-by-zero risk remains unguarded
NormalizeWeightsstill performsrcp(sum)without checking for a zero sum, leading toINFandNaNpropagation in rare degenerate cases. The guard proposed in the previous review is still required.
158-160: Height-blend contrast sign is still inverted
Using(1.0 - HEIGHT_INFLUENCE)reduces contrast as height influence increases, which is the opposite of the stated intent in the comment above. Swap to(1.0 + HEIGHT_INFLUENCE)(or just multiply byHEIGHT_INFLUENCE) so higher influence yields sharper transitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
| # if defined(TERRAIN_VARIATION) | ||
| sh = GetTerrainHeight(noise, input, coords + rayDir * multipliers.x, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, distance, heights); | ||
| sh = GetTerrainHeight(noise, input, coords + rayDir * multipliers.x, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, heights); | ||
| if (quality > 0.25) | ||
| sh.y = GetTerrainHeight(noise, input, coords + rayDir * multipliers.y, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, distance, heights); | ||
| sh.y = GetTerrainHeight(noise, input, coords + rayDir * multipliers.y, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, heights); | ||
| if (quality > 0.5) | ||
| sh.z = GetTerrainHeight(noise, input, coords + rayDir * multipliers.z, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, distance, heights); | ||
| sh.z = GetTerrainHeight(noise, input, coords + rayDir * multipliers.z, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, heights); | ||
| if (quality > 0.75) | ||
| sh.w = GetTerrainHeight(noise, input, coords + rayDir * multipliers.w, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, distance, heights); | ||
| sh.w = GetTerrainHeight(noise, input, coords + rayDir * multipliers.w, mipLevel, params, quality, input.LandBlendWeights1, input.LandBlendWeights2.xy, sharedOffset, dx, dy, heights); | ||
| # else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same redundant work in shadow sampling path
GetParallaxSoftShadowMultiplierTerrain repeats the pattern of re-calculating weights for each probe sample though the result is discarded. Similar refactor as above would remove several expensive texture reads per pixel.
[performance]
🤖 Prompt for AI Agents
In features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
around lines 505 to 513, the function GetParallaxSoftShadowMultiplierTerrain
redundantly recalculates 'weights' for each probe sample, causing unnecessary
expensive texture reads. Refactor the code to calculate 'weights' once before
the loop or repeated calls, store the result, and reuse it for all probe samples
to eliminate redundant computations and improve performance.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (3)
554-590: Duplication déjà-vu – same GetTerrainHeight logic re-implementedThe new “non-stochastic” overload copies almost the entire stochastic path minus two parameters. This was already highlighted in previous reviews; the duplication persists. Please extract common sampling into a helper and gate only the offset logic.
648-776: ParallaxCoords overload duplicates >120 linesIdentical state machine, only the
GetTerrainHeightcall differs. Same DRY concern as above – factor out the traversal loop and pass a function pointer / macro for the sampler call.
792-827: Shadow-multiplier overload follows the same copy-paste patternAgain, logic is 95 % identical to the stochastic variant. Consolidate or future fixes will be missed in one path.
🧹 Nitpick comments (1)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
121-181: No early-out when blend weights are ~0 – six redundant texture readsInside the TRUE_PBR branch we still issue texture reads even if the aggregated land-tile weights are below visibility threshold. A quick mask check before any sampling would shave ~25-30 % of texture bandwidth on flat or single-texture tiles.
if (all(w1 < 0.01.xxxy) && all(w2 < 0.01.xx)) { total = 0; ProcessTerrainHeightWeights(...); return total; }Cheap ALU, noticeable gain on distant terrain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)
🔇 Additional comments (1)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
44-55: Hard-coded tex-dim halving is fragileBlindly dividing
textureDimsby 2 behind several unrelated#ifswitches bakes in undocumented assumptions about packing & LOD conventions. This will silently break if:
- a ½-resolution virtual-texture path is disabled/enabled at runtime;
- a platform ships different packing (e.g. array textures, sparse residency).
Please gate the scale-down behind an explicit constant or per-resource flag instead of ad-hoc pre-processor tests, or compute the ratio from real
GetDimensions()queries.
| void ProcessTerrainHeightWeights(float heightBlend, float4 w1, float2 w2, float heights[6], inout float weights[6], out float totalHeight) | ||
| { | ||
| float heightBlend = 1 + blendFactor * HEIGHT_POWER; | ||
| weights[0] = w1.x; | ||
| weights[1] = w1.y; | ||
| weights[2] = w1.z; | ||
| weights[3] = w1.w; | ||
| weights[4] = w2.x; | ||
| weights[5] = w2.y; | ||
| float total = 0; | ||
|
|
||
| totalHeight = 0; | ||
| [unroll] for (int i = 0; i < 6; i++) | ||
| { | ||
| totalHeight += heights[i] * weights[i]; | ||
| weights[i] *= pow(heightBlend, HEIGHT_MULT * heights[i]); | ||
| } | ||
|
|
||
| [unroll] for (int i = 0; i < 6; i++) | ||
| { | ||
| weights[i] = min(100, pow(weights[i], heightBlend)); | ||
| } | ||
|
|
||
| float wsum = 0; | ||
| [unroll] for (int i = 0; i < 6; i++) | ||
| { | ||
| wsum += weights[i]; | ||
| } | ||
|
|
||
| float invwsum = rcp(wsum); | ||
| [unroll] for (int i = 0; i < 6; i++) | ||
| { | ||
| weights[i] *= invwsum; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Weight normalisation loop is numerically unstable & branchy
pow(heightBlend, HEIGHT_MULT * heights[i]) followed by pow(weights[i], heightBlend) can overflow (>1e38) for large heightBlend or high-bit textures.
Additionally, three serial loops and pow in the inner pixel shader are expensive.
Consider:
- clamping
heightBlendto a safe range (e.g.saturate(heightBlend, 0, 4)); - merging the three loops into one
unrollblock to reduce VGPR pressure; - replacing
powwithexp2/fastpowor a fixed-degree polynomial.
Will save ALU and avoid NaNs on malformed height maps.
🤖 Prompt for AI Agents
In features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
around lines 87 to 119, the current implementation uses multiple separate loops
with pow calls that can overflow and cause numerical instability. To fix this,
clamp heightBlend to a safe range like saturate(heightBlend, 0, 4) at the start,
then merge the three separate unroll loops into a single unroll loop to reduce
VGPR usage, and replace pow calls with more efficient and stable alternatives
such as exp2, fastpow, or a fixed-degree polynomial approximation to improve
performance and avoid NaNs.
| // Find min of change in u and v across quad: compute du and dv magnitude across quad | ||
| //float2 dTexCoords = dxSize * dxSize + dySize * dySize; | ||
|
|
||
| // Compute the current mip level (* 0.5 is effectively computing a square root before ) | ||
| float mipLevel = max(0.5 * log2(minTexCoordDelta), 0); | ||
| // Standard mipmapping uses max here | ||
| float minTexCoordDelta = min(dot(dxSize, dxSize), dot(dySize, dySize)); | ||
|
|
||
| #if !defined(PARALLAX) && !defined(TRUE_PBR) | ||
| mipLevel++; | ||
| #endif | ||
| // Compute the current mip level (* 0.5 is effectively computing a square root before ) | ||
| float mipLevel = max(0.5 * log2(minTexCoordDelta), 0); | ||
|
|
There was a problem hiding this comment.
min instead of max mis-computes mip → over-sampling & cache thrash
Standard mipmapping chooses the maximum derivative length to avoid undersampling.
Using min(dot(dx), dot(dy)) underestimates the desired LOD, forces lower-mip fetches and inflates texture bandwidth for oblique surfaces.
- float minTexCoordDelta = min(dot(dxSize, dxSize), dot(dySize, dySize));
+ float minTexCoordDelta = max(dot(dxSize, dxSize), dot(dySize, dySize));Unless this is a deliberate artistic hack, please switch to max or add a comment justifying the deviation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Find min of change in u and v across quad: compute du and dv magnitude across quad | |
| //float2 dTexCoords = dxSize * dxSize + dySize * dySize; | |
| // Compute the current mip level (* 0.5 is effectively computing a square root before ) | |
| float mipLevel = max(0.5 * log2(minTexCoordDelta), 0); | |
| // Standard mipmapping uses max here | |
| float minTexCoordDelta = min(dot(dxSize, dxSize), dot(dySize, dySize)); | |
| #if !defined(PARALLAX) && !defined(TRUE_PBR) | |
| mipLevel++; | |
| #endif | |
| // Compute the current mip level (* 0.5 is effectively computing a square root before ) | |
| float mipLevel = max(0.5 * log2(minTexCoordDelta), 0); | |
| // Find min of change in u and v across quad: compute du and dv magnitude across quad | |
| //float2 dTexCoords = dxSize * dxSize + dySize * dySize; | |
| // Standard mipmapping uses max here | |
| float minTexCoordDelta = max(dot(dxSize, dxSize), dot(dySize, dySize)); | |
| // Compute the current mip level (* 0.5 is effectively computing a square root before ) | |
| float mipLevel = max(0.5 * log2(minTexCoordDelta), 0); |
🤖 Prompt for AI Agents
In features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
around lines 61 to 69, the code uses min(dot(dxSize, dxSize), dot(dySize,
dySize)) to compute mipLevel, which underestimates the mip level and causes
oversampling. Replace the min function with max to correctly compute the mip
level using the maximum derivative length, or add a comment explaining why min
is intentionally used if this is a deliberate artistic choice.
| float blendFactor = SharedData::extendedMaterialSettings.EnableHeightBlending ? sqrt(saturate(1 - nearBlendToFar)) : 0; | ||
| float4 w1 = lerp(input.LandBlendWeights1, smoothstep(0, 1, input.LandBlendWeights1), blendFactor); | ||
| float2 w2 = lerp(input.LandBlendWeights2.xy, smoothstep(0, 1, input.LandBlendWeights2.xy), blendFactor); | ||
| float scale = max(params[0].HeightScale * w1.x, max(params[1].HeightScale * w1.y, max(params[2].HeightScale * w1.z, max(params[3].HeightScale * w1.w, max(params[4].HeightScale * w2.x, params[5].HeightScale * w2.y))))); | ||
| float scalercp = rcp(scale); | ||
| float maxHeight = 0.1 * scale; | ||
| # else |
There was a problem hiding this comment.
Potential div-by-zero in scalercp
scale can legitimately be 0 when all contributing blend weights have zero displacement. rcp(scale) will then produce INF/NaN propagating through lighting. The soft-shadow variant explicitly bails out, but the main parallax path does not.
Guard before the reciprocal:
float scale = ...;
if (scale < 1e-5) { pixelOffset = 0; return coords; }
float scalercp = rcp(scale);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
around lines 654 to 660, the calculation of scalercp uses rcp(scale) without
checking if scale is zero, which can cause division by zero and produce INF/NaN
values. To fix this, add a guard to ensure scale is not zero before computing
scalercp, for example by using max(scale, a small epsilon) or an if-condition to
avoid division by zero.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
43-47: Verify the hashLOD function implementation.The function declares a
float2return type but the implementation appears to only compute a single hash value that gets replicated. This could reduce randomness quality.
101-105: Guard against division by zero in weight normalization.The
NormalizeWeightsfunction usesrcp()without checking if the weight sum is zero, which could produce infinite values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli(8 hunks)features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🔇 Additional comments (15)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (7)
11-24: Well-structured constant definitions with good documentation.The new constants are well-organized and include helpful comments explaining their purpose. The pre-computed values like
SKEW_MATRIXandWORLD_SCALEwill improve performance by avoiding runtime calculations.
50-62: Efficient barycentric coordinate computation.The
ComputeBarycentricVertsfunction provides a clean, unified approach to barycentric coordinate calculation with good use of pre-computed matrices. The conditional logic correctly handles both triangle orientations.
64-75: Clean stochastic offset computation.The
ComputeStochasticOffsetsfunction effectively centralizes the offset generation logic and uses the barycentric computation efficiently.
77-97: Optimized LOD variant with reduced sample count.The
ComputeStochasticOffsetsLODfunction appropriately reduces complexity for LOD terrain by using only 2 samples instead of 3. The pre-computed scaling constant and simplified weights are good performance optimizations.
108-126: Effective LOD sampling with good performance optimizations.The
StochasticSampleLODfunction uses efficient pseudo-rotation and a simple 2-sample blend. The weighted blend toward the first sample (0.65) is appropriate for LOD terrain where detail preservation is less critical.
129-169: Improved stochastic sampling with mip-based optimization.The refactored
StochasticEffectfunction has several good improvements:
- Mip-based early exit reduces unnecessary sampling for distant terrain
- Direct height calculation without matrix operations
- Combined weight calculation and normalization for better performance
The transition from high-quality to single sample based on mip level is well-implemented.
172-196: Efficient no-height variant for performance-critical paths.The
StochasticEffectNoHeightfunction provides a useful optimization by skipping height-based blending when not needed. The implementation correctly maintains the same mip-based optimization while using simpler barycentric blending.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (8)
8-10: Simplified include guard improves clarity.Moving the
TerrainVariation.hlsliinclude to only be used whenTERRAIN_VARIATIONis defined is a good optimization that reduces unnecessary includes.
91-123: Excellent refactoring to eliminate code duplication.The
ProcessTerrainHeightWeightsfunction successfully consolidates the repeated weight calculation logic. This addresses the previous code duplication concerns while maintaining the same functionality.
135-186: Verify the weight threshold change impact.The change from
w1.x > 0.0tow1.x > 0.01(and similar for other weights) introduces a small threshold that could skip very small weight contributions. While this may improve performance, ensure this doesn't cause visual artifacts for subtle blending.The threshold change may affect terrain blending quality. Consider testing with:
- Very subtle terrain transitions
- Low-weight terrain tiles
- Edge cases where weights are just above/below the threshold
138-182: Good adoption of StochasticEffectNoHeight.The switch to using
StochasticEffectNoHeightis consistent with the refactoring goals and should provide better performance by avoiding unnecessary height-based blending calculations in terrain height sampling.
588-690: Well-implemented non-stochastic overloads.The addition of
GetTerrainHeightoverloads withoutStochasticOffsetsprovides necessary functionality for when tiling fix is disabled. The implementation correctly mirrors the stochastic versions while using direct texture sampling.
692-835: Complete parallax functionality preserved in overload.The
GetParallaxCoordsoverload maintains the full parallax occlusion mapping algorithm while removing stochastic sampling dependencies. This provides a clean separation between stochastic and non-stochastic paths.
837-874: Consistent shadow multiplier overload.The
GetParallaxSoftShadowMultiplierTerrainoverload completes the set of non-stochastic functions, maintaining functionality while removing the stochastic sampling dependency.
44-84: Conditional mip level adjustments need verification.The mip level calculations have been reorganized with new conditional increments. The logic appears correct, but the interaction between different feature flags could be complex.
Verify that the mip level calculations produce expected results across different feature combinations:
#!/bin/bash # Search for all mip level related calculations to verify consistency ast-grep --pattern $'mipLevel$_' rg -A 3 -B 3 "mipLevel.*\+\+"
| [branch] if ((Permutation::ExtraFeatureDescriptor & Permutation::ExtraFeatureFlags::THLand3HasDisplacement) != 0 && w1.w > 0.01) | ||
| { | ||
| heights[3] = ScaleDisplacement(TexLandTHDisp3Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).x, params[3]); | ||
| } | ||
| else if (w1.w > 0.01) | ||
| { | ||
| heights[3] = ScaleDisplacement(TexLandColor4Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).w, params[3]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent weight threshold in conditional structure.
The conditional at line 661 uses w1.w > 0.01 but the following else if at line 665 also uses w1.w > 0.01. This creates redundant conditions and may cause confusion.
Consider simplifying the conditional structure:
-[branch] if ((Permutation::ExtraFeatureDescriptor & Permutation::ExtraFeatureFlags::THLand3HasDisplacement) != 0 && w1.w > 0.01)
- {
- heights[3] = ScaleDisplacement(TexLandTHDisp3Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).x, params[3]);
- }
- else if (w1.w > 0.01)
- {
- heights[3] = ScaleDisplacement(TexLandColor4Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).w, params[3]);
- }
+if (w1.w > 0.01) {
+ [branch] if ((Permutation::ExtraFeatureDescriptor & Permutation::ExtraFeatureFlags::THLand3HasDisplacement) != 0)
+ {
+ heights[3] = ScaleDisplacement(TexLandTHDisp3Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).x, params[3]);
+ }
+ else
+ {
+ heights[3] = ScaleDisplacement(TexLandColor4Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).w, params[3]);
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [branch] if ((Permutation::ExtraFeatureDescriptor & Permutation::ExtraFeatureFlags::THLand3HasDisplacement) != 0 && w1.w > 0.01) | |
| { | |
| heights[3] = ScaleDisplacement(TexLandTHDisp3Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).x, params[3]); | |
| } | |
| else if (w1.w > 0.01) | |
| { | |
| heights[3] = ScaleDisplacement(TexLandColor4Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[3]).w, params[3]); | |
| } | |
| if (w1.w > 0.01) { | |
| [branch] if ((Permutation::ExtraFeatureDescriptor & Permutation::ExtraFeatureFlags::THLand3HasDisplacement) != 0) | |
| { | |
| heights[3] = ScaleDisplacement( | |
| TexLandTHDisp3Sampler.SampleLevel( | |
| SampTerrainParallaxSampler, coords, mipLevels[3] | |
| ).x, | |
| params[3] | |
| ); | |
| } | |
| else | |
| { | |
| heights[3] = ScaleDisplacement( | |
| TexLandColor4Sampler.SampleLevel( | |
| SampTerrainParallaxSampler, coords, mipLevels[3] | |
| ).w, | |
| params[3] | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
around lines 661 to 668, the condition w1.w > 0.01 is redundantly checked in
both the if and else if branches. To fix this, restructure the condition to
check w1.w > 0.01 once before the nested if that distinguishes between the
ExtraFeatureDescriptor flag states, eliminating the repeated condition and
simplifying the logic.
davo0411
force-pushed
the
TV-NO-DIST-ALWAYSON
branch
from
cd48eb4 to
f5f9b0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
37-41: Hash function still returns correlated values.The
hash2D2Dfunction continues to have the same correlation issue identified in previous reviews. It returns identical components becausesin(s.x + s.y)produces a scalar that gets broadcast to both components of thefloat2, reducing 2-D randomness and weakening the tiling-break effect.-inline float2 hash2D2D(float2 s) -{ - s = s * HASH_MULTIPLIER; - return frac(sin(s.x + s.y) * HASH_SINE_MULTIPLIER); -} +inline float2 hash2D2D(float2 p) +{ + // Independent hashes for x and y – avoid cross-channel correlation + const float2 k1 = float2(127.1, 311.7); + const float2 k2 = float2(269.5, 183.3); + return frac(sin(float2(dot(p, k1), dot(p, k2))) * 43758.5453); +}
43-47: hashLOD function returns scalar replicated to both components.Similar to
hash2D2D, thehashLODfunction also has correlation issues. It computes a single scalar value and replicates it, eliminating 2-D randomness.-inline float2 hashLOD(float2 p) -{ - p = frac(p * 0.318); - return frac(p.x + p.y * 17.0); -} +inline float2 hashLOD(float2 p) +{ + p = frac(p * 0.318); + return frac(float2(p.x + p.y * 17.0, p.y + p.x * 23.0)); +}
101-105: Missing zero-division guard in NormalizeWeights function.The function uses
rcp()on the sum of weights without checking if the sum is zero, which can cause infinite results when weights sum to zero (e.g., in degenerate barycentric cases).-inline float3 NormalizeWeights(float3 weights) -{ - float rcpWeightSum = rcp(weights.x + weights.y + weights.z); - return weights * rcpWeightSum; -} +inline float3 NormalizeWeights(float3 weights) +{ + float weightSum = weights.x + weights.y + weights.z; + return weightSum > 0 ? weights / weightSum : DEFAULT_WEIGHTS; +}
153-154: Height blend contrast calculation still incorrect.The contrast factor calculation continues to use
(1.0 - HEIGHT_INFLUENCE), which reduces contrast as height influence increases, contrary to the intended effect where higher height influence should result in sharper transitions.- float contrastFactor = HEIGHT_BLEND_CONTRAST * (1.0 - HEIGHT_INFLUENCE); + float contrastFactor = HEIGHT_BLEND_CONTRAST * (1.0 + HEIGHT_INFLUENCE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Shaders/Lighting.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (1)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli(2 hunks)
🔇 Additional comments (4)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (4)
139-146: Mip-based early exit is well-implemented.The smooth transition factor using mip level is a good optimization that addresses the distance-based early exit suggestion from previous reviews. The threshold of 0.8 for the mip factor provides a good balance between performance and quality.
164-171: Conditional compilation for height calculation is robust.The
#if defined(TRUE_PBR)block provides a clean separation between PBR and Complex Material systems, with appropriate fallbacks. The alpha mask calculation usingstep(0.001, alphaValues)effectively handles zero alpha values.
183-211: StochasticEffectNoHeight function provides good performance optimization.This dedicated function without height blending is a smart optimization for cases where height influence isn't needed. The code structure mirrors
StochasticEffectbut skips expensive height calculations, providing better performance for terrain height computations.
77-97: ComputeStochasticOffsetsLOD function has potential issues.The LOD-specific offset computation uses simplified 2-sample weighting, but the weights
float3(0.65, 0.35, 0.0)don't sum to 1.0. This could cause normalization issues when used withNormalizeWeights.- // Simplified weights since we only use 2 samples now - offsetsLOD.weights = float3(0.65, 0.35, 0.0); + // Simplified weights for 2 samples - sum to 1.0 + offsetsLOD.weights = float3(0.65, 0.35, 0.0); + offsetsLOD.offset3 = float2(0.0, 0.0); // Ensure unused offset is initializedAdditionally, consider whether
NormalizeWeightsshould handle the 2-sample case differently or if the weights should be pre-normalized.Likely an incorrect or invalid review comment.
| inline float4 StochasticSampleLOD(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsetsLOD, float2 dx, float2 dy) | ||
| { | ||
| // Apply mip bias to match normal sampling behavior | ||
| float adjustedMipLevel = mipLevel + SharedData::MipBias; | ||
| float offsetScale = 0.01; | ||
|
|
||
| // Cheap pseudo-rotation using simple transforms | ||
| float2 dir1 = float2(rnd - 0.5, frac(rnd * 1.618) - 0.5); | ||
| float2 dir2 = float2(dir1.y, -dir1.x); | ||
|
|
||
| // Apply simple scaled offsets | ||
| float2 microOffset1 = (offsetsLOD.offset1 + dir1) * offsetScale; | ||
| float2 microOffset2 = (offsetsLOD.offset2 + dir2) * offsetScale; | ||
|
|
||
| // Sample only two offsets | ||
| float4 sample1 = tex.SampleLevel(samp, uv + microOffset1, adjustedMipLevel); | ||
| float4 sample2 = tex.SampleLevel(samp, uv + microOffset2, adjustedMipLevel); | ||
|
|
||
| // Simple 2-sample blend weighted toward first sample | ||
| return lerp(sample2, sample1, 0.65); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
StochasticSampleLOD function uses hardcoded weights.
The function uses a hardcoded lerp weight of 0.65, but this doesn't utilize the computed offsetsLOD.weights from ComputeStochasticOffsetsLOD. This inconsistency could lead to different blending behavior than expected.
- // Simple 2-sample blend weighted toward first sample
- return lerp(sample2, sample1, 0.65);
+ // Use computed weights for consistent blending
+ return lerp(sample2, sample1, offsetsLOD.weights.x);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline float4 StochasticSampleLOD(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsetsLOD, float2 dx, float2 dy) | |
| { | |
| // Apply mip bias to match normal sampling behavior | |
| float adjustedMipLevel = mipLevel + SharedData::MipBias; | |
| float offsetScale = 0.01; | |
| // Cheap pseudo-rotation using simple transforms | |
| float2 dir1 = float2(rnd - 0.5, frac(rnd * 1.618) - 0.5); | |
| float2 dir2 = float2(dir1.y, -dir1.x); | |
| // Apply simple scaled offsets | |
| float2 microOffset1 = (offsetsLOD.offset1 + dir1) * offsetScale; | |
| float2 microOffset2 = (offsetsLOD.offset2 + dir2) * offsetScale; | |
| // Sample only two offsets | |
| float4 sample1 = tex.SampleLevel(samp, uv + microOffset1, adjustedMipLevel); | |
| float4 sample2 = tex.SampleLevel(samp, uv + microOffset2, adjustedMipLevel); | |
| // Simple 2-sample blend weighted toward first sample | |
| return lerp(sample2, sample1, 0.65); | |
| } | |
| inline float4 StochasticSampleLOD(float rnd, float mipLevel, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsetsLOD, float2 dx, float2 dy) | |
| { | |
| // Apply mip bias to match normal sampling behavior | |
| float adjustedMipLevel = mipLevel + SharedData::MipBias; | |
| float offsetScale = 0.01; | |
| // Cheap pseudo-rotation using simple transforms | |
| float2 dir1 = float2(rnd - 0.5, frac(rnd * 1.618) - 0.5); | |
| float2 dir2 = float2(dir1.y, -dir1.x); | |
| // Apply simple scaled offsets | |
| float2 microOffset1 = (offsetsLOD.offset1 + dir1) * offsetScale; | |
| float2 microOffset2 = (offsetsLOD.offset2 + dir2) * offsetScale; | |
| // Sample only two offsets | |
| float4 sample1 = tex.SampleLevel(samp, uv + microOffset1, adjustedMipLevel); | |
| float4 sample2 = tex.SampleLevel(samp, uv + microOffset2, adjustedMipLevel); | |
| // Use computed weights for consistent blending | |
| return lerp(sample2, sample1, offsetsLOD.weights.x); | |
| } |
🤖 Prompt for AI Agents
In features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
around lines 108 to 128, the StochasticSampleLOD function uses a hardcoded lerp
weight of 0.65 instead of the weights computed in offsetsLOD.weights from
ComputeStochasticOffsetsLOD. To fix this, replace the hardcoded 0.65 with the
appropriate weight value from offsetsLOD.weights to ensure consistent blending
behavior aligned with the computed stochastic offsets.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. A summary of the edits CodeRabbit can apply:
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets): ✏️ features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsliPhase 1: Create unified parameter structures Add after the DisplacementParams struct (line 18): // Unified parameter structures to eliminate massive function signatures
struct TerrainSamplingParams
{
float2 coords;
float mipLevels[6];
DisplacementParams params[6];
float blendFactor;
float4 w1;
float2 w2;
float screenNoise;
bool useStochastic;
#if defined(TERRAIN_VARIATION)
StochasticOffsets stochasticOffsets;
float2 dx, dy;
#endif
};
// Quality levels for adaptive sampling
static const uint QUALITY_ULTRA = 3; // Full 3-sample stochastic
static const uint QUALITY_HIGH = 2; // 2-sample stochastic
static const uint QUALITY_MEDIUM = 1; // 1-sample stochastic
static const uint QUALITY_LOW = 0; // Single standard sample
// Adaptive quality determination
inline uint GetTerrainQuality(float distance, float4 w1, float2 w2, float avgMipLevel)
{
// Check if one texture dominates (>85% weight)
float maxWeight = max(max(max(w1.x, w1.y), max(w1.z, w1.w)), max(w2.x, w2.y));
bool isUniform = maxWeight > 0.85;
if (distance > 1024.0 || avgMipLevel > 4.0 || isUniform) return QUALITY_LOW;
if (distance > 512.0 || avgMipLevel > 3.0) return QUALITY_MEDIUM;
if (distance > 256.0 || avgMipLevel > 2.0) return QUALITY_HIGH;
return QUALITY_ULTRA;
}Phase 2: Create unified terrain layer sampling helper Add before the existing GetTerrainHeight functions: // Unified terrain layer sampler - eliminates 6-layer duplication
inline float SampleTerrainLayerDisplacement(uint layerIndex, in TerrainSamplingParams params, PS_INPUT input, uint quality)
{
float weight = (layerIndex < 4) ? params.w1[layerIndex] : params.w2[layerIndex - 4];
if (weight <= 0.01) return 0.0;
float displacement = 0.0;
float mipLevel = params.mipLevels[layerIndex];
// Increase mip level for lower quality to maintain performance
if (quality < QUALITY_HIGH) mipLevel += 1.0;
if (quality < QUALITY_MEDIUM) mipLevel += 1.0;
#if defined(TRUE_PBR)
// PBR path - use displacement textures when available
[branch] switch(layerIndex)
{
case 0: if ((PBRFlags & PBR::TerrainFlags::LandTile0HasDisplacement) != 0) {
#if defined(TERRAIN_VARIATION)
displacement = (params.useStochastic && quality >= QUALITY_MEDIUM) ?
StochasticEffectNoHeight(params.screenNoise, mipLevel, TexLandDisplacement0Sampler, SampTerrainParallaxSampler, params.coords, params.stochasticOffsets, params.dx, params.dy).x :
TexLandDisplacement0Sampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).x;
#else
displacement = TexLandDisplacement0Sampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).x;
#endif
} break;
case 1: if ((PBRFlags & PBR::TerrainFlags::LandTile1HasDisplacement) != 0) {
#if defined(TERRAIN_VARIATION)
displacement = (params.useStochastic && quality >= QUALITY_MEDIUM) ?
StochasticEffectNoHeight(params.screenNoise, mipLevel, TexLandDisplacement1Sampler, SampTerrainParallaxSampler, params.coords, params.stochasticOffsets, params.dx, params.dy).x :
TexLandDisplacement1Sampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).x;
#else
displacement = TexLandDisplacement1Sampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).x;
#endif
} break;
// Continue for layers 2-5 with same pattern...
}
#else
// Non-PBR path - use dedicated displacement textures or fallback to color alpha
[branch] switch(layerIndex)
{
case 0: {
if ((Permutation::ExtraFeatureDescriptor & Permutation::ExtraFeatureFlags::THLand0HasDisplacement) != 0) {
#if defined(TERRAIN_VARIATION)
displacement = (params.useStochastic && quality >= QUALITY_MEDIUM) ?
StochasticEffectNoHeight(params.screenNoise, mipLevel, TexLandTHDisp0Sampler, SampTerrainParallaxSampler, params.coords, params.stochasticOffsets, params.dx, params.dy).x :
TexLandTHDisp0Sampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).x;
#else
displacement = TexLandTHDisp0Sampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).x;
#endif
} else {
#if defined(TERRAIN_VARIATION)
displacement = (params.useStochastic && quality >= QUALITY_MEDIUM) ?
StochasticEffectNoHeight(params.screenNoise, mipLevel, TexColorSampler, SampTerrainParallaxSampler, params.coords, params.stochasticOffsets, params.dx, params.dy).w :
TexColorSampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).w;
#else
displacement = TexColorSampler.SampleLevel(SampTerrainParallaxSampler, params.coords, mipLevel).w;
#endif
}
} break;
// Continue for layers 1-5...
}
#endif
return ScaleDisplacement(displacement, params.params[layerIndex]);
}Phase 3: Create optimized terrain height calculator with adaptive sampling Add the unified terrain height function: // Optimized terrain height calculation with adaptive quality
inline float CalculateOptimizedTerrainHeight(in TerrainSamplingParams params, PS_INPUT input, uint quality, out float weights[6])
{
float heightBlend = 1 + params.blendFactor * HEIGHT_POWER;
float heights[6] = { 0, 0, 0, 0, 0, 0 };
// Adaptive sampling based on quality level
if (quality == QUALITY_LOW)
{
// Sample only the dominant layer for low quality
float maxWeight = max(max(max(params.w1.x, params.w1.y), max(params.w1.z, params.w1.w)), max(params.w2.x, params.w2.y));
uint dominantLayer = 0;
if (maxWeight == params.w1.y) dominantLayer = 1;
else if (maxWeight == params.w1.z) dominantLayer = 2;
else if (maxWeight == params.w1.w) dominantLayer = 3;
else if (maxWeight == params.w2.x) dominantLayer = 4;
else if (maxWeight == params.w2.y) dominantLayer = 5;
heights[dominantLayer] = SampleTerrainLayerDisplacement(dominantLayer, params, input, quality);
}
else if (quality == QUALITY_MEDIUM)
{
// Sample only layers with significant weight (>10%)
[unroll] for (uint i = 0; i < 6; ++i)
{
float weight = (i < 4) ? params.w1[i] : params.w2[i - 4];
if (weight > 0.1) {
heights[i] = SampleTerrainLayerDisplacement(i, params, input, quality);
}
}
}
else
{
// Full quality sampling for all layers
[unroll] for (uint i = 0; i < 6; ++i)
{
heights[i] = SampleTerrainLayerDisplacement(i, params, input, quality);
}
}
float totalHeight;
ProcessTerrainHeightWeights(heightBlend, params.w1, params.w2, heights, weights, totalHeight);
return totalHeight;
}Phase 4: Replace all GetTerrainHeight variants with unified implementation Replace the existing GetTerrainHeight functions (lines 125-304 and 588-690) with: #if defined(LANDSCAPE)
// Unified GetTerrainHeight - replaces all 4 variants
float GetTerrainHeight(float screenNoise, PS_INPUT input, float2 coords, float mipLevels[6], DisplacementParams params[6], float blendFactor, float4 w1, float2 w2,
#if defined(TERRAIN_VARIATION)
StochasticOffsets sharedOffset, float2 dx, float2 dy,
#endif
out float weights[6])
{
TerrainSamplingParams samplingParams;
samplingParams.coords = coords;
samplingParams.blendFactor = blendFactor;
samplingParams.w1 = w1;
samplingParams.w2 = w2;
samplingParams.screenNoise = screenNoise;
[unroll] for (int i = 0; i < 6; i++) {
samplingParams.mipLevels[i] = mipLevels[i];
samplingParams.params[i] = params[i];
}
#if defined(TERRAIN_VARIATION)
samplingParams.useStochastic = true;
samplingParams.stochasticOffsets = sharedOffset;
samplingParams.dx = dx;
samplingParams.dy = dy;
#else
samplingParams.useStochastic = false;
#endif
// Calculate average mip level for quality determination
float avgMipLevel = (mipLevels[0] + mipLevels[1] + mipLevels[2] + mipLevels[3] + mipLevels[4] + mipLevels[5]) / 6.0;
uint quality = GetTerrainQuality(length(input.WorldPosition - SharedData::CameraPosAdjust[0].xyz), w1, w2, avgMipLevel);
return CalculateOptimizedTerrainHeight(samplingParams, input, quality, weights);
}
// Simplified overload for non-stochastic cases
#if defined(TERRAIN_VARIATION)
float GetTerrainHeight(PS_INPUT input, float2 coords, float mipLevels[6], DisplacementParams params[6], float blendFactor, float4 w1, float2 w2, out float weights[6])
{
StochasticOffsets dummyOffset;
float2 dummyDerivatives = float2(0, 0);
return GetTerrainHeight(0.0, input, coords, mipLevels, params, blendFactor, w1, w2, dummyOffset, dummyDerivatives, dummyDerivatives, weights);
}
#endif
#endifPhase 5: Optimize the expensive POM loop In the GetParallaxCoords function, replace the expensive GetTerrainHeight calls in the POM loop (around lines 377-465) with: // Cached samples to avoid redundant GetTerrainHeight calls
struct POMSampleCache
{
float4 heights;
float2 coords;
bool isValid;
};
static POMSampleCache g_pomCache[2];
[loop] while (numSteps > 0)
{
float4 currentOffset[2];
currentOffset[0] = prevOffset.xyxy - float4(1, 1, 2, 2) * offsetPerStep.xyxy;
currentOffset[1] = prevOffset.xyxy - float4(3, 3, 4, 4) * offsetPerStep.xyxy;
float4 currentBound = prevBound.xxxx - float4(1, 2, 3, 4) * stepSize;
float4 currHeight;
// Use adaptive quality for POM - lower quality than final shading
uint pomQuality = max(0, (int)quality - 1); // One quality level lower
// Interleaved sampling pattern to distribute work across neighboring pixels
uint pixelPattern = uint(input.Position.x + input.Position.y * 2) % 4;
bool useFullSampling = (pixelPattern == 0) || (numSteps <= 8);
if (useFullSampling)
{
// Full 4-sample evaluation
TerrainSamplingParams tempParams = samplingParams;
tempParams.coords = currentOffset[0].xy;
currHeight.x = CalculateOptimizedTerrainHeight(tempParams, input, pomQuality, weights);
tempParams.coords = currentOffset[0].zw;
currHeight.y = CalculateOptimizedTerrainHeight(tempParams, input, pomQuality, weights);
tempParams.coords = currentOffset[1].xy;
currHeight.z = CalculateOptimizedTerrainHeight(tempParams, input, pomQuality, weights);
tempParams.coords = currentOffset[1].zw;
currHeight.w = CalculateOptimizedTerrainHeight(tempParams, input, pomQuality, weights);
}
else
{
// Reduced sampling - interpolate between previous and single new sample
TerrainSamplingParams tempParams = samplingParams;
tempParams.coords = currentOffset[0].xy;
currHeight.x = CalculateOptimizedTerrainHeight(tempParams, input, QUALITY_LOW, weights);
// Interpolate other samples to reduce work
currHeight.yzw = lerp(currHeight.xxx, prevHeight.yzw, 0.6);
}
#if defined(TRUE_PBR)
currHeight = currHeight * scalercp + 0.5;
#else
currHeight = currHeight + 0.5;
#endif
// ... rest of POM logic unchanged
}Phase 6: Optimize shadow sampling with unified approach Replace the duplicated GetParallaxSoftShadowMultiplierTerrain functions (lines 527-586 and 838-873) with: #if defined(LANDSCAPE)
float GetParallaxSoftShadowMultiplierTerrain(PS_INPUT input, float2 coords, float mipLevel[6], float3 L, float sh0, float quality, float noise, DisplacementParams params[6]
#if defined(TERRAIN_VARIATION)
, StochasticOffsets sharedOffset, float2 dx, float2 dy
#endif
)
{
if (quality <= 0.0) return 1.0;
// Adaptive shadow quality based on overall quality setting
uint shadowQuality = (quality > 0.75) ? QUALITY_HIGH :
(quality > 0.5) ? QUALITY_MEDIUM : QUALITY_LOW;
TerrainSamplingParams samplingParams;
samplingParams.blendFactor = quality; // Use quality as blend factor for shadows
samplingParams.w1 = input.LandBlendWeights1;
samplingParams.w2 = input.LandBlendWeights2.xy;
samplingParams.screenNoise = noise;
[unroll] for (int i = 0; i < 6; i++) {
samplingParams.mipLevels[i] = mipLevel[i];
samplingParams.params[i] = params[i];
}
#if defined(TERRAIN_VARIATION)
samplingParams.useStochastic = true;
samplingParams.stochasticOffsets = sharedOffset;
samplingParams.dx = dx;
samplingParams.dy = dy;
#else
samplingParams.useStochastic = false;
#endif
float4 multipliers = rcp((float4(1, 2, 3, 4) + noise));
float2 rayDir = L.xy * 0.1;
#if defined(TRUE_PBR)
float scale = max(params[0].HeightScale * input.LandBlendWeights1.x,
max(params[1].HeightScale * input.LandBlendWeights1.y,
max(params[2].HeightScale * input.LandBlendWeights1.z,
max(params[3].HeightScale * input.LandBlendWeights1.w,
max(params[4].HeightScale * input.LandBlendWeights2.x,
params[5].HeightScale * input.LandBlendWeights2.y)))));
if (scale < 0.01) return 1.0;
rayDir *= scale;
#endif
// Adaptive shadow sample count
uint maxShadowSamples = (shadowQuality == QUALITY_LOW) ? 1 :
(shadowQuality == QUALITY_MEDIUM) ? 2 : 4;
float4 sh = 0;
float weights[6];
[unroll] for (uint i = 0; i < 4; i++) {
if (i < maxShadowSamples) {
samplingParams.coords = coords + rayDir * multipliers[i];
sh[i] = CalculateOptimizedTerrainHeight(samplingParams, input, shadowQuality, weights);
}
}
#if defined(TRUE_PBR)
return pow(1.0 - saturate(dot(max(0, sh - sh0) / scale, 1.0)) * quality, 2.0);
#else
return pow(1.0 - saturate(dot(max(0, sh - sh0), 1.0)) * quality, 2.0);
#endif
}
#endifPhase 7: Add compile-time performance tuning defines Add at the top of the file after the includes: // Performance tuning defines - can be adjusted per platform/quality setting
#ifndef TERRAIN_POM_MAX_STEPS
#define TERRAIN_POM_MAX_STEPS 16 // Reduce to 8 for lower-end hardware
#endif
#ifndef TERRAIN_SHADOW_MAX_SAMPLES
#define TERRAIN_SHADOW_MAX_SAMPLES 4 // Reduce to 2 for lower-end hardware
#endif
#ifndef TERRAIN_ADAPTIVE_QUALITY
#define TERRAIN_ADAPTIVE_QUALITY 1 // Set to 0 to disable adaptive quality
#endif
#ifndef TERRAIN_STOCHASTIC_MIP_THRESHOLD
#define TERRAIN_STOCHASTIC_MIP_THRESHOLD 3.0 // Disable stochastic at mip > this value
#endif
// Performance override macros
#if TERRAIN_ADAPTIVE_QUALITY
#define APPLY_ADAPTIVE_LIMITS(maxSteps, maxShadows, quality) \
if (quality <= QUALITY_MEDIUM) { \
maxSteps = min(maxSteps / 2, TERRAIN_POM_MAX_STEPS / 2); \
maxShadows = min(maxShadows / 2, TERRAIN_SHADOW_MAX_SAMPLES / 2); \
}
#else
#define APPLY_ADAPTIVE_LIMITS(maxSteps, maxShadows, quality)
#endifPhase 8: Remove all duplicate functions and clean up Delete the following duplicate function implementations:
Update the GetParallaxCoords function to use the performance tuning defines: // In GetParallaxCoords, update the step calculation:
uint maxSteps = min(TERRAIN_POM_MAX_STEPS, SharedData::InInterior ? 8 : 16);
uint numSteps = uint((maxSteps * (1.0 - nearBlendToFar)) + 0.5);
// Apply adaptive quality limits
APPLY_ADAPTIVE_LIMITS(maxSteps, unusedShadows, quality);
numSteps = clamp((numSteps + 3) & ~0x03, 4, maxSteps);Phase 9: Add performance monitoring and documentation Add performance documentation at the top of the namespace: namespace ExtendedMaterials
{
/*
* TERRAIN PARALLAX OPTIMIZATION SUMMARY
* ====================================
*
* This implementation eliminates massive code duplication and implements
* adaptive quality scaling to dramatically reduce terrain sampling costs.
*
* Key Optimizations:
* 1. Unified Functions: Collapsed 4 variants each of GetTerrainHeight,
* GetParallaxCoords, and GetParallaxSoftShadowMultiplierTerrain into
* single implementations (~70% code reduction)
*
* 2. Adaptive Quality System:
* - QUALITY_LOW: Single sample on dominant layer only
* - QUALITY_MEDIUM: Sample layers with >10% weight, reduced stochastic
* - QUALITY_HIGH: Full sampling with 2-sample stochastic
* - QUALITY_ULTRA: Full 3-sample stochastic sampling
*
* 3. POM Loop Optimization:
* - Interleaved sampling reduces work by 50% on average
* - Quality-based sample reduction (1-4 samples vs always 4)
* - Early mip-level thresholds disable expensive stochastic sampling
*
* 4. Shadow Optimization:
* - Adaptive shadow sample count (1-4 vs always 4)
* - Quality-based ray sampling
*
* Expected Performance Gains:
* - Distance > 1024: 75% fewer samples (18 -> 4.5 avg)
* - Distance > 512: 50% fewer samples (18 -> 9 avg)
* - Distance > 256: 25% fewer samples (18 -> 13.5 avg)
* - POM iterations: 30-60% fewer GetTerrainHeight calls
* - Shader cache: Better coherency due to unified code paths
*
* Compile-time Tuning:
* - TERRAIN_POM_MAX_STEPS: Cap POM iterations (default 16)
* - TERRAIN_SHADOW_MAX_SAMPLES: Cap shadow samples (default 4)
* - TERRAIN_ADAPTIVE_QUALITY: Enable/disable adaptive system (default 1)
*/Phase 10: Final validation and testing recommendations Add debug helpers and testing recommendations: // Debug visualization helpers (enable for development builds)
#ifdef TERRAIN_DEBUG_VISUALIZATION
inline float3 VisualizeTerrainQuality(uint quality)
{
switch(quality) {
case QUALITY_LOW: return float3(1, 0, 0); // Red - lowest quality
case QUALITY_MEDIUM: return float3(1, 1, 0); // Yellow - medium quality
case QUALITY_HIGH: return float3(0, 1, 0); // Green - high quality
case QUALITY_ULTRA: return float3(0, 0, 1); // Blue - ultra quality
default: return float3(1, 0, 1); // Magenta - error
}
}
inline float3 VisualizeSampleCount(uint sampleCount)
{
// Heat map: Blue (few samples) -> Red (many samples)
float intensity = saturate(sampleCount / 18.0);
return lerp(float3(0, 0, 1), float3(1, 0, 0), intensity);
}
#endif
/*
* TESTING RECOMMENDATIONS:
*
* 1. Performance Testing:
* - Use GPU profilers to measure texture sample reduction
* - Test with/without TERRAIN_ADAPTIVE_QUALITY
* - Measure frame time in terrain-heavy scenes
*
* 2. Quality Validation:
* - Enable TERRAIN_DEBUG_VISUALIZATION to see quality levels
* - Compare visual output with original implementation
* - Test edge cases: uniform terrain, high-frequency detail
*
* 3. Platform Tuning:
* - Mobile: TERRAIN_POM_MAX_STEPS=8, TERRAIN_SHADOW_MAX_SAMPLES=2
* - Console: Default settings
* - PC High-end: Increase thresholds for quality
*
* 4. Shader Variant Testing:
* - Test all combinations: TRUE_PBR/non-PBR × TERRAIN_VARIATION/none
* - Verify unified functions handle all cases correctly
* - Check compile-time optimization effectiveness
*/
|
2 participants
Just a draft for now so people can easier look at my code.
Some help with optimisation would be appreciated. Any attempts I have made to successfully reduce sample count has done little to nothing.
I will look further into optimising before nexus / when I can.
Outperforms dev by around 5%
The +1000 -600 should change dramatically between now and final. Lots to cut from EMAT before it is done.
Changes from dev:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores