FBX export: embed textures (no sidecar .material/.png) - #311
Conversation
Export Video.Content as raw bytes (FBX 'R') so textures are embedded inside the .fbx, matching Mixamo-style single-file exports. The exporter now resolves texture resources by scanning resource groups. Tests: provide a real PNG resource for coverage and assert Video.Content exists; also guard coverage tests behind canLoadMeshFiles() to avoid segfaults when no GL context is available. Made-with: Cursor
FBX exports should be single-file when textures are embedded. Skip exportMaterial()/exportTextures sidecar dumping for FBX in the entity export path, the Assimp export path, and pose export path. Made-with: Cursor
📝 WalkthroughWalkthroughAdds conditional embedding of texture payloads into FBX Video objects by resolving Ogre texture resources to streams, reading bytes into vectors, emitting Changes
Sequence DiagramsequenceDiagram
participant Exporter as FBXExporter
participant OgreRM as Ogre Resource Manager
participant Stream as Resource Stream
participant FBX as FBX Video Object
Exporter->>OgreRM: resolve texture name (preferred group → default → all groups)
OgreRM-->>Exporter: DataStreamPtr (or not found)
Exporter->>Stream: read bytes (use size or chunked reads)
Stream-->>Exporter: byte[] payload
Exporter->>FBX: create Video node
Exporter->>FBX: emit Content child with property 'R' and payload
FBX-->>Exporter: Video.Content embedded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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: 6b42410863
ℹ️ 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".
| bool ok = FBXExporter::exportFBX(e, _uri); | ||
| if (ok) | ||
| exportMaterial(e, file); | ||
| else | ||
| // FBXExporter embeds textures (Video.Content) so avoid emitting sidecar | ||
| // .material and extracted image files next to the FBX. | ||
| if (!ok) | ||
| return -1; |
There was a problem hiding this comment.
This branch now suppresses exportMaterial for every FBX export, but FBXExporter only embeds Video.Content when readOgreResourceBytes can reopen the texture via ResourceGroupManager. Textures created from embedded imports are loaded in-memory (MaterialProcessor::loadTexture uses TextureManager::loadImage/loadRawData) and are not guaranteed to be reopenable as file resources, so Content can be omitted while sidecar images are no longer written. In that case the FBX still references texture names but ships no texture data, causing missing textures on re-import.
Useful? React with 👍 / 👎.
| if (fmt != "FBX Binary (*.fbx)") | ||
| exportMaterial(entity, file); |
There was a problem hiding this comment.
In exportCurrentPose, FBX output goes through Assimp and this function builds materials with texture path properties only; it does not populate embedded aiTexture payloads. Skipping exportMaterial for fmt == "FBX Binary (*.fbx)" therefore removes the only code path that writes texture files for this export mode, so pose FBX exports can lose textures unless matching files already exist beside the output.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/FBX/FBXExporter_test.cpp (1)
474-500: Assert the embedded payload bytes, not just thatContentis non-empty.Right now this still passes if the exporter resolves a different
diffuse_tex.pngfrom another resource group. Comparingcontent->properties[0].stringValagainst thepngbytes written inSetUp()would make the test catch wrong-resource regressions.Also applies to: 1921-1926
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FBX/FBXExporter_test.cpp` around lines 474 - 500, The test currently only checks that the exported Video.Content is non-empty, which can pass if a different texture is picked up; modify the FBXExporter_test (the block that writes the 1x1 PNG into texPath using the png QByteArray) to assert that the embedded payload equals the exact png bytes written by SetUp(), e.g. compare content->properties[0].stringVal (or the Video.Content buffer extracted from the exporter result) against the original png QByteArray, and add the same exact assertion to the other failing location referenced (around lines 1921-1926) so the test fails if a wrong resource is used.
🤖 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/FBX/FBXExporter.cpp`:
- Around line 1689-1697: When embedding texture bytes via
readOgreResourceBytes(texName) in FBXExporter (the block that calls
m_w.beginNode("Content") / m_w.writePropertyR(bytes)), do not silently continue
when bytes.empty(): call SentryReporter::addBreadcrumb with category
"file.export" (and a message like "Missing texture payload for <texName> during
FBX export") to record the missing Video.Content, and then surface failure by
returning/propagating an export error (e.g., set/export failure flag or return
false / throw an ExportException from the surrounding export function) so the
caller knows the texture was not embedded; ensure the breadcrumb message
includes texName and any context so downstream code (and logs) can correlate the
missing payload.
- Around line 1587-1608: The helper readOgreResourceBytes currently searches all
resource groups by bare resourceName and can return the wrong file when multiple
groups have the same filename; change it to accept an optional preferredGroup
parameter (or overload) and first try rgm.resourceExists(preferredGroup,
resourceName) / rgm.openResource(resourceName, preferredGroup), then fallback to
DEFAULT_RESOURCE_GROUP_NAME, and only then scan all groups; update the caller in
writeTextureObjects to pass the bound material/texture group (e.g. use
mat->getGroup() or the loaded texture resource's getGroup()) so the exporter
resolves bytes from the correct resource before falling back to a global scan.
In `@src/MeshImporterExporter.cpp`:
- Around line 1504-1508: The pose-FBX export path currently skips
exportMaterial() when fmt == "FBX Binary (*.fbx)", which removes the only
exported texture image files because exportCurrentPose() uses Assimp and only
writes texture references; restore texture file export by ensuring
exportMaterial(entity, file) (or at least exportTextures()) is called for FBX in
MeshImporterExporter.cpp so referenced images are written alongside the FBX
(e.g., re-enable exportMaterial in the block that currently checks fmt, or call
a new exportTextures(entity, file) from exportCurrentPose/exportMaterial),
keeping exportCurrentPose(), exportMaterial(), and exportTextures() references
intact so pose-FBXs reopen with textures until a true embedded-texture path is
implemented.
---
Nitpick comments:
In `@src/FBX/FBXExporter_test.cpp`:
- Around line 474-500: The test currently only checks that the exported
Video.Content is non-empty, which can pass if a different texture is picked up;
modify the FBXExporter_test (the block that writes the 1x1 PNG into texPath
using the png QByteArray) to assert that the embedded payload equals the exact
png bytes written by SetUp(), e.g. compare content->properties[0].stringVal (or
the Video.Content buffer extracted from the exporter result) against the
original png QByteArray, and add the same exact assertion to the other failing
location referenced (around lines 1921-1926) so the test fails if a wrong
resource is used.
🪄 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: ec8fe20f-e5b9-4c49-b165-f11e8dc36c9e
📒 Files selected for processing (3)
src/FBX/FBXExporter.cppsrc/FBX/FBXExporter_test.cppsrc/MeshImporterExporter.cpp
| // Embed texture bytes when the resource is available. Many tools (e.g. Mixamo exports) | ||
| // expect texture payloads to be embedded via Video.Content. | ||
| const auto bytes = readOgreResourceBytes(texName); | ||
| if (!bytes.empty()) { | ||
| m_w.beginNode("Content"); | ||
| m_w.writePropertyR(bytes); | ||
| m_w.endProperties(); | ||
| m_w.endNodeLeaf(); | ||
| } |
There was a problem hiding this comment.
Don't silently drop Video.Content when embedding fails.
If bytes is empty here, the FBX still exports but the texture payload disappears. Since src/MeshImporterExporter.cpp now skips FBX sidecars, that turns a textured export into an untextured one with no signal to the caller. Please surface this as an export failure or at least log/capture the missing texture instead of silently continuing.
As per coding guidelines: Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use ui.action for toolbar/menu clicks, ai.tool_call for MCP tool invocations, file.import/file.export for I/O operations
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FBX/FBXExporter.cpp` around lines 1689 - 1697, When embedding texture
bytes via readOgreResourceBytes(texName) in FBXExporter (the block that calls
m_w.beginNode("Content") / m_w.writePropertyR(bytes)), do not silently continue
when bytes.empty(): call SentryReporter::addBreadcrumb with category
"file.export" (and a message like "Missing texture payload for <texName> during
FBX export") to record the missing Video.Content, and then surface failure by
returning/propagating an export error (e.g., set/export failure flag or return
false / throw an ExportException from the surrounding export function) so the
caller knows the texture was not embedded; ensure the breadcrumb message
includes texName and any context so downstream code (and logs) can correlate the
missing payload.
| // For FBX, prefer a single-file export. Assimp FBX export may still | ||
| // reference textures by name, but our current expectation for FBX is | ||
| // "no sidecar dumps" (materials/textures are embedded or handled by importer). | ||
| if (fmt != "FBX Binary (*.fbx)") | ||
| exportMaterial(entity, file); |
There was a problem hiding this comment.
Keep texture export for pose FBX until this path really embeds textures.
exportCurrentPose() still routes FBX through Assimp (Lines 1468-1470), and that path only writes texture references into the FBX material data. Skipping exportMaterial() here removes the only exported texture files, so textured pose FBXs will reopen without their textures.
Minimal fix
- if (fmt != "FBX Binary (*.fbx)")
- exportMaterial(entity, file);
+ exportMaterial(entity, file);A cleaner follow-up would be to split exportTextures() from .material emission so pose-FBX can keep referenced image files without reintroducing an unnecessary .material sidecar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 1504 - 1508, The pose-FBX export
path currently skips exportMaterial() when fmt == "FBX Binary (*.fbx)", which
removes the only exported texture image files because exportCurrentPose() uses
Assimp and only writes texture references; restore texture file export by
ensuring exportMaterial(entity, file) (or at least exportTextures()) is called
for FBX in MeshImporterExporter.cpp so referenced images are written alongside
the FBX (e.g., re-enable exportMaterial in the block that currently checks fmt,
or call a new exportTextures(entity, file) from
exportCurrentPose/exportMaterial), keeping exportCurrentPose(),
exportMaterial(), and exportTextures() references intact so pose-FBXs reopen
with textures until a true embedded-texture path is implemented.
Refactor readOgreResourceBytes() to reduce nesting, avoid deprecated DataStreamPtr::isNull(), and catch specific exceptions to satisfy SonarCloud maintainability on new code. Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/FBX/FBXExporter.cpp (1)
1699-1706:⚠️ Potential issue | 🟠 MajorDon't silently skip
Video.Contenton missing payload — surface the failure.When
readOgreResourceBytes(texName)returns empty, theContentnode is omitted but the FBX still completes successfully. Sincesrc/MeshImporterExporter.cppnow skips sidecar.material/image emission for the FBX path on the assumption that this exporter embeds textures, a texture that can't be resolved (e.g., procedural/SD-generated textures registered as GPU textures only, or assets loaded outside a resource group) produces an FBX with neither the embedded payload nor a sidecar — a textured mesh silently exports as untextured with no signal to the caller.At minimum, log a warning and add a Sentry breadcrumb so the regression is observable; ideally, treat persistent failures as an export error or fall back to extracting bytes from
Ogre::TexturePtr::convertToImage()for the boundTextureUnitStatebefore giving up.🛡️ Suggested direction
- if (const auto bytes = readOgreResourceBytes(texName); !bytes.empty()) { - m_w.beginNode("Content"); - m_w.writePropertyR(bytes); - m_w.endProperties(); - m_w.endNodeLeaf(); - } + const auto bytes = readOgreResourceBytes(texName); + if (!bytes.empty()) { + m_w.beginNode("Content"); + m_w.writePropertyR(bytes); + m_w.endProperties(); + m_w.endNodeLeaf(); + } else { + Ogre::LogManager::getSingleton().logWarning( + "FBXExporter: missing embedded payload for texture '" + texName + + "' — exported FBX will reference '" + texName + "' but contain no Video.Content."); + SentryReporter::addBreadcrumb("file.export", + "Missing FBX Video.Content for texture: " + QString::fromStdString(texName)); + // Optionally: fall back to tus->_getTexturePtr()->convertToImage() and encode to PNG bytes here. + }As per coding guidelines: "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use … file.import/file.export for I/O operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FBX/FBXExporter.cpp` around lines 1699 - 1706, The code currently omits the Video.Content node when readOgreResourceBytes(texName) returns empty — change this to (1) log a warning and call SentryReporter::addBreadcrumb("file.export", formatted message including texName) whenever bytes.empty() is true so missing payloads are observable, (2) attempt a fallback by resolving the TextureUnitState / Ogre::TexturePtr bound to that texName and calling convertToImage() to extract bytes to embed before giving up, and (3) if the fallback also fails, propagate an export failure (return/throw an error) instead of silently continuing; edit the block around readOgreResourceBytes(texName), the m_w.beginNode("Content") emission, and add the SentryReporter::addBreadcrumb and fallback logic so MeshImporterExporter.cpp’s assumption that FBX exports embed textures remains valid.
🧹 Nitpick comments (1)
src/FBX/FBXExporter.cpp (1)
1638-1642: Consider logging a warning when an exception swallows a resource read.Both catch blocks return an empty vector with no log entry, so a transient I/O error or a corrupt resource in
Ogre::DataStreambecomes indistinguishable from "resource not found". Combined with the silent-skip at the call site (lines 1701-1706), an FBX with a real read error will export as if everything succeeded. At minimum, log viaOgre::LogManagerso the failure mode is observable.♻️ Suggested change
- } catch (const Ogre::Exception&) { - return {}; - } catch (const std::exception&) { - return {}; - } + } catch (const Ogre::Exception& ex) { + Ogre::LogManager::getSingleton().logWarning( + "FBXExporter: failed to read embedded resource '" + resourceName + "': " + ex.what()); + return {}; + } catch (const std::exception& ex) { + Ogre::LogManager::getSingleton().logWarning( + "FBXExporter: failed to read embedded resource '" + resourceName + "': " + ex.what()); + return {}; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FBX/FBXExporter.cpp` around lines 1638 - 1642, The two catch blocks that currently swallow exceptions (catch (const Ogre::Exception&) and catch (const std::exception&)) should capture the exception by name and log a warning/error via Ogre::LogManager before returning an empty vector; for example, change to catch (const Ogre::Exception& e) and use e.getFullDescription() and catch (const std::exception& e) and use e.what(), and call Ogre::LogManager::getSingleton().logMessage(...) (include a brief context string like "FBXExporter: failed to read resource" plus the exception text) so the transient I/O/corruption errors are observable while keeping the existing return {} behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/FBX/FBXExporter.cpp`:
- Around line 1699-1706: The code currently omits the Video.Content node when
readOgreResourceBytes(texName) returns empty — change this to (1) log a warning
and call SentryReporter::addBreadcrumb("file.export", formatted message
including texName) whenever bytes.empty() is true so missing payloads are
observable, (2) attempt a fallback by resolving the TextureUnitState /
Ogre::TexturePtr bound to that texName and calling convertToImage() to extract
bytes to embed before giving up, and (3) if the fallback also fails, propagate
an export failure (return/throw an error) instead of silently continuing; edit
the block around readOgreResourceBytes(texName), the m_w.beginNode("Content")
emission, and add the SentryReporter::addBreadcrumb and fallback logic so
MeshImporterExporter.cpp’s assumption that FBX exports embed textures remains
valid.
---
Nitpick comments:
In `@src/FBX/FBXExporter.cpp`:
- Around line 1638-1642: The two catch blocks that currently swallow exceptions
(catch (const Ogre::Exception&) and catch (const std::exception&)) should
capture the exception by name and log a warning/error via Ogre::LogManager
before returning an empty vector; for example, change to catch (const
Ogre::Exception& e) and use e.getFullDescription() and catch (const
std::exception& e) and use e.what(), and call
Ogre::LogManager::getSingleton().logMessage(...) (include a brief context string
like "FBXExporter: failed to read resource" plus the exception text) so the
transient I/O/corruption errors are observable while keeping the existing return
{} behavior.
|
1 participant
Summary
Video.Contentso exports are single-file (Mixamo-style)..materialand extracted texture images for FBX exports.Test plan
QtMeshEditor --cli convert <textured_asset> -o out.fbxand verify no.material/.pngsidecars are created.Objects/Video/Content(rawRpayload).UnitTests --gtest_filter=FBXExporter*.Summary by CodeRabbit
New Features
Tests