Repository navigation
Applying a material override merges the local textures it tracks into the render material - #451
Merged
Merged
Conversation
… the render material applyOverride meant to merge the override's tracking map into the render material's, but passed one iterator as both ends of the range, so std::map inserted an empty range and merged nothing. It has been that way since local texture tracking came in with upstream's SL-20523 (begin() as both ends then; the hashing rework later shortened it to the same iterator twice). A local texture set on an override was never tracked on the render material: updateTextureTracking never associated it with its local bitmap, so the object did not follow changes to the file, and the local texture digest the hash carries left it out. Where both track a local texture, the override's entry now wins, as its texture ids do and as every other write to the map assigns. On an LLFetchedGLTFMaterial the choice is brief anyway: updateTextureTracking associates each tracking id with its bitmap, which resets the entry to the bitmap's current world id. The test applies an override carrying a tracked local texture and checks that the render material keeps its own entry, gains the override's, finds both in updateTextureTracking and digests both; and that the override's entry wins a tracking id both hold. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 47 minutes. View limit details
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 |
RyeMutt
added a commit
that referenced
this pull request
Oct 12, 2026
Two pull requests merged into develop on the same morning. #451 added a TUT test<13> to llgltfmaterial_test.cpp, which #449 had converted to Catch2, so llprimitive's tests no longer compiled anywhere: llgltfmaterial_object_t no longer exists. Its two parts are now two TEST_CASEs, as the rest of the file is written. Each finding of a tracked texture is a REQUIRE before the entry is read; the rest are CHECKs. What they check is unchanged: the override's local texture tracked beside the material's own, both found by updateTextureTracking and covered by the digest, and the override's world id winning where both track the same texture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
LLGLTFMaterial::applyOverridewas meant to merge the override's local texture tracking map into the render material's, but it passed one iterator as both ends of the range:std::maptakes that as an empty range, so nothing was merged. This dates from upstream's SL-20523, where the line wasinsert(begin(), begin()); the later hashing rework shortened it and kept the empty range.As a result, a local texture set on a material override was never tracked on the render material.
updateTextureTrackingnever linked the render material to that local bitmap, so the object didn't update when the file changed, and the local texture digest included in the material's hash left it out.Keep or overwrite
Where the base and the override both track a local texture, the override's entry now wins (
insert_or_assign). That matches the rest ofapplyOverride, where the override's texture ids replace the base's, and every other write to the map already overwrites. In practice the two values almost never differ:LLLocalBitmap::updateGLTFMaterialsupdates every registered material before reapplying overrides, and on anLLFetchedGLTFMaterial,updateTextureTrackingresets each entry to the bitmap's current world id anyway.Test
llgltfmaterial_test.cppgets a new TUTtest<13>:updateTextureTracking(seen through a small subclass) finds both, and the digest covers both.Checked:
insert(keep), only the both-track check fails, so it locks in the overwrite choiceninja-os,-DAL_BUILD_TESTS=ON, RelWithDebInfo, macOS)Whichever of this and #449 merges second will need to convert
test<13>to Catch2.🤖 Generated with Claude Code