refactor(volumetric-lighting): use constant for threshold#2257
Conversation
📝 WalkthroughWalkthroughA local constant ChangesVolumetric Lighting Threshold Bias Constant
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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)
Comment |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package/Shaders/ISApplyVolumetricLighting.hlsl (2)
1-1: 💤 Low valueConventional commit title suggestion (optional).
Your current title looks close to conventional commits; consider making it more specific about the actual change: e.g.
refactor(vol lighting): replace VL bias magic(and keep the body under 72 chars describing the0.0078125→kVLThresholdBiasreplacement).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISApplyVolumetricLighting.hlsl` at line 1, Update the commit title to follow conventional commits and be specific about the change (e.g., "refactor(vol lighting): replace VL bias magic"), and shorten the commit body to <=72 characters describing the replacement of the magic constant (0.0078125) with a named constant (kVLThresholdBias) so the title and body clearly communicate the refactor affecting volumetric lighting bias.
34-38: 💤 Low valueMove
kVLThresholdBiasto file scope (optional).Behavior should be unchanged, but putting the constant at file scope (instead of inside
main) improves readability and avoids any ambiguity about per-entry duplication.Suggested tweak
+static const float kVLThresholdBias = 1.0 / 128.0; PS_OUTPUT main(PS_INPUT input) { PS_OUTPUT psout; - static const float kVLThresholdBias = 1.0 / 128.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISApplyVolumetricLighting.hlsl` around lines 34 - 38, The constant kVLThresholdBias is declared inside the PS_ENTRY function main(PS_INPUT input), which can imply per-call duplication; move the declaration of static const float kVLThresholdBias = 1.0 / 128.0; out of main to file scope (top of the shader) and remove the in-function declaration so main continues to reference the same symbol but the constant lives at file scope for clarity and no behavioral change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/ISApplyVolumetricLighting.hlsl`:
- Line 53: The call to max in the adjustedVl computation uses an integer literal
which can cause implicit int→float coercion; update the expression in the
adjustedVl assignment (the line computing float adjustedVl = max(...)) to use a
floating-point literal such as 0.0 or 0.0f (e.g., max(0.0, noiseGrad + vl -
kVLThresholdBias)) so the types remain consistent and avoid shader validation
warnings/errors.
---
Nitpick comments:
In `@package/Shaders/ISApplyVolumetricLighting.hlsl`:
- Line 1: Update the commit title to follow conventional commits and be specific
about the change (e.g., "refactor(vol lighting): replace VL bias magic"), and
shorten the commit body to <=72 characters describing the replacement of the
magic constant (0.0078125) with a named constant (kVLThresholdBias) so the title
and body clearly communicate the refactor affecting volumetric lighting bias.
- Around line 34-38: The constant kVLThresholdBias is declared inside the
PS_ENTRY function main(PS_INPUT input), which can imply per-call duplication;
move the declaration of static const float kVLThresholdBias = 1.0 / 128.0; out
of main to file scope (top of the shader) and remove the in-function declaration
so main continues to reference the same symbol but the constant lives at file
scope for clarity and no behavioral change.
🪄 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: 993b0e4f-426d-43fb-ba69-b362d7167089
📒 Files selected for processing (1)
package/Shaders/ISApplyVolumetricLighting.hlsl
There was a problem hiding this comment.
Pull request overview
Refactors the volumetric lighting apply pixel shader to replace an inline “magic number” threshold with a named constant, improving readability/maintainability while preserving the numeric value.
Changes:
- Introduced a named constant for the VL threshold bias (1/128).
- Replaced the hard-coded
0.0078125with the named constant in the VL adjustment computation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Where did your magic number come from? I believe the number you're replacing came from vanilla. |
|
The magic number is exactly 1/128. |
…shaders#2257) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…shaders#2257) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…shaders#2257) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…shaders#2257) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
small fix i saw when doing hdr sun
Summary by CodeRabbit