fix(truepbr): apply vertex color to base color for pbr metals#2060
Conversation
📝 WalkthroughWalkthroughTRUE_PBR shader path now multiplies baseColor and emissive by vertex color; skylighting vertexColor is forced to 1 for TRUE_PBR while non-PBR retains vertex-color behavior (HAIR/CS_HAIR exceptions preserved). Changes
Sequence Diagram(s)(omitted — changes are localized shader logic without multi-component sequential interactions that benefit from a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 `@package/Shaders/Lighting.hlsl`:
- Line 2758: Add a short explanatory comment above the emissive tint line to
document why vertex color is applied to emissive (emitColor *= input.Color.xyz):
reference that this mirrors the base color vertex tint (see the base color tint
at the earlier base color multiply, e.g., baseColor *= input.Color.xyz) and
state the intent/rationale (consistency for PBR metals or intentional artistic
tinting), and note any exceptions (UI/decals or cases where emissive should
remain untinted) so future readers understand the behavior.
🪄 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: d656635d-fdca-4346-a51b-913fc44488af
📒 Files selected for processing (1)
package/Shaders/Lighting.hlsl
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package/Shaders/Lighting.hlsl (1)
1-1: Tighten PR title format and add issue linkage.Suggested title (<= 50 chars, conventional style):
fix(truepbr): apply vertex color to pbr metal baseIf this PR closes a tracked bug, add a keyword reference like
Fixes #<id>in the description.As per coding guidelines: provide conventional commit titles (
type(scope): description, <=50 chars) and include issue references when fixing bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` at line 1, The PR title and description do not follow the repository's conventional commit format and lack an issue linkage; rename the PR title to a concise conventional commit like "fix(truepbr): apply vertex color to pbr metal base" and, if this change closes a tracked bug, add an issue-closing keyword such as "Fixes #<id>" in the PR description so reviewers and CI can track it; ensure the updated title and description appear alongside the change that defines LIGHTING (symbol: LIGHTING) so the intent of the shader fix is clear.
🤖 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/Lighting.hlsl`:
- Around line 2758-2760: The emissive vertex tint (emitColor *= input.Color.xyz)
is being applied for non-PBR paths as well, causing double vertex-color
multiplication when color.xyz is already multiplied by vertexColor; modify the
shader so the emitColor *= input.Color.xyz line is executed only when TRUE_PBR
is enabled (i.e., gate it behind the TRUE_PBR macro/flag), ensuring non-PBR
codepaths that already do color.xyz *= vertexColor are not tinted twice.
---
Nitpick comments:
In `@package/Shaders/Lighting.hlsl`:
- Line 1: The PR title and description do not follow the repository's
conventional commit format and lack an issue linkage; rename the PR title to a
concise conventional commit like "fix(truepbr): apply vertex color to pbr metal
base" and, if this change closes a tracked bug, add an issue-closing keyword
such as "Fixes #<id>" in the PR description so reviewers and CI can track it;
ensure the updated title and description appear alongside the change that
defines LIGHTING (symbol: LIGHTING) so the intent of the shader fix is clear.
🪄 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: 37b06e4c-abca-4a02-92fb-e753f9d29abc
📒 Files selected for processing (1)
package/Shaders/Lighting.hlsl
There was a problem hiding this comment.
🧹 Nitpick comments (2)
package/Shaders/Lighting.hlsl (2)
2869-2873: Same suggestion: consider adding a brief comment.For consistency and clarity, a similar comment would help here.
📝 Suggested comment addition
# if defined(TRUE_PBR) + // Vertex color already applied to baseColor; set to 1 to avoid double application float3 vertexColor = 1; # else float3 vertexColor = input.Color.xyz; # endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 2869 - 2873, Add a brief clarifying comment above the preprocessor block that sets vertexColor to 1 when TRUE_PBR is defined and to input.Color.xyz otherwise; reference the TRUE_PBR macro and the vertexColor variable (and input.Color.xyz) so readers understand why we ignore per-vertex color in PBR mode and use it otherwise.
2865-2867: Consider adding a brief comment explaining the override.The code correctly forces
vertexColor = 1for TRUE_PBR to prevent double application (since vertex color was already applied tobaseColorat line 2121). However, unlike the emissive block which has a detailed comment, this override lacks explanation. A brief comment would improve maintainability.📝 Suggested comment addition
# if defined(TRUE_PBR) + // Vertex color already applied to baseColor; set to 1 to avoid double application vertexColor = 1; # endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Lighting.hlsl` around lines 2865 - 2867, Add a brief comment above the TRUE_PBR override that sets vertexColor = 1 explaining why it is forced (e.g., to prevent double application because vertex color was already baked into baseColor earlier), similar to the emissive comment style; update the block containing the TRUE_PBR macro and the vertexColor assignment to include that explanatory note referencing vertexColor and TRUE_PBR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 2869-2873: Add a brief clarifying comment above the preprocessor
block that sets vertexColor to 1 when TRUE_PBR is defined and to input.Color.xyz
otherwise; reference the TRUE_PBR macro and the vertexColor variable (and
input.Color.xyz) so readers understand why we ignore per-vertex color in PBR
mode and use it otherwise.
- Around line 2865-2867: Add a brief comment above the TRUE_PBR override that
sets vertexColor = 1 explaining why it is forced (e.g., to prevent double
application because vertex color was already baked into baseColor earlier),
similar to the emissive comment style; update the block containing the TRUE_PBR
macro and the vertexColor assignment to include that explanatory note
referencing vertexColor and TRUE_PBR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aaafc6a9-5f03-487d-9716-820e82c99a7d
📒 Files selected for processing (1)
package/Shaders/Lighting.hlsl
…ity-shaders#2060) (cherry picked from commit 554d425)
3 participants
Summary by CodeRabbit