Phase 1: Scene Editing Power Tools (#256) - #262
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds scene-node duplication with deep-copy semantics and undo/redo; persistent snapping (operator, controller, UI, presets, accumulators, tests); a CLI/MCP “pose” export flow and MeshImporterExporter::exportCurrentPose; new MCP tools for duplication, snap settings, and pose export; CLI and docker entrypoint accept Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as User/UI
participant MW as MainWindow
participant MGR as Manager
participant Ogre as Ogre Engine
participant Undo as UndoManager
UI->>MW: Trigger Duplicate (Ctrl+D / menu)
MW->>MW: duplicateSelected()
MW->>MGR: duplicateSceneNode(sourceNode)
MGR->>Ogre: Create SceneNode, Clone Entity/Mesh/Skeleton
Ogre-->>MGR: Cloned resources
MGR-->>MW: ClonedNode(s)
MW->>Undo: Push DuplicateCommand(sources, clones)
MW->>MW: Clear selection and select clones
Note right of MW: SentryReporter::addBreadcrumb(rgba(0,128,0,0.5),"Duplicate selected objects")
sequenceDiagram
autonumber
participant CLI as CLI / CLIPipeline
participant Ogre as Ogre headless
participant Entity as Ogre::Entity
participant MIE as MeshImporterExporter
participant FS as Filesystem
CLI->>Ogre: Initialize headless runtime
CLI->>Ogre: Import model file
Ogre-->>CLI: Scene / Entity list
CLI->>Entity: Select entity + enable animation / set time
CLI->>MIE: exportCurrentPose(Entity, outputPath, format)
MIE->>Entity: Request software skinning, evaluate animation
MIE->>MIE: Build aiScene with deformed vertices (no bones/animations)
MIE->>FS: Write exported file via Assimp
FS-->>MIE: Success/Failure
MIE-->>CLI: Return status
Note right of CLI: SentryReporter::addBreadcrumb(rgba(128,0,128,0.5),"pose export failure") / addMessage on errors
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96468334d1
ℹ️ 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
+268
to
+273
| Ogre::SceneNode* newNode = addSceneNode(baseName); | ||
|
|
||
| // Copy transform | ||
| newNode->setPosition(source->getPosition()); | ||
| newNode->setOrientation(source->getOrientation()); | ||
| newNode->setScale(source->getScale()); |
There was a problem hiding this comment.
The duplicate path creates a fresh scene node and copies only transform/entity data, but it never copies source->getUserObjectBindings(). Primitive support relies on that binding (PrimitiveObject::isPrimitive), so duplicated primitive nodes are no longer recognized as primitives and lose primitive-specific editing paths (for example the Properties panel primitive controls). This is a functional regression for the stated primitive duplication scenario.
Useful? React with 👍 / 👎.
Comment on lines
+286
to
+290
| QString cloneMeshName = QString::fromStdString(newNode->getName()) + "_mesh"; | ||
|
|
||
| // Remove stale resources from a previous undo cycle (redo re-creates them) | ||
| if (Ogre::MeshManager::getSingleton().getByName(cloneMeshName.toStdString())) | ||
| Ogre::MeshManager::getSingleton().remove(cloneMeshName.toStdString()); |
There was a problem hiding this comment.
Inside the attached-object loop, cloneMeshName is derived only from newNode->getName(), so every attached entity on that node reuses the same mesh/skeleton resource names. On the second entity, the code can remove the first clone as “stale” and then create another entity on the same node name path, which can throw Ogre duplicate-name exceptions or leave broken clones when a source node has multiple attached entities.
Useful? React with 👍 / 👎.
Comment on lines
+487
to
+488
| cmd->undo(); | ||
| EXPECT_FALSE(entity->getVisible()); |
There was a problem hiding this comment.
This test invokes cmd->undo() and then reads entity->getVisible(), but DuplicateCommand::undo() destroys the nodes it tracks via Manager::destroySceneNode. In this test setup, the tracked node is the same one that owns entity, so the assertion dereferences freed state and can crash or flap instead of verifying behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/commands/TransformCommands_test.cpp (1)
440-450: Verify the DuplicateCommand constructor usage.The test passes the same list (
clones) for both constructor parameters. Based on the AI summary,DuplicateCommandtakes(clones, sources)where sources are the original nodes. Passing the same node as both clone and source may work for basic visibility testing but doesn't accurately represent the real use case where clones and sources are distinct nodes.Consider creating separate source and clone nodes to better reflect actual usage:
TEST_F(TransformCommandsTests, DuplicateCommand_Constructor) { Manager* mgr = Manager::getSingleton(); - Ogre::SceneNode* node = mgr->addSceneNode("DupCmdNode1"); - ASSERT_NE(node, nullptr); + Ogre::SceneNode* source = mgr->addSceneNode("DupCmdSource1"); + Ogre::SceneNode* clone = mgr->addSceneNode("DupCmdClone1"); + ASSERT_NE(source, nullptr); + ASSERT_NE(clone, nullptr); - QList<Ogre::SceneNode*> clones = {node}; - auto* cmd = new DuplicateCommand(clones, clones); + QList<Ogre::SceneNode*> clones = {clone}; + QList<Ogre::SceneNode*> sources = {source}; + auto* cmd = new DuplicateCommand(clones, sources);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/TransformCommands_test.cpp` around lines 440 - 450, The test uses the same QList for both parameters when constructing DuplicateCommand, which misrepresents real usage where clones and sources are distinct; update the test to create two separate SceneNode instances via Manager::getSingleton()->addSceneNode (e.g., one for the source and one for the clone), build two distinct QList<Ogre::SceneNode*> (sources and clones) and pass them to the DuplicateCommand constructor, ensuring the constructor call and expectations use the separate lists (referencing DuplicateCommand, Manager::getSingleton, and addSceneNode).src/Manager.cpp (2)
262-268: Consider adding a Sentry breadcrumb for duplication.Per the coding guidelines in CLAUDE.md, user-facing actions should be tracked with
SentryReporter::addBreadcrumb. The duplication operation is a significant user action invoked via Ctrl+D and MCP.If the callers (
MainWindow::duplicateSelectedandtoolDuplicateEntity) already add breadcrumbs, this can be skipped. Otherwise, consider adding one here:Ogre::SceneNode* Manager::duplicateSceneNode(Ogre::SceneNode* source) { if (!source || !mSceneMgr) return nullptr; + SentryReporter::addBreadcrumb("scene", "Duplicate scene node"); // Generate a unique name based on the sourceAs per coding guidelines: "All user-facing actions and significant operations must be tracked with
SentryReporter::addBreadcrumb".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Manager.cpp` around lines 262 - 268, The duplicateSceneNode function lacks a Sentry breadcrumb for this user-facing action; add a call to SentryReporter::addBreadcrumb inside Manager::duplicateSceneNode (e.g., before or after creating newNode) to record the duplication event with a short message and relevant metadata (source node name, new node name or id). If MainWindow::duplicateSelected or toolDuplicateEntity already add breadcrumbs, skip adding here; otherwise insert SentryReporter::addBreadcrumb(...) into Manager::duplicateSceneNode to satisfy the CLAUDE.md guideline for tracking user actions.
342-358: Silent exception handling may hide issues.Line 349 silently catches exceptions when associating bones with tracks. While defensive coding is appropriate here, a warning log would help diagnose skeleton-related issues during duplication.
🛡️ Suggested improvement
try { if (srcTrack->getAssociatedNode()) { Ogre::Bone* dstBone = clonedSkel->getBone(handle); if (dstBone) dstTrack->setAssociatedNode(dstBone); } - } catch (...) { /* handle not found — skip association */ } + } catch (const Ogre::Exception& e) { + qWarning("Bone association skipped for handle %u: %s", + handle, e.getFullDescription().c_str()); + } catch (...) { /* unexpected error — skip association */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Manager.cpp` around lines 342 - 358, The catch-all in the loop over srcAnim->_getNodeTrackList() silently swallows errors when trying to associate bones (clonedSkel->getBone and dstTrack->setAssociatedNode), which hides skeleton issues; update the catch to log a warning via the project's logger (include context: handle, srcAnim/dstAnim identifiers if available, and the exception message) so failures during dstAnim->createNodeTrack/association are visible for debugging while still allowing the loop to continue. Ensure you reference the same scope (the try around clonedSkel->getBone / dstTrack->setAssociatedNode) and avoid changing control flow — only replace the empty catch with a safe logging statement that includes the handle and error details.
🤖 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/commands/TransformCommands.cpp`:
- Around line 180-185: The redo path is creating incomplete clones because
Manager::duplicateSceneNode does not duplicate child nodes; update the logic in
the loop in TransformCommands.cpp (the for over mSourceNodeNames which calls
Manager::getSingleton()->duplicateSceneNode) to duplicate the entire subtree
instead of only the single node: either extend Manager::duplicateSceneNode to
recursively clone child SceneNodes (preserving hierarchy and attachments) or
implement a helper that traverses src (obtained via getSceneMgr()->getSceneNode)
and for each node creates a new node, re-parents children under the
corresponding cloned parent, and then append the top-level clone to
mClonedNodes; ensure the new code preserves transforms and any attached objects
so undo/redo keeps the full subtree intact.
In `@src/MCPServer.cpp`:
- Around line 2414-2438: The MCPServer::toolDuplicateEntity implementation
currently calls Manager::getSingleton()->duplicateSceneNode(sourceNode) directly
which bypasses undo/redo; instead construct and execute/push a DuplicateCommand
for the source node so the operation is recorded. Replace the direct duplicate
call with creating a DuplicateCommand (e.g. new DuplicateCommand(sourceNode))
and run it through the app's command/undo facility (the CommandManager/UndoStack
execute/push API used elsewhere) and then use the resulting cloned node from the
command to build the success result; keep SelectionSet usage for the selection
path unchanged.
---
Nitpick comments:
In `@src/commands/TransformCommands_test.cpp`:
- Around line 440-450: The test uses the same QList for both parameters when
constructing DuplicateCommand, which misrepresents real usage where clones and
sources are distinct; update the test to create two separate SceneNode instances
via Manager::getSingleton()->addSceneNode (e.g., one for the source and one for
the clone), build two distinct QList<Ogre::SceneNode*> (sources and clones) and
pass them to the DuplicateCommand constructor, ensuring the constructor call and
expectations use the separate lists (referencing DuplicateCommand,
Manager::getSingleton, and addSceneNode).
In `@src/Manager.cpp`:
- Around line 262-268: The duplicateSceneNode function lacks a Sentry breadcrumb
for this user-facing action; add a call to SentryReporter::addBreadcrumb inside
Manager::duplicateSceneNode (e.g., before or after creating newNode) to record
the duplication event with a short message and relevant metadata (source node
name, new node name or id). If MainWindow::duplicateSelected or
toolDuplicateEntity already add breadcrumbs, skip adding here; otherwise insert
SentryReporter::addBreadcrumb(...) into Manager::duplicateSceneNode to satisfy
the CLAUDE.md guideline for tracking user actions.
- Around line 342-358: The catch-all in the loop over
srcAnim->_getNodeTrackList() silently swallows errors when trying to associate
bones (clonedSkel->getBone and dstTrack->setAssociatedNode), which hides
skeleton issues; update the catch to log a warning via the project's logger
(include context: handle, srcAnim/dstAnim identifiers if available, and the
exception message) so failures during dstAnim->createNodeTrack/association are
visible for debugging while still allowing the loop to continue. Ensure you
reference the same scope (the try around clonedSkel->getBone /
dstTrack->setAssociatedNode) and avoid changing control flow — only replace the
empty catch with a safe logging statement that includes the handle and error
details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eafbf48-e210-4613-959a-de9f610ebf48
📒 Files selected for processing (12)
CLAUDE.mdsrc/AIChatManager.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/Manager.cppsrc/Manager.hsrc/commands/TransformCommands.cppsrc/commands/TransformCommands.hsrc/commands/TransformCommands_test.cppsrc/mainwindow.cppsrc/mainwindow.hui_files/mainwindow.ui
Comment on lines
+180
to
+185
| for (const QString& name : mSourceNodeNames) { | ||
| if (!Manager::getSingleton()->hasSceneNode(name)) continue; | ||
| Ogre::SceneNode* src = Manager::getSingleton()->getSceneMgr() | ||
| ->getSceneNode(name.toStdString()); | ||
| Ogre::SceneNode* clone = Manager::getSingleton()->duplicateSceneNode(src); | ||
| if (clone) mClonedNodes.append(clone); |
There was a problem hiding this comment.
Redo can recreate incomplete clones for hierarchical nodes.
Line 184 relies on Manager::duplicateSceneNode, and that implementation currently does not recurse child scene nodes. Duplicating a parent node and then undo/redo can drop its subtree structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/TransformCommands.cpp` around lines 180 - 185, The redo path is
creating incomplete clones because Manager::duplicateSceneNode does not
duplicate child nodes; update the logic in the loop in TransformCommands.cpp
(the for over mSourceNodeNames which calls
Manager::getSingleton()->duplicateSceneNode) to duplicate the entire subtree
instead of only the single node: either extend Manager::duplicateSceneNode to
recursively clone child SceneNodes (preserving hierarchy and attachments) or
implement a helper that traverses src (obtained via getSceneMgr()->getSceneNode)
and for each node creates a new node, re-parents children under the
corresponding cloned parent, and then append the top-level clone to
mClonedNodes; ensure the new code preserves transforms and any attached objects
so undo/redo keeps the full subtree intact.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/MCPServer.cpp (1)
2418-2442:⚠️ Potential issue | 🟠 Major
duplicate_entitystill bypasses undo/redo stack.Line 2435 duplicates directly and returns success, but never records a
DuplicateCommand, so MCP duplication cannot be undone/redone like UI duplication.Proposed fix
+#include "UndoManager.h" +#include "commands/TransformCommands.h" @@ - Ogre::SceneNode* clone = Manager::getSingleton()->duplicateSceneNode(sourceNode); + Ogre::SceneNode* clone = Manager::getSingleton()->duplicateSceneNode(sourceNode); if (!clone) return makeErrorResult("Error: Failed to duplicate node."); + + // Keep MCP duplication behavior consistent with UI duplication. + QList<Ogre::SceneNode*> sources{sourceNode}; + QList<Ogre::SceneNode*> clones{clone}; + UndoManager::getSingleton()->push(new DuplicateCommand(sources, clones));
🤖 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/MCPServer.cpp`:
- Around line 482-491: The MCP tool dispatches for toolDuplicateEntity,
toolCameraControl, toolGetCameraInfo, toolSetSnapSettings, and
toolGetSnapSettings currently log via callTool() using category "mcp.tool";
update the invocation logging to use the required breadcrumb category by calling
SentryReporter::addBreadcrumb(...) with category "ai.tool_call" (or adjust
callTool to emit that breadcrumb) whenever these tools are invoked so all MCP
tool calls are tracked as ai.tool_call; ensure the breadcrumb includes the tool
name and relevant args/context consistent with other tool invocations.
- Around line 2520-2562: The handler MCPServer::toolSetSnapSettings mutates
TransformOperator via setSnapEnabled, setSnapGridSize, setSnapAngleStep and
setSnapScaleStep as it validates each field, causing partial commits on later
validation failure; make the operation atomic by first validating and collecting
all new values (e.g., optional<bool> newEnabled, optional<double>
newGridSize/angleStep/scaleStep) without calling TransformOperator, return
makeErrorResult on any invalid input, then—only after all inputs are valid—call
top->setSnapEnabled/setSnapGridSize/setSnapAngleStep/setSnapScaleStep and build
the changes list, finally returning makeSuccessResult. Ensure you still handle
the case of no fields provided by returning the same makeErrorResult message.
In `@src/MCPServer.h`:
- Around line 165-167: The MCP surface was expanded with new handlers
(toolDuplicateEntity, toolSetSnapSettings, toolGetSnapSettings) but
SERVER_VERSION in MCPServer.h wasn’t updated; update the SERVER_VERSION constant
in MCPServer.h to a new patch/minor version (e.g., 1.2.0 or appropriate per your
versioning policy) so clients can detect the new MCP capabilities, and ensure
any accompanying version-string references or build metadata that rely on
SERVER_VERSION are updated consistently.
In `@src/TransformOperator_test.cpp`:
- Around line 600-642: The tests mutate persistent QSettings via
TransformOperator's setters (setSnapEnabled, setSnapGridSize, setSnapAngleStep,
setSnapScaleStep) and then assert signal counts for snapSettingsChanged, so make
the tests deterministic by isolating QSettings: in the test fixture
(TransformOperatorTests) before constructing the TransformOperator instance,
either redirect QSettings to a temporary INI file or use
QSettings::beginGroup/clear/endGroup to back up and clear the "Snap" group (or
restore it after tests); ensure the fixture restores the original settings or
removes the temporary store in teardown so subsequent runs start from a clean
Snap state and the signal-count assertions remain stable.
In `@src/TransformOperator.cpp`:
- Around line 678-684: Snapping is being applied after converting local deltas
back to world space, so in SPACE_LOCAL mode quantization uses world axes;
instead, when mode == SPACE_LOCAL use the already computed localDelta / localRot
and accumulate and snap in gizmo-local coordinates (use mSnapTranslationAccum
and mSnapRotationAccum but store and quantize the local-space values via
snapTranslation/snapRotation with mSnapGridSize), then transform the snapped
local result back to world before applying to the selection; update both the
translation path (where mSnapTranslationAccum and snapTranslation are used) and
the rotation path (where localRot is handled around the same region) to follow
this local-space accumulate -> snap -> world-transform flow so local
moves/rotations preserve the local step size.
- Around line 845-856: mSnapScaleAccum currently yields per-step deltas which
are being applied as multiplicative increments via scaleSelected(snappedFactor),
causing compounded or stalled scaling; change the logic to track a cumulative
snapped factor since drag start (e.g., a new member like mSnapScaleAppliedTotal
initialized to Ogre::Vector3::UNIT_SCALE at drag start), compute the new snapped
total from mSnapScaleAccum and mSnapScaleStep (using snapScale), derive the
deltaFactor = newSnappedTotal / mSnapScaleAppliedTotal, call
scaleSelected(deltaFactor) (not the absolute snappedFactor), then set
mSnapScaleAppliedTotal = newSnappedTotal and only reset mScaleStartDistance when
snappedDelta is zero as before; update uses of mSnapScaleAccum, snapScale(),
snappedDelta, snappedFactor and scaleSelected() accordingly.
- Around line 87-92: Loaded snap steps (mSnapGridSize, mSnapAngleStep,
mSnapScaleStep) are used without validation and can be zero/negative/inf; clamp
each after reading from QSettings to a safe positive range (e.g., min > 0 and
reasonable max) and normalize NaNs/infs to defaults, and make snapValue(double
value, double step) defensive by returning the original value if step is invalid
(<= epsilon, NaN or infinite) instead of dividing; update places that reference
these members to rely on the validated values and use a small EPS constant when
checking step validity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f1f1d5c-d315-4f9d-a3ea-e9027bf06f3b
📒 Files selected for processing (8)
qml/PropertiesPanel.qmlsrc/MCPServer.cppsrc/MCPServer.hsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/TransformOperator.cppsrc/TransformOperator.hsrc/TransformOperator_test.cpp
Comment on lines
+482
to
+491
| } else if (name == "duplicate_entity") { | ||
| toolResult = toolDuplicateEntity(args); | ||
| } else if (name == "camera_control") { | ||
| toolResult = toolCameraControl(args); | ||
| } else if (name == "get_camera_info") { | ||
| toolResult = toolGetCameraInfo(args); | ||
| } else if (name == "set_snap_settings") { | ||
| toolResult = toolSetSnapSettings(args); | ||
| } else if (name == "get_snap_settings") { | ||
| toolResult = toolGetSnapSettings(args); |
There was a problem hiding this comment.
Use the required breadcrumb category for MCP tool invocations.
These newly dispatched MCP tools are currently logged through callTool() with category mcp.tool; the guideline requires ai.tool_call.
Proposed fix
- SentryReporter::addBreadcrumb("mcp.tool", QStringLiteral("Tool call: %1").arg(name));
+ SentryReporter::addBreadcrumb("ai.tool_call", QStringLiteral("Tool call: %1").arg(name));
@@
- SentryReporter::addBreadcrumb("mcp.tool",
+ SentryReporter::addBreadcrumb("ai.tool_call",
QStringLiteral("Tool error: %1").arg(name), "error");As per coding guidelines: Track all user-facing actions with SentryReporter::addBreadcrumb() using category ai.tool_call for MCP tool invocations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer.cpp` around lines 482 - 491, The MCP tool dispatches for
toolDuplicateEntity, toolCameraControl, toolGetCameraInfo, toolSetSnapSettings,
and toolGetSnapSettings currently log via callTool() using category "mcp.tool";
update the invocation logging to use the required breadcrumb category by calling
SentryReporter::addBreadcrumb(...) with category "ai.tool_call" (or adjust
callTool to emit that breadcrumb) whenever these tools are invoked so all MCP
tool calls are tracked as ai.tool_call; ensure the breadcrumb includes the tool
name and relevant args/context consistent with other tool invocations.
Comment on lines
+600
to
+642
| TEST_F(TransformOperatorTests, SnapSettingsRoundTripAndEmitSignal) | ||
| { | ||
| QSignalSpy spy(op, &TransformOperator::snapSettingsChanged); | ||
| ASSERT_TRUE(spy.isValid()); | ||
|
|
||
| op->setSnapEnabled(true); | ||
| EXPECT_TRUE(op->isSnapEnabled()); | ||
| EXPECT_EQ(spy.count(), 1); | ||
|
|
||
| op->setSnapGridSize(2.0); | ||
| EXPECT_DOUBLE_EQ(op->snapGridSize(), 2.0); | ||
| EXPECT_EQ(spy.count(), 2); | ||
|
|
||
| op->setSnapAngleStep(45.0); | ||
| EXPECT_DOUBLE_EQ(op->snapAngleStep(), 45.0); | ||
| EXPECT_EQ(spy.count(), 3); | ||
|
|
||
| op->setSnapScaleStep(0.5); | ||
| EXPECT_DOUBLE_EQ(op->snapScaleStep(), 0.5); | ||
| EXPECT_EQ(spy.count(), 4); | ||
|
|
||
| // Setting same value should not emit again | ||
| op->setSnapEnabled(true); | ||
| EXPECT_EQ(spy.count(), 4); | ||
|
|
||
| op->setSnapGridSize(2.0); | ||
| EXPECT_EQ(spy.count(), 4); | ||
| } | ||
|
|
||
| TEST_F(TransformOperatorTests, SnapSettingsRejectInvalidValues) | ||
| { | ||
| op->setSnapGridSize(1.0); | ||
| op->setSnapGridSize(-1.0); | ||
| EXPECT_DOUBLE_EQ(op->snapGridSize(), 1.0); | ||
|
|
||
| op->setSnapAngleStep(15.0); | ||
| op->setSnapAngleStep(0.0); | ||
| EXPECT_DOUBLE_EQ(op->snapAngleStep(), 15.0); | ||
|
|
||
| op->setSnapScaleStep(0.25); | ||
| op->setSnapScaleStep(-0.5); | ||
| EXPECT_DOUBLE_EQ(op->snapScaleStep(), 0.25); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find TransformOperator files
find . -type f -name "TransformOperator*" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 156
🏁 Script executed:
# Check the test file structure and fixture
head -100 src/TransformOperator_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2630
🏁 Script executed:
# Search for QSettings usage in TransformOperator.cpp
rg "QSettings" src/TransformOperator.cpp -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 1090
🏁 Script executed:
# Check the test fixture definition for any QSettings handling
ast-grep --pattern 'class TransformOperatorTests : $$$' src/TransformOperator_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 4380
🏁 Script executed:
# Look for setUp/tearDown or fixture methods
rg "SetUp|TearDown|setUp|tearDown" src/TransformOperator_test.cpp -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 363
🏁 Script executed:
# Check TransformOperator::kill() implementation
rg "void kill\(\)" src/TransformOperator.cpp -A 10Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if TransformOperator destructor clears QSettings
rg "~TransformOperator" src/TransformOperator.cpp -A 10Repository: fernandotonon/QtMeshEditor
Length of output: 357
🏁 Script executed:
# Look for any QSettings isolation in test setup or other test files
rg "QSettings" src/ -l | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 294
🏁 Script executed:
# Check if there are any environment-specific or test-specific QSettings handling
rg "temporaryPath\|tempDir\|QStandardPaths.*Test" src/TransformOperator_test.cpp -A 3Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# See the full snap-related tests to understand the exact behavior
sed -n '600,642p' src/TransformOperator_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1246
Isolate the snap-setting tests from the real QSettings store.
TransformOperator persists these setters to QSettings, so after this test runs once a later run can start with snapEnabled=true or different step values. This makes the signal-count assertions order-dependent and pollutes local developer settings. Redirect QSettings to a temporary store, or clear/restore the Snap/* group in the fixture before constructing TransformOperator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator_test.cpp` around lines 600 - 642, The tests mutate
persistent QSettings via TransformOperator's setters (setSnapEnabled,
setSnapGridSize, setSnapAngleStep, setSnapScaleStep) and then assert signal
counts for snapSettingsChanged, so make the tests deterministic by isolating
QSettings: in the test fixture (TransformOperatorTests) before constructing the
TransformOperator instance, either redirect QSettings to a temporary INI file or
use QSettings::beginGroup/clear/endGroup to back up and clear the "Snap" group
(or restore it after tests); ensure the fixture restores the original settings
or removes the temporary store in teardown so subsequent runs start from a clean
Snap state and the signal-count assertions remain stable.
Comment on lines
+87
to
+92
| // Load snap settings from QSettings | ||
| QSettings settings; | ||
| mSnapEnabled = settings.value("Snap/enabled", false).toBool(); | ||
| mSnapGridSize = settings.value("Snap/gridSize", 1.0).toDouble(); | ||
| mSnapAngleStep = settings.value("Snap/angleStep", 15.0).toDouble(); | ||
| mSnapScaleStep = settings.value("Snap/scaleStep", 0.25).toDouble(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and get its basic info
fd -t f "TransformOperator.cpp" --type fRepository: fernandotonon/QtMeshEditor
Length of output: 95
🏁 Script executed:
# Check the file exists and get line count
wc -l src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 100
🏁 Script executed:
# Read lines 87-92 to verify the settings loading code
sed -n '85,94p' src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 483
🏁 Script executed:
# Read lines 220-230 to check the "Also applies to" location
sed -n '220,230p' src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 381
🏁 Script executed:
# Find and read the snapValue() method to verify it divides by step
ast-grep --pattern 'double snapValue($_, $_) { $$$ }' | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Also search for snapValue with rg to find its implementation
rg -A 5 'double.*snapValue.*step' src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 281
🏁 Script executed:
# Check if there are any existing guards or validation around snapValue calls
rg -B 2 -A 2 'snapValue\(' src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 811
🏁 Script executed:
# Check if there's any validation of the snap settings after loading
sed -n '87,120p' src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1172
Validate persisted snap steps before using them.
The constructor loads Snap/* values from QSettings without validation. snapValue() divides by step unconditionally, so a corrupted config returning 0, a negative number, or inf will produce NaN or inf on the first snapping operation. Clamp the loaded values and make snapValue() fail-safe for invalid steps.
Defensive clamp for persisted settings
+namespace {
+double sanitizeSnapStep(const QVariant& value, double fallback)
+{
+ const double step = value.toDouble();
+ return std::isfinite(step) && step > 0.0 ? step : fallback;
+}
+}
+
QSettings settings;
mSnapEnabled = settings.value("Snap/enabled", false).toBool();
- mSnapGridSize = settings.value("Snap/gridSize", 1.0).toDouble();
- mSnapAngleStep = settings.value("Snap/angleStep", 15.0).toDouble();
- mSnapScaleStep = settings.value("Snap/scaleStep", 0.25).toDouble();
+ mSnapGridSize = sanitizeSnapStep(settings.value("Snap/gridSize", 1.0), 1.0);
+ mSnapAngleStep = sanitizeSnapStep(settings.value("Snap/angleStep", 15.0), 15.0);
+ mSnapScaleStep = sanitizeSnapStep(settings.value("Snap/scaleStep", 0.25), 0.25);
...
double TransformOperator::snapValue(double value, double step)
{
+ if (!std::isfinite(step) || step <= 0.0)
+ return value;
return std::round(value / step) * step;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator.cpp` around lines 87 - 92, Loaded snap steps
(mSnapGridSize, mSnapAngleStep, mSnapScaleStep) are used without validation and
can be zero/negative/inf; clamp each after reading from QSettings to a safe
positive range (e.g., min > 0 and reasonable max) and normalize NaNs/infs to
defaults, and make snapValue(double value, double step) defensive by returning
the original value if step is invalid (<= epsilon, NaN or infinite) instead of
dividing; update places that reference these members to rely on the validated
values and use a small EPS constant when checking step validity.
Comment on lines
+678
to
+684
| // Apply snap if Ctrl is held or snap is permanently enabled | ||
| bool snapping = mSnapEnabled || (e->modifiers() & Qt::ControlModifier); | ||
| if (snapping) | ||
| { | ||
| mSnapTranslationAccum += translation; | ||
| Ogre::Vector3 snapped = snapTranslation(mSnapTranslationAccum, mSnapGridSize); | ||
| if (snapped.isZeroLength()) |
There was a problem hiding this comment.
Keep snapping in gizmo-local coordinates for SPACE_LOCAL.
The code already computes localDelta / localRot, but the new snap paths quantize after converting those values back into world space. Once the selection is rotated, a one-step local move or local rotation is rounded against world components instead of the active local axis, so local mode no longer preserves its own step size.
Also applies to: 761-786
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator.cpp` around lines 678 - 684, Snapping is being applied
after converting local deltas back to world space, so in SPACE_LOCAL mode
quantization uses world axes; instead, when mode == SPACE_LOCAL use the already
computed localDelta / localRot and accumulate and snap in gizmo-local
coordinates (use mSnapTranslationAccum and mSnapRotationAccum but store and
quantize the local-space values via snapTranslation/snapRotation with
mSnapGridSize), then transform the snapped local result back to world before
applying to the selection; update both the translation path (where
mSnapTranslationAccum and snapTranslation are used) and the rotation path (where
localRot is handled around the same region) to follow this local-space
accumulate -> snap -> world-transform flow so local moves/rotations preserve the
local step size.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/CLIPipeline.cpp (1)
1331-1334: Consider adding mutual exclusivity check for--timeand--count.Currently, if both
--timeand--countare provided,--countsilently takes precedence (line 1378 checkscount > 0first). This could confuse users who expect an error or explicit documentation of precedence.Proposed fix to reject conflicting options
if (time < 0.0f && count <= 0) { err() << "Error: Specify --time <t> or --count <N>." << Qt::endl; return 2; } + + if (time >= 0.0f && count > 0) { + err() << "Error: --time and --count are mutually exclusive." << Qt::endl; + return 2; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CLIPipeline.cpp` around lines 1331 - 1334, Add a mutual exclusivity check that rejects using both --time and --count together: inside the CLI parsing/validation block where the variables time and count are inspected (the same area that currently checks if (time < 0.0f && count <= 0)), add a condition that detects time >= 0.0f && count > 0 and prints an error via err() with a clear message (e.g., "Error: --time and --count are mutually exclusive.") and return a non-zero exit code; this will prevent the current silent precedence behavior where count wins and ensure users are informed of the conflict.
🤖 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/PropertiesPanel.qml`:
- Around line 1186-1204: The Export Pose button is hidden because entityGroups
is built by PropertiesPanelController.animationData(), which currently skips
entities with no animation states; update animationData() to also include
entities that have a skeleton (even if states->getAnimationStates() is empty) so
grp.hasSkeleton will be true and the existing Rectangle/UI (id exportPoseMouse,
text "Export Pose", onClicked: PropertiesPanelController.exportCurrentPose())
becomes visible for skeleton-only models; alternatively, if you prefer a UI
change instead, add a separate Skeleton section in PropertiesPanel.qml that
iterates skeleton-bearing entities (from a new controller method like
PropertiesPanelController.skeletonData()) and exposes an Export Pose button
wired to PropertiesPanelController.exportCurrentPose().
In `@src/MeshImporterExporter.cpp`:
- Around line 1297-1450: The pose-bake needs to be exception-safe: create an
RAII guard that calls entity->addSoftwareAnimationRequest(false) in its ctor and
entity->removeSoftwareAnimationRequest(false) in its dtor (so the request is
always released), and manage the aiScene* using a smart pointer (e.g.,
std::unique_ptr<aiScene> with a deleter) so the scene is freed on exceptions;
wrap the whole bake/build block (everything after addSoftwareAnimationRequest up
to compactAiMesh calls) in a try/catch that catches exceptions, ensures the
function returns -1 on failure, and reuses entity->_updateAnimation() as before
(call it after the RAII guard is constructed) — reference the symbols
entity->addSoftwareAnimationRequest, entity->removeSoftwareAnimationRequest,
entity->_updateAnimation, aiScene, and compactAiMesh to locate where to apply
the RAII and the try/catch.
- Around line 1483-1487: The export currently applies
aiProcess_ConvertToLeftHanded for every non-"x" format, which incorrectly flips
right-handed formats like glTF/GLB (and Ogre); change the exportFlags logic
around the export call (the exportFlags variable used with
Assimp::Exporter::Export) to set exportFlags = 0 when formatId equals "x" OR
"gltf" OR "glb" (and any other right-handed exporters you expect), and only use
aiProcess_ConvertToLeftHanded for formats that require left-handed conversion;
update the conditional that computes exportFlags before calling exporter.Export.
In `@src/PropertiesPanelController.cpp`:
- Around line 487-519: PropertiesPanelController::exportCurrentPose is missing a
Sentry breadcrumb for the user-facing export action; add a call to
SentryReporter::addBreadcrumb("file.export", <message>) to record the export
event. Specifically, after you determine the target entity (animatedEntity) and
after you have a final outputPath (i.e., right before calling
MeshImporterExporter::exportCurrentPose), call SentryReporter::addBreadcrumb
with category "file.export" and a concise message including the entity name
(animatedEntity->getName()) and the outputPath (or "cancelled" if the user
aborted) so the export attempt and target file are logged. Ensure the breadcrumb
is added in both code paths (when path is provided and when the save dialog is
used) and keep the message clear like "Export current pose: <entity>: <path>".
---
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 1331-1334: Add a mutual exclusivity check that rejects using both
--time and --count together: inside the CLI parsing/validation block where the
variables time and count are inspected (the same area that currently checks if
(time < 0.0f && count <= 0)), add a condition that detects time >= 0.0f && count
> 0 and prints an error via err() with a clear message (e.g., "Error: --time and
--count are mutually exclusive.") and return a non-zero exit code; this will
prevent the current silent precedence behavior where count wins and ensure users
are informed of the conflict.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 967f0513-31ff-4df7-a087-0a4bb73eef6f
📒 Files selected for processing (12)
CLAUDE.mddocker-entrypoint.shqml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/MCPServer.cppsrc/MCPServer.hsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/main.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/MCPServer.h
- src/PropertiesPanelController.h
- src/MCPServer.cpp
Comment on lines
+1297
to
+1450
| // Request software skinning so we can read CPU-side deformed vertices | ||
| entity->addSoftwareAnimationRequest(false); | ||
| entity->_updateAnimation(); | ||
|
|
||
| const Ogre::MeshPtr mesh = entity->getMesh(); | ||
| const unsigned int numSub = mesh->getNumSubMeshes(); | ||
|
|
||
| // Build an aiScene with deformed vertex positions, NO skeleton, NO animations | ||
| auto* scene = new aiScene(); | ||
| scene->mRootNode = new aiNode(); | ||
| scene->mRootNode->mName = aiString(entity->getName()); | ||
| scene->mRootNode->mNumMeshes = numSub; | ||
| scene->mRootNode->mMeshes = new unsigned int[numSub]; | ||
| for (unsigned int si = 0; si < numSub; ++si) | ||
| scene->mRootNode->mMeshes[si] = si; | ||
|
|
||
| // Materials | ||
| std::vector<Ogre::MaterialPtr> materials; | ||
| std::set<std::string, std::less<>> seen; | ||
| for (const auto* sub : entity->getSubEntities()) | ||
| { | ||
| auto mat = sub->getMaterial(); | ||
| if (seen.insert(mat->getName()).second) | ||
| materials.push_back(mat); | ||
| } | ||
| scene->mNumMaterials = static_cast<unsigned int>(materials.size()); | ||
| scene->mMaterials = new aiMaterial*[scene->mNumMaterials]; | ||
| std::map<std::string, unsigned int, std::less<>> matIndexMap; | ||
| for (unsigned int i = 0; i < scene->mNumMaterials; ++i) { | ||
| scene->mMaterials[i] = buildAiMaterialFromOgre(materials[i]); | ||
| matIndexMap[materials[i]->getName()] = i; | ||
| } | ||
|
|
||
| // Meshes — read deformed positions from software-skinned buffers | ||
| scene->mNumMeshes = numSub; | ||
| scene->mMeshes = numSub > 0 ? new aiMesh*[numSub] : nullptr; | ||
|
|
||
| for (unsigned int si = 0; si < numSub; ++si) | ||
| { | ||
| const Ogre::SubMesh* subMesh = mesh->getSubMesh(si); | ||
| Ogre::SubEntity* subEnt = entity->getSubEntity(si); | ||
|
|
||
| // Get the software-skinned vertex data (deformed positions/normals) | ||
| Ogre::VertexData* animData = subMesh->useSharedVertices | ||
| ? entity->_getSkelAnimVertexData() | ||
| : subEnt->_getSkelAnimVertexData(); | ||
|
|
||
| // Fall back to bind-pose vertex data if skinned data unavailable | ||
| const Ogre::VertexData* bindData = subMesh->useSharedVertices | ||
| ? mesh->sharedVertexData : subMesh->vertexData; | ||
|
|
||
| if (!animData && !bindData) { | ||
| scene->mMeshes[si] = new aiMesh(); | ||
| continue; | ||
| } | ||
|
|
||
| auto* aiM = new aiMesh(); | ||
| scene->mMeshes[si] = aiM; | ||
| aiM->mPrimitiveTypes = aiPrimitiveType_TRIANGLE; | ||
|
|
||
| // Use animData for positions and normals, bindData for UVs and indices | ||
| const Ogre::VertexData* posSource = animData ? animData : bindData; | ||
| aiM->mNumVertices = static_cast<unsigned int>(posSource->vertexCount); | ||
| aiM->mVertices = new aiVector3D[aiM->mNumVertices]; | ||
|
|
||
| // Material index | ||
| auto matIt = matIndexMap.find(subEnt->getMaterial()->getName()); | ||
| aiM->mMaterialIndex = (matIt != matIndexMap.end()) ? matIt->second : 0; | ||
|
|
||
| // Read deformed positions | ||
| const auto* posElem = posSource->vertexDeclaration->findElementBySemantic(Ogre::VES_POSITION); | ||
| if (posElem) | ||
| { | ||
| auto vbuf = posSource->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 deformed normals (from animData if available, otherwise bindData) | ||
| const auto* normElem = posSource->vertexDeclaration->findElementBySemantic(Ogre::VES_NORMAL); | ||
| if (normElem) | ||
| { | ||
| aiM->mNormals = new aiVector3D[aiM->mNumVertices]; | ||
| auto vbuf = posSource->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 UVs from bind-pose data (skinning doesn't affect UVs) | ||
| if (bindData) | ||
| { | ||
| const auto* tcElem = bindData->vertexDeclaration->findElementBySemantic(Ogre::VES_TEXTURE_COORDINATES); | ||
| if (tcElem) | ||
| { | ||
| aiM->mTextureCoords[0] = new aiVector3D[aiM->mNumVertices]; | ||
| aiM->mNumUVComponents[0] = 2; | ||
| auto vbuf = bindData->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 from the original submesh | ||
| 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; | ||
|
|
||
| const unsigned int indexStart = static_cast<unsigned int>(iData->indexStart); | ||
| 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)[indexStart + f * 3 + v] | ||
| : reinterpret_cast<const uint16_t*>(ibase)[indexStart + f * 3 + v]; | ||
| aiM->mFaces[f].mIndices[v] = idx; | ||
| } | ||
| } | ||
| ibuf->unlock(); | ||
| } | ||
|
|
||
| compactAiMesh(aiM); | ||
| } | ||
|
|
||
| // No animations, no bones — this is a static mesh | ||
|
|
||
| // Release software animation request | ||
| entity->removeSoftwareAnimationRequest(false); | ||
|
|
There was a problem hiding this comment.
Make the pose-bake cleanup exception-safe.
Any exception during the bake/build phase before Line 1449 skips removeSoftwareAnimationRequest(false) and escapes this int-returning API. The MCP pose-export caller only disables the active animation after exportCurrentPose() returns, so a failed export can leave both the software-animation request and the animation state stuck on. Wrap the request and scene lifetime in RAII and catch around the whole bake path so failures consistently return -1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 1297 - 1450, The pose-bake needs
to be exception-safe: create an RAII guard that calls
entity->addSoftwareAnimationRequest(false) in its ctor and
entity->removeSoftwareAnimationRequest(false) in its dtor (so the request is
always released), and manage the aiScene* using a smart pointer (e.g.,
std::unique_ptr<aiScene> with a deleter) so the scene is freed on exceptions;
wrap the whole bake/build block (everything after addSoftwareAnimationRequest up
to compactAiMesh calls) in a try/catch that catches exceptions, ensures the
function returns -1 on failure, and reuses entity->_updateAnimation() as before
(call it after the RAII guard is constructed) — reference the symbols
entity->addSoftwareAnimationRequest, entity->removeSoftwareAnimationRequest,
entity->_updateAnimation, aiScene, and compactAiMesh to locate where to apply
the RAII and the try/catch.
Comment on lines
+487
to
+519
| bool PropertiesPanelController::exportCurrentPose(const QString& path) | ||
| { | ||
| auto entities = SelectionSet::getSingleton()->getResolvedEntities(); | ||
| Ogre::Entity* animatedEntity = nullptr; | ||
| for (Ogre::Entity* ent : entities) | ||
| { | ||
| if (ent->hasSkeleton()) { | ||
| animatedEntity = ent; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!animatedEntity) return false; | ||
|
|
||
| QString outputPath = path; | ||
| if (outputPath.isEmpty()) | ||
| { | ||
| QString filter = "STL (*.stl)"; | ||
| outputPath = QFileDialog::getSaveFileName( | ||
| nullptr, | ||
| QObject::tr("Export Current Pose"), | ||
| QString::fromStdString(animatedEntity->getName()) + "_pose", | ||
| MeshImporterExporter::exportFileDialogFilter(), | ||
| &filter, | ||
| QFileDialog::DontUseNativeDialog); | ||
| if (outputPath.isEmpty()) return false; | ||
|
|
||
| outputPath = MeshImporterExporter::formatFileURI(outputPath, filter); | ||
| } | ||
|
|
||
| int result = MeshImporterExporter::exportCurrentPose(animatedEntity, outputPath); | ||
| return result == 0; | ||
| } |
There was a problem hiding this comment.
Missing Sentry breadcrumb for pose export action.
Per the newly added coding guideline in CLAUDE.md: "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message)." The exportCurrentPose function is a file export operation triggered from the UI but doesn't record a breadcrumb.
As per coding guidelines: "file.import" / "file.export" for I/O operations.
Proposed fix to add Sentry breadcrumb
+#include "SentryReporter.h"
bool PropertiesPanelController::exportCurrentPose(const QString& path)
{
auto entities = SelectionSet::getSingleton()->getResolvedEntities();
Ogre::Entity* animatedEntity = nullptr;
for (Ogre::Entity* ent : entities)
{
if (ent->hasSkeleton()) {
animatedEntity = ent;
break;
}
}
if (!animatedEntity) return false;
QString outputPath = path;
if (outputPath.isEmpty())
{
// ... file dialog code ...
}
+ SentryReporter::addBreadcrumb("file.export",
+ QString("Export pose: %1").arg(QFileInfo(outputPath).fileName()));
+
int result = MeshImporterExporter::exportCurrentPose(animatedEntity, outputPath);
return result == 0;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PropertiesPanelController.cpp` around lines 487 - 519,
PropertiesPanelController::exportCurrentPose is missing a Sentry breadcrumb for
the user-facing export action; add a call to
SentryReporter::addBreadcrumb("file.export", <message>) to record the export
event. Specifically, after you determine the target entity (animatedEntity) and
after you have a final outputPath (i.e., right before calling
MeshImporterExporter::exportCurrentPose), call SentryReporter::addBreadcrumb
with category "file.export" and a concise message including the entity name
(animatedEntity->getName()) and the outputPath (or "cancelled" if the user
aborted) so the export attempt and target file are logged. Ensure the breadcrumb
is added in both code paths (when path is provided and when the save dialog is
used) and keep the message clear like "Export current pose: <entity>: <path>".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
qml/PropertiesPanel.qml (1)
237-241: Unused Timer — dead code.
snapRepaintTimeris defined but never started. TheforceRepaint()function resets Repeater models directly and doesn't use this timer.🧹 Remove unused Timer
- // Timer to force QQuickWidget repaint — toggling opacity marks - // the entire subtree dirty in the scene graph, forcing a redraw. - Timer { - id: snapRepaintTimer - interval: 1; repeat: false - onTriggered: snapCol.opacity = 1.0 - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/PropertiesPanel.qml` around lines 237 - 241, The Timer object snapRepaintTimer is dead code (never started and not referenced by forceRepaint()), so remove the Timer block (id: snapRepaintTimer) and its onTriggered handler that sets snapCol.opacity, and ensure forceRepaint() continues to reset the Repeater models (e.g., those used by snapCol/Repeater) without relying on the timer; search for any remaining references to snapRepaintTimer and delete them so no unused identifier remains.
🤖 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/PropertiesPanel.qml`:
- Around line 251-256: The local snap state initialized in Component.onCompleted
(snapOn, activeGridIdx, activeAngleIdx, activeScaleIdx using findIdx and
PropertiesPanelController.snapEnabled/snapGridSize/snapAngleStep/snapScaleStep)
will go stale when settings change externally; add a Connections block targeting
PropertiesPanelController and connect its NOTIFY signals (snapEnabledChanged,
snapGridSizeChanged, snapAngleStepChanged, snapScaleStepChanged) to handlers
that update snapOn and recompute activeGridIdx/activeAngleIdx/activeScaleIdx
(using findIdx with the updated controller properties) so the UI stays in sync
with external changes.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 237-241: The Timer object snapRepaintTimer is dead code (never
started and not referenced by forceRepaint()), so remove the Timer block (id:
snapRepaintTimer) and its onTriggered handler that sets snapCol.opacity, and
ensure forceRepaint() continues to reset the Repeater models (e.g., those used
by snapCol/Repeater) without relying on the timer; search for any remaining
references to snapRepaintTimer and delete them so no unused identifier remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 735da394-cd34-47d8-a085-5ebc14ea2ccf
📒 Files selected for processing (2)
qml/PropertiesPanel.qmlsrc/mainwindow.cpp
- All user-facing actions must be tracked with SentryReporter::addBreadcrumb - Version format is strictly X.Y.Z (never prepend v) — the update check feature compares against GitHub release tags and v prefix breaks it
- Manager::duplicateSceneNode(): clones scene node with transform, all attached entities, and per-sub-entity material assignments - DuplicateCommand: undo hides clones, redo shows them (same pattern as DeleteCommand for safe undo/redo without node destruction) - Ctrl+D shortcut via QAction in Edit menu (after Undo/Redo) - MainWindow::duplicateSelected(): handles multi-selection, pushes undo command, auto-selects clones - MCP tool: duplicate_entity (by name or current selection) - Added to AI chat tool filter - Sentry breadcrumb tracking for the UI action - 5 unit tests for DuplicateCommand Part of #256 (Phase 1: Scene Editing Power Tools) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
duplicateSceneNode() was sharing the MeshPtr (shallow copy), so renaming an animation on a clone also changed the original. Now uses Mesh::clone() to create a fully independent mesh resource per duplicate with its own skeleton and animation data. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ogre 14 Skeleton has no clone() method, so manually create a new skeleton resource and copy: all bones with initial state, bone parent hierarchy, binding pose, all animations with their node tracks and keyframes. This ensures renaming/editing an animation on a duplicated entity does not affect the original. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cut breadcrumbs DuplicateCommand now properly destroys cloned nodes on undo (removing them from the scene tree) and re-duplicates from source nodes on redo. The previous hide/show pattern left invisible ghost nodes in the tree. Also adds Sentry breadcrumbs for all keyboard shortcuts (Q/W/E/R/F/X/Del) so we can track shortcut usage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
On redo, duplicateSceneNode generates the same resource names as the original duplicate. MeshManager and SkeletonManager still held the old entries from before undo (destroySceneNode removes the entity/node but not the underlying resource registrations). Now removes any existing mesh/skeleton resource by name before cloning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Grid/angle/scale snapping during gizmo drags. Hold Ctrl or enable via the Inspector panel. Accumulator-based: sub-threshold mouse deltas accumulate until they exceed the snap step, then a quantized transform is applied. Core: - TransformOperator: snap settings (gridSize, angleStep, scaleStep), snap math helpers, accumulator-based drag snapping in translate/ rotate/scale handlers, QSettings persistence - PropertiesPanelController: QML property bridge for all snap settings - PropertiesPanel.qml: "Snap Settings" collapsible section with preset buttons (0.1-5.0 grid, 5-90 angle, 0.1-0.5 scale) - MCP tools: set_snap_settings, get_snap_settings - 8 unit tests for snap math and settings Part of #256 (Phase 1: Scene Editing Power Tools) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Export animated models as static meshes frozen at any pose. Reads
software-skinned vertex positions from Ogre and writes a new mesh
with no skeleton or animations — ideal for 3D printing.
Core:
- MeshImporterExporter::exportCurrentPose(): request software skinning,
read deformed positions/normals per SubEntity, build aiScene, export
via Assimp to STL/OBJ/glTF/FBX/DAE
- GUI: "Export Pose" button in Inspector Animation section
- CLI: qtmesh pose model.fbx --animation "Walk" --time 0.5 -o posed.stl
qtmesh pose model.fbx --animation "Dance" --count 4 -o pose_%02d.stl
- MCP tool: export_pose (entity, animation, time, output_path)
- Docker entrypoint: added pose/validate/lod to recognized subcommands
- CLAUDE.md: documented pose subcommand
Part of #256 (Phase 1: Scene Editing Power Tools)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace Qt Controls CheckBox with custom themed checkbox matching the animation panel style (Rectangle + checkmark + MouseArea) - Change Row to Flow with proportional widths so all presets fit and resize with the panel (no more overflow on narrow widths) - Add hover highlight on preset buttons Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The property bool active binding was not re-evaluating when the snap setting changed. Replaced with inline bindings in the color expression that directly reference the controller property, so only the currently active preset shows the highlight color. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QML bindings weren't re-evaluating because all 4 snap properties shared one generic snapSettingsChanged signal. Split into dedicated signals (snapEnabledChanged, snapGridSizeChanged, snapAngleStepChanged, snapScaleStepChanged) so QML precisely tracks each property change and updates button highlights reactively. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QML Repeater delegates weren't re-evaluating bindings that referenced the C++ singleton properties directly. Fixed by mirroring snap values into local QML properties (curGridSize, curAngleStep, etc.) on the parent Column, updated via explicit Connections handlers. Delegates bind to the local properties which QML tracks reliably. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e colors - Replace reactive bindings with model-reset approach: on click, the Repeater model is nulled and restored, destroying and recreating all delegates with correct colors (QQuickWidget in QDockWidget doesn't repaint sibling delegates on property changes) - Change default button color from buttonColor (QPalette::Button, light even in dark mode on macOS) to headerColor (QPalette::Window darkened) - Add Timer-based opacity toggle as additional repaint trigger - Request QML scene graph frame update from C++ on snap changes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Select individual sub-meshes in the scene tree and transform them independently by modifying vertex buffer data directly. Core: - SubMeshTransform: translate/rotate/scale sub-mesh vertices, read/write vertex positions, compute centroid, recalculate mesh bounds - SubEntityHighlight: visual tint on selected sub-entities via cloned material with emissive overlay, auto-updates on selection change - SubMeshTransformCommand: full vertex snapshot undo/redo - TransformOperator: gizmo integration — positions at sub-mesh centroid, applies vertex transforms on drag, pushes undo on mouse release - MCP tool: transform_submesh (entity, submesh_index, translate/rotate/scale) - 4 unit tests for vertex transform, centroid, read/write round-trip Part of #256 (Phase 1: Scene Editing Power Tools) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SubEntityHighlight and TransformOperator accessed sub-entity pointers that could become stale when entities are destroyed or the scene changes. - Copy selection list before iterating (avoids concurrent modification) - Null-check sub->getParent() and getParentSceneNode() chains - Wrap critical sections in try-catch to recover from stale pointers - Clear highlight tracking map on exception Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After modifying vertex buffer data, Ogre's GPU-side buffer wasn't being re-uploaded because needUpdate(true) only marks the scene node transform as dirty, not the mesh data. Fixed by detaching and re-attaching the entity from its parent scene node after each vertex modification, which forces Ogre to rebuild its render operation and re-upload the vertex buffer to the GPU. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… ops The detach/re-attach approach didn't force Ogre to re-read vertex buffer data. entity->_initialise(true) rebuilds all internal SubEntity render operations from the mesh, picking up modified vertex positions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
HBL_NORMAL lock/unlock wasn't triggering GPU buffer re-upload on all drivers. _initialise(true) crashed by invalidating SubEntity pointers. New approach: readData() the entire vertex buffer into local memory, modify positions in the copy, then writeData(..., discardWholeBuffer=true) which forces Ogre to upload the full buffer to GPU regardless of shadow buffer state. Also refactored scale/rotate to use readPositions/writePositions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ogre renders skeletal entities from SubEntity's animation blend buffer, not the mesh VBO. Now writes to BOTH buffers: 1. Mesh vertex buffer (bind pose) — for persistence and export 2. SubEntity::_getSkelAnimVertexData() — for immediate GPU rendering Also upgrades static buffers to HBU_DYNAMIC on first write and uses HBL_DISCARD lock for guaranteed GPU upload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Bump SERVER_VERSION 1.2.0 → 1.3.0 for new MCP tools added in Phase 1 - Manager::duplicateSceneNode preserves UserObjectBindings (PrimitiveObject type etc.) and uses per-entity index in clone names to avoid collisions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/phase1-scene-editing-tools
branch
from
April 9, 2026 00:13
a4a519a to
e4ae735
Compare
12 tasks
Review fixes: - MCP duplicate_entity now pushes DuplicateCommand for undo support - Snap settings validation is atomic (validate all fields before applying) - DuplicateCommand tests use hasSceneNode() instead of dereferencing potentially destroyed entity pointers - QML snap state syncs from external changes via Connections block - Breadcrumb confirmed in PropertiesPanelController::exportCurrentPose New tests (22 total): - MCPServer: 18 tests for duplicate_entity, transform_submesh, set/get_snap_settings, export_pose (valid + error cases) - TransformCommands: 4 tests for scaleSubMesh, rotateSubMesh, centroid invariance under rotation and scaling Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SnapSettingsRoundTripAndEmitSignal expected setSnapEnabled(true) to emit, but snap may already be enabled from persisted QSettings on CI. Reset to known state (disabled, default values) before starting the signal count. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
9 tasks
3 tasks
4 tasks
This was referenced Jul 19, 2026
Merged
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
Phase 1 of Scene Editing Power Tools (#256).
Completed
Test plan