fix(truepbr): resolve MATO swap bug#2046
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPer-material “fork-before-write” added to TESBoundObject_Clone3D: it reads static Changes
Sequence DiagramsequenceDiagram
participant Clone3D as TESBoundObject_Clone3D
participant Mato as MaterialObj (currentMato)
participant PBR as PBR Data (pbrData)
participant ShMat as BSLightingShaderMaterialPBR (pooled)
participant Ext as MaterialExtensions
participant Refr as TESObjectREFR
Clone3D->>Mato: read materialObj (currentMato)
Clone3D->>PBR: GetPBRMaterialObjectData(currentMato)
alt pbrData present
Clone3D->>ShMat: inspect Ext (materialObjectData, lastOwnerRefFormID)
Clone3D->>Refr: get ref->GetFormID()
alt ownership or pointer conflict
Clone3D->>ShMat: clone via Make()
ShMat-->>Clone3D: cloned material
Clone3D->>ShMat: CopyMembers into clone
Clone3D->>Clone3D: rebind shaderProperty->material to clone
else no conflict
Note over Clone3D,ShMat: reuse pooled material
end
Clone3D->>ShMat: ApplyMaterialObjectData(pbrData)
Clone3D->>Ext: set materialObjectData = pbrData, lastOwnerRefFormID = refID
else no pbrData
Clone3D->>ShMat: ClearMaterialObjectData()
Clone3D->>Ext: set materialObjectData = nullptr, lastOwnerRefFormID = 0
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
Author
|
Addresses issue #1816 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1343-1386:⚠️ Potential issue | 🟠 MajorMissing handling for references without PBR MATO config causes stale data persistence.
When
pbrDataisnullptr(the MATO exists but has no PBR config), the code skips the entire block. However, if this geometry was previously used by a different ref that did have PBR MATO data, the pooled material still carries the old projected values fromCopyMembers. The newly addedClearMaterialObjectData()method was designed for exactly this case but is never invoked.🐛 Proposed fix to clear stale data when no PBR config exists
if (currentMato != nullptr && currentMato->directionalData.singlePass) { auto* pbrData = truePBR->GetPBRMaterialObjectData(currentMato); if (pbrData != nullptr) { RE::BSVisit::TraverseScenegraphGeometries(result, [pbrData, ref](RE::BSGeometry* geometry) { // ... existing fork-before-write logic ... return RE::BSVisit::BSVisitControl::kContinue; }); + } else { + // Clear stale MATO data on materials that may have been + // contaminated by a previous owner's PBR config. + RE::BSVisit::TraverseScenegraphGeometries(result, [](RE::BSGeometry* geometry) { + if (auto* shaderProperty = static_cast<RE::BSShaderProperty*>(geometry->GetGeometryRuntimeData().shaderProperty.get())) { + if (shaderProperty->GetMaterialType() == RE::BSShaderMaterial::Type::kLighting && + shaderProperty->flags.any(RE::BSShaderProperty::EShaderPropertyFlag::kVertexLighting)) { + if (auto* material = static_cast<BSLightingShaderMaterialPBR*>(shaderProperty->material)) { + auto& ext = BSLightingShaderMaterialPBR::All[material]; + if (ext.lastOwnerRefFormID != 0) { + material->ClearMaterialObjectData(); + ext.materialObjectData = nullptr; + ext.lastOwnerRefFormID = 0; + } + } + } + } + return RE::BSVisit::BSVisitControl::kContinue; + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TruePBR.cpp` around lines 1343 - 1386, When pbrData is nullptr we must clear any previously-stored PBR data on encountered materials to prevent stale projected values; inside the RE::BSVisit::TraverseScenegraphGeometries lambda (where you inspect shaderProperty and cast to BSLightingShaderMaterialPBR), add an else branch for the pbrData==nullptr case that retrieves the extension via BSLightingShaderMaterialPBR::All[material] and calls its ClearMaterialObjectData() and resets ext.lastOwnerRefFormID (and ext.materialObjectData = nullptr if needed) so pooled materials no longer carry the previous ref's projected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/TruePBR.cpp`:
- Around line 1367-1374: When BSLightingShaderMaterialPBR::Make() can return
nullptr, avoid leaving targetMat pointing at the original material and writing
into shared state; after calling BSLightingShaderMaterialPBR::Make() in the
wouldContaminate branch, check if freshMat is null and if so log a warning (or
early-return/skip) and do not assign shaderProperty->material or set targetMat,
so ApplyMaterialObjectData will not modify the original material; only assign
shaderProperty->material = freshMat and set targetMat when freshMat is non-null.
In `@src/TruePBR/BSLightingShaderMaterialPBR.h`:
- Around line 71-74: When TESBoundObject_Clone3D::thunk checks for pbrData, it
currently only handles the pbrData != nullptr case and leaves pooled material
fields populated by CopyMembers when pbrData == nullptr; add an else branch
after that if to call targetMat->ClearMaterialObjectData() so stale
projected-material fields are reset on materials that lack a MATO/PBR config
(use the ClearMaterialObjectData method on the targetMat instance).
---
Outside diff comments:
In `@src/TruePBR.cpp`:
- Around line 1343-1386: When pbrData is nullptr we must clear any
previously-stored PBR data on encountered materials to prevent stale projected
values; inside the RE::BSVisit::TraverseScenegraphGeometries lambda (where you
inspect shaderProperty and cast to BSLightingShaderMaterialPBR), add an else
branch for the pbrData==nullptr case that retrieves the extension via
BSLightingShaderMaterialPBR::All[material] and calls its
ClearMaterialObjectData() and resets ext.lastOwnerRefFormID (and
ext.materialObjectData = nullptr if needed) so pooled materials no longer carry
the previous ref's projected values.
🪄 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: 91d0fcda-3c00-44be-9d1d-c1dc40261a03
📒 Files selected for processing (3)
src/TruePBR.cppsrc/TruePBR/BSLightingShaderMaterialPBR.cppsrc/TruePBR/BSLightingShaderMaterialPBR.h
|
✅ A pre-release build is available for this PR: |
alandtse
changed the title
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/TruePBR.cpp (2)
1367-1376:⚠️ Potential issue | 🟡 MinorDo not fall through to
ApplyMaterialObjectData()after a failed clone.If
Make()returnsnullptr,targetMatis still the sharedmaterial, so Line 1376 reintroduces the cross-ref contamination this branch is trying to prevent. Log and returnkContinueinstead of falling through.🛡️ Suggested fix
if (wouldContaminate) { auto* freshMat = BSLightingShaderMaterialPBR::Make(); if (freshMat) { freshMat->CopyMembers(material); shaderProperty->material = freshMat; targetMat = freshMat; + } else { + logger::warn("[TruePBR] Failed to clone PBR material for ref {:08X}; skipping per-ref MATO write", ref->GetFormID()); + return RE::BSVisit::BSVisitControl::kContinue; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TruePBR.cpp` around lines 1367 - 1376, The branch that tries to clone the material using BSLightingShaderMaterialPBR::Make() must not fall through to call ApplyMaterialObjectData on the original shared material when the clone fails: after attempting to create freshMat, if freshMat is nullptr, log an error (including context such as shaderProperty and material pointers) and return kContinue immediately instead of proceeding; otherwise keep the existing CopyMembers, assignment to shaderProperty->material and setting of targetMat, and then call targetMat->ApplyMaterialObjectData(*pbrData) only when targetMat is the valid fresh clone.
1345-1387:⚠️ Potential issue | 🟠 MajorClear projected MATO state when no usable MATO config exists.
src/TruePBR/BSLightingShaderMaterialPBR.hLines 71-74 addClearMaterialObjectData(), but this branch only handlespbrData != nullptr. WhencurrentMatois null,singlePassis false, orGetPBRMaterialObjectData()returns null, the values copied in byCopyMembers()persist on the pooled material and a ref with no valid MATO can inherit the previous ref's projected settings. Mirror the traversal with a clear pass in the non-apply path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TruePBR.cpp` around lines 1345 - 1387, The traversal currently only applies PBR data when pbrData != nullptr, leaving pooled BSLightingShaderMaterialPBR instances contaminated when currentMato is null/singlePass is false or GetPBRMaterialObjectData() returns null; add a mirrored traversal for the same RE::BSVisit::TraverseScenegraphGeometries scope that, when no usable MATO exists, visits each geometry's shaderProperty (same check for kLighting and kVertexLighting), obtains the BSLightingShaderMaterialPBR instance (from shaderProperty->material and BSLightingShaderMaterialPBR::All), and clears the projected state by calling the new ClearMaterialObjectData() (or resetting materialObjectData/lastOwnerRefFormID) on the target material (handling fork-before-write cloning the same way as the apply path via CopyMembers/Make if the material is owned by another ref) so no ref inherits a previous ref's projected settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/TruePBR.cpp`:
- Around line 1353-1363: The current wouldContaminate check only compares
prevOwnerRefFormID and baseColorScale; change it to compare the previous MATO
identity/payload instead: use ext.materialObjectData (or the full projected
material payload stored on ext) against the incoming pbrData projected payload
rather than prevColorScale alone. Update the predicate that sets
wouldContaminate (near BSLightingShaderMaterialPBR::All[material], using
ext.lastOwnerRefFormID and ext.materialObjectData) so it returns true if the
previous materialObjectData differs from the new pbrData projected payload (or
other MATO fields like roughness/specular/glint), ensuring a clone is made when
the prior MATO differs even if baseColorScale matches.
In `@src/TruePBR/BSLightingShaderMaterialPBR.h`:
- Around line 40-43: ClearMaterialObjectData currently only resets projected
shader values; update it to also clear the material extension entry and owner ID
so pooled materials don't retain prior state. In the ClearMaterialObjectData
implementation reset/erase extensions.materialObjectData (e.g. set to
null/empty/std::nullopt as used elsewhere) and set lastOwnerRefFormID = 0 so the
fork-before-write check and live-editor propagation no longer see stale data;
touch the function that currently manipulates projected shader values to include
these two clears.
---
Duplicate comments:
In `@src/TruePBR.cpp`:
- Around line 1367-1376: The branch that tries to clone the material using
BSLightingShaderMaterialPBR::Make() must not fall through to call
ApplyMaterialObjectData on the original shared material when the clone fails:
after attempting to create freshMat, if freshMat is nullptr, log an error
(including context such as shaderProperty and material pointers) and return
kContinue immediately instead of proceeding; otherwise keep the existing
CopyMembers, assignment to shaderProperty->material and setting of targetMat,
and then call targetMat->ApplyMaterialObjectData(*pbrData) only when targetMat
is the valid fresh clone.
- Around line 1345-1387: The traversal currently only applies PBR data when
pbrData != nullptr, leaving pooled BSLightingShaderMaterialPBR instances
contaminated when currentMato is null/singlePass is false or
GetPBRMaterialObjectData() returns null; add a mirrored traversal for the same
RE::BSVisit::TraverseScenegraphGeometries scope that, when no usable MATO
exists, visits each geometry's shaderProperty (same check for kLighting and
kVertexLighting), obtains the BSLightingShaderMaterialPBR instance (from
shaderProperty->material and BSLightingShaderMaterialPBR::All), and clears the
projected state by calling the new ClearMaterialObjectData() (or resetting
materialObjectData/lastOwnerRefFormID) on the target material (handling
fork-before-write cloning the same way as the apply path via CopyMembers/Make if
the material is owned by another ref) so no ref inherits a previous ref's
projected settings.
🪄 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: 791fabac-87dd-4d20-9fc1-c52ca424ddb4
📒 Files selected for processing (3)
src/TruePBR.cppsrc/TruePBR/BSLightingShaderMaterialPBR.cppsrc/TruePBR/BSLightingShaderMaterialPBR.h
✅ Files skipped from review due to trivial changes (1)
- src/TruePBR/BSLightingShaderMaterialPBR.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/TruePBR.cpp (1)
1353-1363:⚠️ Potential issue | 🟠 MajorClone detection still ignores non-color MATO differences.
wouldContaminateonly keys onbaseColorScale, so two refs with the same tint but different roughness, specular, or glint settings will still share and overwrite the same pooled material.💡 Minimal fix
- const auto prevColorScale = material->GetProjectedMaterialBaseColorScale(); + const auto* prevMaterialObjectData = ext.materialObjectData; @@ const bool wouldContaminate = (prevOwnerRefID != 0) && (prevOwnerRefID != ref->GetFormID()) && - (prevColorScale != pbrData->baseColorScale); + (prevMaterialObjectData != nullptr) && + (prevMaterialObjectData != pbrData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TruePBR.cpp` around lines 1353 - 1363, wouldContaminate currently only compares baseColorScale (material->GetProjectedMaterialBaseColorScale()) so materials with identical tint but differing MATO properties (roughness, specular, glint, etc.) will still be shared; update the check to compare the full projected MATO state instead of just baseColorScale — either add a helper like ProjectedMatoEquals(material, pbrData) or compare all relevant pbrData fields (e.g., roughness, specular, glint flags) against their projected equivalents on the material instance and use that combined equality in the wouldContaminate expression (references: BSLightingShaderMaterialPBR::All, ext.lastOwnerRefFormID, material->GetProjectedMaterialBaseColorScale(), pbrData->baseColorScale).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/TruePBR.cpp`:
- Around line 1345-1347: The cleanup only runs when currentMato exists and is
singlePass but lacks PBR data, leaving projected values intact when currentMato
is null or not singlePass; change the logic around the block that calls
truePBR->GetPBRMaterialObjectData(currentMato) (and the similar block later) so
that the same "clear projected values" cleanup path is executed whenever
currentMato is null, currentMato->directionalData.singlePass is false, or
GetPBRMaterialObjectData(currentMato) returns nullptr, rather than only when all
three conditions are true; update the conditional(s) guarding the cleanup to
explicitly test for these negative cases and invoke the existing clear/reset
code for projected values.
---
Duplicate comments:
In `@src/TruePBR.cpp`:
- Around line 1353-1363: wouldContaminate currently only compares baseColorScale
(material->GetProjectedMaterialBaseColorScale()) so materials with identical
tint but differing MATO properties (roughness, specular, glint, etc.) will still
be shared; update the check to compare the full projected MATO state instead of
just baseColorScale — either add a helper like ProjectedMatoEquals(material,
pbrData) or compare all relevant pbrData fields (e.g., roughness, specular,
glint flags) against their projected equivalents on the material instance and
use that combined equality in the wouldContaminate expression (references:
BSLightingShaderMaterialPBR::All, ext.lastOwnerRefFormID,
material->GetProjectedMaterialBaseColorScale(), pbrData->baseColorScale).
🪄 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: 4e5a4d9c-052b-45eb-949b-94be4c36b09b
📒 Files selected for processing (1)
src/TruePBR.cpp
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
alandtse
approved these changes
Apr 4, 2026
Collaborator
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
In the future, resolving the AI may just involve reading and just hitting the resolve button. The AI is a check that you should consider but doesn't always require any changes. The AI can be wrong.
alandtse
pushed a commit
to alandtse/open-shaders
that referenced
this pull request
Apr 5, 2026
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
ParticleTroned
pushed a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
May 2, 2026
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit ca02a0b)
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
2 participants
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.
Summary by CodeRabbit