feat(export): vertex colors + .mesh sidecar materials - #346
Merged
fernandotonon merged 11 commits intoApr 30, 2026
Conversation
Assimp-based exports now emit vertex colors from VES_DIFFUSE and FBX exports write LayerElementColor. Also load exported .material sidecars when reimporting .mesh so material names don't fall back to BaseWhite. Made-with: Cursor
📝 WalkthroughWalkthroughAdds per-vertex color export to FBX by emitting a LayerElementColor (ByPolygonVertex/Direct) expanded to polygon-vertex order; persists vertex colors through Assimp meshes and exports bind-pose colors; adds sidecar Changes
Sequence Diagram(s)sequenceDiagram
participant Mesh as Mesh (positions, indices, VES_DIFFUSE)
participant Exporter as FBXExporter
participant Geometry as FBX Geometry Node
participant Layer as FBX Layer Registry
Mesh->>Exporter: provide vertex data + VES_DIFFUSE
Exporter->>Exporter: detect VES_DIFFUSE, convert to RGBA doubles
Exporter->>Exporter: expand to ByPolygonVertex order (reversed winding)
Exporter->>Geometry: create LayerElementColor (Version, Name, Mapping=ByPolygonVertex, Reference=Direct, Colors[])
Exporter->>Layer: register LayerElement(Type="LayerElementColor", TypedIndex=0)
Layer->>Geometry: attach color layer to geometry
sequenceDiagram
participant Importer as MeshImporterExporter
participant FS as File System
participant Parser as Material Parser
participant Resources as Ogre Resource Manager
participant Mesh as Ogre Mesh Loader
Importer->>FS: check for "<mesh>.material" sidecar
FS-->>Importer: file found / not found
alt sidecar exists
Importer->>FS: read .material script
FS-->>Importer: script contents
Importer->>Parser: parse material script
Parser-->>Importer: parsed material names
Importer->>Resources: init/load resource group & register parsed materials
Resources->>Mesh: materials available for mesh import
Mesh->>Importer: imported mesh with sidecar materials resolved
else no sidecar
Importer->>Importer: proceed without sidecar
end
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 387f337e6a
ℹ️ 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".
| { | ||
| // `.mesh` stores material names, but not the script definitions. If the | ||
| // corresponding `.material` isn't loaded, Ogre falls back to BaseWhite. | ||
| const QString sidecar = meshFile.path() + "/" + meshFile.baseName() + ".material"; |
There was a problem hiding this comment.
QFileInfo::baseName() truncates at the first dot, so a mesh like robot.v2.mesh looks for robot.material instead of robot.v2.material. In that common versioned-file naming case, the sidecar script is never loaded and import still falls back to BaseWhite, which defeats the new .mesh sidecar fix.
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: 2
🧹 Nitpick comments (1)
src/FBX/FBXExporter_test.cpp (1)
1274-1303: Assert the exported color payload, not just the node presence.This test would still pass if
LayerElementColoris emitted with an empty or misexpandedColorsarray. Verifying the payload shape here would better protect the new export path.Suggested tightening of the test
- EXPECT_NE(geomNodes[0]->find("LayerElementColor"), nullptr); + auto* colorElement = geomNodes[0]->find("LayerElementColor"); + ASSERT_NE(colorElement, nullptr); + auto* colors = colorElement->find("Colors"); + ASSERT_NE(colors, nullptr); + EXPECT_EQ(colors->properties[0].doubleArray.size(), 12u);🤖 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 1274 - 1303, The test VertexColors_WritesLayerElementColor currently only checks for the presence of a LayerElementColor node; update it to assert the actual exported color payload by locating the matching LayerElement (via layer->findAll("LayerElement") and Type=="LayerElementColor") and then verifying it contains a "Colors" child with the expected array length and values (compare against the colors from createInMemoryTriangleMeshWithVertexColors) and that each Colors entry has the correct property types/values; do this using the existing exportAndParse/find/findAll helpers on geomNodes[0] so the test fails if Colors is empty or misserialized.
🤖 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/MeshImporterExporter_test.cpp`:
- Around line 332-380: The test Importer_MeshLoadsSidecarMaterialScript can pass
incorrectly because SidecarMaterial already exists in-process; before calling
MeshImporterExporter::importer({outMesh}) remove or clear the runtime material
so only the sidecar script can provide it. Locate the test's setup where
Ogre::MaterialPtr mat is created (SidecarMaterial) and before reimport call into
MeshImporterExporter::importer, call the MaterialManager to destroy/remove the
runtime material (e.g. use Ogre::MaterialManager::getSingleton().remove or
equivalent to remove "SidecarMaterial") or reset the manager's material state so
the reimport must load the sidecar file to satisfy the material lookup. Ensure
the code removes the exact name "SidecarMaterial" and that no lingering
references remain before import.
In `@src/MeshImporterExporter.cpp`:
- Around line 314-334: exportCurrentPose() currently creates its own aiMesh but
never copies Ogre::VES_DIFFUSE, so vertex colors are dropped; modify
exportCurrentPose() (or factor out the color-copy logic into a helper used by
both paths) to detect colElem =
vData->vertexDeclaration->findElementBySemantic(Ogre::VES_DIFFUSE) for the
pose's vertex buffer, allocate aiMesh->mColors[0] with new
aiColor4D[aiMesh->mNumVertices], read the buffer via vbuf->lock(...), convert
using colElem->getType() (VET_COLOUR_ABGR vs default) into Ogre::ColourValue
then assign aiColor4D(cv.r,cv.g,cv.b,cv.a) for each vertex and vbuf->unlock();
ensure proper ownership/cleanup consistent with the shared export path.
---
Nitpick comments:
In `@src/FBX/FBXExporter_test.cpp`:
- Around line 1274-1303: The test VertexColors_WritesLayerElementColor currently
only checks for the presence of a LayerElementColor node; update it to assert
the actual exported color payload by locating the matching LayerElement (via
layer->findAll("LayerElement") and Type=="LayerElementColor") and then verifying
it contains a "Colors" child with the expected array length and values (compare
against the colors from createInMemoryTriangleMeshWithVertexColors) and that
each Colors entry has the correct property types/values; do this using the
existing exportAndParse/find/findAll helpers on geomNodes[0] so the test fails
if Colors is empty or misserialized.
🪄 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: 83b15001-2289-4b5b-9b9d-ce94e99dd821
📒 Files selected for processing (4)
src/FBX/FBXExporter.cppsrc/FBX/FBXExporter_test.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
Deferred GPU upload from vertex painting could leave Ogre buffers stale when export ran in the same frame. Sync EditableMesh colors to the entity before reading geometry for Assimp and FBX paths (including scene glTF export). Made-with: Cursor
…x colors - Sidecar material test: destroy export entity, release material handle, remove from MaterialManager when still loaded so reimport must parse .material. - FBX vertex color test: assert ByPolygonVertex Colors array length and RGBA order after reversed winding (v0 red, v2 blue, v1 green). - exportCurrentPose: copy VES_DIFFUSE from bind-pose buffers like UVs. - Shorten flushPendingVertexPaintForEntity doc comment. Made-with: Cursor
ensureResourceGroup initialises the mesh folder before tryLoadSidecarMaterialScript; a second initialiseResourceGroup is a no-op, so materials from the in-memory parse never reached a loaded state and mesh import fell back to BaseWhite. Made-with: Cursor
- Use mutable QByteArray and read-only MemoryDataStream instead of (void*)constData() (const-correctness / Sonar). - Extract loadUnloadedOgreMaterialsInGroup to keep tryLoadSidecarMaterialScript simpler. - Keep explicit load of UNLOADED materials in the mesh folder group so .mesh reimport resolves sidecar materials after parseScript. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/FBX/FBXExporter_test.cpp (1)
1275-1278: ⚡ Quick winUse unique mesh/node names to avoid test resource collisions.
Lines 1275 and 1277 use hardcoded resource names. Reusing
uniqueName(...)(as the rest of this fixture does) reduces flakiness when tests are repeated in the same process.Suggested patch
- auto meshPtr = createInMemoryTriangleMeshWithVertexColors("fbx_colors"); + const auto name = uniqueName("fbx_colors"); + auto meshPtr = createInMemoryTriangleMeshWithVertexColors(name); ASSERT_TRUE(!!meshPtr); - auto* node = Manager::getSingleton()->addSceneNode("fbx_colors_node"); + auto* node = Manager::getSingleton()->addSceneNode(name + "_node");🤖 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 1275 - 1278, Replace the hardcoded resource names with generated unique names to prevent collisions: call uniqueName(...) when creating the mesh via createInMemoryTriangleMeshWithVertexColors (instead of "fbx_colors") and when adding the scene node via Manager::getSingleton()->addSceneNode (instead of "fbx_colors_node"), and pass the matched unique name into Manager::getSingleton()->createEntity so the mesh/node pair use the same unique identifier.src/EditModeController.cpp (1)
1483-1489: ⚡ Quick winAdd a
file.exportbreadcrumb for this export-time flush path.This method performs a significant export-prep operation (forcing pending vertex colors into Ogre buffers) but currently emits no breadcrumb.
Suggested patch
void EditModeController::flushPendingVertexPaintForEntity(Ogre::Entity* entity) { if (!entity || !m_editModeActive || m_editEntity != entity || !m_editableMesh) return; + SentryReporter::addBreadcrumb( + "file.export", + QStringLiteral("Flushed pending vertex paint before export for entity '%1'") + .arg(QString::fromStdString(entity->getName()))); m_vertexPaintFlushPending = false; m_editableMesh->commitVertexColorsToEntity(m_editEntity); }As per coding guidelines: “All user-facing actions and significant operations must be tracked 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/EditModeController.cpp` around lines 1483 - 1489, Add a Sentry breadcrumb when forcing pending vertex colors to the Ogre buffers in EditModeController::flushPendingVertexPaintForEntity: after confirming the early-return checks and before/after clearing m_vertexPaintFlushPending and calling m_editableMesh->commitVertexColorsToEntity(m_editEntity), invoke SentryReporter::addBreadcrumb("file.export", "flushPendingVertexPaintForEntity: committed vertex colors for entity"); this ensures the export-time flush is tracked as a file.export I/O breadcrumb.
🤖 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_test.cpp`:
- Around line 1284-1286: The test dereferences the result of
findTopLevel(r.nodes, "Objects") (stored in objects) without a null check;
update the test to assert objects is not null before using it (e.g., replace
direct dereference with an ASSERT_NE(objects, nullptr) or ASSERT_TRUE(objects)
immediately after calling findTopLevel), then proceed to call
objects->findAll("Geometry") and ASSERT_EQ(geomNodes.size(), 1u) only after that
guard; reference findTopLevel, objects, findAll, and ASSERT_EQ to locate the
lines to update.
---
Nitpick comments:
In `@src/EditModeController.cpp`:
- Around line 1483-1489: Add a Sentry breadcrumb when forcing pending vertex
colors to the Ogre buffers in
EditModeController::flushPendingVertexPaintForEntity: after confirming the
early-return checks and before/after clearing m_vertexPaintFlushPending and
calling m_editableMesh->commitVertexColorsToEntity(m_editEntity), invoke
SentryReporter::addBreadcrumb("file.export", "flushPendingVertexPaintForEntity:
committed vertex colors for entity"); this ensures the export-time flush is
tracked as a file.export I/O breadcrumb.
In `@src/FBX/FBXExporter_test.cpp`:
- Around line 1275-1278: Replace the hardcoded resource names with generated
unique names to prevent collisions: call uniqueName(...) when creating the mesh
via createInMemoryTriangleMeshWithVertexColors (instead of "fbx_colors") and
when adding the scene node via Manager::getSingleton()->addSceneNode (instead of
"fbx_colors_node"), and pass the matched unique name into
Manager::getSingleton()->createEntity so the mesh/node pair use the same unique
identifier.
🪄 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: acdaf90f-ec9d-42bf-a5b8-8fe36a3f3d0b
📒 Files selected for processing (5)
src/EditModeController.cppsrc/EditModeController.hsrc/FBX/FBXExporter_test.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
…aterials - Resolve sidecar as completeBaseName.material so model.v2.mesh finds model.v2.material. - After parseScript, load only materials declared on Ogre material script lines (prefix material + name); remove loadResourceGroup and full-group material iteration so unrelated broken scripts cannot fail import. - FBX vertex-color test: unique mesh/node names, ASSERT_NE(objects). - EditModeController: file.export Sentry breadcrumb in flushPendingVertexPaintForEntity; expand flush API doc comment (CodeRabbit docstring / observability). Made-with: Cursor
- Run loadResourceGroup immediately after parseScript (matches CI-passing flow). - Keep declared-name loads, then sweep only UNLOADED materials in the mesh folder group. - Removes try/catch that could mask load failures; declared-name trim/load unchanged. Made-with: Cursor
fernandotonon
force-pushed
the
feat/export-vertex-colors-material-reimport
branch
from
April 30, 2026 02:26
b079449 to
add7506
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/MeshImporterExporter.cpp (1)
1063-1080:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep sidecar loading scoped to the parsed script.
loadResourceGroup(group)plus the finalLOADSTATE_UNLOADEDsweep still makes this import depend on every material resource in the mesh directory, not just the sidecar you just parsed. That reintroduces the same failure mode this PR is trying to remove: one unrelated broken sibling.materialor missing texture can throw here and make a valid.meshfall back again. Please keep the post-parse load limited to the material names extracted fromscriptBytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 1063 - 1080, The current code calls rgm.loadResourceGroup(group) and then iterates all Ogre::MaterialManager resources to load any LOADSTATE_UNLOADED entries, which forces loading every material in the folder and reintroduces failures from unrelated siblings; instead, remove the broad rgm.loadResourceGroup(group) + UNLOADED sweep and restrict loading to only the materials parsed from the scriptBytes. Modify loadMaterialsDeclaredInOgreMaterialScript to return (or expose) the list of material names it found, then iterate that list and for each name call Ogre::MaterialManager::getSingleton().getByName(materialName, group) (or getResourceByName) and call res->load() only if res exists and res->getLoadingState() == Ogre::Resource::LOADSTATE_UNLOADED; keep rgm.initialiseResourceGroup(group) only if needed for sidecar lookup but do not perform a global group load or a full MaterialManager iterator sweep.
🤖 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/MeshImporterExporter.cpp`:
- Around line 1063-1080: The current code calls rgm.loadResourceGroup(group) and
then iterates all Ogre::MaterialManager resources to load any LOADSTATE_UNLOADED
entries, which forces loading every material in the folder and reintroduces
failures from unrelated siblings; instead, remove the broad
rgm.loadResourceGroup(group) + UNLOADED sweep and restrict loading to only the
materials parsed from the scriptBytes. Modify
loadMaterialsDeclaredInOgreMaterialScript to return (or expose) the list of
material names it found, then iterate that list and for each name call
Ogre::MaterialManager::getSingleton().getByName(materialName, group) (or
getResourceByName) and call res->load() only if res exists and
res->getLoadingState() == Ogre::Resource::LOADSTATE_UNLOADED; keep
rgm.initialiseResourceGroup(group) only if needed for sidecar lookup but do not
perform a global group load or a full MaterialManager iterator sweep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5837ea09-b0b9-4d60-861d-fd1f1450e3ac
📒 Files selected for processing (4)
src/EditModeController.cppsrc/EditModeController.hsrc/FBX/FBXExporter_test.cppsrc/MeshImporterExporter.cpp
✅ Files skipped from review due to trivial changes (1)
- src/EditModeController.cpp
- loadMaterialsDeclaredInOgreMaterialScript returns whether any declared material exists in the mesh folder group after disk init - Run memory parseScript only when none were found (avoids duplicate- material exceptions that skipped loadResourceGroup and left BaseWhite) - Keep unload sweep after each load path Made-with: Cursor
MeshSerializer writes SubMesh::getMaterialName(); SubEntity::setMaterial does not update the mesh asset. Without this the exported .mesh did not reference SidecarMaterial and reimport always resolved to BaseWhite. Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/MeshImporterExporter.cpp (1)
1057-1094:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the sidecar load to this script’s materials only.
loadResourceGroup(group)andsweepUnloadedInGroup()still load every material in the folder resource group, so one unrelated broken.materialor missing texture can still derail this mesh import and send it back toBaseWhite. Please keep the load set limited to the names declared inscriptBytesinstead of doing a group-wide load/sweep.Possible direction
- if (rgm.isResourceGroupInitialised(group)) - rgm.loadResourceGroup(group); - else + if (!rgm.isResourceGroupInitialised(group)) rgm.initialiseResourceGroup(group); - auto sweepUnloadedInGroup = [&group]() { - ... - }; - - bool haveDeclared = loadMaterialsDeclaredInOgreMaterialScript(scriptBytes, group); - sweepUnloadedInGroup(); + const auto declaredNames = /* parse names from scriptBytes once */; + bool haveDeclared = /* check/load only declaredNames */; if (!haveDeclared) { ... Ogre::MaterialManager::getSingleton().parseScript(ds, group); - if (rgm.isResourceGroupInitialised(group)) - rgm.loadResourceGroup(group); - loadMaterialsDeclaredInOgreMaterialScript(scriptBytes, group); - sweepUnloadedInGroup(); + /* load only declaredNames here too */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 1057 - 1094, The group-wide loads are too broad—replace calls to rgm.loadResourceGroup(group) and sweepUnloadedInGroup() with logic that only parses and loads the materials named in the currently parsed scriptBytes; use loadMaterialsDeclaredInOgreMaterialScript(scriptBytes, group) to obtain the set/list of declared material names, then for each name call Ogre::MaterialManager::getSingleton().getByName(name, group) or getResource(name) and explicitly load() that ResourcePtr (and only sweep/load those entries) instead of loading the entire resource group or iterating all materials; keep the parseScript(ds, group) step but remove the subsequent rgm.loadResourceGroup(group)/sweepUnloadedInGroup() calls so failures are scoped to the script’s declared materials.
🤖 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/MeshImporterExporter.cpp`:
- Around line 1057-1094: The group-wide loads are too broad—replace calls to
rgm.loadResourceGroup(group) and sweepUnloadedInGroup() with logic that only
parses and loads the materials named in the currently parsed scriptBytes; use
loadMaterialsDeclaredInOgreMaterialScript(scriptBytes, group) to obtain the
set/list of declared material names, then for each name call
Ogre::MaterialManager::getSingleton().getByName(name, group) or
getResource(name) and explicitly load() that ResourcePtr (and only sweep/load
those entries) instead of loading the entire resource group or iterating all
materials; keep the parseScript(ds, group) step but remove the subsequent
rgm.loadResourceGroup(group)/sweepUnloadedInGroup() calls so failures are scoped
to the script’s declared materials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc68dfa8-6051-4545-9f81-aa0cf0e32928
📒 Files selected for processing (2)
src/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
|
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
1 participant
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
VES_DIFFUSE.LayerElementColor(ByPolygonVertex/Direct)..meshreimport material fallback by auto-loading<basename>.materialsidecar when present (preventsBaseWhite).Test plan
UnitTests(CI) — added coverage tests:MeshImporterExporterTest.Importer_MeshLoadsSidecarMaterialScriptFBXExporterCoverageTest.VertexColors_WritesLayerElementColorMade with Cursor
Summary by CodeRabbit
New Features
Tests