Skip to content

perf(tv): improve compile time#1415

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

perf(tv): improve compile time#1415
davo0411 wants to merge 2 commits into
community-shaders:devfrom
davo0411:tv-compiler-opt

Conversation

@davo0411
Copy link
Copy Markdown
Collaborator

@davo0411 davo0411 commented Aug 19, 2025

Summary by CodeRabbit

  • New Features

    • Runtime toggle for terrain variation in shaders.
    • Enhanced terrain relief: displacement height boosted when variation is enabled.
  • Bug Fixes

    • Reduced visible texture tiling on terrain surfaces when variation is active.
  • Refactor

    • Unified terrain texture sampling across layers and passes for consistent visuals and fewer branches.
    • Centralized variation setup to minimize duplicate work and streamline shader execution.
  • Performance

    • Potential gains from simplified branching and consolidated sampling paths.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Aug 19, 2025

Walkthrough

Centralizes terrain texture sampling behind new inline helpers in ExtendedMaterials.hLSLI and Lighting.hlsl. Introduces runtime-controlled variation flag and shared offsets/gradients, replacing scattered preprocessor branches and ad-hoc sampling. All terrain height, color, normal, and RM/AO samples now route through the helpers, with overloads preserving behavior when TERRAIN_VARIATION is undefined.

Changes

Cohort / File(s) Summary
Centralized terrain sampling helper (Extended Materials)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
Added inline helper SampleTerrainTextureEM (two overloads) to select between StochasticEffectParallax and SampleLevel based on runtime useVariation; refactored terrain displacement/height and related sampling calls to use the helper; moved 1.3× height boost behind runtime check; added comments and reduced preprocessor branching.
Centralized terrain sampling helper (Lighting pipeline)
package/Shaders/Lighting.hlsl
Added inline helper SampleTerrainTexture (two overloads) for unified sampling across terrain layers; computed useTerrainVariation, dx/dy, and sharedOffset once; replaced per-layer conditional sampling for colors, normals, and RM/AO (layers 1–6) with helper calls; preserved non-variation behavior via dummy overload.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant PS as PixelShader
  participant Helper as SampleTerrainTexture/EM
  participant Stoch as StochasticEffect/Parallax
  participant Tex as Texture2D

  PS->>Helper: sample(tex, samp, uv, useVariation, offsets, dx, dy)
  alt useVariation == true
    Helper->>Stoch: StochasticEffect(uv, offsets, dx, dy)
    Stoch-->>Helper: uv'/parallax-adjusted sample
    Helper->>Tex: SampleLevel/SampleGrad (biased as needed)
    Helper-->>PS: varied sample
  else useVariation == false
    Helper->>Tex: SampleLevel(tex, samp, uv, mip)
    Helper-->>PS: regular sample
  end
  note over PS: Height path multiplies by 1.3 when useVariation is true
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • alandtse
  • doodlum

Poem

In dunes of code I hop and weave,
A single path for sands I cleave.
Flip the flag, the grains align,
Stochastic ripples now refine.
One nibble, two—no branches sprawl,
Carrot-bright textures for us all. 🥕✨

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Automated formatting by clang-format, prettier, and other hooks.
See https://pre-commit.ci for details.
@github-actions
Copy link
Copy Markdown

Using provided base ref: dcdbaf3
Using base ref: dcdbaf3
Base commit date: 2025-08-17T20:52:19+01:00 (Sunday, August 17, 2025 08:52 PM)
No actionable suggestions for changed features.

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: 2

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

1264-1294: Optional: re-derive dx/dy and offsets after parallax warp to align with final UVs

If ExtendedMaterials::GetParallaxCoords modifies uv (common case), stochastic sampling for color/normal/RMAOS layers will be more accurate if gradients/offsets are recomputed after the warp. This keeps the stochastic ellipse aligned with the final sampling coordinates while still letting EMAT’s parallax use the initial dx/dy.

You can insert the following right after the parallax block (guarded by TERRAIN_VARIATION):

#if defined(TERRAIN_VARIATION)
if (useTerrainVariation) {
    dx = ddx(uv);
    dy = ddy(uv);
    sharedOffset = ComputeStochasticOffsets(uv);
}
#endif

This is a quality/stability tweak; not strictly required for correctness.


1286-1292: Nit: Unused variable shadowMultiplier

shadowMultiplier is computed but not used. Consider removing it to avoid confusing future readers.

-            float shadowMultiplier = ExtendedMaterials::GetParallaxSoftShadowMultiplierTerrain(input, uv, mipLevels, DirLightDirection, sh0, parallaxShadowQuality, screenNoise, displacementParams, sharedOffset, dx, dy);
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (3)

172-176: Verify the 1.3x height boost matches intended parity across pipelines

The runtime boost is applied post-weighting in both paths. If legacy (pre-PR) outputs depended on compile-time tuning around TERRAIN_VARIATION, confirm 1.3 accurately preserves perceived depth across materials. If needed, consider a shared constant or setting to tune in data.

Would you like me to wire this through SharedData (e.g., terrainVariationSettings.heightBoost) so downstream tuning doesn’t require shader recompiles?

Also applies to: 252-256


501-527: Shadow contrast branch: retain scaling consistency with TRUE_PBR path

The TV-enabled branch omits the scale normalization used in the TRUE_PBR path’s non-TV branch. That can subtly change penumbra width with high HeightScale values. If this divergence is intentional for visual parity with the new stochastic sampling, all good; otherwise, consider normalizing by scale here as well.

I can prep an A/B toggle to make this easy to compare if helpful.

Also applies to: 549-557


16-21: Minor: avoid unused parameter warnings in the non-TV fallback

useVariation/dummy parameters are intentionally unused. If your shader warnings are strict, you can mark them consumed with a no-op.

Apply a no-op consume:

 inline float4 SampleTerrainTextureEM(Texture2D tex, SamplerState samp, float2 coords, float mipLevel, bool useVariation, float4 dummy1, float2 dummy2, float2 dummy3)
 {
-    return tex.SampleLevel(samp, coords, mipLevel);
+    (void)useVariation; (void)dummy1; (void)dummy2; (void)dummy3;
+    return tex.SampleLevel(samp, coords, mipLevel);
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between dcdbaf3 and 7da6558.

📒 Files selected for processing (2)
  • features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (3 hunks)
  • package/Shaders/Lighting.hlsl (20 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

  • features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
  • package/Shaders/Lighting.hlsl
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}

📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)

features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)

Files:

  • features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
  • GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
  • GitHub Check: Build plugin and addons
🔇 Additional comments (4)
package/Shaders/Lighting.hlsl (2)

968-979: Centralized terrain sampling helper is clean and reduces compile-time divergence

The two overloads of SampleTerrainTexture (stochastic path vs. fallback) are a good abstraction. They keep call sites uniform and allow runtime toggling via useVariation without proliferating preprocessor branches. No register or binding changes introduced here.


1340-1374: LGTM: Per-layer sampling unified through the helper

Replacing the per-layer sampling blocks for colors, normals, and RMAOS (layers 1–6) with SampleTerrainTexture is consistent and removes duplicated branches. Passing the shared useTerrainVariation, offsets, dx, dy keeps behavior aligned regardless of compile-time TERRAIN_VARIATION.

Also applies to: 1385-1419, 1430-1466, 1474-1508, 1519-1554, 1564-1599

features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli (2)

11-21: Centralized terrain sampling helper is a solid simplification

The inline helper and its non-TV fallback read cleanly and centralize the sampling decision behind a single callsite. This is exactly the kind of consolidation that reduces compile-time branching.


8-10: Include guard matches usage sites

Including TerrainVariation only when both TERRAIN_VARIATION and LANDSCAPE are defined is correct, given all stochastic calls are within LANDSCAPE blocks.

Comment on lines +138 to 147
// Centralized terrain variation check
bool useVariation = false;
# if defined(TERRAIN_VARIATION)
heights[0] = ScaleDisplacement(StochasticEffectParallax(TexLandDisplacement0Sampler, SampTerrainParallaxSampler, coords, mipLevels[0], sharedOffset, dx, dy).x, params[0]);
# else
heights[0] = ScaleDisplacement(TexLandDisplacement0Sampler.SampleLevel(SampTerrainParallaxSampler, coords, mipLevels[0]).x, params[0]);
useVariation = SharedData::terrainVariationSettings.enableTilingFix;
# endif

[branch] if ((PBRFlags & PBR::TerrainFlags::LandTile0HasDisplacement) != 0 && w1.x > 0.01)
{
heights[0] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement0Sampler, SampTerrainParallaxSampler, coords, mipLevels[0], useVariation, sharedOffset, dx, dy).x, params[0]);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Build breaks when TERRAIN_VARIATION is undefined: sharedOffset/dx/dy referenced unconditionally

Within both GetTerrainHeight variants, calls to SampleTerrainTextureEM pass sharedOffset, dx, dy unconditionally. When TERRAIN_VARIATION is not defined, those identifiers do not exist (they’re not function parameters), causing compilation errors. The fallback overload of SampleTerrainTextureEM doesn’t help here because the arguments themselves are unresolved.

Fix by introducing a small macro for the argument list and using it at all callsites. This avoids scattering #if guards and compiles cleanly in both configurations.

Apply the following diffs:

  1. Introduce TV_ARGS macro once (after the helper definitions are closed):
@@
 #endif
+
+// Expand TV-only call arguments safely when TERRAIN_VARIATION is undefined.
+#if defined(TERRAIN_VARIATION)
+#  define TV_ARGS sharedOffset, dx, dy
+#else
+#  define TV_ARGS float4(0,0,0,0), 0, 0
+#endif
  1. Replace direct uses of sharedOffset, dx, dy with TV_ARGS in all SampleTerrainTextureEM callsites inside GetTerrainHeight:
@@
-            heights[0] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement0Sampler, SampTerrainParallaxSampler, coords, mipLevels[0], useVariation, sharedOffset, dx, dy).x, params[0]);
+            heights[0] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement0Sampler, SampTerrainParallaxSampler, coords, mipLevels[0], useVariation, TV_ARGS).x, params[0]);
@@
-            heights[1] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement1Sampler, SampTerrainParallaxSampler, coords, mipLevels[1], useVariation, sharedOffset, dx, dy).x, params[1]);
+            heights[1] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement1Sampler, SampTerrainParallaxSampler, coords, mipLevels[1], useVariation, TV_ARGS).x, params[1]);
@@
-            heights[2] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement2Sampler, SampTerrainParallaxSampler, coords, mipLevels[2], useVariation, sharedOffset, dx, dy).x, params[2]);
+            heights[2] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement2Sampler, SampTerrainParallaxSampler, coords, mipLevels[2], useVariation, TV_ARGS).x, params[2]);
@@
-            heights[3] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement3Sampler, SampTerrainParallaxSampler, coords, mipLevels[3], useVariation, sharedOffset, dx, dy).x, params[3]);
+            heights[3] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement3Sampler, SampTerrainParallaxSampler, coords, mipLevels[3], useVariation, TV_ARGS).x, params[3]);
@@
-            heights[4] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement4Sampler, SampTerrainParallaxSampler, coords, mipLevels[4], useVariation, sharedOffset, dx, dy).x, params[4]);
+            heights[4] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement4Sampler, SampTerrainParallaxSampler, coords, mipLevels[4], useVariation, TV_ARGS).x, params[4]);
@@
-            heights[5] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement5Sampler, SampTerrainParallaxSampler, coords, mipLevels[5], useVariation, sharedOffset, dx, dy).x, params[5]);
+            heights[5] = ScaleDisplacement(SampleTerrainTextureEM(TexLandDisplacement5Sampler, SampTerrainParallaxSampler, coords, mipLevels[5], useVariation, TV_ARGS).x, params[5]);
@@
-                heights[0] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp0Sampler, SampTerrainParallaxSampler, coords, mipLevels[0], useVariation, sharedOffset, dx, dy).x, params[0]);
+                heights[0] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp0Sampler, SampTerrainParallaxSampler, coords, mipLevels[0], useVariation, TV_ARGS).x, params[0]);
@@
-                heights[0] = ScaleDisplacement(SampleTerrainTextureEM(TexColorSampler, SampTerrainParallaxSampler, coords, mipLevels[0], useVariation, sharedOffset, dx, dy).w, params[0]);
+                heights[0] = ScaleDisplacement(SampleTerrainTextureEM(TexColorSampler, SampTerrainParallaxSampler, coords, mipLevels[0], useVariation, TV_ARGS).w, params[0]);
@@
-                heights[1] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp1Sampler, SampTerrainParallaxSampler, coords, mipLevels[1], useVariation, sharedOffset, dx, dy).x, params[1]);
+                heights[1] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp1Sampler, SampTerrainParallaxSampler, coords, mipLevels[1], useVariation, TV_ARGS).x, params[1]);
@@
-                heights[1] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor2Sampler, SampTerrainParallaxSampler, coords, mipLevels[1], useVariation, sharedOffset, dx, dy).w, params[1]);
+                heights[1] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor2Sampler, SampTerrainParallaxSampler, coords, mipLevels[1], useVariation, TV_ARGS).w, params[1]);
@@
-            heights[3] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp3Sampler, SampTerrainParallaxSampler, coords, mipLevels[3], useVariation, sharedOffset, dx, dy).x, params[3]);
+            heights[3] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp3Sampler, SampTerrainParallaxSampler, coords, mipLevels[3], useVariation, TV_ARGS).x, params[3]);
@@
-            heights[3] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor4Sampler, SampTerrainParallaxSampler, coords, mipLevels[3], useVariation, sharedOffset, dx, dy).w, params[3]);
+            heights[3] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor4Sampler, SampTerrainParallaxSampler, coords, mipLevels[3], useVariation, TV_ARGS).w, params[3]);
@@
-            heights[4] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp4Sampler, SampTerrainParallaxSampler, coords, mipLevels[4], useVariation, sharedOffset, dx, dy).x, params[4]);
+            heights[4] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp4Sampler, SampTerrainParallaxSampler, coords, mipLevels[4], useVariation, TV_ARGS).x, params[4]);
@@
-            heights[4] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor5Sampler, SampTerrainParallaxSampler, coords, mipLevels[4], useVariation, sharedOffset, dx, dy).w, params[4]);
+            heights[4] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor5Sampler, SampTerrainParallaxSampler, coords, mipLevels[4], useVariation, TV_ARGS).w, params[4]);
@@
-            heights[5] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp5Sampler, SampTerrainParallaxSampler, coords, mipLevels[5], useVariation, sharedOffset, dx, dy).x, params[5]);
+            heights[5] = ScaleDisplacement(SampleTerrainTextureEM(TexLandTHDisp5Sampler, SampTerrainParallaxSampler, coords, mipLevels[5], useVariation, TV_ARGS).x, params[5]);
@@
-            heights[5] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor6Sampler, SampTerrainParallaxSampler, coords, mipLevels[5], useVariation, sharedOffset, dx, dy).w, params[5]);
+            heights[5] = ScaleDisplacement(SampleTerrainTextureEM(TexLandColor6Sampler, SampTerrainParallaxSampler, coords, mipLevels[5], useVariation, TV_ARGS).w, params[5]);

Also applies to: 194-201, 201-203, 207-213, 227-231, 235-239, 243-247

🤖 Prompt for AI Agents
In features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
around lines 138-147, SampleTerrainTextureEM is being called with sharedOffset,
dx, dy even when TERRAIN_VARIATION is not defined causing unresolved identifier
compile errors; define a TV_ARGS macro once (after helper definitions) that
expands to ", sharedOffset, dx, dy" when TERRAIN_VARIATION is defined and to an
empty string otherwise, then replace every SampleTerrainTextureEM call inside
both GetTerrainHeight variants (including the occurrences at lines ~194-201,
201-203, 207-213, 227-231, 235-239, 243-247) to pass TV_ARGS instead of the raw
identifiers so the calls compile in both configurations.

Comment on lines +1225 to 1245
// Optimize terrain variation setup - compute once, use everywhere
bool useTerrainVariation = false;
float2 dx = 0, dy = 0;
# if defined(TERRAIN_VARIATION)
StochasticOffsets sharedOffset;
sharedOffset.offset1 = float2(0, 0);
sharedOffset.offset2 = float2(0, 0);
sharedOffset.offset3 = float2(0, 0);
sharedOffset.weights = float3(0, 0, 0);

useTerrainVariation = SharedData::terrainVariationSettings.enableTilingFix;
if (useTerrainVariation)
{
dx = ddx(input.TexCoord0.zw);
dy = ddy(input.TexCoord0.zw);
sharedOffset = ComputeStochasticOffsets(input.TexCoord0.zw);
}
# else
float4 sharedOffset = float4(0, 0, 0, 0); // Dummy for fallback
# endif

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Potential gradient/coordinate mismatch: compute dx/dy and offsets from the same UVs you sample

You compute dx/dy and sharedOffset from input.TexCoord0.zw, but all subsequent texture samples in this block use uv. For stochastic sampling to be stable and anisotropy-correct, gradients and offsets must be derived from the same coordinate space as the sampling coordinate.

Recommend deriving from uv instead:

-    dx = ddx(input.TexCoord0.zw);
-    dy = ddy(input.TexCoord0.zw);
-    sharedOffset = ComputeStochasticOffsets(input.TexCoord0.zw);
+    dx = ddx(uv);
+    dy = ddy(uv);
+    sharedOffset = ComputeStochasticOffsets(uv);

Please sanity-check this with a quick visual A/B (tiling/shimmering at oblique angles). If you can, test both:

  • TERRAIN_VARIATION on/off
  • With/without EMAT terrain parallax

I’ll inspect how uv is defined and used relative to input.TexCoord0.zw.


🏁 Script executed:

#!/bin/bash
# Locate uv declaration
rg -nP '\bfloat2\s+uv\b' -C3 package/Shaders/Lighting.hlsl

# Find any assignments to uv
rg -nP '\buv\s*=' -C3 package/Shaders/Lighting.hlsl

# Dump lines 1200–1300 for full context around the reviewed block
sed -n '1200,1300p' package/Shaders/Lighting.hlsl

Length of output: 10080


Derive gradients and offsets from uv to match the sampling space

By computing dx, dy and sharedOffset from input.TexCoord0.zw, you’re using a different coordinate space than the one used for actual terrain sampling (uv). Update the calls so they’re derived from uv instead:

  • File: package/Shaders/Lighting.hlsl
  • Location: inside the #if defined(TERRAIN_VARIATION) block (~lines 1225–1245)
-    dx = ddx(input.TexCoord0.zw);
-    dy = ddy(input.TexCoord0.zw);
-    sharedOffset = ComputeStochasticOffsets(input.TexCoord0.zw);
+    dx = ddx(uv);
+    dy = ddy(uv);
+    sharedOffset = ComputeStochasticOffsets(uv);

This ensures your derivatives and stochastic offsets come from the exact UV space used downstream, preventing mismatches (tiling artifacts/shimmering) at oblique angles.

📝 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.

Suggested change
// Optimize terrain variation setup - compute once, use everywhere
bool useTerrainVariation = false;
float2 dx = 0, dy = 0;
# if defined(TERRAIN_VARIATION)
StochasticOffsets sharedOffset;
sharedOffset.offset1 = float2(0, 0);
sharedOffset.offset2 = float2(0, 0);
sharedOffset.offset3 = float2(0, 0);
sharedOffset.weights = float3(0, 0, 0);
useTerrainVariation = SharedData::terrainVariationSettings.enableTilingFix;
if (useTerrainVariation)
{
dx = ddx(input.TexCoord0.zw);
dy = ddy(input.TexCoord0.zw);
sharedOffset = ComputeStochasticOffsets(input.TexCoord0.zw);
}
# else
float4 sharedOffset = float4(0, 0, 0, 0); // Dummy for fallback
# endif
// Optimize terrain variation setup - compute once, use everywhere
bool useTerrainVariation = false;
float2 dx = 0, dy = 0;
# if defined(TERRAIN_VARIATION)
StochasticOffsets sharedOffset;
sharedOffset.offset1 = float2(0, 0);
sharedOffset.offset2 = float2(0, 0);
sharedOffset.offset3 = float2(0, 0);
sharedOffset.weights = float3(0, 0, 0);
useTerrainVariation = SharedData::terrainVariationSettings.enableTilingFix;
if (useTerrainVariation)
{
dx = ddx(uv);
dy = ddy(uv);
sharedOffset = ComputeStochasticOffsets(uv);
}
# else
float4 sharedOffset = float4(0, 0, 0, 0); // Dummy for fallback
# endif
🤖 Prompt for AI Agents
In package/Shaders/Lighting.hlsl around lines 1225 to 1245, the derivatives and
stochastic offsets are computed from input.TexCoord0.zw but must be derived from
the actual sampling UV (uv) to avoid sampling-space mismatches; replace
ddx(input.TexCoord0.zw), ddy(input.TexCoord0.zw) and
ComputeStochasticOffsets(input.TexCoord0.zw) with ddx(uv), ddy(uv) and
ComputeStochasticOffsets(uv) (or otherwise convert TexCoord0.zw to the uv space
before calling) while preserving types (float2/float3) and the existing
enableTilingFix guard so behavior is unchanged when TERRAIN_VARIATION is not
used.

@github-actions
Copy link
Copy Markdown

✅ A pre-release build is available for this PR:
Download

@davo0411 davo0411 changed the title fix(tv): compile time improvements for terrain variation perf(tv): compile time improvements for terrain variation Aug 19, 2025
# include "TerrainVariation/TerrainVariation.hlsli"

// Optimized terrain sampling helper for Extended Materials
inline float4 SampleTerrainTextureEM(Texture2D tex, SamplerState samp, float2 coords, float mipLevel, bool useVariation, StochasticOffsets offsets, float2 dx, float2 dy)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Move into namespace

@alandtse alandtse changed the title perf(tv): compile time improvements for terrain variation perf(tv): improve compile time Aug 23, 2025
@davo0411 davo0411 closed this Aug 25, 2025
@davo0411 davo0411 deleted the tv-compiler-opt branch December 31, 2025 04:37
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.

2 participants