fix: PBR generation renders the surface too dark (roughness/metallic FFP wiring) - #795
Conversation
Generating PBR maps in the Material Editor, or the image-to-3D PBR step, made the surface render "darkish, like in shadow". The exported+reloaded material looked fine, which localized the bug to the LIVE bind path. Root cause: the FFP fallback wired the roughness map as LBX_MODULATE_X2 and the metallic map as LBX_ADD_SIGNED against the running colour. Roughness and metallic are BRDF specular-lobe inputs, NOT colour channels — multiplying a map straight into the diffuse tints the whole surface. A typical generated roughness map has mean ~0.44, and MODULATE_X2 multiplies the visible colour by roughness*2 ≈ 0.87 (darker still in rough regions), i.e. the surface is literally multiplied by the roughness map. Measured on the live bind path: lit-face brightness 126 (before) → 150 (after), ~19% recovered. Fix: in the FFP fallback make the roughness and metallic units inert — mark them non-FFP and pass the current colour through unchanged (LBX_SOURCE1), the same treatment normal maps already get. The real metal-roughness BRDF still comes from applyPbrIfTagged's Cook-Torrance SRS when the material is PBR-tagged and IBL is present. Applied in both copies of the wiring (RTShaderHelper::wirePbrSlotsForFFP and MaterialPresetLibrary's configurePbrSlots) plus the stale doc comment in MaterialEditorQML. Adds a regression test asserting roughness/metallic resolve to LBX_SOURCE1 (inert) and albedo stays LBX_MODULATE after wirePbrSlotsForFFP. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMetallic and roughness PBR texture-unit handling changes from FFP color modification to passthrough behavior, with updated comments and regression coverage. Normal-map application is moved to entity-based wiring for generated and edited materials. ChangesPBR FFP passthrough fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660a92700d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ogre::LBS_TEXTURE, | ||
| Ogre::LBS_CURRENT); | ||
| } else if (n == "roughness") { | ||
| } else if (n == "metallic" || n == "roughness") { |
There was a problem hiding this comment.
When the user applies the Specular-Glossiness preset, these same canonical slots are reused as specular/glossiness inputs, and applyPbrIfTagged explicitly leaves that workflow on the slice-E FFP path rather than attaching Cook-Torrance. This unconditional metallic/roughness branch therefore makes specular/glossiness maps inert too, so adding those maps to a spec-gloss material no longer affects the render. Gate the inert behavior to metallic-roughness materials or use distinct handling for the specular-glossiness workflow.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/RTShaderHelper.cpp (1)
810-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHelper name no longer matches its usage.
markNormalUnitNonFfpis now also called formetallic/roughness, not just normal maps. Consider a more generic name (e.g.markUnitInertNonFfp) so future readers aren't misled about its scope.Also applies to: 845-845
🤖 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/RTShaderHelper.cpp` at line 810, The helper name is too specific for its expanded use, since markNormalUnitNonFfp is now applied to metallic/roughness units as well as normal maps. Rename the helper to a more generic symbol such as markUnitInertNonFfp, and update all call sites in RTShaderHelper.cpp so the name matches its broader behavior and future readers are not misled.src/MaterialPresetLibrary.cpp (1)
90-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing this duplicated wiring logic.
The exact same non-FFP/passthrough sequence is now duplicated between here and
RTShaderHelper.cpp'smarkNormalUnitNonFfp. The comment even calls out that this mirrors the other file. This duplication is exactly why the same darkening bug had to be fixed in two places for this PR — extracting a shared helper (exposed viaRTShaderHelper's header) would prevent future drift.🤖 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/MaterialPresetLibrary.cpp` around lines 90 - 101, The non-FFP passthrough setup for metallic/roughness is duplicated here and in RTShaderHelper::markNormalUnitNonFfp, so factor the shared wiring into one helper instead of keeping two copies. Move the _markNonFFP and setColourOperationEx sequence into a reusable RTShaderHelper function exposed through its header, then call that helper from MaterialPresetLibrary::... and the existing RTShaderHelper path. Keep the behavior identical, just centralize the logic to avoid future drift.src/RTShaderHelper_syncMaterial_test.cpp (1)
74-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo equivalent regression test for
MaterialPresetLibrary::configurePbrSlots.This test only covers
RTShaderHelper::wirePbrSlotsForFFP. SinceMaterialPresetLibrary.cppduplicates the identical fix, an analogous assertion there would guard against the two implementations drifting apart again.🤖 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/RTShaderHelper_syncMaterial_test.cpp` around lines 74 - 98, The regression coverage only exercises RTShaderHelper::wirePbrSlotsForFFP, but MaterialPresetLibrary::configurePbrSlots contains the same PBR slot logic and could drift independently. Add an equivalent test alongside PbrFfpRoughnessMetallicDoNotDarkenDiffuse that creates a material, configures albedo/roughness/metallic texture units, calls MaterialPresetLibrary::configurePbrSlots, and asserts the same blend modes so both implementations stay aligned.
🤖 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.
Nitpick comments:
In `@src/MaterialPresetLibrary.cpp`:
- Around line 90-101: The non-FFP passthrough setup for metallic/roughness is
duplicated here and in RTShaderHelper::markNormalUnitNonFfp, so factor the
shared wiring into one helper instead of keeping two copies. Move the
_markNonFFP and setColourOperationEx sequence into a reusable RTShaderHelper
function exposed through its header, then call that helper from
MaterialPresetLibrary::... and the existing RTShaderHelper path. Keep the
behavior identical, just centralize the logic to avoid future drift.
In `@src/RTShaderHelper_syncMaterial_test.cpp`:
- Around line 74-98: The regression coverage only exercises
RTShaderHelper::wirePbrSlotsForFFP, but MaterialPresetLibrary::configurePbrSlots
contains the same PBR slot logic and could drift independently. Add an
equivalent test alongside PbrFfpRoughnessMetallicDoNotDarkenDiffuse that creates
a material, configures albedo/roughness/metallic texture units, calls
MaterialPresetLibrary::configurePbrSlots, and asserts the same blend modes so
both implementations stay aligned.
In `@src/RTShaderHelper.cpp`:
- Line 810: The helper name is too specific for its expanded use, since
markNormalUnitNonFfp is now applied to metallic/roughness units as well as
normal maps. Rename the helper to a more generic symbol such as
markUnitInertNonFfp, and update all call sites in RTShaderHelper.cpp so the name
matches its broader behavior and future readers are not misled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 791fa3e2-415f-49fe-a5bb-41cfc7597515
📒 Files selected for processing (4)
src/MaterialEditorQML.cppsrc/MaterialPresetLibrary.cppsrc/RTShaderHelper.cppsrc/RTShaderHelper_syncMaterial_test.cpp
…ormal map Root cause (distinct from the roughness-tint fix in the previous commit): generating PBR in the Material Editor or the image-to-3D flow wired the RTSS SRS_NORMALMAP sub-render-state onto a mesh that has NO per-vertex tangents. Ogre's normal-map shader needs tangents to build the tangent-space basis; without them the basis is degenerate, the perturbed normal collapses, N·L goes to ~0, and the surface renders as if the lights were off — while every other (imported) mesh lit normally. This is exactly why an export→reload looked correct: the mesh importer already builds tangents before wiring normal maps (MeshImporterExporter::applyNormalMapsToEntity, "Build tangent vectors … required by RTSS normal mapping"); the live generation paths skipped that step. Fix: route both live paths through the same tested importer routine: - image-to-3D (MeshGenBuilder): defer normal-map wiring until after the entity exists, then call applyNormalMapsToEntity (builds tangents when UVs are present, then wires SRS_NORMALMAP). - Material Editor (generatePbrFromDiffuse): after binding the maps, run applyNormalMapsToEntity on every scene entity using the material; fall back to the direct applyNormalMap only when no entity uses it. Result: live PBR generation now lights identically to an export→reload of the same mesh (verified in the viewport). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PbrSlotColourOpsApproximatePbrSemantics still asserted the old FFP ops (metallic=ADD_SIGNED, roughness=MODULATE_X2) that the darkness fix replaced with the inert LBX_SOURCE1 pass-through. Update the expectations (and the comment) to the corrected semantics — roughness/metallic are BRDF inputs and must not modulate the diffuse in the FFP fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…es inert Codex P2: the metallic/roughness canonical slots are reused by the Specular-Glossiness preset as specular + glossiness maps, and that workflow stays on the FFP path (applyPbrIfTagged skips it) — so making them unconditionally inert removed the only thing that made spec-gloss maps visible. Thread the workflow into configurePbrSlots and gate the inert pass-through to metallic-roughness; spec-gloss keeps the legacy ADD_SIGNED (specular) / MODULATE_X2 (glossiness) approximations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@chatgpt-codex-connector re: specular-glossiness slot wiring — fixed in 6e6b65f. |
|
To use Codex here, create an environment for this repo. |
|
1 participant
Summary
Generating PBR maps (Material Editor "Generate PBR maps" or the image-to-3D PBR step) made the model render dark — "like the lights are off on the model," while other meshes lit normally. Export→reload of the same mesh looked correct, which localized both bugs to the live in-session material wiring.
Two independent root causes, both fixed:
1. Roughness/metallic darkened the diffuse (FFP wiring)
The FFP fallback wired
roughnessasLBX_MODULATE_X2andmetallicasLBX_ADD_SIGNEDagainst the running colour — but those are BRDF specular-lobe inputs, not colour channels. A typical generated roughness map (mean ≈ 0.44) multiplied the visible colour byroughness × 2 ≈ 0.87, tinting the surface darker. Fix: make roughness/metallic inert in the FFP path (LBX_SOURCE1, non-FFP), like normal maps; the real contribution comes from the Cook-Torrance SRS when PBR-tagged with IBL. (RTShaderHelper::wirePbrSlotsForFFP+MaterialPresetLibrary::configurePbrSlots.)2. RTSS normal map without tangents → unlit surface (the main "lights off" symptom)
The live paths wired the RTSS
SRS_NORMALMAPsub-render-state on a mesh with no per-vertex tangents. Ogre's normal-map shader needs tangents to build the tangent-space basis; without them the basis is degenerate, the perturbed normal collapses,N·L → ~0, and the surface renders as if unlit. The mesh importer already builds tangents before wiring normal maps (applyNormalMapsToEntity, "Build tangent vectors … required by RTSS normal mapping") — which is exactly why export→reload looked fine but live generation didn't. Fix: route both live paths through that same tested importer routine:MeshGenBuilder): defer normal-map wiring until the entity exists, then callapplyNormalMapsToEntity(builds tangents when UVs exist, then wires SRS_NORMALMAP).generatePbrFromDiffuse): after binding the maps, runapplyNormalMapsToEntityon every scene entity using the material.Verification
LBX_SOURCE1.applyNormalMap); after the fix the image-to-3D generated mesh lights the same as an export→reload (verified in the viewport).Paths covered
Both
wirePbrSlotsForFFP(Material Editor / image-to-3D / CLI / MCP) andapplyNormalMapsToEntityrouting (the two live viewport paths). Export/CLI/MCP paths were already correct.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests