Add scene save/load via glTF - #194
Merged
Merged
Conversation
Persist entire scenes (meshes, transforms, materials, skeletons, animations) to a single glTF file. Handles Assimp's shared-skin merging by using entity-name-prefixed bone filtering for correct round-trip of multiple skeletal entities. - Add sceneExporter()/sceneImporter() to MeshImporterExporter - Add Open Scene (Ctrl+O) / Save Scene (Ctrl+S) to File menu - Add save_scene/open_scene MCP tools - Fix crash when opening scene with skeleton debug/bone weights active - Fix TransformOperator null camera guard - Fix Manager isForbiddenNodeName for empty names - Add unit tests for scene save/load and MCP tools - Update docs, README, and HTML landing page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds end-to-end scene persistence: new MeshImporterExporter::sceneExporter/sceneImporter for full-scene glTF export/import (meshes, transforms, materials, per-entity skeletons and animations), MCP server tools and tests, UI actions for open/save scene, Manager signaling for scene clearing, and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant MainWindow
participant MeshImporterExporter
participant Assimp as Assimp Library
participant FileSystem as File System
participant Scene as Scene Graph
rect rgba(100,150,200,0.5)
Note over User,Scene: Scene Export Flow
User->>MainWindow: Trigger Save Scene
MainWindow->>MainWindow: Open Save Dialog
User->>MainWindow: Select Destination
MainWindow->>MeshImporterExporter: sceneExporter(filePath)
MeshImporterExporter->>Scene: Gather entities, meshes, materials, skeletons, animations
MeshImporterExporter->>MeshImporterExporter: Deduplicate materials, prefix bones per-entity, assemble animations
MeshImporterExporter->>Assimp: Export aiScene -> glTF/glb
Assimp->>FileSystem: Write file
FileSystem-->>Assimp: Success
Assimp-->>MeshImporterExporter: Export status
MeshImporterExporter-->>MainWindow: Return status
MainWindow-->>User: Confirm export
end
sequenceDiagram
participant User as User/UI
participant MainWindow
participant MeshImporterExporter
participant FileSystem as File System
participant Assimp as Assimp Library
participant Scene as Scene Graph
rect rgba(150,200,100,0.5)
Note over User,Scene: Scene Import Flow
User->>MainWindow: Trigger Open Scene
MainWindow->>MainWindow: Open File Dialog
User->>MainWindow: Select Source File
MainWindow->>MeshImporterExporter: sceneImporter(filePath)
MeshImporterExporter->>FileSystem: Read file
FileSystem-->>MeshImporterExporter: File data
MeshImporterExporter->>Assimp: Parse aiScene
Assimp-->>MeshImporterExporter: aiScene structure
MeshImporterExporter->>MeshImporterExporter: Decompose transforms, map animations, strip entity prefixes from bones
MeshImporterExporter->>Scene: Create nodes/entities, attach meshes, materials, skeletons, animations
Scene-->>MeshImporterExporter: Created entities
MeshImporterExporter-->>MainWindow: Loaded scene info
MainWindow-->>User: Scene restored
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 24b1c4be3f
ℹ️ 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".
Comment on lines
+1550
to
+1554
| // Clear existing scene | ||
| SelectionSet::getSingleton()->clearList(); | ||
| auto* manager = Manager::getSingleton(); | ||
| auto sceneNodesCopy = manager->getSceneNodes(); | ||
| for (auto* sn : sceneNodesCopy) |
There was a problem hiding this comment.
In MeshImporterExporter::sceneImporter, the existing scene is destroyed before attempting to parse the new file. If the file exists but Assimp cannot read it (ReadFile fails), the function returns early and leaves the application with an empty scene, which is user-visible data loss when opening a corrupt or unsupported scene file. Clear the current scene only after the new scene has been parsed successfully.
Useful? React with 👍 / 👎.
| if (!QFile::exists(filePath)) | ||
| return makeErrorResult("Error: File not found: " + filePath); | ||
|
|
||
| MeshImporterExporter::sceneImporter(filePath); |
There was a problem hiding this comment.
toolOpenScene always proceeds to build a success response after calling MeshImporterExporter::sceneImporter(filePath), but the importer returns void and handles parse failures internally. For an existing but invalid .gltf/.glb, this path reports success to MCP clients even though loading failed (and may have already cleared the scene), which can corrupt downstream automation decisions.
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: 6
🧹 Nitpick comments (2)
src/MeshImporterExporter_test.cpp (2)
1617-1670: Exercise the exact transform and animation data this PR is supposed to preserve.
RoundTrip_TwoEntities_PreservesTransformsnever sets rotation and only checks one scale axis, whileRoundTrip_SkeletonEntity_PreservesAnimationsonly asserts>= 1animation after reload. Those tests will still pass if orientation is dropped or animation filtering regresses. Add a non-identity rotation and assert all scale components, then verify the expected animation count/name that should round-trip.Also applies to: 1716-1749
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 1617 - 1670, The test RoundTrip_TwoEntities_PreservesTransforms must exercise and assert the full transform: set a non-identity rotation on SceneNode1 (use the scene node API used elsewhere, e.g., setOrientation/setRotation or whichever method exists on SceneNode), assert all three scale components (x,y,z) on reload rather than only x, and verify the rotation was preserved (compare the reloaded node's orientation/rotation to the original within a small tolerance). Likewise, update RoundTrip_SkeletonEntity_PreservesAnimations to assert the exact expected animation count and specific animation names (not just >= 1) after reimport to ensure animation filtering/round-trip is preserved. Use the existing symbols Manager, SceneNode (sn1/sn2), MeshImporterExporter::sceneExporter/sceneImporter, and the test names to locate and change the tests.
1672-1701: This test does not prove material deduplication yet.
MaterialDedup_SharedMaterial_ExportedOncecurrently only reimports the scene and recounts nodes, so duplicated material entries in the written.scene.gltfwould still pass. Since this path already exports text glTF, please assert the exportedmaterialsarray stays at one entry, or at least confirm both reloaded entities resolve to the same material resource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter_test.cpp` around lines 1672 - 1701, The test currently only checks node count but not material deduplication; update SceneSaveLoadTest::MaterialDedup_SharedMaterial_ExportedOnce to assert deduplication by either (A) reading the exported text glTF at sceneFile after MeshImporterExporter::sceneExporter and parsing its JSON to assert the "materials" array has size 1, or (B) after MeshImporterExporter::sceneImporter query the two reloaded entities from Manager::getSingleton() and assert they resolve to the same material resource (compare material pointers/names). Use the existing symbols MeshImporterExporter::sceneExporter, MeshImporterExporter::sceneImporter, and Manager::getSingleton()/createEntity to locate where to add the new assertion and fail the test if deduplication is not observed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/index.html`:
- Around line 584-585: Update the hard-coded tool count in the docs so it
matches the actual MCP tool list (or remove the count entirely): locate the
descriptive strings in docs/index.html that say "27 tools" (and the other
occurrence around lines 804-806) and either change the number to the current
count exposed by buildToolsList() in src/MCPServer.cpp (29 after adding
save_scene and open_scene) or remove the numeric count text so it reads
generically (e.g., "tools for materials, meshes..." ), ensuring the copy stays
accurate when buildToolsList() is modified in the future.
In `@src/mainwindow.cpp`:
- Around line 603-621: The recent-files flow currently records scene files via
addToRecentFiles(fileName) so openRecentFile later treats them like mesh imports
(via importMeshs()/MeshImporterExporter::importer()), which fails to restore
full scenes; fix by detecting scene extensions and routing to sceneImporter():
update openRecentFile to check the selected path's extension (e.g., ".scene.glb"
or ".scene.gltf") and call MeshImporterExporter::sceneImporter(path) (or the
same transaction-wrapped logic from on_actionOpen_Scene_triggered) instead of
calling importMeshs()/MeshImporterExporter::importer(); alternatively, keep
existing openRecentFile logic and change addToRecentFiles to store scene files
in a separate recent-scenes list so openRecentFile can open them with
sceneImporter()—adjust whichever place (addToRecentFiles or openRecentFile) to
differentiate scene files and ensure sceneImporter() is invoked for those
entries.
In `@src/MCPServer.cpp`:
- Around line 2608-2612: The tool description for "open_scene" passed to
buildToolDefinition promises "positions" but the open_scene implementation only
reports node/entity names and animation counts; either remove "positions" from
that description string or modify the open_scene success message generation to
include each entity's transform/position. Locate the "open_scene"
buildToolDefinition call and update its description text to omit "positions", or
alternatively update the code that constructs the open_scene success response
(the success text/summary emitted after loading scenes) to append per-entity
transforms/positions so the description matches the actual payload.
- Around line 2053-2090: MeshImporterExporter::sceneImporter currently returns
void and can clear the scene on failure, so modify it to return a boolean or a
Result/Status indicating success/failure (e.g., bool or an enum) and ensure it
does not unconditionally clear the scene on failure; update its signature and
all call sites accordingly (including the call in MCPServer.cpp) so
MCPServer.cpp checks the returned status before building the "Scene loaded..."
message; in MCPServer.cpp (the code that calls
MeshImporterExporter::sceneImporter(filePath)) only call makeSuccessResult(...)
when the importer returned success and otherwise return a failure result with
the import error message/details propagated from the new return value (ensure
you reference MeshImporterExporter::sceneImporter and
Manager::getSingletonPtr()/getSceneNodes() when locating the code to change).
In `@src/MeshImporterExporter.cpp`:
- Around line 1550-1555: Don't clear the live scene before parsing: instead,
parse the incoming file into a temporary/staging scene (e.g., create a temporary
Manager or staging container and use ReadFile() against that) and only call
SelectionSet::getSingleton()->clearList(), Manager::getSingleton(),
getSceneNodes(), and destroySceneNode(...) to tear down the live scene after the
new scene has parsed and validated successfully; apply the same staging/swap
pattern for the other teardown block referenced around lines 1577-1583 so the
live scene is replaced only on a known-good import.
- Around line 1639-1708: The exporter currently unconditionally treats nested
mesh nodes as the synthetic "<entity>/<entity>_mesh" pattern and enables
entityPrefix bone/animation filtering; change this to first detect that pattern
and only apply the synthetic naming logic when present. Specifically, in the
MeshImporterExporter code path that sets entityPrefix (use variables nodeName,
meshName, entityPrefix, skelName and the surrounding logic that checks
node->mParent and meshNodes.size()), add a guard that verifies the child node
name is exactly parentName + "_mesh" (or the exact pattern produced by
buildSceneAiScene()) before setting entityPrefix and before deriving
meshName/skelName from the child; if the pattern is present, restore the logical
entity name (parentName) and use parentName-derived mesh/skeleton names and
entityPrefix = parentName + "_"; otherwise treat the node as a normal grouped
glTF node (leave entityPrefix empty and keep meshName/skelName based on
nodeName) so skeletons/animations are not incorrectly filtered.
---
Nitpick comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 1617-1670: The test RoundTrip_TwoEntities_PreservesTransforms must
exercise and assert the full transform: set a non-identity rotation on
SceneNode1 (use the scene node API used elsewhere, e.g.,
setOrientation/setRotation or whichever method exists on SceneNode), assert all
three scale components (x,y,z) on reload rather than only x, and verify the
rotation was preserved (compare the reloaded node's orientation/rotation to the
original within a small tolerance). Likewise, update
RoundTrip_SkeletonEntity_PreservesAnimations to assert the exact expected
animation count and specific animation names (not just >= 1) after reimport to
ensure animation filtering/round-trip is preserved. Use the existing symbols
Manager, SceneNode (sn1/sn2), MeshImporterExporter::sceneExporter/sceneImporter,
and the test names to locate and change the tests.
- Around line 1672-1701: The test currently only checks node count but not
material deduplication; update
SceneSaveLoadTest::MaterialDedup_SharedMaterial_ExportedOnce to assert
deduplication by either (A) reading the exported text glTF at sceneFile after
MeshImporterExporter::sceneExporter and parsing its JSON to assert the
"materials" array has size 1, or (B) after MeshImporterExporter::sceneImporter
query the two reloaded entities from Manager::getSingleton() and assert they
resolve to the same material resource (compare material pointers/names). Use the
existing symbols MeshImporterExporter::sceneExporter,
MeshImporterExporter::sceneImporter, and Manager::getSingleton()/createEntity to
locate where to add the new assertion and fail the test if deduplication is not
observed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec78336b-9663-41cb-a949-ef1505199729
📒 Files selected for processing (15)
CLAUDE.mdREADME.mddocs/index.htmlsrc/AnimationWidget.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/MCPServer_test.cppsrc/Manager.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/MeshImporterExporter_test.cppsrc/TransformOperator.cppsrc/mainwindow.cppsrc/mainwindow.hui_files/mainwindow.ui
- Parse file before clearing scene so invalid files don't destroy user's work - Change sceneImporter() to return bool; MCP checks result before reporting success - Guard entityPrefix detection for exact synthetic "_mesh" pattern only - Route scene files from recent files menu to sceneImporter() instead of importer() - Fix open_scene MCP tool description (remove "positions" claim) - Fix tool count in docs (27 → 29) - Strengthen tests: rotation + all scale axes, material dedup via glTF JSON, exact animation count/name assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/MeshImporterExporter.cpp (1)
1577-1583:⚠️ Potential issue | 🔴 CriticalParsing first is only a partial fix here.
The live scene is still destroyed before
loadScene(), skeleton reconstruction, andcreateMesh()have succeeded. Any exception on that path now returnsfalsewith the editor empty or partially rebuilt. Please stage the import and swap only after the whole scene is known-good.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 1577 - 1583, The current code clears the live scene immediately (SelectionSet::getSingleton()->clearList() and Manager::getSingleton()->destroySceneNode(...)) which can leave the editor empty if loadScene(), skeleton reconstruction, or createMesh() later throw; instead, stage the import into temporary structures and only swap into the live Manager on success: build the new scene off-line (e.g., using a temporary Manager instance or by constructing new scene nodes/meshes/skeletons into a local container), run loadScene(), skeleton reconstruction, and createMesh() against that staged data, and if all succeed replace the live Manager’s scene atomically (clear and destroy existing nodes and move staged nodes into Manager) so exceptions never leave the editor in a partially rebuilt state. Ensure rollback/cleanup of staged resources on failure.
🤖 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 615-622: MeshImporterExporter::sceneImporter()'s boolean return is
ignored, allowing failed imports to still call
SentryReporter::finishTransaction(txn) and addToRecentFiles(fileName); change
the call to capture the result (e.g., bool ok =
MeshImporterExporter::sceneImporter(fileName)), then if ok is false call
SentryReporter::finishTransaction(txn) and return/throw (so addToRecentFiles is
not executed), otherwise proceed to finish the transaction and call
addToRecentFiles; also ensure finishTransaction(txn) is only called once in both
success and failure paths (remove duplicate calls if present).
In `@src/MeshImporterExporter.cpp`:
- Around line 1195-1196: The prefix matching using raw node name plus "_"
(variable bonePrefix created from sn->getName()) is ambiguous; change the token
to a collision-free separator (e.g., append a fixed, unlikely token like
"__NODE__" or "::NODE::" instead of "_") wherever bonePrefix is formed (the
bonePrefix assignment that uses nodeEntities/hasSkeleton and sn->getName()) and
update all matching logic that tests name starts/startswith to use this new
token; also apply the same change to the other two spots mentioned (the similar
prefix constructions at the blocks around the original locations referenced) so
bone/animation matching uses nodeName + uniqueToken rather than nodeName + "_"
to avoid accidental collisions with names like "Hero_Alt".
- Around line 44-50: The file MeshImporterExporter.cpp relies on std::function
(used in two places) but doesn't include <functional> directly; add an explicit
`#include` <functional> to the top include block alongside the other headers so
the translation unit doesn't rely on transitive includes and compiles across
toolchains.
- Around line 1095-1108: The material deduplication currently keys materials by
mat->getName() which collapses materials from different resource groups; change
the key for matIndexMap to use the pair (mat->getGroup(), mat->getName())
instead of name alone, update the map type and lookup/insertion sites in the
loop that builds materials (referencing materials and matIndexMap) and any later
lookup that uses matIndexMap (e.g., the usage around the code referenced near
line ~1286) so you compute/find by the same group+name pair when assigning
material indices.
---
Duplicate comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 1577-1583: The current code clears the live scene immediately
(SelectionSet::getSingleton()->clearList() and
Manager::getSingleton()->destroySceneNode(...)) which can leave the editor empty
if loadScene(), skeleton reconstruction, or createMesh() later throw; instead,
stage the import into temporary structures and only swap into the live Manager
on success: build the new scene off-line (e.g., using a temporary Manager
instance or by constructing new scene nodes/meshes/skeletons into a local
container), run loadScene(), skeleton reconstruction, and createMesh() against
that staged data, and if all succeed replace the live Manager’s scene atomically
(clear and destroy existing nodes and move staged nodes into Manager) so
exceptions never leave the editor in a partially rebuilt state. Ensure
rollback/cleanup of staged resources on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92b75036-4c3a-4025-a09e-9e98299a9dd2
📒 Files selected for processing (6)
docs/index.htmlsrc/MCPServer.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/MeshImporterExporter_test.cppsrc/mainwindow.cpp
Comment on lines
+615
to
+622
| MeshImporterExporter::sceneImporter(fileName); | ||
| } catch (...) { | ||
| SentryReporter::finishTransaction(txn); | ||
| throw; | ||
| } | ||
| SentryReporter::finishTransaction(txn); | ||
| addToRecentFiles(fileName); | ||
| } |
There was a problem hiding this comment.
Handle failed scene imports before reporting success or updating recent files.
Line 615 and Line 1401 ignore sceneImporter()’s boolean result, so failed imports can still look successful and get promoted in recents.
💡 Proposed fix
void MainWindow::on_actionOpen_Scene_triggered()
{
@@
- try {
- MeshImporterExporter::sceneImporter(fileName);
+ bool imported = false;
+ try {
+ imported = MeshImporterExporter::sceneImporter(fileName);
} catch (...) {
SentryReporter::finishTransaction(txn);
throw;
}
SentryReporter::finishTransaction(txn);
- addToRecentFiles(fileName);
+ if (!imported) {
+ QMessageBox::warning(this, tr("Open Scene"), tr("Failed to open scene."));
+ return;
+ }
+ addToRecentFiles(fileName);
}
@@
void MainWindow::openRecentFile()
{
@@
- addToRecentFiles(filePath);
- if (filePath.endsWith(".scene.glb") || filePath.endsWith(".scene.gltf"))
- MeshImporterExporter::sceneImporter(filePath);
- else
+ if (filePath.endsWith(".scene.glb", Qt::CaseInsensitive) ||
+ filePath.endsWith(".scene.gltf", Qt::CaseInsensitive)) {
+ if (!MeshImporterExporter::sceneImporter(filePath)) {
+ QMessageBox::warning(this, tr("Open Scene"),
+ tr("Failed to open scene file \"%1\".").arg(filePath));
+ return;
+ }
+ } else {
mUriList.append(filePath);
+ }
+ addToRecentFiles(filePath);Also applies to: 1399-1403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 615 - 622,
MeshImporterExporter::sceneImporter()'s boolean return is ignored, allowing
failed imports to still call SentryReporter::finishTransaction(txn) and
addToRecentFiles(fileName); change the call to capture the result (e.g., bool ok
= MeshImporterExporter::sceneImporter(fileName)), then if ok is false call
SentryReporter::finishTransaction(txn) and return/throw (so addToRecentFiles is
not executed), otherwise proceed to finish the transaction and call
addToRecentFiles; also ensure finishTransaction(txn) is only called once in both
success and failure paths (remove duplicate calls if present).
Comment on lines
+44
to
+50
| #include "SelectionSet.h" | ||
| #include "SentryReporter.h" | ||
| #include "Assimp/Importer.h" | ||
| #include "Assimp/MaterialProcessor.h" | ||
| #include "Assimp/MeshProcessor.h" | ||
| #include "Assimp/BoneProcessor.h" | ||
| #include "Assimp/AnimationProcessor.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that src/MeshImporterExporter.cpp uses std::function without a direct <functional> include.
rg -n 'std::function|#include <functional>' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 236
🏁 Script executed:
sed -n '40,60p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 677
🏁 Script executed:
sed -n '1,50p' src/MeshImporterExporter.cpp | grep -n "QDebug\|#include <set>"Repository: fernandotonon/QtMeshEditor
Length of output: 108
🏁 Script executed:
sed -n '34,42p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 271
Add the direct <functional> include.
This translation unit uses std::function twice (at lines 1207 and 1618), but the include block relies on transitive headers to provide it. Please include <functional> explicitly so this keeps compiling across toolchains.
Minimal fix
`#include` <QDebug>
`#include` <set>
+#include <functional>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 44 - 50, The file
MeshImporterExporter.cpp relies on std::function (used in two places) but
doesn't include <functional> directly; add an explicit `#include` <functional> to
the top include block alongside the other headers so the translation unit
doesn't rely on transitive includes and compiles across toolchains.
Comment on lines
+1095
to
+1108
| // --- Deduplicate materials across all entities --- | ||
| std::vector<Ogre::MaterialPtr> materials; | ||
| std::map<std::string, unsigned int, std::less<>> matIndexMap; | ||
| for (const auto& [sn, entity] : nodeEntities) | ||
| { | ||
| for (const auto* sub : entity->getSubEntities()) | ||
| { | ||
| auto mat = sub->getMaterial(); | ||
| if (matIndexMap.find(mat->getName()) == matIndexMap.end()) | ||
| { | ||
| matIndexMap[mat->getName()] = static_cast<unsigned int>(materials.size()); | ||
| materials.push_back(mat); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to understand its structure and verify the code at the mentioned lines
wc -l src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 103
🏁 Script executed:
# Read the code around lines 1095-1108
sed -n '1090,1115p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1020
🏁 Script executed:
# Read the code around lines 1284-1287
sed -n '1280,1295p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 968
🏁 Script executed:
# Search for usage of matIndexMap to understand how it's being used
rg "matIndexMap" src/MeshImporterExporter.cpp -nRepository: fernandotonon/QtMeshEditor
Length of output: 768
🏁 Script executed:
# Check if Ogre::Material has getGroup() method by searching for Material usage patterns
rg "getMaterial|getGroup" src/MeshImporterExporter.cpp -n -A 1 -B 1Repository: fernandotonon/QtMeshEditor
Length of output: 890
🏁 Script executed:
# Check includes and Ogre Material API usage
head -50 src/MeshImporterExporter.cpp | grep -i "include\|ogre"Repository: fernandotonon/QtMeshEditor
Length of output: 747
🏁 Script executed:
# Check for includes related to Ogre
rg "#include.*[Oo]gre" src/MeshImporterExporter.cpp -n | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 199
🏁 Script executed:
# Search for any documentation or comments about material groups in the code
rg "group|Group" src/MeshImporterExporter.cpp -n -i | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 827
🏁 Script executed:
# Check the Ogre header files for Material API - look at what methods are available
fd "Material.h" --type fRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there's any example of using getGroup() in the codebase
rg "getGroup\|getResourceGroup" . -r -n --type cpp --type h | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for how materials are created or loaded to understand resource groups
rg "create.*material\|Material.*group\|ResourceGroup" . -r -n --type cpp --type h -i | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for all includes to see what Ogre headers are available
rg "#include" src/MeshImporterExporter.h -nRepository: fernandotonon/QtMeshEditor
Length of output: 199
🏁 Script executed:
# Search for material creation or management in the entire codebase
rg "MaterialManager\|Material\(" . -r -n --type cpp --type h -i | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check the CLAUDE.md file for context about Ogre 14.x and MaterialSerializer
cat CLAUDE.md 2>/dev/null || echo "File not found"Repository: fernandotonon/QtMeshEditor
Length of output: 12384
🏁 Script executed:
# Look for Ogre Material API usage - specifically check what methods are called on MaterialPtr
rg "mat->" src/MeshImporterExporter.cpp -n | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 346
🏁 Script executed:
# Check if there's any indication in the codebase of Ogre version
rg "ogre.*version\|OGRE.*VERSION" . -r -n -i --type cpp --type h --type cmake | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if Material has getGroup() method - search for actual usage patterns
rg "Material\*\|MaterialPtr" src/MeshImporterExporter.cpp -n -A 2 | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for how materials are accessed/created to understand the API
rg "Material" src/ -n --type cpp --type h | grep -E "getGroup|getResourceGroup|resourceGroup|group\(" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Look for any Ogre Material documentation or API hints in comments
rg "Material.*group\|resource.*group" src/ -n -i --type cpp --type h | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check how materials are retrieved from entities elsewhere
rg "getMaterial\|createMaterial" src/ -n --type cpp --type h -A 2 | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for Ogre includes and Material usage - broader search
rg "Ogre::" src/MeshImporterExporter.cpp -n | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 3116
🏁 Script executed:
# Look for any references to resource groups in material-related code
rg "DEFAULT_RESOURCE_GROUP" src/ -n --type cpp --type hRepository: fernandotonon/QtMeshEditor
Length of output: 8542
🏁 Script executed:
# Check if there are any comments or documentation about material identity
rg -i "material.*unique\|material.*identity\|material.*duplicate" src/ -n --type cpp --type hRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for material creation patterns to understand if group matters
rg "create.*material\|Material::" src/ -n --type cpp --type h -i | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Try to find Ogre header information or API usage
python3 << 'EOF'
# Since we're dealing with Ogre 14.x, let's check the common patterns
# In Ogre, materials are typically identified by (name, resourceGroup) tuple
# Let's verify if the current code actually has the bug
# Read the code around line 1097
with open('src/MeshImporterExporter.cpp', 'r') as f:
lines = f.readlines()
# Check lines around 1097
print("Code around line 1097 (0-indexed 1096):")
for i in range(1090, 1115):
if i < len(lines):
print(f"{i+1}: {lines[i]}", end='')
print("\n\nCode around line 1286 (0-indexed 1285):")
for i in range(1280, 1295):
if i < len(lines):
print(f"{i+1}: {lines[i]}", end='')
# Check what methods are called on mat variable
print("\n\nMethods called on mat:")
import re
for i, line in enumerate(lines):
if 'mat->' in line and i > 1090 and i < 1110:
print(f"{i+1}: {line.strip()}")
EOFRepository: fernandotonon/QtMeshEditor
Length of output: 2303
🏁 Script executed:
# Search for any getGroup or getResourceGroup usage in the codebase
rg "getGroup|getResourceGroup" . -r -n --type cpp --type hRepository: fernandotonon/QtMeshEditor
Length of output: 372
🏁 Script executed:
# Check Ogre API documentation or header hints by searching for Material class definition
fd "\.h" --type f -x grep -l "class Material" {} \; 2>/dev/nullRepository: fernandotonon/QtMeshEditor
Length of output: 245
🏁 Script executed:
# Check if there's any comment about material group or resource group in the material-related code
rg -B 3 -A 3 "MaterialPtr\|MaterialManager" src/MeshImporterExporter.cpp -n | head -60Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check the actual Ogre Material API by looking at how materials are compared/used
# Search for Material pointer dereferencing and methods
rg "mat\.|material\.|Material::" src/ -n --type cpp --type h | grep -E "getGroup|getResourceGroup|getName|getPtr" | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there's any code that uses MaterialPtr identity comparison
rg "MaterialPtr.*==" src/ -n --type cpp --type h | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if the problem actually manifests - search for any existing material deduplication logic elsewhere
rg -B 5 -A 5 "Deduplicate.*material\|material.*dedup" src/ -n -i --type cpp --type hRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Look for patterns where two materials with same name but different groups might occur
rg "ResourceGroup\|resource.*group" src/MeshImporterExporter.cpp -n -B 2 -A 2 | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there are imports from different resource groups that could cause the collision
rg "ensureResourceGroup\|addResourceLocation\|resourceLocationExists" src/MeshImporterExporter.cpp -n -B 3 -A 3Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Let's check if we can use MaterialPtr identity comparison instead of trying to call getGroup()
# First, check what attributes MaterialPtr might have
rg "MaterialPtr" src/ -n --type cpp --type h | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 3555
🏁 Script executed:
# Search for how materials are retrieved from the scene/entities to understand the context
rg "sub->getMaterial\|entity->.*Material\|getSubEntities" src/MeshImporterExporter.cpp -n -B 2 -A 2 | head -60Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if MaterialPtr can be compared directly or used in maps
rg "map.*Material\|set.*Material" src/ -n --type cpp --type h | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Look at the broader context of where this function is called to understand if materials really can have duplicate names
sed -n '1050,1150p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3651
🏁 Script executed:
# Check if there's any test that exercises this code path
rg "sceneExporter\|buildAiScene" src/ -n --type cpp --type h | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Let's check if there's any way to get the resource group from a MaterialPtr
# Search for how materials maintain their resource group information
rg "getGroup\(\)|getResourceGroup\(\)|getCreator" src/ -n --type cpp --type h | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 1122
🏁 Script executed:
# Check if MaterialPtr can be used as a map key directly
python3 << 'EOF'
# Let's analyze the actual problem more carefully
# In Ogre 14.x, materials are typically identified by (name, resourceGroup)
# Let's check if we can find any documentation or pattern
# Read the surrounding code to understand the function better
with open('src/MeshImporterExporter.cpp', 'r') as f:
lines = f.readlines()
# Look at the entire function to understand context
for i in range(1040, 1320):
if i < len(lines):
if 'buildSceneAiScene' in lines[i] or 'buildAiScene' in lines[i] or 'for (unsigned int i = 0; i < scene->mNumMaterials' in lines[i]:
print(f"{i+1}: {lines[i]}", end='')
print("\n\n=== Checking what methods are called on materials ===")
for i in range(1095, 1120):
if i < len(lines):
print(f"{i+1}: {lines[i]}", end='')
EOFRepository: fernandotonon/QtMeshEditor
Length of output: 1355
🏁 Script executed:
# Check if Ogre Material has a method to get its resource group
# Let's search for how materials are retrieved using resource groups
rg "getByName.*DEFAULT_RESOURCE_GROUP\|create.*DEFAULT_RESOURCE_GROUP" src/MeshImporterExporter.cpp -n -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check the test file to understand better how materials are used in this code
head -100 src/MeshImporterExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3048
🏁 Script executed:
# Confirm that Ogre Material has getGroup() method - found in RTShaderHelper.cpp
grep -n "getGroup()" src/RTShaderHelper.cppRepository: fernandotonon/QtMeshEditor
Length of output: 221
🏁 Script executed:
# Verify the issue exists in both locations mentioned in the review comment
sed -n '1095,1110p' src/MeshImporterExporter.cpp && echo "---" && sed -n '1284,1290p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1041
🏁 Script executed:
# Check if there are other matIndexMap usages that also need fixing
rg "matIndexMap" src/MeshImporterExporter.cpp -nRepository: fernandotonon/QtMeshEditor
Length of output: 768
🏁 Script executed:
# Check the similar pattern at lines 248-250 to see if it has the same issue
sed -n '245,255p' src/MeshImporterExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 472
Use (group, name) pair as material key, not name alone.
Materials with identical names in different resource groups will incorrectly collapse into a single aiMaterial. Key the deduplication map by (mat->getGroup(), mat->getName()) instead of just mat->getName():
Fix for lines 1097–1108 and 1286
- std::map<std::string, unsigned int, std::less<>> matIndexMap;
+ using MaterialKey = std::pair<std::string, std::string>;
+ std::map<MaterialKey, unsigned int> matIndexMap;
for (const auto& [sn, entity] : nodeEntities)
{
for (const auto* sub : entity->getSubEntities())
{
auto mat = sub->getMaterial();
- if (matIndexMap.find(mat->getName()) == matIndexMap.end())
+ MaterialKey key{mat->getGroup(), mat->getName()};
+ if (matIndexMap.find(key) == matIndexMap.end())
{
- matIndexMap[mat->getName()] = static_cast<unsigned int>(materials.size());
+ matIndexMap[key] = static_cast<unsigned int>(materials.size());
materials.push_back(mat);
}
}
}
...
- auto matIt = matIndexMap.find(subEnt->getMaterial()->getName());
+ MaterialKey key{subEnt->getMaterial()->getGroup(), subEnt->getMaterial()->getName()};
+ auto matIt = matIndexMap.find(key);📝 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.
Suggested change
| // --- Deduplicate materials across all entities --- | |
| std::vector<Ogre::MaterialPtr> materials; | |
| std::map<std::string, unsigned int, std::less<>> matIndexMap; | |
| for (const auto& [sn, entity] : nodeEntities) | |
| { | |
| for (const auto* sub : entity->getSubEntities()) | |
| { | |
| auto mat = sub->getMaterial(); | |
| if (matIndexMap.find(mat->getName()) == matIndexMap.end()) | |
| { | |
| matIndexMap[mat->getName()] = static_cast<unsigned int>(materials.size()); | |
| materials.push_back(mat); | |
| } | |
| } | |
| // --- Deduplicate materials across all entities --- | |
| std::vector<Ogre::MaterialPtr> materials; | |
| using MaterialKey = std::pair<std::string, std::string>; | |
| std::map<MaterialKey, unsigned int> matIndexMap; | |
| for (const auto& [sn, entity] : nodeEntities) | |
| { | |
| for (const auto* sub : entity->getSubEntities()) | |
| { | |
| auto mat = sub->getMaterial(); | |
| MaterialKey key{mat->getGroup(), mat->getName()}; | |
| if (matIndexMap.find(key) == matIndexMap.end()) | |
| { | |
| matIndexMap[key] = static_cast<unsigned int>(materials.size()); | |
| materials.push_back(mat); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 1095 - 1108, The material
deduplication currently keys materials by mat->getName() which collapses
materials from different resource groups; change the key for matIndexMap to use
the pair (mat->getGroup(), mat->getName()) instead of name alone, update the map
type and lookup/insertion sites in the loop that builds materials (referencing
materials and matIndexMap) and any later lookup that uses matIndexMap (e.g., the
usage around the code referenced near line ~1286) so you compute/find by the
same group+name pair when assigning material indices.
Comment on lines
+1195
to
+1196
| std::string bonePrefix = (nodeEntities.size() > 1 && hasSkeleton) | ||
| ? std::string(sn->getName()) + "_" : ""; |
There was a problem hiding this comment.
The <entity>_ namespace is ambiguous.
With names like Hero and Hero_Alt, the current prefix checks make Hero_ accept Hero_Alt_* bones and animations too. That can merge rigs back together for valid node names. Use a collision-free token instead of raw nodeName + "_" prefix matching.
Also applies to: 1714-1717, 1795-1800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 1195 - 1196, The prefix matching
using raw node name plus "_" (variable bonePrefix created from sn->getName()) is
ambiguous; change the token to a collision-free separator (e.g., append a fixed,
unlikely token like "__NODE__" or "::NODE::" instead of "_") wherever bonePrefix
is formed (the bonePrefix assignment that uses nodeEntities/hasSkeleton and
sn->getName()) and update all matching logic that tests name starts/startswith
to use this new token; also apply the same change to the other two spots
mentioned (the similar prefix constructions at the blocks around the original
locations referenced) so bone/animation matching uses nodeName + uniqueToken
rather than nodeName + "_" to avoid accidental collisions with names like
"Hero_Alt".
… test - Add Manager::sceneClearing signal emitted before scene teardown loop - AnimationWidget connects to sceneClearing to disableAllSkeletonDebug() before any entities are destroyed, preventing SkeletonDebug timer from accessing dangling entity pointers - Remove Qt.WindowStaysOnTopHint from ViewCube so it doesn't render on top of material editor modals and dock widgets - Fix ManagerHeadlessTest.IsForbiddenNodeName to expect empty string as forbidden (matches the isForbiddenNodeName change from prior commit) - Replace deprecated getAttachedObjectIterator() in MCPServer Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/MeshImporterExporter.cpp (4)
1097-1106:⚠️ Potential issue | 🟠 MajorMaterial deduplication key is unsafe across resource groups.
At Line 1097 and Line 1286, keying by
mat->getName()alone can collapse distinct materials that share a name but live in different Ogre groups.Proposed fix
- std::map<std::string, unsigned int, std::less<>> matIndexMap; + using MaterialKey = std::pair<std::string, std::string>; // (group, name) + std::map<MaterialKey, unsigned int> matIndexMap; @@ - if (matIndexMap.find(mat->getName()) == matIndexMap.end()) + MaterialKey key{mat->getGroup(), mat->getName()}; + if (matIndexMap.find(key) == matIndexMap.end()) { - matIndexMap[mat->getName()] = static_cast<unsigned int>(materials.size()); + matIndexMap[key] = static_cast<unsigned int>(materials.size()); materials.push_back(mat); } @@ - auto matIt = matIndexMap.find(subEnt->getMaterial()->getName()); + MaterialKey key{subEnt->getMaterial()->getGroup(), subEnt->getMaterial()->getName()}; + auto matIt = matIndexMap.find(key);Also applies to: 1286-1287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 1097 - 1106, The material deduplication currently keys matIndexMap by mat->getName(), which can incorrectly merge materials with the same name from different resource groups; update the key to include the material's resource group (e.g., combine mat->getName() and mat->getGroup() or another unique group identifier) wherever matIndexMap is populated/queried (reference symbols: matIndexMap, nodeEntities loop using getSubEntities() and getMaterial(), and the materials vector) and apply the same change to the other occurrence around the second block (the lines referenced at 1286-1287) so materials are unique per resource group.
37-38:⚠️ Potential issue | 🟡 MinorAdd a direct
<functional>include forstd::function.
std::functionis used at Line 1207 and Line 1619, but this TU doesn’t include<functional>explicitly.Minimal fix
`#include` <QDebug> `#include` <set> +#include <functional>Also applies to: 1207-1207, 1619-1619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 37 - 38, This translation unit uses std::function in two places (around lines 1207 and 1619) but does not include <functional>; add a direct `#include` <functional> to the top of MeshImporterExporter.cpp alongside the other includes (e.g., after the existing <set> include) so the uses of std::function compile reliably across toolchains.
1577-1583:⚠️ Potential issue | 🟠 MajorImport is still non-transactional after parse success.
The current scene is cleared at Line 1577 before mesh/skeleton/material reconstruction completes; if later processing throws, users still lose the existing scene.
Also applies to: 1585-1926
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 1577 - 1583, The import clears the live scene early (SelectionSet::getSingleton()->clearList(), emit manager->sceneClearing(), manager->destroySceneNode(...)) so if reconstruction throws later the original scene is lost; make the operation transactional by building the imported meshes/skeletons/materials into a temporary container or newly allocated scene nodes (not attached to the live Manager) and only call SelectionSet::getSingleton()->clearList(), emit manager->sceneClearing(), and manager->destroySceneNode(...) to remove the old nodes after the entire reconstruction completes without exception; alternatively wrap reconstruction in try/catch and on success swap the temporary nodes into Manager::getSingleton(), on failure leave Manager untouched and clean up temporaries. Ensure the unique symbols to change are the pre-clear sequence (SelectionSet::getSingleton()->clearList(); emit manager->sceneClearing(); auto sceneNodesCopy = manager->getSceneNodes(); for (auto* sn : sceneNodesCopy) manager->destroySceneNode(sn);) and the code paths that allocate new scene nodes so they use the temporary container and only get attached on successful completion.
1195-1196:⚠️ Potential issue | 🟠 Major
nodeName + "_"prefix scoping is ambiguous for similarly named entities.Using
_as the namespace separator can cross-match unrelated entities (e.g.,Hero_also matchesHero_Alt_*), which can re-mix bones/animations during import.Proposed fix (collision-resistant separator)
- std::string bonePrefix = (nodeEntities.size() > 1 && hasSkeleton) - ? std::string(sn->getName()) + "_" : ""; + static const std::string kEntitySep = "__NODE__"; + std::string bonePrefix = (nodeEntities.size() > 1 && hasSkeleton) + ? std::string(sn->getName()) + kEntitySep : ""; @@ - entityPrefix = nodeName.toStdString() + "_"; + entityPrefix = nodeName.toStdString() + kEntitySep;Also applies to: 1674-1677, 1715-1718, 1796-1801, 1875-1876
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MeshImporterExporter.cpp` around lines 1195 - 1196, The current use of "_" as the bone/namespace separator is ambiguous and can collide with similarly named entities; replace the separator with a collision-resistant delimiter (e.g., "::" or a rarely-used token) wherever bonePrefix is built (the code using variable bonePrefix and sn->getName() when nodeEntities.size() > 1 && hasSkeleton) and in the other similar concatenations noted (the other occurrences that construct names for bones/animations). Update all matching/parsing code that later splits or compares those names to use the new delimiter so prefix matching is exact (search for uses of bonePrefix, concatenations with sn->getName(), and any name-splitting logic and change both construction and parsing to the new separator).
🤖 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 1097-1106: The material deduplication currently keys matIndexMap
by mat->getName(), which can incorrectly merge materials with the same name from
different resource groups; update the key to include the material's resource
group (e.g., combine mat->getName() and mat->getGroup() or another unique group
identifier) wherever matIndexMap is populated/queried (reference symbols:
matIndexMap, nodeEntities loop using getSubEntities() and getMaterial(), and the
materials vector) and apply the same change to the other occurrence around the
second block (the lines referenced at 1286-1287) so materials are unique per
resource group.
- Around line 37-38: This translation unit uses std::function in two places
(around lines 1207 and 1619) but does not include <functional>; add a direct
`#include` <functional> to the top of MeshImporterExporter.cpp alongside the other
includes (e.g., after the existing <set> include) so the uses of std::function
compile reliably across toolchains.
- Around line 1577-1583: The import clears the live scene early
(SelectionSet::getSingleton()->clearList(), emit manager->sceneClearing(),
manager->destroySceneNode(...)) so if reconstruction throws later the original
scene is lost; make the operation transactional by building the imported
meshes/skeletons/materials into a temporary container or newly allocated scene
nodes (not attached to the live Manager) and only call
SelectionSet::getSingleton()->clearList(), emit manager->sceneClearing(), and
manager->destroySceneNode(...) to remove the old nodes after the entire
reconstruction completes without exception; alternatively wrap reconstruction in
try/catch and on success swap the temporary nodes into Manager::getSingleton(),
on failure leave Manager untouched and clean up temporaries. Ensure the unique
symbols to change are the pre-clear sequence
(SelectionSet::getSingleton()->clearList(); emit manager->sceneClearing(); auto
sceneNodesCopy = manager->getSceneNodes(); for (auto* sn : sceneNodesCopy)
manager->destroySceneNode(sn);) and the code paths that allocate new scene nodes
so they use the temporary container and only get attached on successful
completion.
- Around line 1195-1196: The current use of "_" as the bone/namespace separator
is ambiguous and can collide with similarly named entities; replace the
separator with a collision-resistant delimiter (e.g., "::" or a rarely-used
token) wherever bonePrefix is built (the code using variable bonePrefix and
sn->getName() when nodeEntities.size() > 1 && hasSkeleton) and in the other
similar concatenations noted (the other occurrences that construct names for
bones/animations). Update all matching/parsing code that later splits or
compares those names to use the new delimiter so prefix matching is exact
(search for uses of bonePrefix, concatenations with sn->getName(), and any
name-splitting logic and change both construction and parsing to the new
separator).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dd12773-b1fb-4e81-951e-634e61e55320
📒 Files selected for processing (6)
qml/ViewCubeWindow.qmlsrc/AnimationWidget.cppsrc/MCPServer.cppsrc/Manager.hsrc/Manager_test.cppsrc/MeshImporterExporter.cpp
… macOS Replace the top-level QML Window with a QQuickWidget managed by ViewCubeController. On macOS, Qt::Tool windows float above all app windows (NSFloatingWindowLevel), causing the ViewCube to render on top of the material editor and dialogs. Fix by lowering the NSWindow level to NSNormalWindowLevel via the Objective-C runtime, so other windows can appear above the cube when focused. Key changes: - ViewCubeController now owns a QQuickWidget (initWidget) instead of MainWindow creating a QQmlApplicationEngine - ViewCubeWindow.qml root changed from Window to Rectangle (embeddable) - Removed windowX/Y/positionChanged — positioning is internal via mapToGlobal in reposition() - Cube is raised on viewport mouse click/wheel and main window activate to stay visible over the viewport at NSNormalWindowLevel - visibilityChanged only emits from setVisible (user toggle), not from viewport lifecycle events — closing a viewport no longer unchecks the menu toggle - No widget reparenting — avoids crash when switching viewport layouts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/mainwindow.cpp (3)
1387-1387:⚠️ Potential issue | 🟡 MinorHandle failed scene imports in
openRecentFile().Similar to
on_actionOpen_Scene_triggered(), the return value ofsceneImporter()is ignored here. A failed import should notify the user rather than silently failing.🔧 Proposed fix
- if (filePath.endsWith(".scene.glb", Qt::CaseInsensitive) || - filePath.endsWith(".scene.gltf", Qt::CaseInsensitive)) - MeshImporterExporter::sceneImporter(filePath); - else + if (filePath.endsWith(".scene.glb", Qt::CaseInsensitive) || + filePath.endsWith(".scene.gltf", Qt::CaseInsensitive)) { + if (!MeshImporterExporter::sceneImporter(filePath)) { + QMessageBox::warning(this, tr("Open Scene"), + tr("Failed to open scene file \"%1\".").arg(filePath)); + return; + } + } else { mUriList.append(filePath); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` at line 1387, In openRecentFile() the call to MeshImporterExporter::sceneImporter(filePath) currently ignores its return value; update openRecentFile() to capture the boolean result from MeshImporterExporter::sceneImporter(filePath), and if it returns false show a user-visible error (e.g., QMessageBox::critical or a status bar message) and abort further processing (return early), mirroring the behavior in on_actionOpen_Scene_triggered(); ensure the error message gives context (e.g., "Failed to import scene: <filename>") so users are notified of the failure.
1386-1389:⚠️ Potential issue | 🟡 MinorUse case-insensitive comparison for scene file extensions.
The
endsWith()calls don't useQt::CaseInsensitive, so files likeMyScene.SCENE.GLBortest.Scene.GltFwon't be recognized as scene files and will be incorrectly processed as mesh imports.🔧 Proposed fix
- if (filePath.endsWith(".scene.glb") || filePath.endsWith(".scene.gltf")) + if (filePath.endsWith(".scene.glb", Qt::CaseInsensitive) || + filePath.endsWith(".scene.gltf", Qt::CaseInsensitive)) MeshImporterExporter::sceneImporter(filePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 1386 - 1389, The endsWith checks on filePath are case-sensitive so files like "MyScene.SCENE.GLB" won't match; update the logic where filePath is tested (the endsWith calls before calling MeshImporterExporter::sceneImporter or mUriList.append) to perform case-insensitive comparisons by passing Qt::CaseInsensitive (e.g., filePath.endsWith(".scene.glb", Qt::CaseInsensitive) and similarly for ".scene.gltf") so scene files are recognized regardless of extension case.
601-607:⚠️ Potential issue | 🟠 MajorHandle failed scene imports before adding to recent files.
The return value of
MeshImporterExporter::sceneImporter(fileName)is ignored. If the import fails (returnsfalse), the file is still added to recent files at line 607, which could confuse users when re-opening a corrupted or invalid scene file.🐛 Proposed fix
auto txn = SentryReporter::startTransaction("ui.import", "scene.import"); + bool imported = false; try { - MeshImporterExporter::sceneImporter(fileName); + imported = MeshImporterExporter::sceneImporter(fileName); } catch (...) { SentryReporter::finishTransaction(txn); throw; } SentryReporter::finishTransaction(txn); + if (!imported) { + QMessageBox::warning(this, tr("Open Scene"), tr("Failed to open scene.")); + return; + } addToRecentFiles(fileName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 601 - 607, MeshImporterExporter::sceneImporter(fileName) currently returns a success flag that's ignored, so change the flow to capture its boolean result, call SentryReporter::finishTransaction(txn) in all paths as now, and only call addToRecentFiles(fileName) when the import succeeded (result == true); preserve the existing catch(...) rethrow behavior and ensure SentryReporter::finishTransaction(txn) is still invoked before rethrowing.
🧹 Nitpick comments (1)
src/ViewCube/ViewCubeController.h (1)
68-68: Consider usingQPointer<QQuickWidget>for consistency withm_activeWidget.The
m_activeWidgetmember usesQPointer<OgreWidget>for safe tracking when the widget is externally destroyed. Whilem_cubeWidgetis owned by this controller (created ininitWidget()), usingQPointerwould provide consistent null-safety semantics across widget members and guard against accidental double-delete scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController.h` at line 68, Replace the raw pointer member m_cubeWidget with QPointer<QQuickWidget> to match m_activeWidget's null-safe semantics: update the declaration in ViewCubeController to QPointer<QQuickWidget> m_cubeWidget, ensure <QPointer> is included, and audit usages (e.g., in initWidget() and any destruction/ownership code) to check for .isNull()/.clear() or direct pointer access via operator->/operator*; this keeps ownership behavior but guards against external deletion and accidental double-delete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/ViewCubeWindow.qml`:
- Line 4: Restore the QML root to Window (replace the Rectangle root in
ViewCubeWindow.qml) so the component can carry window flags, and update
ViewCubeController::initWidget() to set the missing Qt::WindowStaysOnTopHint in
addition to Qt::FramelessWindowHint and Qt::Tool when creating/applying window
flags; also enforce software rendering for this widget by calling
QQuickWindow::setGraphicsApi(QSGRendererInterface::Software) on the
QQuickWidget's window (or equivalent) during initWidget() to avoid GL conflicts
with Ogre.
---
Duplicate comments:
In `@src/mainwindow.cpp`:
- Line 1387: In openRecentFile() the call to
MeshImporterExporter::sceneImporter(filePath) currently ignores its return
value; update openRecentFile() to capture the boolean result from
MeshImporterExporter::sceneImporter(filePath), and if it returns false show a
user-visible error (e.g., QMessageBox::critical or a status bar message) and
abort further processing (return early), mirroring the behavior in
on_actionOpen_Scene_triggered(); ensure the error message gives context (e.g.,
"Failed to import scene: <filename>") so users are notified of the failure.
- Around line 1386-1389: The endsWith checks on filePath are case-sensitive so
files like "MyScene.SCENE.GLB" won't match; update the logic where filePath is
tested (the endsWith calls before calling MeshImporterExporter::sceneImporter or
mUriList.append) to perform case-insensitive comparisons by passing
Qt::CaseInsensitive (e.g., filePath.endsWith(".scene.glb", Qt::CaseInsensitive)
and similarly for ".scene.gltf") so scene files are recognized regardless of
extension case.
- Around line 601-607: MeshImporterExporter::sceneImporter(fileName) currently
returns a success flag that's ignored, so change the flow to capture its boolean
result, call SentryReporter::finishTransaction(txn) in all paths as now, and
only call addToRecentFiles(fileName) when the import succeeded (result == true);
preserve the existing catch(...) rethrow behavior and ensure
SentryReporter::finishTransaction(txn) is still invoked before rethrowing.
---
Nitpick comments:
In `@src/ViewCube/ViewCubeController.h`:
- Line 68: Replace the raw pointer member m_cubeWidget with
QPointer<QQuickWidget> to match m_activeWidget's null-safe semantics: update the
declaration in ViewCubeController to QPointer<QQuickWidget> m_cubeWidget, ensure
<QPointer> is included, and audit usages (e.g., in initWidget() and any
destruction/ownership code) to check for .isNull()/.clear() or direct pointer
access via operator->/operator*; this keeps ownership behavior but guards
against external deletion and accidental double-delete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81885412-d24c-49aa-b0f2-3ada0d2b30b0
📒 Files selected for processing (6)
qml/ViewCubeWindow.qmlsrc/ViewCube/ViewCubeController.cppsrc/ViewCube/ViewCubeController.hsrc/ViewCube/ViewCubeController_test.cppsrc/mainwindow.cppsrc/mainwindow.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mainwindow.h
| import ViewCubeModule 1.0 | ||
|
|
||
| Window { | ||
| Rectangle { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the required Window flags and software backend setup still exist somewhere in the PR branch.
rg -n -C2 'WindowStaysOnTopHint|FramelessWindowHint|Qt::Tool|setSceneGraphBackend|QSG_RHI_BACKEND|software'Repository: fernandotonon/QtMeshEditor
Length of output: 49671
🏁 Script executed:
cat -n qml/ViewCubeWindow.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 20759
🏁 Script executed:
cat -n src/ViewCube/ViewCubeController.cpp | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 3852
Restore Window root and add the missing Qt::WindowStaysOnTopHint flag.
Line 4 changes the root to Rectangle, which cannot carry window flags and makes this component non-compliant with the ViewCube rendering contract. Even though ViewCubeController.cpp applies flags in C++, it is missing Qt::WindowStaysOnTopHint (only sets FramelessWindowHint and Tool). The guideline explicitly requires all three flags along with software rendering backend to avoid GL conflicts with Ogre.
Suggested patch (QML side)
import QtQuick
+import QtQuick.Window
import ViewCubeModule 1.0
-Rectangle {
+Window {
id: root
width: 64
height: 64
+ flags: Qt.FramelessWindowHint | Qt.Tool | Qt.WindowStaysOnTopHint
color: "transparent"Additionally, ensure ViewCubeController::initWidget() enforces software rendering for this widget (e.g., QQuickWindow::setGraphicsApi(QSGRendererInterface::Software) on the QQuickWidget's window).
📝 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.
Suggested change
| Rectangle { | |
| import QtQuick | |
| import QtQuick.Window | |
| import ViewCubeModule 1.0 | |
| Window { | |
| id: root | |
| width: 64 | |
| height: 64 | |
| flags: Qt.FramelessWindowHint | Qt.Tool | Qt.WindowStaysOnTopHint | |
| color: "transparent" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/ViewCubeWindow.qml` at line 4, Restore the QML root to Window (replace
the Rectangle root in ViewCubeWindow.qml) so the component can carry window
flags, and update ViewCubeController::initWidget() to set the missing
Qt::WindowStaysOnTopHint in addition to Qt::FramelessWindowHint and Qt::Tool
when creating/applying window flags; also enforce software rendering for this
widget by calling QQuickWindow::setGraphicsApi(QSGRendererInterface::Software)
on the QQuickWidget's window (or equivalent) during initWidget() to avoid GL
conflicts with Ogre.
|
4 tasks
This was referenced Mar 23, 2026
This was referenced Apr 1, 2026
7 tasks
This was referenced Jul 9, 2026
7 tasks
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
sceneExporter()/sceneImporter()inMeshImporterExportersave_sceneandopen_scenetools for AI agent integration.scene.glb/.scene.gltfextensionsChanges
src/MeshImporterExporter.cpp/h— CoresceneExporter()andsceneImporter()withbuildSceneAiScene()(863 new lines)src/MCPServer.cpp/h—save_scene/open_scenetool implementationssrc/AnimationWidget.cpp— Fix crash on scene load with active skeleton debug/bone weight overlayssrc/Manager.cpp— GuardisForbiddenNodeNameagainst empty stringssrc/TransformOperator.cpp— Null camera guardsrc/mainwindow.cpp/h+ui_files/mainwindow.ui— File menu integrationdocs/index.html— Scene Save & Load feature card, updated MCP tool countREADME.md/CLAUDE.md— Documentation updatesTest plan
SceneSaveLoadTestsuite inMeshImporterExporter_test.cpp(round-trip with transforms, materials, skeleton/animations)save_scene/open_scenetests inMCPServer_test.cpp.scene.glbin external viewer (e.g. gltf-viewer.donmccurdy.com) to verify valid glTF🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
UI
Bug Fixes
Tests