refactor(pbr): centralize constants#1437
Conversation
WalkthroughIntroduces PBR::Constants in PBR.hlsli with eight float bounds for roughness and glint parameters. Updates Lighting.hlsl to replace hard-coded clamp literals with references to these constants. No control flow or external interfaces are changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
package/Shaders/Common/PBR.hlsli (1)
50-60: Consider adding a constant for GlintScreenSpaceScale’s lower bound and use it at call sites.You still have max(1, …) in Lighting.hlsl (Line 2113). Define a named constant to avoid reintroducing magic numbers downstream.
Apply this diff here:
namespace Constants { static const float MinRoughness = 0.04f; static const float MaxRoughness = 1.0f; + // Prevents undersampling in screen-space glints + static const float MinGlintScreenSpaceScale = 1.0f; static const float MinGlintDensity = 1.0f; static const float MaxGlintDensity = 40.0f; static const float MinGlintRoughness = 0.005f; static const float MaxGlintRoughness = 0.3f; static const float MinGlintDensityRandomization = 0.0f; static const float MaxGlintDensityRandomization = 5.0f; }And then use it in Lighting.hlsl (see my comment there).
package/Shaders/Lighting.hlsl (2)
2108-2117: Also replace max(1, glintParameters.x) with a named constant.Keeps all glint-related thresholds in one place and prevents drift if defaults change.
Update Line 2113 as follows (requires adding MinGlintScreenSpaceScale in PBR::Constants as suggested in PBR.hlsli):
pbrSurfaceProperties.GlintScreenSpaceScale = max(PBR::Constants::MinGlintScreenSpaceScale, glintParameters.x);
2108-2117: Centralize magic-number clamp ranges
There are numerous literal clamp/min/max calls across shaders (e.g. Utility.hlsl:856, ISApplyVolumetricLighting.hlsl:47, Lighting.hlsl:2113). Consider moving common ranges (wetness, hair, coat, glint, etc.) into shared constants (e.g. PBR::Constants) for consistency.
📜 Review details
Configuration used: Path: .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.
📒 Files selected for processing (2)
package/Shaders/Common/PBR.hlsli(1 hunks)package/Shaders/Lighting.hlsl(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
package/Shaders/Lighting.hlslpackage/Shaders/Common/PBR.hlsli
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
package/Shaders/Lighting.hlsl
⏰ 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 (2)
package/Shaders/Common/PBR.hlsli (1)
50-60: LGTM: Centralized PBR bounds improve readability and consistency.Replacing magic numbers with PBR::Constants is a solid refactor with no behavior change.
package/Shaders/Lighting.hlsl (1)
2108-2117: LGTM: Magic-number clamps replaced with PBR::Constants.Behavior preserved; improves maintainability and makes tuning easier.
|
✅ A pre-release build is available for this PR: |
alandtse
changed the title
3 participants
Relates to issue #1353
Summary by CodeRabbit
Refactor
Improvements