perf: terrain variation & parallax#2220
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors terrain/landscape parallax and stochastic sampling: splits ExtendedMaterials into parallax core and terrain headers, reduces TerrainVariation sampling from 3→2 samples, removes stochastic mip noise from GetMipLevel, adds parallax/shadow tunables and terrain shadow helpers, consolidates per-layer blending, and removes the CPU ExtendShadows setting and its UI wiring. ChangesExtended Terrain & Parallax Refactor
Sequence Diagram(s)sequenceDiagram
participant PS as PixelShader (EMAT)
participant EM as ExtendedMaterials Parallax Core
participant ET as ExtendedMaterials Terrain
participant TV as TerrainVariation (stochastic offsets)
participant Tex as Terrain Textures
participant CPU as CPU Settings
PS->>EM: GetMipLevel(uv, parallaxTex)
EM-->>PS: mipLevel (deterministic)
PS->>TV: ComputeStochasticOffsets(sharedUV) [if TERRAIN_VARIATION]
TV-->>PS: sharedOffset
PS->>ET: InitializeTerrainMipLevels(coords)
ET-->>PS: mipLevels[6]
PS->>EM: GetParallaxCoords(..., mipLevels, viewDir, tbn, noise, params, sharedOffset)
EM->>Tex: sample displacement/color (stochastic or SampleLevel)
Tex-->>EM: height samples
EM->>EM: ray-march & secant refine -> pixelOffset, parallax UV
EM-->>PS: parallax UV, pixelOffset, hasPOM
PS->>ET: ComputeTerrainParallaxShadowBaseHeight(..., sharedOffset)
ET->>Tex: sample heights along light dir
Tex-->>ET: shadow samples
ET-->>PS: terrain parallax shadow multiplier
CPU->>PS: shared settings (parallax/shadow quality, EnableShadows)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
No actionable suggestions for changed features. |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
package/Shaders/Common/LightingLandscape.hlsli (1)
126-126: Align closing header-guard comment with macro nameThe closing comment references
__LANDSCAPE_LAYERS_HLSLI__, but the actual guard is__LIGHTING_LANDSCAPE_HLSLI__. Consider syncing for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/LightingLandscape.hlsli` at line 126, The closing header-guard comment is mismatched: update the comment after `#endif` to match the actual macro __LIGHTING_LANDSCAPE_HLSLI__ (replace __LANDSCAPE_LAYERS_HLSLI__ with __LIGHTING_LANDSCAPE_HLSLI__), ensuring the comment aligns with the guard macro used at the top of this file.features/Terrain Variation/Shaders/Features/TerrainVariation.ini (1)
1-2: Consider adding a linked issue keyword in the PR descriptionIf this PR implements tracked perf/feature work, add a reference like
Addresses #<id>for traceability.As per coding guidelines, feature PRs should include issue references using keywords such as “Addresses
#123” / “Implements#123”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Terrain` Variation/Shaders/Features/TerrainVariation.ini around lines 1 - 2, This PR is missing an issue reference for traceability—update the PR description to include a keywords-style reference like "Addresses #<issue-id>" or "Implements #<issue-id>" with the actual issue number; additionally, add a RelatedIssue field under the [Info] section in TerrainVariation.ini (e.g., add "RelatedIssue = `#123`" next to "Version = 1-1-0") so the feature (TerrainVariation.ini / [Info] / Version) explicitly records the linked issue.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
23-27: PR metadata follow-up: add scope in title and link tracking issue.Suggested title:
perf(extended-materials): optimize terrain parallax path
If this maps to a tracked task, appendAddresses #<id>(orImplements #<id>) in the PR description.As per coding guidelines, "When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... and Issue References ..."
🤖 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 23 - 27, Update the PR title to include scope and the suggested conventional-commit format — e.g. change it to "perf(extended-materials): optimize terrain parallax path" — and add an issue reference in the PR description like "Addresses #<id>" (or "Implements #<id>") if this change maps to a tracked task; note this PR touches constants such as ParallaxCheapDistance, ParallaxNearShadowQuality, ParallaxFarShadowQuality, and TerrainParallaxShadowMaxMipLevel so include the issue link and short summary referencing the terrain parallax/extended-materials change in the description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Around line 127-139: Normalize guard: after summing weights[] into wsum, avoid
calling rcp(wsum) when wsum is zero/near-zero by computing invwsum =
rcp(max(wsum, 1e-6)) (or choose a small epsilon) and then multiply weights[k] by
invwsum; update both occurrences that use rcp(wsum) (the block that references
heightBlend, weights, wsum, invwsum and the similar block later in the file) so
degenerate/invalid blends no longer produce INF/NaN.
In `@package/Shaders/Common/LightingLandscape.hlsli`:
- Around line 91-94: The non-full-PBR branch double-applies weight by
multiplying landRMAOS with weight at assignment and again when accumulating into
blendedRMAOS; change the assignment of landRMAOS inside that branch to not
multiply by weight (e.g., set landRMAOS = float4(1 - glossiness.x, 0, 1, 0)) and
keep the accumulation blendedRMAOS += landRMAOS * weight so each layer’s RMAOS
is weighted exactly once (references: landRMAOS, blendedRMAOS, blendedRGB,
landColorRGB, glossiness.x, weight).
---
Nitpick comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Around line 23-27: Update the PR title to include scope and the suggested
conventional-commit format — e.g. change it to "perf(extended-materials):
optimize terrain parallax path" — and add an issue reference in the PR
description like "Addresses #<id>" (or "Implements #<id>") if this change maps
to a tracked task; note this PR touches constants such as ParallaxCheapDistance,
ParallaxNearShadowQuality, ParallaxFarShadowQuality, and
TerrainParallaxShadowMaxMipLevel so include the issue link and short summary
referencing the terrain parallax/extended-materials change in the description.
In `@features/Terrain` Variation/Shaders/Features/TerrainVariation.ini:
- Around line 1-2: This PR is missing an issue reference for traceability—update
the PR description to include a keywords-style reference like "Addresses
#<issue-id>" or "Implements #<issue-id>" with the actual issue number;
additionally, add a RelatedIssue field under the [Info] section in
TerrainVariation.ini (e.g., add "RelatedIssue = `#123`" next to "Version = 1-1-0")
so the feature (TerrainVariation.ini / [Info] / Version) explicitly records the
linked issue.
In `@package/Shaders/Common/LightingLandscape.hlsli`:
- Line 126: The closing header-guard comment is mismatched: update the comment
after `#endif` to match the actual macro __LIGHTING_LANDSCAPE_HLSLI__ (replace
__LANDSCAPE_LAYERS_HLSLI__ with __LIGHTING_LANDSCAPE_HLSLI__), ensuring the
comment aligns with the guard macro used at the top of this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57ffdc06-34ee-4729-b25a-f3f07a561d70
📒 Files selected for processing (10)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Terrain Variation/Shaders/Features/TerrainVariation.inifeatures/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlslipackage/Shaders/Common/LightingLandscape.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslsrc/Features/ExtendedMaterials.cppsrc/Features/ExtendedMaterials.hsrc/Features/TerrainVariation.cppsrc/Features/TerrainVariation.h
💤 Files with no reviewable changes (1)
- src/Features/ExtendedMaterials.cpp
There was a problem hiding this comment.
Pull request overview
This PR restructures landscape/terrain shader paths to reduce branching and improve performance, while reworking Terrain Variation sampling and updating Extended Materials’ parallax/shadow logic (including secant-based refinement).
Changes:
- Refactors
Lighting.hlsllandscape layer blending into a shared include (LightingLandscape.hlsli) and streamlines Terrain Variation integration. - Removes
ExtendShadowssetting and adjusts Extended Materials shader constants/helpers for cheaper parallax + shadow evaluation. - Reworks Terrain Variation stochastic sampling to a consistent 2-sample approach and updates feature settings structs/padding accordingly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Features/TerrainVariation.h | Adjusts shader-define gating and aligns Settings layout to 16 bytes. |
| src/Features/TerrainVariation.cpp | Updates UI toggles to write uint32_t settings and removes vertex-desc “dirty” forcing. |
| src/Features/ExtendedMaterials.h | Removes ExtendShadows and fixes Settings padding/alignment. |
| src/Features/ExtendedMaterials.cpp | Drops JSON/UI handling for ExtendShadows. |
| package/Shaders/Lighting.hlsl | Major landscape/parallax/TV integration changes; adds landscape blend helpers and terrain shadow caching. |
| package/Shaders/Common/SharedData.hlsli | Removes ExtendShadows from shader-visible CPM settings and adjusts padding. |
| package/Shaders/Common/LightingLandscape.hlsli | New shared landscape blending macro include to reduce duplicated per-layer code. |
| features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli | Reworks TV stochastic sampling (2-sample blend, distance/minification fallbacks, LOD jitter helpers). |
| features/Terrain Variation/Shaders/Features/TerrainVariation.ini | Bumps Terrain Variation feature version. |
| features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli | Adds new parallax/shadow helpers, secant refinement, and terrain shadow evaluation helpers. |
|
@coderabbitai full review please be mean like doodlum and no niceties. Point out any magic numbers and sloppily written code. Suggest improvements where you feel appropriate. Suggest ideas to further reduce compile times |
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
🧹 Nitpick comments (7)
features/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlsli (2)
27-33: 💤 Low valueConsider documenting or removing
offset3fromStochasticOffsets.With the transition to a 2-sample model,
offset3is always set to0(lines 101, 118). The field is retained for struct compatibility, but this could confuse future maintainers who might expect it to be used.Consider adding a comment explaining that
offset3is unused (kept for ABI stability), or remove it if no external consumers depend on the struct layout.🤖 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 27 - 33, The struct StochasticOffsets contains an unused field offset3 (always set to zero under the new 2-sample model); update the declaration of StochasticOffsets to either (a) add a clear comment above offset3 explaining it is intentionally unused and kept only for ABI/struct compatibility, referencing the struct name StochasticOffsets and the field offset3, or (b) remove offset3 entirely if you confirm no external consumers rely on the existing layout (search for StochasticOffsets usages before removing). Ensure the chosen change is applied where StochasticOffsets is defined so future maintainers understand why offset3 is present or has been removed.
111-113: 💤 Low valueMagic numbers in hash computation.
Lines 111 and 113 use unexplained constants (
255437.0,127.0) for the LOD hash computation. While these are typical in hash functions, brief comments would help future maintainers understand the rationale (e.g., "large prime for uniform distribution" or "offset for second independent sample").🤖 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 111 - 113, The hash computation uses unexplained magic constants (255437.0 and 127.0) in the cellID and second sample; update the code around the cellID calculation and the hashLOD calls to either replace these literals with named constants (e.g., LARGE_PRIME_FOR_HASH and SECOND_SAMPLE_OFFSET) or add brief comments above the lines mentioning why the values were chosen (e.g., "large prime for uniform distribution" and "offset for independent second sample"), referencing the variables cellID, landscapeUV and the hashLOD(...) calls so maintainers can locate and understand the rationale.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (2)
735-737: 💤 Low valueIncorrect closing comment.
Line 736's comment says
// defined(LANDSCAPE) && defined(TERRAIN_VARIATION)but this#endifcloses the#if defined(LANDSCAPE)block that started at line 346 (and implicitly includes line 584). TheTERRAIN_VARIATIONpart is not required for this guard.♻️ Suggested fix
# undef TERRAIN_HEIGHT_AT -#endif // defined(LANDSCAPE) && defined(TERRAIN_VARIATION) +#endif // defined(LANDSCAPE)🤖 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 735 - 737, The closing preprocessor comment on the `#endif` after the `#undef` TERRAIN_HEIGHT_AT is wrong: it references "defined(LANDSCAPE) && defined(TERRAIN_VARIATION)" but the block being closed is the `#if` defined(LANDSCAPE) block (and not dependent on TERRAIN_VARIATION). Update the comment on that `#endif` to accurately reflect the opening guard (e.g., // defined(LANDSCAPE)) so the `#endif` matches the `#if` that began the LANDSCAPE section; refer to the surrounding symbols (`#if` defined(LANDSCAPE), TERRAIN_VARIATION, `#undef` TERRAIN_HEIGHT_AT) to locate the correct closing directive.
619-628: 💤 Low valueInconsistent tap-count thresholds between functions.
TerrainDirectionalShadowTapCountuses thresholds[0.7, 0.25, 0.0]whileParallaxShadowTapCountuses[0.75, 0.5, 0.25]. The inconsistency may be intentional (directional shadows use fewer taps), but the differing threshold values (0.7 vs 0.75, 0.25 vs 0.5) could benefit from a brief comment explaining the rationale.🤖 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 619 - 628, TerrainDirectionalShadowTapCount currently uses thresholds (0.7, 0.25, 0.0) that differ from ParallaxShadowTapCount's (0.75, 0.5, 0.25); either align the thresholds if this was accidental or add a concise comment above the TerrainDirectionalShadowTapCount definition explaining the intentional rationale (e.g., fewer taps for directional shadows, performance/quality tradeoffs and why 0.7/0.25 were chosen vs 0.75/0.5/0.25), referencing the function names TerrainDirectionalShadowTapCount and ParallaxShadowTapCount so future readers know the difference is deliberate.package/Shaders/Lighting.hlsl (3)
2257-2268: 💤 Low valueVariable
lightDirectionTSshadows outer declaration.Line 2261 redeclares
lightDirectionTSinside the conditional block, but it's already declared and computed at line 2252 with the same expression. This variable shadowing is unnecessary and could confuse maintainers.♻️ Suggested fix
if (LANDSCAPE_PARALLAX_ENABLED) { if (ExtendedMaterials::TerrainHasSignificantBlend(input.LandBlendWeights1, input.LandBlendWeights2.xy) && ExtendedMaterials::TerrainHasAnyDisplacement()) { float terrainHeightScale = ExtendedMaterials::TerrainMaxWeightedHeightScale(input, displacementParams); if (terrainHeightScale > 0.01) { - float3 lightDirectionTS = normalize(mul(refractedLightDirection, tbn).xyz); # if defined(TERRAIN_VARIATION) parallaxShadow = ExtendedMaterials::GetParallaxSoftShadowMultiplierTerrain(input, uv, terrainShadowMipLevels, lightDirectionTS, sh0, terrainDirectionalShadowQuality, screenNoise, displacementParams, sharedOffset); # else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 2257 - 2268, The variable lightDirectionTS is being redeclared inside the LANDSCAPE_PARALLAX_ENABLED branch although it was already computed earlier (using normalize(mul(refractedLightDirection, tbn).xyz);), causing shadowing; remove the local re-declaration and reuse the previously declared lightDirectionTS before calling ExtendedMaterials::GetParallaxSoftShadowMultiplierTerrain (and keep the existing arguments: input, uv, terrainShadowMipLevels, lightDirectionTS, sh0, terrainDirectionalShadowQuality, screenNoise, displacementParams, sharedOffset) so behavior is unchanged.
1025-1029: 💤 Low valueRedundant assignment of
terrainDirectionalShadowQuality.On line 1026,
terrainDirectionalShadowQualityis initialized toparallaxShadowQuality, and immediately on lines 1027-1029, it's conditionally reassigned to the same value whenLANDSCAPEis defined. The second assignment is a no-op.♻️ Suggested simplification
float parallaxShadowQuality = ExtendedMaterials::ParallaxNearShadowQuality; - float terrainDirectionalShadowQuality = parallaxShadowQuality; -# if defined(LANDSCAPE) - terrainDirectionalShadowQuality = ExtendedMaterials::ParallaxNearShadowQuality; -# endif + float terrainDirectionalShadowQuality = ExtendedMaterials::ParallaxNearShadowQuality;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 1025 - 1029, The variable terrainDirectionalShadowQuality is initialized to parallaxShadowQuality and then conditionally reassigned to the same ExtendedMaterials::ParallaxNearShadowQuality inside the LANDSCAPE block, which is redundant; remove the conditional reassignment (the lines inside the `#if` defined(LANDSCAPE) / `#endif` that set terrainDirectionalShadowQuality = ExtendedMaterials::ParallaxNearShadowQuality) so terrainDirectionalShadowQuality remains initialized from parallaxShadowQuality without a no-op repeat, or alternatively move the initial assignment into the conditional and drop the unconditional one—update references to parallaxShadowQuality, terrainDirectionalShadowQuality, and the LANDSCAPE conditional accordingly.
1031-1036: 💤 Low valueConsider documenting implicit macro dependencies.
The
COMPUTE_TERRAIN_SHADOW_BASEandEVAL_TERRAIN_DIR_SHADOWmacros capture multiple variables implicitly (input,uv,terrainShadowMipLevels,terrainDirectionalShadowQuality,screenNoise,displacementParams,sharedOffset). While this reduces duplication, it makes the code harder to reason about.A brief comment listing the expected variables in scope would help maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 1031 - 1036, Add a short comment above the COMPUTE_TERRAIN_SHADOW_BASE and EVAL_TERRAIN_DIR_SHADOW macro definitions that lists the implicit variables those macros expect in scope (e.g., input, uv, terrainShadowMipLevels, terrainDirectionalShadowQuality, screenNoise, displacementParams, sharedOffset) and mention which ExtendedMaterials functions they map to (ExtendedMaterials::ComputeTerrainParallaxShadowBaseHeight and ExtendedMaterials::EvaluateTerrainDirectionalParallaxShadowMultiplier) so future readers can quickly see required context before the macros are used.
🤖 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 735-737: The closing preprocessor comment on the `#endif` after the
`#undef` TERRAIN_HEIGHT_AT is wrong: it references "defined(LANDSCAPE) &&
defined(TERRAIN_VARIATION)" but the block being closed is the `#if`
defined(LANDSCAPE) block (and not dependent on TERRAIN_VARIATION). Update the
comment on that `#endif` to accurately reflect the opening guard (e.g., //
defined(LANDSCAPE)) so the `#endif` matches the `#if` that began the LANDSCAPE
section; refer to the surrounding symbols (`#if` defined(LANDSCAPE),
TERRAIN_VARIATION, `#undef` TERRAIN_HEIGHT_AT) to locate the correct closing
directive.
- Around line 619-628: TerrainDirectionalShadowTapCount currently uses
thresholds (0.7, 0.25, 0.0) that differ from ParallaxShadowTapCount's (0.75,
0.5, 0.25); either align the thresholds if this was accidental or add a concise
comment above the TerrainDirectionalShadowTapCount definition explaining the
intentional rationale (e.g., fewer taps for directional shadows,
performance/quality tradeoffs and why 0.7/0.25 were chosen vs 0.75/0.5/0.25),
referencing the function names TerrainDirectionalShadowTapCount and
ParallaxShadowTapCount so future readers know the difference is deliberate.
In `@features/Terrain` Variation/Shaders/TerrainVariation/TerrainVariation.hlsli:
- Around line 27-33: The struct StochasticOffsets contains an unused field
offset3 (always set to zero under the new 2-sample model); update the
declaration of StochasticOffsets to either (a) add a clear comment above offset3
explaining it is intentionally unused and kept only for ABI/struct
compatibility, referencing the struct name StochasticOffsets and the field
offset3, or (b) remove offset3 entirely if you confirm no external consumers
rely on the existing layout (search for StochasticOffsets usages before
removing). Ensure the chosen change is applied where StochasticOffsets is
defined so future maintainers understand why offset3 is present or has been
removed.
- Around line 111-113: The hash computation uses unexplained magic constants
(255437.0 and 127.0) in the cellID and second sample; update the code around the
cellID calculation and the hashLOD calls to either replace these literals with
named constants (e.g., LARGE_PRIME_FOR_HASH and SECOND_SAMPLE_OFFSET) or add
brief comments above the lines mentioning why the values were chosen (e.g.,
"large prime for uniform distribution" and "offset for independent second
sample"), referencing the variables cellID, landscapeUV and the hashLOD(...)
calls so maintainers can locate and understand the rationale.
In `@package/Shaders/Lighting.hlsl`:
- Around line 2257-2268: The variable lightDirectionTS is being redeclared
inside the LANDSCAPE_PARALLAX_ENABLED branch although it was already computed
earlier (using normalize(mul(refractedLightDirection, tbn).xyz);), causing
shadowing; remove the local re-declaration and reuse the previously declared
lightDirectionTS before calling
ExtendedMaterials::GetParallaxSoftShadowMultiplierTerrain (and keep the existing
arguments: input, uv, terrainShadowMipLevels, lightDirectionTS, sh0,
terrainDirectionalShadowQuality, screenNoise, displacementParams, sharedOffset)
so behavior is unchanged.
- Around line 1025-1029: The variable terrainDirectionalShadowQuality is
initialized to parallaxShadowQuality and then conditionally reassigned to the
same ExtendedMaterials::ParallaxNearShadowQuality inside the LANDSCAPE block,
which is redundant; remove the conditional reassignment (the lines inside the
`#if` defined(LANDSCAPE) / `#endif` that set terrainDirectionalShadowQuality =
ExtendedMaterials::ParallaxNearShadowQuality) so terrainDirectionalShadowQuality
remains initialized from parallaxShadowQuality without a no-op repeat, or
alternatively move the initial assignment into the conditional and drop the
unconditional one—update references to parallaxShadowQuality,
terrainDirectionalShadowQuality, and the LANDSCAPE conditional accordingly.
- Around line 1031-1036: Add a short comment above the
COMPUTE_TERRAIN_SHADOW_BASE and EVAL_TERRAIN_DIR_SHADOW macro definitions that
lists the implicit variables those macros expect in scope (e.g., input, uv,
terrainShadowMipLevels, terrainDirectionalShadowQuality, screenNoise,
displacementParams, sharedOffset) and mention which ExtendedMaterials functions
they map to (ExtendedMaterials::ComputeTerrainParallaxShadowBaseHeight and
ExtendedMaterials::EvaluateTerrainDirectionalParallaxShadowMultiplier) so future
readers can quickly see required context before the macros are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e416c1af-4478-4a0b-a78b-1dd58c13a03b
📒 Files selected for processing (6)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Terrain Variation/Shaders/TerrainVariation/TerrainVariation.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslsrc/Features/TerrainVariation.cppsrc/Features/TerrainVariation.h
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Around line 89-97: InitializeTerrainMipLevels currently derives mip LODs from
albedo samplers (TexColorSampler, TexLandColor2/3/4/5/6Sampler) but
GetTerrainHeight and TRUE_PBR paths sample TexLandDisplacement*Sampler, causing
wrong mip selection for secant march and terrain-shadow sampling; update
InitializeTerrainMipLevels (or add a variant) to call GetMipLevel with the
corresponding TexLandDisplacement2Sampler..TexLandDisplacement6Sampler (and
TexLandDisplacementSampler instead of TexColorSampler) when the
TRUE_PBR/displacement path is active, and ensure callers in the TRUE_PBR
codepath call the displacement-aware initializer so mip chains match the
heightmap reads.
- Around line 562-572: GetTerrainHeight updates the out parameter weights during
each probe but after computing final parallaxAmount (r = saturate(fNear / (fNear
- fFar)); parallaxAmount = lerp(tNear, tFar, r)) the code returns a UV that was
never resampled, leaving weights stale; before returning from this function
(after computing pixelOffset/hasPOM and before "return viewDirTS.xy * offset +
coords.xy;") call GetTerrainHeight with the final UV to refresh the out weights
so Lighting.hlsl receives the correct, final blended weights (use the same input
params and the final UV used for the return).
In `@package/Shaders/Lighting.hlsl`:
- Around line 2257-2265: The terrain-parallax shadow call is using
terrainDirectionalShadowQuality (which forces near-quality for landscape)
causing distant point lights to use the expensive near tap profile; in the
LANDSCAPE_PARALLAX_ENABLED branch replace the quality parameter passed to
ExtendedMaterials::GetParallaxSoftShadowMultiplierTerrain from
terrainDirectionalShadowQuality to parallaxShadowQuality so the distance-based
parallaxShadowQuality is used for point-light terrain shadows while retaining
terrainDirectionalShadowQuality only for the cached directional path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a31016da-05f9-481b-a7cf-1273b6f7afa9
📒 Files selected for processing (2)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslipackage/Shaders/Lighting.hlsl
There was a problem hiding this comment.
♻️ Duplicate comments (3)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli (2)
6-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMip levels derived from albedo textures, but TRUE_PBR samples displacement textures.
InitializeTerrainMipLevelsalways usesTexColorSampler/TexLandColor*Samplerfor mip derivation, butGetTerrainHeightreads fromTexLandDisplacement*SamplerunderTRUE_PBR. If those textures have different dimensions or mip chains, the secant march and terrain-shadow sampling will use incorrect mip levels.🛠️ Proposed fix
void InitializeTerrainMipLevels(float2 coords, out float mipLevels[6]) { +#if defined(TRUE_PBR) + mipLevels[0] = GetMipLevel(coords, TexLandDisplacement0Sampler); + mipLevels[1] = GetMipLevel(coords, TexLandDisplacement1Sampler); + mipLevels[2] = GetMipLevel(coords, TexLandDisplacement2Sampler); + mipLevels[3] = GetMipLevel(coords, TexLandDisplacement3Sampler); + mipLevels[4] = GetMipLevel(coords, TexLandDisplacement4Sampler); + mipLevels[5] = GetMipLevel(coords, TexLandDisplacement5Sampler); +#else mipLevels[0] = GetMipLevel(coords, TexColorSampler); mipLevels[1] = GetMipLevel(coords, TexLandColor2Sampler); mipLevels[2] = GetMipLevel(coords, TexLandColor3Sampler); mipLevels[3] = GetMipLevel(coords, TexLandColor4Sampler); mipLevels[4] = GetMipLevel(coords, TexLandColor5Sampler); mipLevels[5] = GetMipLevel(coords, TexLandColor6Sampler); +#endif }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli around lines 6 - 14, InitializeTerrainMipLevels currently derives mip levels from albedo samplers (TexColorSampler / TexLandColor*Sampler) while GetTerrainHeight under TRUE_PBR samples displacement textures (TexLandDisplacement*Sampler), causing mismatched mipchain usage; update InitializeTerrainMipLevels to choose the correct samplers when TRUE_PBR (use TexLandDisplacementSampler and TexLandDisplacement2/3/4/5/6Sampler) so GetMipLevel is called on the displacement textures that GetTerrainHeight actually samples (or add a boolean/compile-time branch parameter to InitializeTerrainMipLevels to select displacement vs color samplers), ensuring mipLevels[] align with the textures used by GetTerrainHeight and terrain shadow/secant march code.
52-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
rcp(wsum)against zero-sum blends.Lines 52 and 76 use
rcp(wsum)without a zero guard. Degenerate or invalid blend inputs (all weights zero) can propagate INF/NaN into terrain weights.🛠️ Proposed fix
- float invwsum = rcp(wsum); + float invwsum = (wsum > 1e-6) ? rcp(wsum) : 0.0;Apply to both occurrences at lines 52 and 76.
Also applies to: 76-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli at line 52, The use of rcp(wsum) for invwsum can produce INF/NaN when all blend weights are zero; update both occurrences (the variable invwsum computed from wsum at the invwsum assignments) to guard against zero by replacing rcp(wsum) with rcp(max(wsum, epsilon)) or explicitly set invwsum = 0 when wsum is zero (choose a small epsilon like 1e-6), and apply the same change at both places where invwsum is computed so downstream terrain weight math uses a safe reciprocal.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli (1)
196-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWeights remain stale after final secant interpolation.
GetTerrainHeightmutatesweightsduring each probe, but after computing the finalparallaxAmountvialerp(tNear, tFar, r), the returned UV was never re-sampled.Lighting.hlslcopies these stale weights back intoinput.LandBlendWeights*, causing height-blended terrain to drift from the actual parallax coordinate.Consider refreshing weights at the final UV before returning.
🛠️ Proposed fix
float offset = (1.0 - parallaxAmount) * -maxHeight + minHeight; pixelOffset = saturate(parallaxAmount); `#if` defined(VR_STEREO_OPT) hasPOM = true; `#endif` - return viewDirTS.xy * offset + coords.xy; + float2 finalCoords = viewDirTS.xy * offset + coords.xy; +#if defined(LANDSCAPE) + // Refresh weights at final parallax UV so callers get consistent blend weights. + GetTerrainHeight(noise, input, finalCoords, mipLevels, params, blendFactor, w1, w2, sharedOffset, weights); +#endif + return finalCoords;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli around lines 196 - 201, GetTerrainHeight currently mutates the `weights` during each secant probe but the code returns the final UV (computed as coords.xy + viewDirTS.xy * offset after calculating `parallaxAmount = lerp(tNear, tFar, r)`) without re-sampling the terrain weights at that final UV, leaving `weights` stale and later copied back into `input.LandBlendWeights*`. Fix: after computing the final `offset`/final UV (the expression `viewDirTS.xy * offset + coords.xy`) call the terrain weight sampling path used inside `GetTerrainHeight` (or a dedicated sampleWeightsAtUV function) to re-evaluate and overwrite the `weights` array at that UV, then proceed to set `pixelOffset`/hasPOM and return—ensuring `weights` now reflect the returned parallax coordinate so `input.LandBlendWeights*` receives the refreshed values.
🧹 Nitpick comments (4)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli (2)
257-257: 💤 Low valueConsider documenting the bit positions for TERRAIN_DISPLACEMENT_MASK.
The mask
(1u << 6u) | ... | (1u << 11u)corresponds toPBR::TerrainFlags::LandTile0-5HasDisplacement. A brief comment would clarify the mapping.♻️ Suggested documentation
# if defined(TRUE_PBR) + // Bits 6–11: PBR::TerrainFlags::LandTile[0-5]HasDisplacement static const uint TERRAIN_DISPLACEMENT_MASK = (1u << 6u) | (1u << 7u) | (1u << 8u) | (1u << 9u) | (1u << 10u) | (1u << 11u); # endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli at line 257, Add a brief inline comment above the TERRAIN_DISPLACEMENT_MASK constant that documents which bit positions it covers and maps them to PBR::TerrainFlags entries (i.e., bits 6..11 correspond to PBR::TerrainFlags::LandTile0HasDisplacement through LandTile5HasDisplacement), so future readers can immediately see the mapping between the mask and PBR::TerrainFlags::LandTile0-5HasDisplacement.
259-260: 💤 Low valueMacro captures surrounding scope implicitly.
TERRAIN_HEIGHT_ATreferencesnoise,input,params, andsharedOffsetfrom the calling scope. This is intentional for conciseness but can be surprising. The comment at line 29 partially documents this pattern; consider adding a brief note here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli around lines 259 - 260, The macro TERRAIN_HEIGHT_AT implicitly captures variables from the caller (noise, input, params, sharedOffset and uses SharedData::extendedMaterialSettings.EnableHeightBlending), which can be surprising; add a brief inline comment above the macro definition calling out that TERRAIN_HEIGHT_AT forwards those specific external symbols to GetTerrainHeight (list noise, input, params, sharedOffset and the EnableHeightBlending usage) so users know it depends on caller scope and what must be present when using the macro.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli (1)
161-161: 💤 Low valueConsider naming the secant iteration count.
The literal
3should be a named constant (e.g.,ParallaxSecantIterations) for clarity and easier tuning, as noted in PR feedback.♻️ Suggested change
+static const uint ParallaxSecantIterations = 3; + // ... in GetParallaxCoords: -[loop] for (uint i = 0; i < 3; i++) +[loop] for (uint i = 0; i < ParallaxSecantIterations; i++)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli at line 161, Replace the magic literal 3 in the secant iteration loop with a named constant to improve clarity and tunability: define a constant like ParallaxSecantIterations (e.g., static const uint ParallaxSecantIterations = 3) and use it in the loop header where you currently have "for (uint i = 0; i < 3; i++)" (reference the loop using the loop variable i in ExtendedMaterialsParallaxCore.hlsli). Ensure any relevant comments mention this constant so future maintainers know it's the secant iteration count to tune.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
20-26: 💤 Low valueConsider removing the unused
offset3field from the stub.Per PR discussion,
StochasticOffsets.offset3is dead. Including it in the stub maintains signature compatibility, but removing it from both the stub and the real struct would clean up the API.🤖 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 20 - 26, Remove the dead field offset3 from the StochasticOffsets struct: delete the float2 offset3 member from the stub in ExtendedMaterials.hlsli and mirror that change in the real/production StochasticOffsets definition and any associated helpers; then update all uses of StochasticOffsets (constructors, initializers, packing/unpacking/serialization functions, shader inputs/CBVs, and any call sites that read/write .offset3) so they no longer reference offset3 to keep signatures consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli:
- Around line 196-201: GetTerrainHeight currently mutates the `weights` during
each secant probe but the code returns the final UV (computed as coords.xy +
viewDirTS.xy * offset after calculating `parallaxAmount = lerp(tNear, tFar, r)`)
without re-sampling the terrain weights at that final UV, leaving `weights`
stale and later copied back into `input.LandBlendWeights*`. Fix: after computing
the final `offset`/final UV (the expression `viewDirTS.xy * offset + coords.xy`)
call the terrain weight sampling path used inside `GetTerrainHeight` (or a
dedicated sampleWeightsAtUV function) to re-evaluate and overwrite the `weights`
array at that UV, then proceed to set `pixelOffset`/hasPOM and return—ensuring
`weights` now reflect the returned parallax coordinate so
`input.LandBlendWeights*` receives the refreshed values.
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli:
- Around line 6-14: InitializeTerrainMipLevels currently derives mip levels from
albedo samplers (TexColorSampler / TexLandColor*Sampler) while GetTerrainHeight
under TRUE_PBR samples displacement textures (TexLandDisplacement*Sampler),
causing mismatched mipchain usage; update InitializeTerrainMipLevels to choose
the correct samplers when TRUE_PBR (use TexLandDisplacementSampler and
TexLandDisplacement2/3/4/5/6Sampler) so GetMipLevel is called on the
displacement textures that GetTerrainHeight actually samples (or add a
boolean/compile-time branch parameter to InitializeTerrainMipLevels to select
displacement vs color samplers), ensuring mipLevels[] align with the textures
used by GetTerrainHeight and terrain shadow/secant march code.
- Line 52: The use of rcp(wsum) for invwsum can produce INF/NaN when all blend
weights are zero; update both occurrences (the variable invwsum computed from
wsum at the invwsum assignments) to guard against zero by replacing rcp(wsum)
with rcp(max(wsum, epsilon)) or explicitly set invwsum = 0 when wsum is zero
(choose a small epsilon like 1e-6), and apply the same change at both places
where invwsum is computed so downstream terrain weight math uses a safe
reciprocal.
---
Nitpick comments:
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli:
- Around line 20-26: Remove the dead field offset3 from the StochasticOffsets
struct: delete the float2 offset3 member from the stub in
ExtendedMaterials.hlsli and mirror that change in the real/production
StochasticOffsets definition and any associated helpers; then update all uses of
StochasticOffsets (constructors, initializers, packing/unpacking/serialization
functions, shader inputs/CBVs, and any call sites that read/write .offset3) so
they no longer reference offset3 to keep signatures consistent.
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli:
- Line 161: Replace the magic literal 3 in the secant iteration loop with a
named constant to improve clarity and tunability: define a constant like
ParallaxSecantIterations (e.g., static const uint ParallaxSecantIterations = 3)
and use it in the loop header where you currently have "for (uint i = 0; i < 3;
i++)" (reference the loop using the loop variable i in
ExtendedMaterialsParallaxCore.hlsli). Ensure any relevant comments mention this
constant so future maintainers know it's the secant iteration count to tune.
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlsli:
- Line 257: Add a brief inline comment above the TERRAIN_DISPLACEMENT_MASK
constant that documents which bit positions it covers and maps them to
PBR::TerrainFlags entries (i.e., bits 6..11 correspond to
PBR::TerrainFlags::LandTile0HasDisplacement through LandTile5HasDisplacement),
so future readers can immediately see the mapping between the mask and
PBR::TerrainFlags::LandTile0-5HasDisplacement.
- Around line 259-260: The macro TERRAIN_HEIGHT_AT implicitly captures variables
from the caller (noise, input, params, sharedOffset and uses
SharedData::extendedMaterialSettings.EnableHeightBlending), which can be
surprising; add a brief inline comment above the macro definition calling out
that TERRAIN_HEIGHT_AT forwards those specific external symbols to
GetTerrainHeight (list noise, input, params, sharedOffset and the
EnableHeightBlending usage) so users know it depends on caller scope and what
must be present when using the macro.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 174fc57c-f2b8-4963-8506-2ab9ffce7fca
📒 Files selected for processing (4)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlslifeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsTerrain.hlslipackage/Shaders/Lighting.hlsl
🚧 Files skipped from review as they are similar to previous changes (1)
- package/Shaders/Lighting.hlsl
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli (2)
161-164: ⚡ Quick winName the secant iteration count.
The literal
5for iteration count is a tuning parameter. For consistency with other named constants (e.g.,ParallaxCheapDistance,ParallaxNearShadowQuality), consider defining aParallaxSecantIterationsconstant.♻️ Suggested refactor
In
ExtendedMaterials.hlslinamespace:static const uint ParallaxSecantIterations = 5;Then here:
- [loop] for (uint i = 0; i < 5; i++) + [loop] for (uint i = 0; i < ParallaxSecantIterations; i++)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli around lines 161 - 164, The loop uses a hardcoded iteration count (literal `5`) in the secant iteration inside ExtendedMaterialsParallaxCore.hlsli; introduce a named constant (e.g., `ParallaxSecantIterations`) alongside other parallax constants (like `ParallaxCheapDistance`, `ParallaxNearShadowQuality`) in ExtendedMaterials.hlsli and replace the literal `5` in the loop (`for (uint i = 0; i < 5; i++)`) with the new `ParallaxSecantIterations` symbol so the iteration count is configurable and consistent across the codebase.
27-32: 💤 Low valueConsider documenting the view direction flattening constants.
The
0.7and0.3constants are empirical tuning values. A brief comment explaining their derivation (e.g., "0.3 floor prevents extreme foreshortening at grazing angles") would aid future maintainers, as requested in the PR review comments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Extended` Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli around lines 27 - 32, Add a brief comment explaining the empirical flattening constants used when computing viewDirTS (the 0.7 multiplier and 0.3 floor) so future maintainers understand their purpose; update the lines handling viewDirTS (both the LANDSCAPE branch with params[0].FlattenAmount and the other branch with params.FlattenAmount) to include a short note like “0.7 scales foreshortening with angle; 0.3 prevents extreme division at grazing angles” placed directly above or inline with the expression so it’s obvious why those constants are used.features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (1)
20-26: 💤 Low valueConsider removing unused
offset3field.Per the PR comments summary,
StochasticOffsets.offset3is identified as a dead/unused field. If it's not consumed anywhere in the terrain sampling paths, removing it would reduce struct size and clarify the API.🤖 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 20 - 26, Remove the dead field offset3 from the StochasticOffsets struct to reduce its size and clarify the API: edit the struct declaration (StochasticOffsets) to delete the float2 offset3 member, then search for any references to StochasticOffsets.offset3 (or usages of the third offset) and update callers/data upload/packing so they no longer expect a third offset (adjust buffer layouts or any SetConstant/CBV population logic that assumed three offsets). Ensure any stride/packing calculations and HLSL/C++ interop that depended on the removed member are updated accordingly.
🤖 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 20-26: Remove the dead field offset3 from the StochasticOffsets
struct to reduce its size and clarify the API: edit the struct declaration
(StochasticOffsets) to delete the float2 offset3 member, then search for any
references to StochasticOffsets.offset3 (or usages of the third offset) and
update callers/data upload/packing so they no longer expect a third offset
(adjust buffer layouts or any SetConstant/CBV population logic that assumed
three offsets). Ensure any stride/packing calculations and HLSL/C++ interop that
depended on the removed member are updated accordingly.
In `@features/Extended`
Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlsli:
- Around line 161-164: The loop uses a hardcoded iteration count (literal `5`)
in the secant iteration inside ExtendedMaterialsParallaxCore.hlsli; introduce a
named constant (e.g., `ParallaxSecantIterations`) alongside other parallax
constants (like `ParallaxCheapDistance`, `ParallaxNearShadowQuality`) in
ExtendedMaterials.hlsli and replace the literal `5` in the loop (`for (uint i =
0; i < 5; i++)`) with the new `ParallaxSecantIterations` symbol so the iteration
count is configurable and consistent across the codebase.
- Around line 27-32: Add a brief comment explaining the empirical flattening
constants used when computing viewDirTS (the 0.7 multiplier and 0.3 floor) so
future maintainers understand their purpose; update the lines handling viewDirTS
(both the LANDSCAPE branch with params[0].FlattenAmount and the other branch
with params.FlattenAmount) to include a short note like “0.7 scales
foreshortening with angle; 0.3 prevents extreme division at grazing angles”
placed directly above or inline with the expression so it’s obvious why those
constants are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c178ad98-bea9-416e-b37a-2fed11f10856
📒 Files selected for processing (3)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslifeatures/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterialsParallaxCore.hlslipackage/Shaders/Common/LightingLandscape.hlsli
🚧 Files skipped from review as they are similar to previous changes (1)
- package/Shaders/Common/LightingLandscape.hlsli
|
@coderabbitai explore the landscape shaders in depth and evaluate a tangible way to massively improve compilation time. This PR apparently has not helped much with it. The landscape shaders are incredibly slow to compile. |
|
@copilot explore the landscape shaders in depth and evaluate a tangible way to massively improve compilation time. This PR apparently has not helped much with it. The landscape shaders are incredibly slow to compile. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me dig into the actual shader code to count the permutation explosion and find the real compilation bottlenecks. 🏁 Script executed: Length of output: 1054 🏁 Script executed: Length of output: 280 🏁 Script executed: Length of output: 299 🏁 Script executed: Length of output: 2490 🏁 Script executed: Length of output: 3484 🏁 Script executed: Length of output: 2420 🏁 Script executed: Length of output: 4860 🏁 Script executed: Length of output: 101 🏁 Script executed: Length of output: 23811 🏁 Script executed: Length of output: 73 🏁 Script executed: Length of output: 1927
Root Cause: Permutation Explosion in a Monolithic Shader
Active compile-time permutation axes for a LANDSCAPE shader
That's 12 binary axes = 4,096 theoretical permutations, of which even a conservative 5–10% surviving game-side pruning means 200–400 LANDSCAPE shader compilations, each of a 2,817-line monolith with 12 headers included per TU. Recommendations, Ranked by Impact🔴
|
| Recommendation | Permutation reduction | Effort |
|---|---|---|
Split Lighting.hlsl by material class |
60–70% fewer LANDSCAPE TUs | High |
TERRAIN_VARIATION → runtime bool |
50% | Medium |
SNOW (LANDSCAPE) → runtime bool |
50% additive | Low–Medium |
Texture array + [unroll] for-loop |
Smaller IR per TU | High |
Guard terrain hlsli under EMAT |
Smaller TU parse | Low |
| Umbrella header PCH | Better cache reuse | Low |
Remove dead EMAT_ENVMAP arms in LANDSCAPE |
Trivial | Trivial |
Recommendations #2 + #3 together are the most immediately actionable without restructuring the build system, and would cut LANDSCAPE shader compile jobs by ~75%. Recommendation #1 (splitting the file) is the nuclear option but yields the most sustained benefit as the feature set continues to grow.
|
@coderabbitai per your review, what if we make terrain variation exclusively defined = on, not defined = off? wouldn't it work correctly then with minimal compiler steps? Just compiling TV ON and vanilla? We also need terrain variation defined separately to allow for it to be installed/uninistalled by the end user no? Question critically and deeply evaluate if i am correct or not. |
|
Tip For best results, initiate chat on the files or code changes.
Your Proposal Is Already Exactly How It Works
// src/Features/TerrainVariation.h (post-PR)
loaded && shaderType == RE::BSShader::Type::LightingWhen TV is not installed: define absent → vanilla permutations compiled. So yes, Where You Are Wrong: This Does NOT Minimize Compile StepsHere is the critical failure in your reasoning. The install/uninstall model you describe produces two full sets of permutations: You are not choosing between "compile TV-ON" or "compile vanilla". The shader cache must contain both, because users can switch. Any LANDSCAPE permutation axis that is a compile-time define doubles the permutation count. This is the exact problem. The Install/Uninstall Concern Is Real But Solvable Without a Compile-Time DefineYou raise a valid concern: TV needs to be installable and uninstallable by end users. But a runtime cbuffer bool handles this just as well — arguably better: // cbuffer (already exists as TerrainVariationSettings)
uint enableTerrainVariation; // ← add this
// In shader (runtime branch, warp-coherent)
StochasticOffsets sharedOffset = (StochasticOffsets)0;
[branch] if (SharedData::terrainVariationSettings.enableTerrainVariation)
sharedOffset = ComputeStochasticOffsets(input.TexCoord0.zw);// HasShaderDefine() returns false unconditionally for Lighting type
// TV feature just writes enableTerrainVariation = 1 to its cbuffer when loadedResult:
The One Legitimate Counter-Argument Against Runtime BoolThere is a real cost: every LANDSCAPE shader binary — even for vanilla users who never install TV — would contain the stochastic sampling code. This means:
Whether (2) matters in practice depends on how register-pressure-bound the LANDSCAPE shader already is. On modern AMD and Nvidia GPUs, Bottom line: If LANDSCAPE is already register-bound (which, given its complexity, it may well be), the runtime bool costs vanilla users a small occupancy penalty. If it's not register-bound, the runtime bool is strictly better in every dimension. Verdict
Your reasoning is partially correct — the defined/!defined semantics are right and already implemented. But the claim that it gives "minimal compiler steps" is wrong — it is literally the maximum compile cost because both variants must exist. The runtime bool is the approach that actually minimizes compile steps, and it handles install/uninstall correctly. The only reason to keep the compile-time define is if LANDSCAPE is measurably register-bound and vanilla-without-TV performance is a priority worth protecting at the cost of doubled compile time. |
Summary by CodeRabbit
New Features
Refactor
Chores