fix(pbr): clamp roughness at a bigger value#1351
Merged
Merged
Conversation
Contributor
WalkthroughThe change updates the minimum clamping value for the roughness parameter in the physically based rendering (PBR) surface property initialization within a pixel shader. The lower bound for roughness is increased from 0.005 to 0.04, affecting the rendering calculations for material appearance. No other properties or function signatures are modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: default parameter values are supported in the hlsl compiler used by the skyrim-community-shaders pro...Applied to files:
📚 Learning: in the skyrim-community-shaders project, simple scalar constants in hlsl shaders use #define (e.g., ...Applied to files:
📚 Learning: in the skyrim-community-shaders project, boolean flags in c++ structs that interface with hlsl shade...Applied to files:
📚 Learning: in the skyrim-community-shaders project, boolean flags in c++ structs that interface with hlsl shade...Applied to files:
📚 Learning: in the skyrim-community-shaders repository, file deletion error handling improvements that replace e...Applied to files:
📚 Learning: in the skyrim community shaders project, the `smoothdrawcalls` array in the `state` class is declare...Applied to files:
⏰ 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)
🔇 Additional comments (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR increases the minimum roughness clamp value in the PBR shader from 0.005 to 0.04 to ensure that GGX specular highlights remain visible.
- Increased minimum roughness value from 0.005 to 0.04 in the roughness clamping operation
| pbrSurfaceProperties.Noise = screenNoise; | ||
|
|
||
| pbrSurfaceProperties.Roughness = clamp(rawRMAOS.x, 0.005, 1.0); | ||
| pbrSurfaceProperties.Roughness = clamp(rawRMAOS.x, 0.04, 1.0); |
There was a problem hiding this comment.
The magic number 0.04 should be defined as a named constant to improve code maintainability and make the physical reasoning behind this value clearer.
Suggested change
| pbrSurfaceProperties.Roughness = clamp(rawRMAOS.x, 0.04, 1.0); | |
| pbrSurfaceProperties.Roughness = clamp(rawRMAOS.x, PBR_MIN_ROUGHNESS, 1.0); |
Contributor
There was a problem hiding this comment.
@coderabbitai let's make this an issue assigned to @jiayev for this constant.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@alandtse I'll create an issue for defining the magic number as a named constant and assign it to @jiayev.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Define PBR minimum roughness as named constant" \
--body "## Description
The recent change in PR #1351 updated the minimum roughness clamp value from 0.005 to 0.04 for the GGX microfacet model to ensure visible specular highlights. However, this magic number should be defined as a named constant to improve code maintainability and make the physical reasoning behind this value clearer.
## Context
Currently in \`package/Shaders/Lighting.hlsl\` line 2108:
\`\`\`hlsl
pbrSurfaceProperties.Roughness = clamp(rawRMAOS.x, 0.04, 1.0);
\`\`\`
This magic number should be replaced with a descriptive named constant like \`PBR_MIN_ROUGHNESS\`.
## Benefits
- Improved code readability and maintainability
- Makes the physical reasoning behind the value more explicit
- Easier to adjust if needed in the future
- Follows best practices for avoiding magic numbers
## References
- PR: https://github.com/doodlum/skyrim-community-shaders/pull/1351
- Comment: https://github.com/doodlum/skyrim-community-shaders/pull/1351#discussion_r2249344753" \
--assignee jiayev
Length of output: 1137
Perfect! I've successfully created issue #1353 "Define PBR minimum roughness as named constant" and assigned it to @jiayev. The issue includes:
- A clear description of the need to replace the magic number 0.04 with a named constant
- Context about the recent change in this PR
- Benefits of making this improvement
- References to both this PR and the comment discussion
The issue is now tracked at: #1353
This will help improve code maintainability by making the physical reasoning behind the minimum roughness value more explicit and following best practices for avoiding magic numbers in the codebase.
🐰✨
🧠 Learnings used
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
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.
|
✅ A pre-release build is available for this PR: |
alandtse
suggested changes
Aug 2, 2025
alandtse
approved these changes
Aug 3, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
clamp roughness at a bigger value. 0.005 is still too small for ggx to generate visible specular.
Summary by CodeRabbit