Fix scene save freeze and non-PNG texture export - #195
Conversation
Add a progress dialog (QProgressDialog) during scene export so the UI stays responsive on large scenes. Convert unsupported texture formats (JPG, DDS, etc.) to PNG before saving, fixing the "STBIImageCodec only supports PNG" error. Bump version to 2.14.1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughBumps project version to 2.14.1, refactors scene export into helper-driven pipeline with texture-name normalization and optional ProgressCallback, updates mainwindow to show a QProgressDialog driven by the exporter, and adds extensive tests for texture naming, progress reporting, and round-trip exports. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MainWindow
participant ProgressDialog as QProgressDialog
participant Exporter as MeshImporterExporter
participant FS as File System
User->>MainWindow: Trigger "Save Scene"
MainWindow->>ProgressDialog: create & show (0%)
MainWindow->>Exporter: sceneExporter(uri, progressCallback)
Exporter->>Exporter: buildAiScene (materials, meshes, animations)
Exporter->>MainWindow: progressCallback(30, "Building scene...")
loop per texture/material
Exporter->>Exporter: exportTextureName(original)
Exporter->>FS: save normalized texture
Exporter->>MainWindow: progressCallback(progress, "Exporting textures...")
end
Exporter->>FS: write scene file
Exporter->>MainWindow: progressCallback(100, "Export complete")
ProgressDialog->>User: update UI / show completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
📝 Coding Plan
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: a6cf49c675
ℹ️ 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".
| static const QStringList supported = {"png", "bmp", "tga", "hdr"}; | ||
| if (supported.contains(ext)) | ||
| return originalName; | ||
| return fi.completeBaseName() + ".png"; |
There was a problem hiding this comment.
Using fi.completeBaseName() here drops directory information, so different source textures like a/diffuse.jpg and b/diffuse.jpg both collapse to diffuse.png. Since this helper is used for both texture file writes and material texture paths, one export can overwrite the other and multiple materials end up referencing the same PNG, which produces incorrect texture assignments in exported scenes that reuse common filenames.
Useful? React with 👍 / 👎.
| [&progressDialog](int progress, const QString& status) { | ||
| progressDialog.setLabelText(status); | ||
| progressDialog.setValue(progress); | ||
| QApplication::processEvents(); |
There was a problem hiding this comment.
The UI now shows a Cancel button, but the callback only updates text/value and never checks wasCanceled() (and sceneExporter has no abort path), so clicking Cancel does not stop the export. In long saves this is misleading because users are told cancellation is available while the export continues to run to completion.
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/MeshImporterExporter_test.cpp (1)
185-188:NullProgresstest currently validates only early-return, not callback-path safety.Line 187 uses an empty URI, so
sceneExporterexits before any callback-capable export flow runs. Consider moving this case to a fixture with a valid temp output path to verifynullptrprogress handling in the real execution path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 185 - 188, The test MeshImporterExporter_NullProgress_DoesNotCrash currently uses an empty URI so sceneExporter("", nullptr) returns early and never exercises the callback path; change the test to use a real temporary output path (create a temp file/dir in the test fixture) and call MeshImporterExporter::sceneExporter(validTempUri, nullptr) so the exporter runs the real export flow with a null progress callback and you can assert it returns the expected code (and does not crash) rather than relying on the early-return behavior.
🤖 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/mainwindow.cpp`:
- Around line 621-633: The progress dialog shows a Cancel button but
cancellation isn't implemented: remove the misleading cancel affordance by
disabling the cancel button on the QProgressDialog (e.g., call
progressDialog.setCancelButton(nullptr) or construct it without a cancel text)
so UI doesn't imply abort support; keep the existing lambda callback and
MeshImporterExporter::sceneExporter/ProgressCallback unchanged unless you choose
to implement full cancellation end-to-end later.
In `@src/MeshImporterExporter.cpp`:
- Around line 105-115: The exportTextureName function currently returns the full
original path for supported extensions but strips directories for converted
names, causing inconsistent output paths; update
MeshImporterExporter::exportTextureName to always strip directory components by
using QFileInfo::fileName() (instead of returning originalName or using
completeBaseName() only) and then apply the existing extension logic against
that filename so both supported (png,bmp,tga,hdr) and converted textures produce
filenames without directories.
- Around line 133-134: The texture export currently ignores the return value of
img.save, so modify the code around exportTextureName/ saveName to check
img.save((file.path() + "/" + saveName).toStdString())'s boolean result; on
false, log or report the failed write (include texture name/saveName and path),
mark the overall export as failed (or return/throw an error) so callers know
export did not succeed, and ensure any cleanup/continuation logic in the
surrounding function handles this failure case.
---
Nitpick comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 185-188: The test MeshImporterExporter_NullProgress_DoesNotCrash
currently uses an empty URI so sceneExporter("", nullptr) returns early and
never exercises the callback path; change the test to use a real temporary
output path (create a temp file/dir in the test fixture) and call
MeshImporterExporter::sceneExporter(validTempUri, nullptr) so the exporter runs
the real export flow with a null progress callback and you can assert it returns
the expected code (and does not crash) rather than relying on the early-return
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 208004ee-d88b-49ed-9061-4c071cdec05f
📒 Files selected for processing (5)
CMakeLists.txtsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/MeshImporterExporter_test.cppsrc/mainwindow.cpp
| QString saveName = exportTextureName(QString::fromStdString(tex->getName())); | ||
| img.save((file.path() + "/" + saveName).toStdString()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -150 src/MeshImporterExporter.cpp | tail -25Repository: fernandotonon/QtMeshEditor
Length of output: 862
🏁 Script executed:
wc -l src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 103
🏁 Script executed:
sed -n '120,150p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1097
🏁 Script executed:
sed -n '100,160p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2189
🏁 Script executed:
rg -i "sentryreporter" src/ --type cppRepository: fernandotonon/QtMeshEditor
Length of output: 14157
🏁 Script executed:
rg "Ogre::LogManager" src/ --type cpp -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 10942
🏁 Script executed:
rg "\.save\(" src/ --type cpp -B 2 -A 1 | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 813
Check texture export success explicitly.
Line 134 ignores img.save() return value, allowing export to report success despite incomplete texture writes. Add error handling to detect and report failures:
- img.save((file.path() + "/" + saveName).toStdString());
+ const QString outPath = file.path() + "/" + saveName;
+ if (!img.save(outPath.toStdString()))
+ {
+ const QString msg = QString("Failed to save texture: %1").arg(outPath);
+ Ogre::LogManager::getSingleton().logError(msg.toStdString());
+ SentryReporter::captureMessage(msg, "error");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QString saveName = exportTextureName(QString::fromStdString(tex->getName())); | |
| img.save((file.path() + "/" + saveName).toStdString()); | |
| QString saveName = exportTextureName(QString::fromStdString(tex->getName())); | |
| const QString outPath = file.path() + "/" + saveName; | |
| if (!img.save(outPath.toStdString())) | |
| { | |
| const QString msg = QString("Failed to save texture: %1").arg(outPath); | |
| Ogre::LogManager::getSingleton().logError(msg.toStdString()); | |
| SentryReporter::captureMessage(msg, "error"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 133 - 134, The texture export
currently ignores the return value of img.save, so modify the code around
exportTextureName/ saveName to check img.save((file.path() + "/" +
saveName).toStdString())'s boolean result; on false, log or report the failed
write (include texture name/saveName and path), mark the overall export as
failed (or return/throw an error) so callers know export did not succeed, and
ensure any cleanup/continuation logic in the surrounding function handles this
failure case.
…verage
Review fixes:
- Remove misleading Cancel button from progress dialog (setCancelButton nullptr)
- Fix exportTextureName to consistently strip directory from all outputs
- Add try/catch for img.save() failures in exportTextures
- Unify normal map detection ("normal_map" || "NormalMap") in single-entity path
Refactoring (net -153 lines):
- Extract buildAiMaterialFromOgre() shared material builder
- Extract readSubmeshGeometry() for vertex/index buffer reading
- Extract assignBoneWeights() for bone weight assignment
- Extract buildAiAnimation() for skeleton animation conversion
- Generalize buildBoneNode() with bonePrefix parameter, removing inline lambda
New tests:
- exportTextureName: empty input, no extension, path stripping for supported
formats, TIF conversion
- SceneExporter_NullProgress_FullExport: null callback through real export path
- RoundTrip_MixedSkeletalAndNonSkeletal: mixed skeletal + non-skeletal scene
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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/MeshImporterExporter.cpp (1)
1303-1322:⚠️ Potential issue | 🟠 MajorPhase-boundary-only callbacks still leave the save path unresponsive.
Progress is emitted immediately before
buildSceneAiScene()andAssimp::Exporter::Export(), but not during either call. For large scenes those are the expensive sections, so the GUI thread still stops pumping events for the full build/write duration and the progress dialog won't actually prevent "not responding" behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 1303 - 1322, The UI freezes because long-running work is done synchronously around reportProgress calls; move the expensive work in buildSceneAiScene() and Assimp::Exporter::Export() off the GUI thread or add intermediate progress/emission points so the event loop can run: either (A) run buildSceneAiScene and the exporter call inside a worker thread (e.g., QThread/QtConcurrent) and call reportProgress from that worker via queued connections, or (B) break up the build/export into smaller steps inside buildSceneAiScene and the exporter wrapper and call reportProgress plus QCoreApplication::processEvents (or emit a progress signal) between steps so the GUI remains responsive; update references in this file to use the chosen approach for functions buildSceneAiScene, reportProgress, and the Assimp::Exporter::Export invocation.
♻️ Duplicate comments (1)
src/MeshImporterExporter.cpp (1)
133-139:⚠️ Potential issue | 🟠 MajorTexture save failures are still reported as a successful export.
This only logs and continues.
sceneExporter()can still return0while the exported scene references texture files that never made it to disk. Please propagate a hard failure out ofexportTextures()/exportMaterial()instead of swallowing it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 133 - 139, The catch in MeshImporterExporter::exportTextures (the block around img.save(...) and catch(Ogre::Exception& ex)) currently only logs and continues, causing sceneExporter()/exportMaterial() to report success despite failed texture writes; modify this by surfacing the failure: either rethrow the Ogre::Exception from the catch or set and return an error indicator (e.g., return false or set a failure flag) from exportTextures() and ensure exportMaterial() and sceneExporter() check that indicator and propagate a non-zero/failed return so the overall export fails when any img.save(...) throws; update callers (exportMaterial(), sceneExporter()) to handle and propagate the error state.
🧹 Nitpick comments (1)
src/MeshImporterExporter_test.cpp (1)
1892-1932: Please add a two-skinned-entity round-trip here.This covers one skinned entity plus one static entity, but it never exercises the actual collision path: two skinned entities whose bones and animations need prefixing in the same glTF scene. A regression there would still pass this test.
Based on learnings: "Multi-entity glTF scenes must use entity-name-prefixed bones to avoid cross-entity skeleton contamination when Assimp merges skins"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 1892 - 1932, Add a new round-trip test that creates two skinned entities (e.g., call createAnimatedTestEntity("SkelEntityA") and createAnimatedTestEntity("SkelEntityB")), place them apart, export with MeshImporterExporter::sceneExporter and re-import with MeshImporterExporter::sceneImporter, then assert both scene nodes are present and both corresponding Ogre::Entity instances report hasSkeleton() true; additionally verify the two reimported skeletons are not the same/shared (compare their skeleton instances or skeleton names from manager->getSceneMgr()->getEntity(sn->getName()) to ensure distinctness/prefixing) to catch cross-entity skeleton contamination when Assimp merges skins.
🤖 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.cpp`:
- Around line 105-114: exportTextureName currently collapses scene-relative
paths to basenames (e.g., "charA/diffuse.png" → "diffuse.png") causing later
textures to overwrite earlier ones; change exportTextureName to produce
scene-unique output names (for example by preserving a sanitized scene-relative
path prefix or appending a deterministic unique suffix/hash based on the
originalName) and maintain a mapping from originalName → resolvedExportName
inside MeshImporterExporter; then update buildAiMaterialFromOgre to consume that
same mapping (use the resolved name lookup when assigning texture filenames to
materials) so exported materials reference the deduplicated, collision-free
filenames consistently.
- Around line 261-330: The code ignores vData->vertexStart and iData->indexStart
causing wrong vertex reads and indices; compute a vertexOffset =
vData->vertexStart (and indexOffset = iData->indexStart) and apply it when
reading vertex buffers (posElem/normElem/tcElem): read from base + (j +
vertexOffset) * vbuf->getVertexSize (or otherwise map sourceIndex = j +
vertexOffset) so aiM->mVertices/mNormals/mTextureCoords fill 0..mNumVertices-1
correctly. For indices, account for iData->indexStart when computing the ibase
pointer offset or source index and then rebase exported indices by subtracting
vertexOffset so aiM->mFaces[].mIndices refer to aiM-local vertex numbering.
Finally, when transferring bone weights (the bone assignment code that uses
original vertex IDs), subtract vertexOffset from those vertex IDs so bone vertex
indices match the rebased aiM vertices. Use the symbols vData, vertexStart,
posElem, normElem, tcElem, ibuf, iData, indexStart, aiM, and aiM->mFaces to
locate the places to change.
---
Outside diff comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 1303-1322: The UI freezes because long-running work is done
synchronously around reportProgress calls; move the expensive work in
buildSceneAiScene() and Assimp::Exporter::Export() off the GUI thread or add
intermediate progress/emission points so the event loop can run: either (A) run
buildSceneAiScene and the exporter call inside a worker thread (e.g.,
QThread/QtConcurrent) and call reportProgress from that worker via queued
connections, or (B) break up the build/export into smaller steps inside
buildSceneAiScene and the exporter wrapper and call reportProgress plus
QCoreApplication::processEvents (or emit a progress signal) between steps so the
GUI remains responsive; update references in this file to use the chosen
approach for functions buildSceneAiScene, reportProgress, and the
Assimp::Exporter::Export invocation.
---
Duplicate comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 133-139: The catch in MeshImporterExporter::exportTextures (the
block around img.save(...) and catch(Ogre::Exception& ex)) currently only logs
and continues, causing sceneExporter()/exportMaterial() to report success
despite failed texture writes; modify this by surfacing the failure: either
rethrow the Ogre::Exception from the catch or set and return an error indicator
(e.g., return false or set a failure flag) from exportTextures() and ensure
exportMaterial() and sceneExporter() check that indicator and propagate a
non-zero/failed return so the overall export fails when any img.save(...)
throws; update callers (exportMaterial(), sceneExporter()) to handle and
propagate the error state.
---
Nitpick comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 1892-1932: Add a new round-trip test that creates two skinned
entities (e.g., call createAnimatedTestEntity("SkelEntityA") and
createAnimatedTestEntity("SkelEntityB")), place them apart, export with
MeshImporterExporter::sceneExporter and re-import with
MeshImporterExporter::sceneImporter, then assert both scene nodes are present
and both corresponding Ogre::Entity instances report hasSkeleton() true;
additionally verify the two reimported skeletons are not the same/shared
(compare their skeleton instances or skeleton names from
manager->getSceneMgr()->getEntity(sn->getName()) to ensure
distinctness/prefixing) to catch cross-entity skeleton contamination when Assimp
merges skins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9a2940a-84bc-4ca2-8def-097cef281f8f
📒 Files selected for processing (3)
src/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cppsrc/mainwindow.cpp
| QString MeshImporterExporter::exportTextureName(const QString& originalName) | ||
| { | ||
| QFileInfo fi(originalName); | ||
| QString ext = fi.suffix().toLower(); | ||
| // STBI codec supports writing: png, bmp, tga, hdr | ||
| // For anything else (jpg, jpeg, dds, etc.), convert to png | ||
| static const QStringList supported = {"png", "bmp", "tga", "hdr"}; | ||
| if (supported.contains(ext)) | ||
| return fi.fileName(); | ||
| return fi.completeBaseName() + ".png"; |
There was a problem hiding this comment.
Basename-only texture names will overwrite distinct source textures.
charA/diffuse.png and charB/diffuse.png both collapse to diffuse.png here, so the later save clobbers the first and both exported materials end up pointing at the same file. This mapping needs scene-scoped collision handling, and buildAiMaterialFromOgre() needs to consume the same resolved names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 105 - 114, exportTextureName
currently collapses scene-relative paths to basenames (e.g., "charA/diffuse.png"
→ "diffuse.png") causing later textures to overwrite earlier ones; change
exportTextureName to produce scene-unique output names (for example by
preserving a sanitized scene-relative path prefix or appending a deterministic
unique suffix/hash based on the originalName) and maintain a mapping from
originalName → resolvedExportName inside MeshImporterExporter; then update
buildAiMaterialFromOgre to consume that same mapping (use the resolved name
lookup when assigning texture filenames to materials) so exported materials
reference the deduplicated, collision-free filenames consistently.
| const auto* posElem = vData->vertexDeclaration->findElementBySemantic(Ogre::VES_POSITION); | ||
| if (posElem) | ||
| { | ||
| auto vbuf = vData->vertexBufferBinding->getBuffer(posElem->getSource()); | ||
| auto* base = static_cast<const unsigned char*>(vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| for (unsigned int j = 0; j < aiM->mNumVertices; ++j) | ||
| { | ||
| const Ogre::Real* p; | ||
| posElem->baseVertexPointerToElement(const_cast<unsigned char*>(base + j * vbuf->getVertexSize()), &p); | ||
| aiM->mVertices[j] = aiVector3D(p[0], p[1], p[2]); | ||
| } | ||
| vbuf->unlock(); | ||
| } | ||
|
|
||
| // Read normals | ||
| const auto* normElem = vData->vertexDeclaration->findElementBySemantic(Ogre::VES_NORMAL); | ||
| if (normElem) | ||
| { | ||
| aiM->mNormals = new aiVector3D[aiM->mNumVertices]; | ||
| auto vbuf = vData->vertexBufferBinding->getBuffer(normElem->getSource()); | ||
| auto* base = static_cast<const unsigned char*>(vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| for (unsigned int j = 0; j < aiM->mNumVertices; ++j) | ||
| { | ||
| const Ogre::Real* p; | ||
| normElem->baseVertexPointerToElement(const_cast<unsigned char*>(base + j * vbuf->getVertexSize()), &p); | ||
| aiM->mNormals[j] = aiVector3D(p[0], p[1], p[2]); | ||
| } | ||
| vbuf->unlock(); | ||
| } | ||
|
|
||
| // Read texture coordinates | ||
| const auto* tcElem = vData->vertexDeclaration->findElementBySemantic(Ogre::VES_TEXTURE_COORDINATES); | ||
| if (tcElem) | ||
| { | ||
| aiM->mTextureCoords[0] = new aiVector3D[aiM->mNumVertices]; | ||
| aiM->mNumUVComponents[0] = 2; | ||
| auto vbuf = vData->vertexBufferBinding->getBuffer(tcElem->getSource()); | ||
| auto* base = static_cast<const unsigned char*>(vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| for (unsigned int j = 0; j < aiM->mNumVertices; ++j) | ||
| { | ||
| const Ogre::Real* p; | ||
| tcElem->baseVertexPointerToElement(const_cast<unsigned char*>(base + j * vbuf->getVertexSize()), &p); | ||
| aiM->mTextureCoords[0][j] = aiVector3D(p[0], p[1], 0.0f); | ||
| } | ||
| vbuf->unlock(); | ||
| } | ||
|
|
||
| // Read indices | ||
| const Ogre::IndexData* iData = subMesh->indexData; | ||
| if (iData && iData->indexCount > 0) | ||
| { | ||
| aiM->mNumFaces = static_cast<unsigned int>(iData->indexCount / 3); | ||
| aiM->mFaces = new aiFace[aiM->mNumFaces]; | ||
| auto ibuf = iData->indexBuffer; | ||
| auto* ibase = static_cast<const unsigned char*>(ibuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| bool use32 = ibuf->getType() == Ogre::HardwareIndexBuffer::IT_32BIT; | ||
|
|
||
| for (unsigned int f = 0; f < aiM->mNumFaces; ++f) | ||
| { | ||
| aiM->mFaces[f].mNumIndices = 3; | ||
| aiM->mFaces[f].mIndices = new unsigned int[3]; | ||
| for (unsigned int v = 0; v < 3; ++v) | ||
| { | ||
| unsigned int idx = use32 | ||
| ? reinterpret_cast<const uint32_t*>(ibase)[f * 3 + v] | ||
| : reinterpret_cast<const uint16_t*>(ibase)[f * 3 + v]; | ||
| aiM->mFaces[f].mIndices[v] = idx; | ||
| } | ||
| } | ||
| ibuf->unlock(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Ogre 14, when exporting a SubMesh from VertexData/IndexData, do vertexStartandindexStart need to be applied as buffer offsets, and should indices/bone assignments be rebased to aiMesh-local vertex numbering?
💡 Result:
Yes.
-
Apply
VertexData::vertexStartandIndexData::indexStartas offsets into their respective buffers when reading/exporting. In Ogre,indexStartis “the first index used” (i.e., an offset into the index buffer, in index units) for shared index buffers. [1] LikewisevertexStartis “the base vertex index to start from” (i.e., the first vertex of this geometry within the bound vertex buffer(s)). [2] -
If you are exporting a SubMesh into an
aiMeshwith its own compact vertex array (vertex 0..N-1), then you must rebase/remap:- Subtract
vertexStart(or do a proper used-vertex remap if you’re also shrinking to only referenced vertices) from every exported triangle index so thataiMesh::mFaces[*].mIndices[*]are aiMesh-local. - Rebase the vertex IDs used by bone weights/assignments the same way (and drop/ignore assignments outside the exported vertex range), because
aiMeshbone weights index local vertices.
- Subtract
If instead you were exporting without copying vertices (keeping a shared/global vertex pool), you’d still need a per-mesh “base vertex” concept—aiMesh doesn’t have that—so the practical approach for Assimp is: copy the vertex slice and rebase indices + bone vertex IDs.
Sources: [1] IndexData::indexStart definition (“first index used…sharing a single index buffer”) [1]; [2] VertexData::vertexStart definition (“base vertex index to start from”) [2].
🏁 Script executed:
#!/bin/bash
# Check the actual code at lines 261-330 in MeshImporterExporter.cpp
# Also check lines 345-350 mentioned as additional concern
# First, let's see the file size and read the relevant sections
wc -l src/MeshImporterExporter.cpp
# Read lines 240-370 to see context around the flagged lines
sed -n '240,370p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 5509
Honor vertexStart and indexStart when reading Ogre buffers and rebase vertex indices accordingly.
Vertex reading loops (positions, normals, texture coordinates) assume buffers start at offset 0, ignoring VertexData::vertexStart. Index reading ignores IndexData::indexStart. When a SubMesh resides in a shared buffer with an offset, this corrupts the exported geometry. Additionally, the bone weight assignment at lines 345–350 assigns vertex IDs directly from the original bone assignments without rebasing them to aiMesh-local numbering; these must be adjusted to match the rebased triangle indices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 261 - 330, The code ignores
vData->vertexStart and iData->indexStart causing wrong vertex reads and indices;
compute a vertexOffset = vData->vertexStart (and indexOffset =
iData->indexStart) and apply it when reading vertex buffers
(posElem/normElem/tcElem): read from base + (j + vertexOffset) *
vbuf->getVertexSize (or otherwise map sourceIndex = j + vertexOffset) so
aiM->mVertices/mNormals/mTextureCoords fill 0..mNumVertices-1 correctly. For
indices, account for iData->indexStart when computing the ibase pointer offset
or source index and then rebase exported indices by subtracting vertexOffset so
aiM->mFaces[].mIndices refer to aiM-local vertex numbering. Finally, when
transferring bone weights (the bone assignment code that uses original vertex
IDs), subtract vertexOffset from those vertex IDs so bone vertex indices match
the rebased aiM vertices. Use the symbols vData, vertexStart, posElem, normElem,
tcElem, ibuf, iData, indexStart, aiM, and aiM->mFaces to locate the places to
change.
Exercises the multi-entity bone name prefixing path that prevents cross-entity skeleton contamination when Assimp merges skins. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MeshImporterExporter_test.cpp (1)
1934-1974: Good round-trip test for multi-skeleton scenes.This test exercises the bone-prefixing logic mentioned in the PR objectives by exporting and reimporting two skeletal entities, verifying both retain their skeletons.
Consider strengthening the test by verifying that bone names are actually prefixed with entity names after reimport, which would directly validate the "entity-name-prefixed bones" behavior. However, the current test adequately verifies the functional outcome (both skeletons survive round-trip without contamination).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 1934 - 1974, Add assertions that after reimport each skeletal entity's bone names are prefixed with the entity name to directly validate bone-prefixing; iterate manager->getSceneNodes(), for each node that maps to an Ogre::Entity via manager->getSceneMgr()->getEntity(sn->getName()) and e->hasSkeleton(), query the entity's skeleton bone names and assert each bone name begins with the corresponding entity/node name (e.g., starts_with(sn->getName() + "/") or the project’s expected separator). Use existing symbols SceneSaveLoadTest, createAnimatedTestEntity, MeshImporterExporter::sceneExporter/sceneImporter, Manager::getSceneNodes and Manager::getSceneMgr to locate where to add these checks after the importer call and before the final EXPECT_EQ(skelCount, 2).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 1934-1974: Add assertions that after reimport each skeletal
entity's bone names are prefixed with the entity name to directly validate
bone-prefixing; iterate manager->getSceneNodes(), for each node that maps to an
Ogre::Entity via manager->getSceneMgr()->getEntity(sn->getName()) and
e->hasSkeleton(), query the entity's skeleton bone names and assert each bone
name begins with the corresponding entity/node name (e.g.,
starts_with(sn->getName() + "/") or the project’s expected separator). Use
existing symbols SceneSaveLoadTest, createAnimatedTestEntity,
MeshImporterExporter::sceneExporter/sceneImporter, Manager::getSceneNodes and
Manager::getSceneMgr to locate where to add these checks after the importer call
and before the final EXPECT_EQ(skelCount, 2).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d02d3928-a101-48da-a680-db1efaf2b0af
📒 Files selected for processing (1)
src/MeshImporterExporter_test.cpp
|
1 participant
Summary
QProgressDialogduring scene export so the UI stays responsive on large scenes instead of freezing and triggering OS "force quit" promptsChanges
MeshImporterExporter::sceneExporter()now accepts an optionalProgressCallback(backward-compatible, defaults tonullptr)exportTextureName()helper converts unsupported extensions to.png; used in both single-entity and scene-level exportersMainWindow::on_actionSave_Scene_triggered()shows a modal progress dialog with phase labels (textures → scene data → file write)exportTextureNamecovering PNG/BMP/TGA/HDR passthrough, JPG/JPEG/DDS conversion, case sensitivity, paths, and multi-dot filenames; 2 forsceneExporteredge cases; 1 integration test verifying progress callback reports monotonically increasing valuesTest plan
.pngtextures are written--gtest_filter="MeshImporterExporterStandaloneTest.ExportTextureName*:MeshImporterExporterStandaloneTest.SceneExporter*"🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores