fix(water-effects): hardcode 512px mip base; remove VR mip bias#2184
Conversation
Compute parallax mip levels against a 512x512 reference and apply a log2 offset to match the texture's actual size. Removes the "/16 (VR) vs /8 (flat)" divisor and the matching "+4 (VR) vs +3" post-offset that made results disagree between editions. Invert the heights read once at the sample site rather than at the aggregated return, so the per-texture subtraction only happens where we sample, not at the end.
📝 WalkthroughWalkthroughBumped Water Effects shader metadata and refactored parallax mip/height logic in WaterParallax.hlsli: switched mip derivation to a fixed 512x512 reference with a computed mip offset, inverted sampled height semantics, and removed VR-specific mip biases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Water` Effects/Shaders/WaterEffects/WaterParallax.hlsli:
- Around line 31-36: The computed mipLevel (from minTexCoordDelta) is being
shifted by mipOffset (log2(actualTextureDims.x / 512.0)) which can drive the
final value negative for sub-512 textures; explicitly clamp the final mipLevel
to zero (e.g., set mipLevel = max(mipLevel, 0.0)) after adding mipOffset so
SampleLevel receives a non-negative LOD and the shader preserves the intended
512px reference behavior; change the code around the mipLevel/mipOffset
calculations (symbols: mipLevel, mipOffset, actualTextureDims.x,
minTexCoordDelta, SampleLevel) to apply this clamp.
🪄 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: 56ed703b-147a-48d3-b618-4e0dcd35e70f
📒 Files selected for processing (2)
features/Water Effects/Shaders/Features/WaterEffects.inifeatures/Water Effects/Shaders/WaterEffects/WaterParallax.hlsli
There was a problem hiding this comment.
♻️ Duplicate comments (1)
features/Water Effects/Shaders/WaterEffects/WaterParallax.hlsli (1)
31-39:⚠️ Potential issue | 🟡 MinorClamp the mip level after applying the size offset.
Line 36 can make
mipLevelnegative for sub-512 textures, so the stochasticfloor/fracstep runs on negative LODs beforeSampleLevelclamps them. Clamp after addingmipOffset.Proposed fix
- mipLevel += mipOffset; + mipLevel = max(mipLevel + mipOffset, 0.0);To verify the HLSL behavior being relied on:
HLSL Texture2D SampleLevel negative LOD clamp behavior Direct3D documentation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Water` Effects/Shaders/WaterEffects/WaterParallax.hlsli around lines 31 - 39, The mipLevel can become negative after applying mipOffset which causes the stochastic floor/frac computation to operate on negative LODs; after computing mipOffset and adding it to mipLevel (the variable mipLevel and local mipOffset in this snippet), clamp mipLevel to a non-negative value (e.g., mipLevel = max(mipLevel, 0.0)) before performing the stochastic selection using floor/frac and before any SampleLevel calls so the fractional LOD logic never runs on negative values.
🧹 Nitpick comments (1)
features/Water Effects/Shaders/WaterEffects/WaterParallax.hlsli (1)
1-1: Consider shortening the PR title and adding an issue reference.The title is descriptive and conventional, but it appears to exceed the repo’s 50-character title limit. A shorter option could be:
fix(water): align parallax mip biasIf this fixes a tracked bug, add
Fixes #<issue>orCloses #<issue>to the PR body.As per coding guidelines, “Length: 50 characters limit for title” and “Suggest adding appropriate GitHub keywords.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Water` Effects/Shaders/WaterEffects/WaterParallax.hlsli at line 1, The PR title is too long per repo guidelines; shorten it (suggestion: "fix(water): align parallax mip bias") and update the PR body to include an issue reference like "Fixes #<issue>" or "Closes #<issue>" if this change addresses a tracked bug; note this change applies to the PR for the WaterParallax shader in features/Water Effects/Shaders/WaterEffects (namespace WaterEffects) so reviewers can correlate the title/issue with that file.
🤖 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/Water` Effects/Shaders/WaterEffects/WaterParallax.hlsli:
- Around line 31-39: The mipLevel can become negative after applying mipOffset
which causes the stochastic floor/frac computation to operate on negative LODs;
after computing mipOffset and adding it to mipLevel (the variable mipLevel and
local mipOffset in this snippet), clamp mipLevel to a non-negative value (e.g.,
mipLevel = max(mipLevel, 0.0)) before performing the stochastic selection using
floor/frac and before any SampleLevel calls so the fractional LOD logic never
runs on negative values.
---
Nitpick comments:
In `@features/Water` Effects/Shaders/WaterEffects/WaterParallax.hlsli:
- Line 1: The PR title is too long per repo guidelines; shorten it (suggestion:
"fix(water): align parallax mip bias") and update the PR body to include an
issue reference like "Fixes #<issue>" or "Closes #<issue>" if this change
addresses a tracked bug; note this change applies to the PR for the
WaterParallax shader in features/Water Effects/Shaders/WaterEffects (namespace
WaterEffects) so reviewers can correlate the title/issue with that file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff1d8bf8-3a94-4291-8811-c1afccf892e0
📒 Files selected for processing (1)
features/Water Effects/Shaders/WaterEffects/WaterParallax.hlsli
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
features/Water Effects/Shaders/WaterEffects/WaterParallax.hlsli (1)
1-1: Shorten the PR title and add an issue reference if applicable.Current title is conventional, but it exceeds the 50-character guideline. Consider:
fix(water): normalize parallax mipsIf this fixes a tracked bug, add
Fixes #<issue>to the PR body. As per coding guidelines, “Format: type(scope): description” and “Length: 50 characters limit for title”; issue references should use keywords such as “Fixes#123”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Water` Effects/Shaders/WaterEffects/WaterParallax.hlsli at line 1, PR title is too long; change it to the 50-character "type(scope): description" format (for example: "fix(water): normalize parallax mips") and, if this change fixes a tracked bug, add an issue reference like "Fixes #<issue>" to the PR body; use the file/feature context (WaterParallax.hlsli / namespace WaterEffects) when updating the title/body so reviewers can quickly connect the PR to the shader change.
🤖 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/Water` Effects/Shaders/WaterEffects/WaterParallax.hlsli:
- Line 1: PR title is too long; change it to the 50-character "type(scope):
description" format (for example: "fix(water): normalize parallax mips") and, if
this change fixes a tracked bug, add an issue reference like "Fixes #<issue>" to
the PR body; use the file/feature context (WaterParallax.hlsli / namespace
WaterEffects) when updating the title/body so reviewers can quickly connect the
PR to the shader change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d4e8e7e-69a8-4f3c-95dc-82c641f28f58
📒 Files selected for processing (1)
features/Water Effects/Shaders/WaterEffects/WaterParallax.hlsli
…unity-shaders#2184) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit a34ef7f)
2 participants
Compute parallax mip levels against a 512x512 reference and apply a log2 offset to match the texture's actual size. Removes the "/16 (VR) vs /8 (flat)" divisor and the matching "+4 (VR) vs +3" post-offset that made results disagree between editions.
Invert the heights read once at the sample site rather than at the aggregated return, so the per-texture subtraction only happens where we sample, not at the end.
Summary by CodeRabbit
Bug Fixes
Chores