Skip to content

perf: rewrite landscape rendering#1959

Closed
davo0411 wants to merge 58 commits into
community-shaders:devfrom
davo0411:tv-opt-2026-2
Closed

perf: rewrite landscape rendering#1959
davo0411 wants to merge 58 commits into
community-shaders:devfrom
davo0411:tv-opt-2026-2

Conversation

@davo0411
Copy link
Copy Markdown
Collaborator

@davo0411 davo0411 commented Mar 7, 2026

full rewrite of lighting terrain layers, emat, parallax, terrain variation.
Done as a wholistic rewrite so that each file can reuse the same helpers and systems to reduce line count and increase perf

Summary by CodeRabbit

  • New Features

    • Mip-level compensation for upscaler render scales, unified terrain-sampling fallback for compatibility, and improved stochastic/height-aware sampling for more accurate texture blending and shadows.
  • Refactor

    • Consolidated terrain sampling into a mask-driven pathway, added a single-active-layer fast path, and simplified parallax/shadow sampling flows.
  • Documentation

    • Updated comments to reflect unified sampling and upscaler compensation.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 7, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Refactors terrain sampling into a unified, mask-driven stochastic sampling API: adds SampleHeightUnified and ComputeActiveMask, replaces per-branch dx/dy with an activeMask + StochasticOffsets/Gradients, adds mipLevel compensation, and provides sampling stubs for absent Terrain Variation.

Changes

Cohort / File(s) Summary
Extended Materials
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
Added SampleHeightUnified and ComputeActiveMask; changed GetTerrainHeight/GetParallaxCoords signatures to accept uint activeMask and StochasticOffsets; added mipLevel compensation (+= SharedData::MipBias) and single-active-layer fast path; replaced per-branch dx/dy sampling with mask-driven logic.
Terrain Variation
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
Reworked stochastic sampling API: removed dx/dy from StochasticSampleLOD/StochasticEffect/StochasticEffectParallax, added SampleTerrain, introduced HEIGHT_INFLUENCE and LOW_WEIGHT_THRESHOLD, reorganized offsets/weights computation and sampling flow.
Lighting / Shader stubs
package/Shaders/Lighting.hlsl
Added stub types StochasticOffsets/StochasticGradients and stub implementations for ComputeStochasticOffsets*, StochasticSampleLOD, StochasticEffectParallax, and SampleTerrain so sampling calls compile/use a unified interface when Terrain Variation is absent.
Constants & Flags
.../ExtendedMaterials.hlsli, .../TerrainVariation.hlsli, .../Lighting.hlsl
Introduced STOCHASTIC_HEIGHT_BOOST = 1.3, STOCHASTIC_SHADOW_GAMMA = 0.8, HEIGHT_INFLUENCE, LOW_WEIGHT_THRESHOLD; updated conditional compilation to centralize terrain-variation gating and stub fallbacks.

Sequence Diagram(s)

sequenceDiagram
    participant VS as Vertex Shader
    participant PS as Pixel Shader
    participant TV as TerrainVariation
    participant EM as ExtendedMaterials
    participant TX as Texture/Atlas

    rect rgba(200,200,255,0.5)
    VS->>TV: ComputeStochasticOffsets(landscapeUV)
    VS->>TV: ComputeStochasticGradients(uv)
    VS->>PS: pass sharedOffset, sharedGrad
    end

    rect rgba(200,255,200,0.5)
    PS->>EM: ComputeActiveMask(weights)
    PS->>EM: Compute mipLevel (+ SharedData::MipBias)
    PS->>EM: SampleHeightUnified(tex,samp,coords,mipLevel,sharedOffset)
    EM->>TX: SampleLevel / SampleBias (texture fetch)
    EM-->>PS: heights, weights
    end

    rect rgba(255,200,200,0.5)
    PS->>TV: SampleTerrain(enabled, tex, samp, uv, offsets, layerWeight)
    TV-->>PS: stochastic or fallback sample
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • alandtse
  • doodlum
  • jiayev

Poem

"I hop through offsets, soft and spry,
I nudge the mips so textures lie.
One mask, six layers, a subtle tune,
Shadows hum low beneath the moon.
A rabbit claps — the hills sing soon!"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'perf: rewrite landscape rendering' partially describes the main changes but is overly broad and generic relative to the actual scope. Consider a more specific title like 'perf: optimize terrain variation with importance sampling' to better reflect the core changes in terrain layer sampling and parallax improvements.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 7, 2026

No actionable suggestions for changed features.

@davo0411 davo0411 marked this pull request as ready for review March 7, 2026 10:17
davo0411 and others added 2 commits March 7, 2026 20:18
Automated formatting by clang-format, prettier, and other hooks.
See https://pre-commit.ci for details.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
package/Shaders/Lighting.hlsl (1)

1206-1213: Consider combining initialization for clarity.

The initialization is correct, but lines 1207 and 1209 could be combined for brevity:

-	bool useTerrainVariation = false;
	StochasticOffsets sharedOffset = (StochasticOffsets)0;
-	useTerrainVariation = SharedData::terrainVariationSettings.enableTilingFix;
+	bool useTerrainVariation = SharedData::terrainVariationSettings.enableTilingFix;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package/Shaders/Lighting.hlsl` around lines 1206 - 1213, Combine the
declaration and initialization for clarity: replace the separate declarations of
useTerrainVariation and sharedOffset followed by setting useTerrainVariation =
SharedData::terrainVariationSettings.enableTilingFix; with a single statement
that initializes useTerrainVariation from
SharedData::terrainVariationSettings.enableTilingFix, and then conditionally
assigns sharedOffset via ComputeStochasticOffsets(input.TexCoord0.zw) inside the
existing [branch] if (useTerrainVariation) block; keep the type names (bool
useTerrainVariation, StochasticOffsets sharedOffset) and the call to
ComputeStochasticOffsets unchanged.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)

131-137: Consider using offsetsLOD.weights for the blend factor.

The hardcoded 0.65 matches offsetsLOD.weights.x from ComputeStochasticOffsetsLOD, but using the struct value would improve maintainability:

♻️ Suggested refactor
 inline float4 StochasticSampleLOD(float rnd, Texture2D tex, SamplerState samp, float2 uv, StochasticOffsets offsetsLOD)
 {
 	float2 dir1 = float2(rnd - 0.5, frac(rnd * 1.618) - 0.5);
 	float4 s1 = tex.SampleBias(samp, uv + (offsetsLOD.offset1 + dir1) * 0.01, SharedData::MipBias);
 	float4 s2 = tex.SampleBias(samp, uv + (offsetsLOD.offset2 + float2(dir1.y, -dir1.x)) * 0.01, SharedData::MipBias);
-	return lerp(s2, s1, 0.65);
+	return lerp(s2, s1, offsetsLOD.weights.x);
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@features/Terrain` Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
around lines 131 - 137, In StochasticSampleLOD, replace the hardcoded blend
factor 0.65 with the corresponding value from the offsetsLOD struct (use
offsetsLOD.weights.x) so the blending matches ComputeStochasticOffsetsLOD;
update the lerp call in StochasticSampleLOD to use offsetsLOD.weights.x as the
third parameter to keep the weighting driven by the offsetsLOD data rather than
a magic constant.

144-155: Verify division safety in weight calculation.

The rcp(w1 + w2) at line 154 could be problematic if both weights are very small. Given that:

  • offsets.weights.x >= offsets.weights.y >= offsets.weights.z (sorted)
  • Barycentric weights sum to 1.0
  • weights.x should always be >= 1/3 after sorting

This should be safe in practice, but consider adding a small epsilon for robustness:

🛡️ Optional defensive fix
-	return lerp(s2, s1, w1 * rcp(w1 + w2));
+	return lerp(s2, s1, w1 * rcp(max(w1 + w2, 1e-5)));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@features/Terrain` Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
around lines 144 - 155, The weight normalization can divide by a near-zero sum;
in StochasticEffect compute the denominator as (w1 + w2) guarded by a small
epsilon and use that safe denominator for rcp to avoid infinities/NaNs — e.g.
define a small constant EPS (or reuse an existing epsilon) and replace rcp(w1 +
w2) with rcp(max(w1 + w2, EPS)) (referencing StochasticEffect, local variables
w1/w2 and HEIGHT_INFLUENCE) so the lerp call uses a stable normalized weight.
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)

179-189: Single-layer fast path - verify height scaling behavior.

The fast path optimization is a good performance improvement. However, note that when only one layer is active, layerWeights[layerIdx] should be close to 1.0 after weight normalization (line 1193-1196 in Lighting.hlsl). The multiplication heights[layerIdx] * layerWeights[layerIdx] may produce slightly different results than the full ProcessTerrainHeightWeights path due to the height-blending power adjustments.

Consider whether this is intentional behavior or if the fast path should simply return heights[layerIdx] when the single layer's weight is effectively 1.0.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli around lines 179 -
189, The single-layer fast path currently returns heights[layerIdx] *
layerWeights[layerIdx], which can differ from the full
ProcessTerrainHeightWeights behavior when layerWeights is effectively 1.0;
modify the fast-path in the block guarded by activeMask countbits==1 (using
variables activeMask, layerIdx, layerWeights, heights) to detect when
layerWeights[layerIdx] is effectively 1.0 (e.g., >= 0.999) and in that case
return heights[layerIdx] (applying the same
SharedData::terrainVariationSettings.enableTilingFix multiplier if present),
otherwise keep the existing heights[layerIdx] * layerWeights[layerIdx] path so
behavior matches the full weighting path when weights are not fully normalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Around line 179-189: The single-layer fast path currently returns
heights[layerIdx] * layerWeights[layerIdx], which can differ from the full
ProcessTerrainHeightWeights behavior when layerWeights is effectively 1.0;
modify the fast-path in the block guarded by activeMask countbits==1 (using
variables activeMask, layerIdx, layerWeights, heights) to detect when
layerWeights[layerIdx] is effectively 1.0 (e.g., >= 0.999) and in that case
return heights[layerIdx] (applying the same
SharedData::terrainVariationSettings.enableTilingFix multiplier if present),
otherwise keep the existing heights[layerIdx] * layerWeights[layerIdx] path so
behavior matches the full weighting path when weights are not fully normalized.

In `@features/Terrain` Variation/Shaders/TerrainVariation/TerrainVariation.hlsli:
- Around line 131-137: In StochasticSampleLOD, replace the hardcoded blend
factor 0.65 with the corresponding value from the offsetsLOD struct (use
offsetsLOD.weights.x) so the blending matches ComputeStochasticOffsetsLOD;
update the lerp call in StochasticSampleLOD to use offsetsLOD.weights.x as the
third parameter to keep the weighting driven by the offsetsLOD data rather than
a magic constant.
- Around line 144-155: The weight normalization can divide by a near-zero sum;
in StochasticEffect compute the denominator as (w1 + w2) guarded by a small
epsilon and use that safe denominator for rcp to avoid infinities/NaNs — e.g.
define a small constant EPS (or reuse an existing epsilon) and replace rcp(w1 +
w2) with rcp(max(w1 + w2, EPS)) (referencing StochasticEffect, local variables
w1/w2 and HEIGHT_INFLUENCE) so the lerp call uses a stable normalized weight.

In `@package/Shaders/Lighting.hlsl`:
- Around line 1206-1213: Combine the declaration and initialization for clarity:
replace the separate declarations of useTerrainVariation and sharedOffset
followed by setting useTerrainVariation =
SharedData::terrainVariationSettings.enableTilingFix; with a single statement
that initializes useTerrainVariation from
SharedData::terrainVariationSettings.enableTilingFix, and then conditionally
assigns sharedOffset via ComputeStochasticOffsets(input.TexCoord0.zw) inside the
existing [branch] if (useTerrainVariation) block; keep the type names (bool
useTerrainVariation, StochasticOffsets sharedOffset) and the call to
ComputeStochasticOffsets unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8f051fc9-9a34-4b4b-a9e8-fcf1d9de8f29

📥 Commits

Reviewing files that changed from the base of the PR and between 783f502 and 204d79b.

📒 Files selected for processing (3)
  • features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
  • features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
  • package/Shaders/Lighting.hlsl

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (2)

160-169: Consider the fast-path weight multiplication edge case.

The fast path applies different logic depending on whether the layer weight is ≥0.999:

float total = layerWeights[layerIdx] >= 0.999 ? heights[layerIdx] : heights[layerIdx] * layerWeights[layerIdx];

When a layer weight crosses the 0.999 threshold (e.g., transitioning from 0.998 to 1.0), the multiplication factor jumps discontinuously from heights * 0.998 to heights * 1.0. This is a ~0.2% discontinuity, which may be imperceptible in practice, but worth verifying there are no visible seams.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli around lines 160 -
169, The ternary in the single-layer fast path creates a discontinuous jump at
the 0.999 threshold (in the block that uses activeMask, firstbitlow,
layerWeights and heights), which can produce seams; replace the abrupt branch
with a continuous computation: either always use heights[layerIdx] *
layerWeights[layerIdx] (and then apply the tiling fix via
SharedData::terrainVariationSettings.enableTilingFix) or implement a small
smooth blend (e.g., a smoothstep or linear lerp over a narrow band near 0.999)
between heights[layerIdx] * weight and heights[layerIdx] to remove the
discontinuity. Ensure you update the code in the fast-path block that currently
sets total using the ternary and preserve the tilingFix multiplication.

185-211: Inconsistent branching hints between layers 0-2 and 3-5.

Layers 0–2 use plain if (activeMask & Xu) while layers 3–5 use [branch] if ((activeMask & Xu) && ...) with else if. This inconsistency could affect shader compilation behavior and readability.

Consider unifying the style — either all use [branch] for consistency with the displacement flag checks, or all use plain if if the compiler handles it well.

Example unified style (using [branch] throughout)
-		if (activeMask & 1u) {
+		[branch] if (activeMask & 1u) {
 			[branch] if ((Permutation::ExtraFeatureDescriptor & Permutation::ExtraFeatureFlags::THLand0HasDisplacement) != 0)
 				heights[0] = ScaleDisplacement(SampleHeightUnified(TexLandTHDisp0Sampler, SampTerrainParallaxSampler, coords, mipLevels[0], sharedOffset).x, params[0]);
 			else heights[0] = ScaleDisplacement(SampleHeightUnified(TexColorSampler, SampTerrainParallaxSampler, coords, mipLevels[0], sharedOffset).w, params[0]);
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli around lines 185 -
211, The branching hints are inconsistent: height sampling for layers 0–2 uses
plain "if (activeMask & ...)" while layers 3–5 use "[branch] if ((activeMask &
...) && (Permutation::ExtraFeatureDescriptor & ...))" with else-if; unify them
by applying the same branch hint style across all layers. Update the checks that
set heights[0..2] to use the [branch] attribute (or remove [branch] from 3..5 if
you prefer the opposite) and ensure the conditional structure matches the
pattern used by SampleHeightUnified/ScaleDisplacement calls and
Permutation::ExtraFeatureFlags checks so each layer (heights[0]..heights[5])
consistently tests activeMask and the THLand#HasDisplacement flag before
choosing the TexLandTHDisp#Sampler vs TexLandColor#Sampler path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 944-970: Remove the duplicated conditional block that re-declares
the TerrainVariation include guard and the stub structs/functions
(StochasticOffsets, StochasticGradients, ComputeStochasticOffsets,
ComputeStochasticGradients, ComputeStochasticOffsetsLOD, StochasticSampleLOD,
StochasticEffectParallax, SampleTerrain) — keep the original definitions already
present earlier and delete the second copy to avoid confusion; ensure only the
first occurrence of the `#if` defined(TERRAIN_VARIATION) ... `#endif` block remains.

---

Nitpick comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Around line 160-169: The ternary in the single-layer fast path creates a
discontinuous jump at the 0.999 threshold (in the block that uses activeMask,
firstbitlow, layerWeights and heights), which can produce seams; replace the
abrupt branch with a continuous computation: either always use heights[layerIdx]
* layerWeights[layerIdx] (and then apply the tiling fix via
SharedData::terrainVariationSettings.enableTilingFix) or implement a small
smooth blend (e.g., a smoothstep or linear lerp over a narrow band near 0.999)
between heights[layerIdx] * weight and heights[layerIdx] to remove the
discontinuity. Ensure you update the code in the fast-path block that currently
sets total using the ternary and preserve the tilingFix multiplication.
- Around line 185-211: The branching hints are inconsistent: height sampling for
layers 0–2 uses plain "if (activeMask & ...)" while layers 3–5 use "[branch] if
((activeMask & ...) && (Permutation::ExtraFeatureDescriptor & ...))" with
else-if; unify them by applying the same branch hint style across all layers.
Update the checks that set heights[0..2] to use the [branch] attribute (or
remove [branch] from 3..5 if you prefer the opposite) and ensure the conditional
structure matches the pattern used by SampleHeightUnified/ScaleDisplacement
calls and Permutation::ExtraFeatureFlags checks so each layer
(heights[0]..heights[5]) consistently tests activeMask and the
THLand#HasDisplacement flag before choosing the TexLandTHDisp#Sampler vs
TexLandColor#Sampler path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e3966223-9ab2-4758-95b6-a0eaa35122ca

📥 Commits

Reviewing files that changed from the base of the PR and between 204d79b and 0cf7a4c.

📒 Files selected for processing (3)
  • features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
  • features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
  • package/Shaders/Lighting.hlsl

Comment thread package/Shaders/Lighting.hlsl Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
package/Shaders/Lighting.hlsl (1)

1205-1212: Redundant initialization of useTerrainVariation.

The variable is initialized to false on line 1206 and immediately reassigned on line 1208. Consider combining into a single declaration.

♻️ Simplified initialization
-	bool useTerrainVariation = false;
-	StochasticOffsets sharedOffset = (StochasticOffsets)0;
-	useTerrainVariation = SharedData::terrainVariationSettings.enableTilingFix;
+	bool useTerrainVariation = SharedData::terrainVariationSettings.enableTilingFix;
+	StochasticOffsets sharedOffset = (StochasticOffsets)0;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package/Shaders/Lighting.hlsl` around lines 1205 - 1212, Combine the
redundant declaration and assignment by declaring useTerrainVariation once and
initializing it from SharedData::terrainVariationSettings.enableTilingFix; also
ensure sharedOffset (type StochasticOffsets) is initialized consistently before
the [branch] block and only set via ComputeStochasticOffsets(input.TexCoord0.zw)
when useTerrainVariation is true — update the lines that declare
useTerrainVariation and sharedOffset near the ComputeStochasticOffsets call to
remove the unnecessary initial false assignment.
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)

185-211: Inconsistent branching pattern between layers 0-2 and layers 3-5.

Layers 0-2 use if (activeMask & Xu) { [branch] if (hasDisplacement) ... else ... } while layers 3-5 use [branch] if ((activeMask & Xu) && hasDisplacement) ... else if (activeMask & Xu) ....

Both patterns are functionally correct, but the inconsistency may cause slightly different shader compilation behavior. Consider unifying the pattern for consistency, though this is a minor style concern.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli around lines 185 -
211, The branching pattern is inconsistent between layers 0–2 and 3–5; update
layers 3–5 to match the outer-mask-first style used for 0–2. For the blocks that
set heights[3], heights[4], heights[5], wrap each in if (activeMask & Xu) {
[branch] if ((Permutation::ExtraFeatureDescriptor &
Permutation::ExtraFeatureFlags::THLand3HasDisplacement) != 0) heights[3] =
ScaleDisplacement(SampleHeightUnified(TexLandTHDisp3Sampler, ...).x, params[3]);
else heights[3] = ScaleDisplacement(SampleHeightUnified(TexLandColor4Sampler,
...).w, params[3]); } (and similarly use
THLand4HasDisplacement/THLand5HasDisplacement,
TexLandTHDisp4Sampler/TexLandTHDisp5Sampler and
TexLandColor5Sampler/TexLandColor6Sampler) so the control flow matches the
pattern used by heights[0..2] and uses the same
SampleHeightUnified/ScaleDisplacement calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Around line 185-211: The branching pattern is inconsistent between layers 0–2
and 3–5; update layers 3–5 to match the outer-mask-first style used for 0–2. For
the blocks that set heights[3], heights[4], heights[5], wrap each in if
(activeMask & Xu) { [branch] if ((Permutation::ExtraFeatureDescriptor &
Permutation::ExtraFeatureFlags::THLand3HasDisplacement) != 0) heights[3] =
ScaleDisplacement(SampleHeightUnified(TexLandTHDisp3Sampler, ...).x, params[3]);
else heights[3] = ScaleDisplacement(SampleHeightUnified(TexLandColor4Sampler,
...).w, params[3]); } (and similarly use
THLand4HasDisplacement/THLand5HasDisplacement,
TexLandTHDisp4Sampler/TexLandTHDisp5Sampler and
TexLandColor5Sampler/TexLandColor6Sampler) so the control flow matches the
pattern used by heights[0..2] and uses the same
SampleHeightUnified/ScaleDisplacement calls.

In `@package/Shaders/Lighting.hlsl`:
- Around line 1205-1212: Combine the redundant declaration and assignment by
declaring useTerrainVariation once and initializing it from
SharedData::terrainVariationSettings.enableTilingFix; also ensure sharedOffset
(type StochasticOffsets) is initialized consistently before the [branch] block
and only set via ComputeStochasticOffsets(input.TexCoord0.zw) when
useTerrainVariation is true — update the lines that declare useTerrainVariation
and sharedOffset near the ComputeStochasticOffsets call to remove the
unnecessary initial false assignment.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 30fe9d89-d1ca-4953-a1fa-ec78b51aca5e

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf7a4c and 29b0fa3.

📒 Files selected for processing (3)
  • features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
  • features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli
  • package/Shaders/Lighting.hlsl

Copilot AI review requested due to automatic review settings March 31, 2026 09:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors terrain variation sampling and integrates it more tightly with landscape/parallax paths to reduce tiling artifacts while improving performance (importance sampling / fewer samples) and keeping parallax/shadows consistent across render scales.

Changes:

  • Updated Lighting.hlsl landscape sampling to use unified SampleTerrain(...) helpers and centralized stochastic offset computation (with stub fallbacks when TV isn’t compiled for a permutation).
  • Reworked TerrainVariation.hlsli to a 2-sample stochastic approach with weight-sorted offsets and updated LOD/parallax sampling.
  • Refactored ExtendedMaterials.hlsli terrain-parallax height/shadow sampling into a unified pathway, added upscaler mip compensation, and introduced an active-layer mask + single-layer fast path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
package/Shaders/Lighting.hlsl Integrates new terrain variation sampling/fallbacks into LANDSCAPE/LOD terrain sampling and EMAT terrain parallax/shadows.
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli Rewrites terrain variation sampling to 2-sample importance-style blending and updates parallax/LOD sampling helpers.
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli Unifies terrain height sampling + shadowing, adds mip compensation for upscalers, and adds active-layer optimizations.
Comments suppressed due to low confidence (1)

features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:403

  • dot(max(0, sh - sh0), 1.0) passes a scalar as the second operand. HLSL dot expects both operands to have the same dimension, and even if this compiles via implicit splat it changes semantics to summing all 4 taps (quickly saturating). Use an explicit float4 weight (e.g., average/desired weighting) to avoid compilation/logic issues.
			float4 sh = 0;
			sh = AdjustDisplacementNormalized(tex.SampleLevel(texSampler, coords + rayDir * multipliers.x, mipLevel)[channel], params);
			if (quality > 0.25)
				sh.y = AdjustDisplacementNormalized(tex.SampleLevel(texSampler, coords + rayDir * multipliers.y, mipLevel)[channel], params);
			if (quality > 0.5)
				sh.z = AdjustDisplacementNormalized(tex.SampleLevel(texSampler, coords + rayDir * multipliers.z, mipLevel)[channel], params);

Comment thread features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli Outdated
davo0411 and others added 13 commits March 31, 2026 20:02
Automated formatting by clang-format, prettier, and other hooks.
See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks.
See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks.
See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks.
See https://pre-commit.ci for details.
@davo0411 davo0411 changed the title perf: terrain variation and parallax perf: rewrite landscape rendering Apr 12, 2026
@davo0411 davo0411 changed the title perf: rewrite landscape rendering feat: rewrite landscape rendering Apr 12, 2026
@davo0411
Copy link
Copy Markdown
Collaborator Author

@copilot resolve the merge conflicts in this pull request

@community-shaders community-shaders deleted a comment from Copilot AI Apr 14, 2026
@doodlum doodlum changed the title feat: rewrite landscape rendering perf: rewrite landscape rendering Apr 18, 2026
@davo0411 davo0411 closed this Apr 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants