fix(truepbr): apply MATO rbg scalars through cbuffer#2310
Conversation
📝 WalkthroughWalkthroughThis PR adds a new shader constant ChangesMaterialObjectRGBScale Shader Constant
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): [00.14][ERROR]: Error: exception Unix_error: No such file or directory stat src/ShaderCache.cpp Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TruePBR.cpp (1)
935-940:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
PBRProjectedUVParams1andPBRProjectedUVParams2to avoid uninitialized values in constant buffersLines 935-940:
PBRProjectedUVParams1is declared as a 4-element array but only indices 0–2 are filled. The 4th element remains uninitialized garbage and gets uploaded to the cbuffer.Additionally,
PBRProjectedUVParams2(lines 941–944) has the same issue: only indices 0–1 are filled, leaving indices 2–3 uninitialized.Since
GetProjectedMaterialBaseColorScale()returns only 3 floats and the other projected parameters are scalars, use 3-element and 2-element arrays respectively with{}initialization, or initialize the unused indices explicitly.Suggested fix
- std::array<float, 4> PBRProjectedUVParams1; + std::array<float, 3> PBRProjectedUVParams1{}; PBRProjectedUVParams1[0] = pbrMaterial->GetProjectedMaterialBaseColorScale()[0]; PBRProjectedUVParams1[1] = pbrMaterial->GetProjectedMaterialBaseColorScale()[1]; PBRProjectedUVParams1[2] = pbrMaterial->GetProjectedMaterialBaseColorScale()[2]; shadowState->SetPSConstant(PBRProjectedUVParams1, RE::BSGraphics::ConstantGroupLevel::PerMaterial, lightingPSConstants.MaterialObjectRGBScale); - std::array<float, 4> PBRProjectedUVParams2; + std::array<float, 2> PBRProjectedUVParams2{}; PBRProjectedUVParams2[0] = pbrMaterial->GetProjectedMaterialRoughness(); PBRProjectedUVParams2[1] = pbrMaterial->GetProjectedMaterialSpecularLevel(); shadowState->SetPSConstant(PBRProjectedUVParams2, RE::BSGraphics::ConstantGroupLevel::PerMaterial, lightingPSConstants.ParallaxOccData);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/TruePBR.cpp` around lines 935 - 940, PBRProjectedUVParams1 and PBRProjectedUVParams2 are partially filled and leave elements uninitialized before calling shadowState->SetPSConstant; fix by fully initializing the arrays (e.g., use brace/init-zero or explicitly set the unused indices) so all 4 elements passed to SetPSConstant are deterministic; locate the declarations PBRProjectedUVParams1 and PBRProjectedUVParams2 near the GetProjectedMaterialBaseColorScale() calls and ensure index 3 (and indices 2–3 for PBRProjectedUVParams2) are set to 0.0f (or change the array size to match the actual data if SetPSConstant supports smaller sizes) before invoking shadowState->SetPSConstant with lightingPSConstants.MaterialObjectRGBScale and the other constant slots.
🧹 Nitpick comments (1)
src/TruePBR.cpp (1)
1-1: PR metadata polish suggestionConsider renaming the title to fix the typo (
rbg→rgb), e.g.fix(truepbr): apply mato rgb scalars via dedicated cbuffer.
If this resolves a tracked bug, add an issue keyword likeFixes #<id>/Closes #<id>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/TruePBR.cpp` at line 1, Update the PR title to correct the typo "rbg" → "rgb" (e.g., use "fix(truepbr): apply mato rgb scalars via dedicated cbuffer") and, if this change closes a tracked bug, append the appropriate issue keyword like "Fixes #<id>" or "Closes #<id>" to the title or PR description so GitHub links the issue automatically; ensure the component name "truepbr" and wording "mato rgb scalars" remain accurate in the revised title.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/TruePBR.cpp`:
- Around line 935-940: PBRProjectedUVParams1 and PBRProjectedUVParams2 are
partially filled and leave elements uninitialized before calling
shadowState->SetPSConstant; fix by fully initializing the arrays (e.g., use
brace/init-zero or explicitly set the unused indices) so all 4 elements passed
to SetPSConstant are deterministic; locate the declarations
PBRProjectedUVParams1 and PBRProjectedUVParams2 near the
GetProjectedMaterialBaseColorScale() calls and ensure index 3 (and indices 2–3
for PBRProjectedUVParams2) are set to 0.0f (or change the array size to match
the actual data if SetPSConstant supports smaller sizes) before invoking
shadowState->SetPSConstant with lightingPSConstants.MaterialObjectRGBScale and
the other constant slots.
---
Nitpick comments:
In `@src/TruePBR.cpp`:
- Line 1: Update the PR title to correct the typo "rbg" → "rgb" (e.g., use
"fix(truepbr): apply mato rgb scalars via dedicated cbuffer") and, if this
change closes a tracked bug, append the appropriate issue keyword like "Fixes
#<id>" or "Closes #<id>" to the title or PR description so GitHub links the
issue automatically; ensure the component name "truepbr" and wording "mato rgb
scalars" remain accurate in the revised title.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f42a64b8-560e-4db0-af03-4da3b2544074
📒 Files selected for processing (4)
package/Shaders/Lighting.hlslsrc/ShaderCache.cppsrc/ShaderCache.hsrc/TruePBR.cpp
|
✅ A pre-release build is available for this PR: |
alandtse
changed the title
(cherry picked from commit d026838)
(cherry picked from commit d026838)
3 participants
Previously, MATO RGB slider values from the TruePBR section of the menu were being passed to Lighting.hlsl via the PS constant EnvmapData, which meant only R was valid, and G and B were either corrupting the intended use for those slots, or were lost entirely.
This change creates a new dedicated PS constant specifically for the RGB slider scalars, which then is multiplied with the projected material color in the Lighting shader.
Summary by CodeRabbit