fix(fbx): embed all referenced textures via Video.Content (#508) - #514
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR implements a process-wide cache for embedded textures to enable robust FBX round-tripping. During import, MaterialProcessor captures texture payloads and stores them in EmbeddedTextureCache. During export, FBXExporter queries the cache first, then falls back to Ogre resource groups and filesystem probes, ensuring textures can be embedded even when not present on disk. ChangesEmbedded Texture Round-Trip Caching
Sequence DiagramsequenceDiagram
participant Importer as Assimp Importer
participant MatProc as MaterialProcessor
participant Cache as EmbeddedTextureCache
participant Exporter as FBXExporter
participant ResGroup as Ogre ResourceGroups
participant FS as Filesystem
Importer->>MatProc: loadTexture(embedded bytes)
MatProc->>Cache: store(textureName, bytes)
Note over Cache: Stores embedded texture bytes<br/>indexed by texture name
Exporter->>Cache: retrieve(textureName)
alt Cache hit
Cache-->>Exporter: cached bytes
else Cache miss
Exporter->>ResGroup: findResource(textureName)
alt Resource found
ResGroup-->>Exporter: texture bytes
else Resource not found
Exporter->>FS: probeFilesystem(textureName)
FS-->>Exporter: texture bytes or empty
end
end
Exporter->>Exporter: embed in Video.Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
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: 01c045737e
ℹ️ 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".
| if (auto cached = EmbeddedTextureCache::retrieve(resourceName); | ||
| !cached.empty()) | ||
| return cached; |
There was a problem hiding this comment.
When MaterialProcessor::loadTexture hits aiTexture::mHeight > 0, it stores uncompressed texel bytes in EmbeddedTextureCache (raw pixel buffer), but this exporter path returns that cache entry verbatim and writes it as Video.Content. In that scenario, the FBX payload is not an encoded image stream (PNG/JPEG/etc.), so downstream FBX importers that decode Video.Content as an image file will fail or show corrupted textures. This regression is specific to models with uncompressed embedded textures; they were previously omitted, but are now embedded in an invalid format.
Useful? React with 👍 / 👎.
01c0457 to
6db950d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/EmbeddedTextureCache_test.cpp (1)
31-38: 💤 Low valueClarify test name to reflect actual behavior.
The test name
StoreRejectsEmptyKeyOrBytesimplies validation or an exception, but the test verifies that retrieval returns empty (silent no-op). Consider renaming toEmptyKeyOrBytesResultInEmptyRetrievalor similar.🤖 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/EmbeddedTextureCache_test.cpp` around lines 31 - 38, Rename the test to reflect its actual behavior: change TEST(EmbeddedTextureCacheStandaloneTest, StoreRejectsEmptyKeyOrBytes) to a name like TEST(EmbeddedTextureCacheStandaloneTest, EmptyKeyOrBytesResultInEmptyRetrieval), keeping the body unchanged (calls to EmbeddedTextureCache::clear(), EmbeddedTextureCache::store("", {1,2,3}), EmbeddedTextureCache::store("y.png", {}), and EXPECT_TRUE(EmbeddedTextureCache::retrieve("").empty()) / EXPECT_TRUE(EmbeddedTextureCache::retrieve("y.png").empty())). Update only the test identifier; do not alter EmbeddedTextureCache::store, EmbeddedTextureCache::retrieve, or test assertions.
🤖 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.
Inline comments:
In `@CLAUDE.md`:
- Line 202: Update the sentence describing EmbeddedTextureCache::clear() to be
explicit about timing: state that call EmbeddedTextureCache::clear() immediately
after completing an import session that has finished using
aiScene::GetEmbeddedTexture results (e.g., after MaterialProcessor::loadTexture
and all FBXExporter::readOgreResourceBytes consumers have finished), or before
starting a new, unrelated import session to release retained raw texture bytes
back to the OS; also note it should be invoked whenever memory usage from cached
textures becomes a concern (large scenes or batch imports).
In `@src/EmbeddedTextureCache_test.cpp`:
- Around line 5-47: Tests only cover the vector overload; add a unit that
exercises the raw-buffer store overload (call
EmbeddedTextureCache::store("raw.png", rawData, sizeof(rawData)) with a uint8_t
array, then retrieve and ASSERT_EQ size and check first/last bytes) and add a
basic concurrency test to validate thread-safety by launching multiple
std::thread writers that call
EmbeddedTextureCache::store("thread_"+std::to_string(id)+".png", data) in
parallel, join them, then retrieve each key and EXPECT_EQ the sizes/contents;
use EmbeddedTextureCache::clear() at test start and include <thread> in the test
file.
---
Nitpick comments:
In `@src/EmbeddedTextureCache_test.cpp`:
- Around line 31-38: Rename the test to reflect its actual behavior: change
TEST(EmbeddedTextureCacheStandaloneTest, StoreRejectsEmptyKeyOrBytes) to a name
like TEST(EmbeddedTextureCacheStandaloneTest,
EmptyKeyOrBytesResultInEmptyRetrieval), keeping the body unchanged (calls to
EmbeddedTextureCache::clear(), EmbeddedTextureCache::store("", {1,2,3}),
EmbeddedTextureCache::store("y.png", {}), and
EXPECT_TRUE(EmbeddedTextureCache::retrieve("").empty()) /
EXPECT_TRUE(EmbeddedTextureCache::retrieve("y.png").empty())). Update only the
test identifier; do not alter EmbeddedTextureCache::store,
EmbeddedTextureCache::retrieve, or test assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13214ce2-53c1-4cc0-9d3d-be4ff5cb7e70
📒 Files selected for processing (8)
CLAUDE.mdsrc/Assimp/MaterialProcessor.cppsrc/CMakeLists.txtsrc/EmbeddedTextureCache.cppsrc/EmbeddedTextureCache.hsrc/EmbeddedTextureCache_test.cppsrc/FBX/FBXExporter.cpptests/CMakeLists.txt
Round-tripping Rumba Dancing.fbx through any FBX-output qtmesh path
(convert / fix / optimize / anim --simplify) produced an FBX whose
re-import rendered without the normal map: the texture reference was
present in the FBX properties, but the `Video.Content` block was
missing, so a downstream loader that depends on the embedded payload
saw "missing normal map" and reverted to flat-lit geometry.
Root cause: Boss_normal.png was an inline texture inside the source
FBX. Assimp's importer extracts the raw bytes via
`scene->GetEmbeddedTexture(...)` and hands them to Ogre via
`TextureManager::loadImage` / `loadRawData`. Those calls feed the GPU
texture but discard the original bytes — the texture's `getOrigin()`
is empty, it lives only in the GPU cache, and the resource-group file
index has no entry for it. `FBXExporter::readOgreResourceBytes` then
hits all three failure paths simultaneously (`findGroupContainingResource`
throws ItemIdentityException, `resourceExists(group, name)` returns
false for every group, `getByName(name, AUTODETECT)` finds the texture
but the group's archives have no file). The Content block is gated on
non-empty bytes, so the exporter silently emits the reference without
the payload.
New pure-data module `EmbeddedTextureCache` (`src/EmbeddedTextureCache.{h,cpp}`)
is a thread-safe process-wide map from texture name → raw bytes.
`MaterialProcessor::loadTexture` stashes the compressed bytes the
moment `GetEmbeddedTexture` produces them (both the
`mHeight == 0`/compressed and uncompressed branches).
`FBXExporter::readOgreResourceBytes` queries the cache first, then
falls through to the existing resource-group / filesystem-probe paths.
A third fallback was added — walk every registered resource location
and probe disk — for textures that landed on disk *after* the group's
file index was built (also relevant to issue #508 secondary cases).
Diagnostic logging used during the debug session was removed before
commit.
Convert: source 6.3MB → output 3.4MB (was 2.4MB before the fix, missing
~1MB of normal-map payload). Output contains 2 `Content` tags, one per
texture. Re-importing the converted FBX from an isolated directory
that has no Boss_*.png next to it loads both textures cleanly — no
"MaterialProcessor: Failed to load normal map" warning. Same shape on
the optimize pipeline output (3.4MB, 2 Content tags).
Unit tests for the cache (round-trip, missing-key empty, replace-prior,
reject-empty, clear) are auto-discovered by the gtest glob and run on
Linux CI under Xvfb.
Closes #508.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6db950d to
1dfc3a3
Compare
|
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests