Phase 1: Remaining items — keyframe resampling, node grouping (#256)#271
Merged
Conversation
Resample animations to reduce keyframe density (e.g., Mixamo's 200+ keyframes down to 30) or keep every Nth keyframe for lighter files. Core: - AnimationMerger::resampleAnimation(): evaluate animation at N evenly-spaced times via getInterpolatedKeyFrame, rebuild with smooth interpolated keyframes - AnimationMerger::decimateAnimation(): keep every Nth keyframe plus always the last, discard the rest (no interpolation) CLI: - qtmesh anim model.fbx --resample 30 -o optimized.fbx - qtmesh anim model.fbx --decimate-step 5 -o lighter.fbx - qtmesh anim model.fbx --resample 30 --animation "Walk" -o out.fbx MCP: resample_animation tool (target_keyframes or decimate_step) Tests: 8 new tests (resample basic + interpolation, decimate basic + keeps-last, null safety, missing animation, step validation) Part of #256 (Phase 1: Scene Editing Power Tools) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Group selected nodes under a shared parent for organized scene hierarchy. Ogre SceneNode parenting propagates transforms automatically. Core: - Manager::groupNodes(): create empty "Group" node at centroid of selection, reparent children preserving world transforms - Manager::ungroupNode(): reparent children to root, destroy empty group - Manager::isGroupNode(): detect group nodes (no entities, has children) - SceneTreeModel: shows "Group" type label, recursive child traversal - GroupCommand/UngroupCommand: full undo/redo support - Ctrl+G (group) and Ctrl+Shift+G (ungroup) in Edit menu - MCP tools: group_nodes, ungroup_node - 8 unit tests Part of #256 (Phase 1: Scene Editing Power Tools) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 animation resampling and decimation APIs with CLI and MCP tool exposure and tests; implements scene-node grouping/ungrouping with undo/redo, reparenting (UI and programmatic), pivot‑mode controls with persistence and gizmo updates, and related model/controller/QML updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CLIP as CLIPipeline
participant AM as AnimationMerger
participant Ogre as Ogre Engine
User->>CLIP: run `qtmesh anim --resample 30 --animation "Walk"`
CLIP->>CLIP: parse flags, validate (N ≥ 2)
CLIP->>AM: collect matching animation names
CLIP->>AM: resampleAnimation(skel,"Walk",30)
AM->>Ogre: read animation length & node tracks
AM->>Ogre: remove old animation
AM->>Ogre: create new animation with same interpolation modes
loop per target time
AM->>Ogre: evaluate interpolated T/R/S on tracks
AM->>Ogre: insert keyframe into new track
end
AM-->>CLIP: return keyframe delta
CLIP->>Ogre: refresh animations
CLIP->>CLIP: export modified skeleton
sequenceDiagram
participant User as User/UI
participant MW as MainWindow
participant Mgr as Manager
participant Ogre as Ogre Scene
participant Undo as UndoManager
User->>MW: Select 2+ nodes, press Ctrl+G
MW->>Mgr: groupNodes(selected_nodes)
Mgr->>Ogre: create group node at centroid
Mgr->>Ogre: recompute children local transforms
Mgr->>Ogre: reparent children under group
Mgr-->>MW: return group node
MW->>Undo: push(GroupCommand)
User->>MW: Select group node, press Ctrl+Shift+G
MW->>Undo: push(UngroupCommand)
MW->>Mgr: ungroupNode(group)
Mgr->>Ogre: reparent children to group's parent, preserve world transforms
Mgr->>Ogre: destroy group node
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)
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 |
12 tasks
![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: 51a74a02c6
ℹ️ 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
+213
to
+216
| Ogre::Node* p = node->getParent(); | ||
| auto* mgr = Manager::getSingleton()->getSceneMgr(); | ||
| info.oldParentName = (p && p != mgr->getRootSceneNode()) ? p->getName() : ""; | ||
| info.oldPosition = node->getPosition(); |
There was a problem hiding this comment.
GroupCommand snapshots each node's oldParentName and local transform from the current scene state, but both grouping entry points call Manager::groupNodes(nodes) before the command is constructed. That means this snapshot records the newly created group as the "old" parent, so undo() cannot restore the original hierarchy and can end up destroying grouped children with the group node instead of ungrouping them. Build the undo snapshot from pre-group data (or pass that data in) before mutating parentage.
Useful? React with 👍 / 👎.
Comment on lines
+284
to
+285
| Ogre::SceneNode* groupNode = sceneMgr->getRootSceneNode()->createChildSceneNode(mGroupNodeName); | ||
| groupNode->setPosition(mGroupPosition); |
There was a problem hiding this comment.
Redo always recreates the group under the root scene node, which changes hierarchy semantics after an undo/redo cycle. When nodes were originally grouped under a non-root (possibly transformed) parent, the redone group no longer inherits that parent's transform, so subsequent parent transforms no longer propagate as they did before undo.
Useful? React with 👍 / 👎.
- Renamed menu items to "Group Nodes" and "Ungroup Nodes" for clarity - Both actions start disabled; enabled/disabled via selectionChanged: Group Nodes requires 2+ selected nodes, Ungroup Nodes requires exactly 1 selected group node Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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.cpp (1)
256-314: First redo's group-node discovery relies on parent heuristic — works but is fragile.Lines 269-273 find the group node by looking at the parent of the first child node. This works because the caller (
MainWindow::groupSelected) creates the group immediately before pushing this command. However, if another operation modified the scene betweengroupNodes()andpush(), the heuristic could fail.The current design is acceptable since the caller pattern is tightly controlled, but consider capturing
mGroupNodeNameas an additional parameter passed from the caller for robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/TransformCommands.cpp` around lines 256 - 314, The redo() method in GroupCommand relies on a fragile heuristic (inspecting the first child's parent) to discover the created group node; instead, modify the command API so the caller passes the created group's name when constructing or initializing the GroupCommand and store it in mGroupNodeName up-front; update the call sites (e.g., MainWindow::groupSelected or Manager::groupNodes where GroupCommand is created/pushed) to provide the new parameter, and remove the parent-based discovery block in GroupCommand::redo so redo always uses the supplied mGroupNodeName.src/AnimationMerger.cpp (1)
230-319: Return value can be negative when upsampling.When
targetKeyframes > originalMaxKeyframes, the return valueoriginalMaxKeyframes - targetKeyframeswill be negative. This could confuse callers expecting a count of "removed" keyframes.Consider documenting this behavior or returning
0when the target exceeds the original (since no keyframes are technically "removed" during upsampling — they're interpolated).📝 Optional: clamp return value to non-negative
- return originalMaxKeyframes - targetKeyframes; + return std::max(0, originalMaxKeyframes - targetKeyframes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 230 - 319, The function AnimationMerger::resampleAnimation currently returns originalMaxKeyframes - targetKeyframes which can be negative when upsampling; change the return to a non-negative count (e.g., return std::max(0, originalMaxKeyframes - targetKeyframes) or return 0 when targetKeyframes > originalMaxKeyframes) so callers always receive a non-negative "removed keyframes" count and update any docs/comments for resampleAnimation to reflect the behavior.src/mainwindow.cpp (1)
739-751: Verify the undo command push order for ungroupSelected().The command is pushed before calling
Manager::ungroupNode(). This works becauseUngroupCommand::redo()uses themFirstRedopattern to skip the initial redo (letting the caller perform the actual operation). However, ifungroupNode()throws or fails after the command is pushed, the undo stack would be inconsistent.Consider wrapping this in a try-catch or checking the result of
ungroupNode()— though the current pattern matchesDuplicateCommandand appears intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 739 - 751, MainWindow::ungroupSelected currently pushes UngroupCommand to UndoManager before calling Manager::ungroupNode, risking an inconsistent undo stack if ungroupNode fails; modify the flow so you either (a) call Manager::ungroupNode(node) first and only on success push new UngroupCommand(node) to UndoManager::getSingleton()->push, or (b) wrap Manager::ungroupNode(node) in a try/catch and if an exception or failure occurs roll back/avoid pushing the UngroupCommand (or remove it if already pushed). Ensure references to UngroupCommand, UndoManager::getSingleton()->push, Manager::getSingleton()->ungroupNode, and MainWindow::ungroupSelected are used to locate and update the code.
🤖 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/Manager.cpp`:
- Around line 448-455: Protect against division-by-zero when computing child
transforms: before doing the component-wise division in the grouping code (where
you compute node->setScale(worldScale / groupWorldScale) and the position
division ((worldPos - groupDerivedPos) / groupWorldScale)), check the components
of groupWorldScale for zeros and replace or clamp them to a small non-zero
epsilon (or 1.0 if you intend to preserve absolute values) to avoid producing
Inf/NaN; apply the same defensive check in the ungroupNode code path that
performs the same divisions so both grouping and ungrouping use the safe,
clamped groupWorldScale for scale and position calculations.
In `@src/MCPServer.cpp`:
- Around line 2173-2178: The condition "isResample && targetKeyframes < 2" is
impossible because isResample is defined as (targetKeyframes >= 2), so remove
the dead validation block that calls makeErrorResult for target_keyframes;
instead keep the existing validations around targetKeyframes/decimateStep
(isResample, targetKeyframes, decimateStep, makeErrorResult) and ensure only the
valid decimateStep check remains for the !isResample branch.
---
Nitpick comments:
In `@src/AnimationMerger.cpp`:
- Around line 230-319: The function AnimationMerger::resampleAnimation currently
returns originalMaxKeyframes - targetKeyframes which can be negative when
upsampling; change the return to a non-negative count (e.g., return std::max(0,
originalMaxKeyframes - targetKeyframes) or return 0 when targetKeyframes >
originalMaxKeyframes) so callers always receive a non-negative "removed
keyframes" count and update any docs/comments for resampleAnimation to reflect
the behavior.
In `@src/commands/TransformCommands.cpp`:
- Around line 256-314: The redo() method in GroupCommand relies on a fragile
heuristic (inspecting the first child's parent) to discover the created group
node; instead, modify the command API so the caller passes the created group's
name when constructing or initializing the GroupCommand and store it in
mGroupNodeName up-front; update the call sites (e.g., MainWindow::groupSelected
or Manager::groupNodes where GroupCommand is created/pushed) to provide the new
parameter, and remove the parent-based discovery block in GroupCommand::redo so
redo always uses the supplied mGroupNodeName.
In `@src/mainwindow.cpp`:
- Around line 739-751: MainWindow::ungroupSelected currently pushes
UngroupCommand to UndoManager before calling Manager::ungroupNode, risking an
inconsistent undo stack if ungroupNode fails; modify the flow so you either (a)
call Manager::ungroupNode(node) first and only on success push new
UngroupCommand(node) to UndoManager::getSingleton()->push, or (b) wrap
Manager::ungroupNode(node) in a try/catch and if an exception or failure occurs
roll back/avoid pushing the UngroupCommand (or remove it if already pushed).
Ensure references to UngroupCommand, UndoManager::getSingleton()->push,
Manager::getSingleton()->ungroupNode, and MainWindow::ungroupSelected are used
to locate and update the code.
🪄 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: 6e68bc34-1edf-4720-8949-1d37e9ea468a
📒 Files selected for processing (16)
CLAUDE.mdsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/Manager.cppsrc/Manager.hsrc/NodeGrouping_test.cppsrc/SceneTreeModel.cppsrc/commands/TransformCommands.cppsrc/commands/TransformCommands.hsrc/mainwindow.cppsrc/mainwindow.hui_files/mainwindow.ui
Comment on lines
+448
to
+455
| Ogre::Vector3 groupWorldScale = groupNode->_getDerivedScale(); | ||
| Ogre::Vector3 groupDerivedPos = groupNode->_getDerivedPosition(); | ||
|
|
||
| node->setOrientation(groupWorldOrient.Inverse() * worldOrient); | ||
| node->setScale(worldScale / groupWorldScale); | ||
| node->setPosition(groupWorldOrient.Inverse() * | ||
| ((worldPos - groupDerivedPos) / groupWorldScale)); | ||
| } |
There was a problem hiding this comment.
Potential division by zero when preserving scale transforms.
At line 452, node->setScale(worldScale / groupWorldScale) performs component-wise division. If any component of groupWorldScale is zero (e.g., a flattened node), this produces infinity or NaN, corrupting the child's scale.
The same issue exists in ungroupNode at line 495.
🛡️ Suggested defensive check
+ // Clamp scale components to avoid division by zero
+ auto safeScale = [](const Ogre::Vector3& s) {
+ const Ogre::Real epsilon = 1e-6f;
+ return Ogre::Vector3(
+ std::abs(s.x) < epsilon ? epsilon : s.x,
+ std::abs(s.y) < epsilon ? epsilon : s.y,
+ std::abs(s.z) < epsilon ? epsilon : s.z);
+ };
+
node->setOrientation(groupWorldOrient.Inverse() * worldOrient);
- node->setScale(worldScale / groupWorldScale);
+ node->setScale(worldScale / safeScale(groupWorldScale));
node->setPosition(groupWorldOrient.Inverse() *
((worldPos - groupDerivedPos) / groupWorldScale));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Manager.cpp` around lines 448 - 455, Protect against division-by-zero
when computing child transforms: before doing the component-wise division in the
grouping code (where you compute node->setScale(worldScale / groupWorldScale)
and the position division ((worldPos - groupDerivedPos) / groupWorldScale)),
check the components of groupWorldScale for zeros and replace or clamp them to a
small non-zero epsilon (or 1.0 if you intend to preserve absolute values) to
avoid producing Inf/NaN; apply the same defensive check in the ungroupNode code
path that performs the same divisions so both grouping and ungrouping use the
safe, clamped groupWorldScale for scale and position calculations.
Comment on lines
+2173
to
+2178
| bool isResample = targetKeyframes >= 2; | ||
|
|
||
| if (isResample && targetKeyframes < 2) | ||
| return makeErrorResult("Error: target_keyframes must be >= 2"); | ||
| if (!isResample && decimateStep < 2) | ||
| return makeErrorResult("Error: decimate_step must be >= 2"); |
There was a problem hiding this comment.
Dead code: the check at lines 2175-2176 can never evaluate to true.
If targetKeyframes >= 2, then isResample is true, which means targetKeyframes < 2 is necessarily false. This makes the condition isResample && targetKeyframes < 2 always false.
The validation still works correctly because invalid cases are caught by lines 2170-2171 (both params missing) and 2177-2178 (invalid decimate_step when not resampling).
🧹 Suggested cleanup
- bool isResample = targetKeyframes >= 2;
-
- if (isResample && targetKeyframes < 2)
- return makeErrorResult("Error: target_keyframes must be >= 2");
- if (!isResample && decimateStep < 2)
+ bool isResample = (targetKeyframes >= 2);
+
+ if (!isResample && decimateStep < 2)
return makeErrorResult("Error: decimate_step must be >= 2");📝 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
| bool isResample = targetKeyframes >= 2; | |
| if (isResample && targetKeyframes < 2) | |
| return makeErrorResult("Error: target_keyframes must be >= 2"); | |
| if (!isResample && decimateStep < 2) | |
| return makeErrorResult("Error: decimate_step must be >= 2"); | |
| bool isResample = (targetKeyframes >= 2); | |
| if (!isResample && decimateStep < 2) | |
| return makeErrorResult("Error: decimate_step must be >= 2"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer.cpp` around lines 2173 - 2178, The condition "isResample &&
targetKeyframes < 2" is impossible because isResample is defined as
(targetKeyframes >= 2), so remove the dead validation block that calls
makeErrorResult for target_keyframes; instead keep the existing validations
around targetKeyframes/decimateStep (isResample, targetKeyframes, decimateStep,
makeErrorResult) and ensure only the valid decimateStep check remains for the
!isResample branch.
Three pivot modes for the transform gizmo: Center (default), Bottom (floor level), and Origin (node local origin). Rotation and scale operate around the active pivot point. Core: - TransformOperator: PivotMode enum, getPivotPoint() computes pivot based on mode and selection bounding box, QSettings persistence - P key cycles through modes; QML Inspector shows 3 toggle buttons - PropertiesPanelController: Q_PROPERTY bridge for pivot mode - MCP tools: set_pivot_mode, get_pivot_mode - Sentry breadcrumbs for mode changes Part of #256 (Phase 1: Scene Editing Power Tools) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collapsible "Undo History" section in the Inspector panel showing the
full QUndoStack. Click any entry to jump to that state.
- PropertiesPanelController: undoHistory property (QVariantList of
{text, isCurrent}), undoIndex, undoToIndex(int), clearUndoHistory()
- QML: ListView with highlighted current position, dimmed redo zone,
"Initial State" entry, hover effects, "Clear History" button
- Sentry breadcrumbs for history jumps and clear
Part of #256 (Phase 1: Scene Editing Power Tools)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
qml/PropertiesPanel.qml (1)
217-217: Unused propertyactivePivot.The
activePivotproperty on line 217 capturesPropertiesPanelController.pivotModebut is never referenced—the color binding on line 231 accessesPropertiesPanelController.pivotModedirectly. Consider removing it to avoid confusion.♻️ Suggested fix
Row { spacing: 4 width: parent.width - 16 - - property int activePivot: PropertiesPanelController.pivotMode Repeater {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/PropertiesPanel.qml` at line 217, The property activePivot is defined as "property int activePivot: PropertiesPanelController.pivotMode" but never used; either remove the activePivot definition to avoid dead code or update references (e.g., the color binding that currently reads PropertiesPanelController.pivotMode) to use activePivot instead; locate the activePivot property in PropertiesPanel.qml and either delete that property declaration or replace direct uses of PropertiesPanelController.pivotMode with activePivot so the property is actually referenced.src/PropertiesPanelController.cpp (1)
554-568: Missing Sentry breadcrumb for user-facing pivot mode change.Per coding guidelines, user-facing actions should log Sentry breadcrumbs.
setPivotModeis invoked from QML when the user clicks a pivot button.🔧 Suggested fix
void PropertiesPanelController::setPivotMode(int mode) { if (mode >= TransformOperator::PIVOT_CENTER && mode <= TransformOperator::PIVOT_ORIGIN) { + SentryReporter::addBreadcrumb("ui.action", + QString("Set pivot mode to %1").arg(mode)); TransformOperator::getSingleton()->setPivotMode( static_cast<TransformOperator::PivotMode>(mode)); - emit pivotModeChanged(); } }As per coding guidelines: "Use Sentry breadcrumbs for all user-facing actions and significant operations: 'ui.action' for toolbar/menu clicks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 554 - 568, PropertiesPanelController::setPivotMode currently changes TransformOperator::PivotMode but does not record a Sentry breadcrumb for this user-facing action; add a Sentry breadcrumb when setPivotMode handles a valid mode (before or after calling TransformOperator::getSingleton()->setPivotMode) with category "ui.action", a descriptive message like "pivot_mode_changed" and include the new mode value, then emit pivotModeChanged(); reference the PropertiesPanelController::setPivotMode method and TransformOperator::setPivotMode/ pivotModeChanged signal when making the change.
🤖 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 2143-2160: The current resolver uses mgr->getEntities() and casts
each result to Ogre::Entity*, which can crash on ManualObject entries; instead
reuse the existing findEntityByName(entityName) helper or iterate scene
nodes/attached objects and check obj->getMovableType() == "Entity" before
casting to Ogre::Entity* (and use ent->hasSkeleton() after a safe cast). Update
the branches that reference Manager::getEntities(), the entityName lookup and
the "first entity with a skeleton" search to perform the movable-type check (or
call findEntityByName()) to avoid invalid casts.
- Around line 2170-2182: The current logic infers mode from values and accepts
invalid combinations; change it to explicitly validate presence so exactly one
mode is chosen: check args.contains("target_keyframes") and
args.contains("decimate_step") (use animName/targetKeyframes/decimateStep
context) and return makeErrorResult if neither or both are provided; if
target_keyframes is present validate it >= 2 and set isResample=true, otherwise
validate decimate_step >= 2 and set isResample=false; keep the existing
makeErrorResult messages but ensure they are triggered based on presence checks
rather than inferred numeric defaults.
In `@src/PropertiesPanelController.cpp`:
- Around line 59-61: The connection in the constructor already forwards
TransformOperator::pivotModeChanged to the controller, so remove the redundant
explicit emit pivotModeChanged() calls from the controller methods
setPivotMode() and cyclePivotMode() (cyclePivotMode already calls setPivotMode),
ensuring only the TransformOperator-triggered signal is emitted and preventing
double signals to QML.
In `@src/TransformOperator.cpp`:
- Around line 582-592: updateGizmoPosition() now emits positions relative to the
pivot but setSelectedPosition() still computes edits relative to the selection
center, causing wrong deltas for center/origin pivots; update
setSelectedPosition() to interpret the incoming position as pivot-relative:
compute the world-space targetPosition = pivotPoint +
incomingPivotRelativePosition (using getPivotPoint()), compute delta =
targetPosition - (pivotPoint + currentPosition) or equivalently delta =
(incomingPivotRelativePosition - currentPosition), and apply that delta to the
selection nodes (use SelectionSet::getSingleton()->translateSelectionNodes or
the code path that moves nodes) so typed edits use the same coordinate space as
updateGizmoPosition(); ensure you reference
currentPosition/currentOrientation/currentScale and
SelectionSet::getSelectionNodesCenter() only when computing world transforms.
- Around line 295-317: The center calculation currently only considers objects
attached directly to each SceneNode (using node->numAttachedObjects() and
node->getAttachedObjects()), then falls back to node->getPosition(), which
ignores meshes on child nodes; update the logic in the block around
TransformOperator::... (the sel->hasNodes() loop using sel->getNodesCount(),
sel->getSceneNode(i)) to compute an AABB that includes all descendant attached
objects (traverse node children or use a recursive helper to collect
getWorldBoundingBox(true) from all descendant attached objects and merge into
aabb via aabb.merge(...)) before falling back to node->getPosition(); apply the
same change to the analogous code at the other location mentioned (the branch at
lines 351-379) so grouped nodes’ true bounds are used for center/bottom pivot
modes.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Line 217: The property activePivot is defined as "property int activePivot:
PropertiesPanelController.pivotMode" but never used; either remove the
activePivot definition to avoid dead code or update references (e.g., the color
binding that currently reads PropertiesPanelController.pivotMode) to use
activePivot instead; locate the activePivot property in PropertiesPanel.qml and
either delete that property declaration or replace direct uses of
PropertiesPanelController.pivotMode with activePivot so the property is actually
referenced.
In `@src/PropertiesPanelController.cpp`:
- Around line 554-568: PropertiesPanelController::setPivotMode currently changes
TransformOperator::PivotMode but does not record a Sentry breadcrumb for this
user-facing action; add a Sentry breadcrumb when setPivotMode handles a valid
mode (before or after calling TransformOperator::getSingleton()->setPivotMode)
with category "ui.action", a descriptive message like "pivot_mode_changed" and
include the new mode value, then emit pivotModeChanged(); reference the
PropertiesPanelController::setPivotMode method and
TransformOperator::setPivotMode/ pivotModeChanged signal when making the change.
🪄 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: cd0e158c-3cd0-4575-8651-bf9ee2c916b2
📒 Files selected for processing (8)
qml/PropertiesPanel.qmlsrc/MCPServer.cppsrc/MCPServer.hsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/TransformOperator.cppsrc/TransformOperator.hsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MCPServer.h
Comment on lines
+2143
to
+2160
| QList<Ogre::Entity*> allEntities = mgr->getEntities(); | ||
| if (!entityName.isEmpty()) { | ||
| for (auto* ent : allEntities) { | ||
| if (ent && QString::fromStdString(ent->getName()) == entityName) { | ||
| entity = ent; | ||
| break; | ||
| } | ||
| } | ||
| if (!entity) | ||
| return makeErrorResult(QString("Error: Entity '%1' not found").arg(entityName)); | ||
| } else { | ||
| // Use first entity with a skeleton | ||
| for (auto* ent : allEntities) { | ||
| if (ent && ent->hasSkeleton()) { | ||
| entity = ent; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid Manager::getEntities() in this resolver.
This path can still crash on scenes that contain ManualObjects, because the invalid cast happens before the loop gets to hasSkeleton(). Reuse findEntityByName() or iterate scene nodes/attached objects with a movable-type check here instead.
As per coding guidelines: src/**/*.cpp: Check obj->getMovableType() == "Entity" before casting to Ogre::Entity* in Manager::getEntities() results to avoid crashes from ManualObjects
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer.cpp` around lines 2143 - 2160, The current resolver uses
mgr->getEntities() and casts each result to Ogre::Entity*, which can crash on
ManualObject entries; instead reuse the existing findEntityByName(entityName)
helper or iterate scene nodes/attached objects and check obj->getMovableType()
== "Entity" before casting to Ogre::Entity* (and use ent->hasSkeleton() after a
safe cast). Update the branches that reference Manager::getEntities(), the
entityName lookup and the "first entity with a skeleton" search to perform the
movable-type check (or call findEntityByName()) to avoid invalid casts.
Comment on lines
+2170
to
+2182
| QString animName = args["animation_name"].toString(); | ||
| int targetKeyframes = args["target_keyframes"].toInt(0); | ||
| int decimateStep = args["decimate_step"].toInt(0); | ||
|
|
||
| if (targetKeyframes <= 0 && decimateStep <= 0) | ||
| return makeErrorResult("Error: Specify 'target_keyframes' (>= 2) for resampling or 'decimate_step' (>= 2) for decimation"); | ||
|
|
||
| bool isResample = targetKeyframes >= 2; | ||
|
|
||
| if (isResample && targetKeyframes < 2) | ||
| return makeErrorResult("Error: target_keyframes must be >= 2"); | ||
| if (!isResample && decimateStep < 2) | ||
| return makeErrorResult("Error: decimate_step must be >= 2"); |
There was a problem hiding this comment.
Validate the resample/decimate mode explicitly.
This inference accepts invalid combinations: passing both fields silently picks resampling, and target_keyframes=1 falls into the decimate_step error path. Validate “exactly one of the two” by presence first, then check the chosen value.
🐛 Suggested validation
QString animName = args["animation_name"].toString();
int targetKeyframes = args["target_keyframes"].toInt(0);
int decimateStep = args["decimate_step"].toInt(0);
+ bool hasTargetKeyframes = args.contains("target_keyframes");
+ bool hasDecimateStep = args.contains("decimate_step");
- if (targetKeyframes <= 0 && decimateStep <= 0)
- return makeErrorResult("Error: Specify 'target_keyframes' (>= 2) for resampling or 'decimate_step' (>= 2) for decimation");
-
- bool isResample = targetKeyframes >= 2;
-
- if (isResample && targetKeyframes < 2)
+ if (hasTargetKeyframes == hasDecimateStep)
+ return makeErrorResult("Error: Specify exactly one of 'target_keyframes' or 'decimate_step'");
+ if (hasTargetKeyframes && targetKeyframes < 2)
return makeErrorResult("Error: target_keyframes must be >= 2");
- if (!isResample && decimateStep < 2)
+ if (hasDecimateStep && decimateStep < 2)
return makeErrorResult("Error: decimate_step must be >= 2");
+
+ bool isResample = hasTargetKeyframes;📝 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
| QString animName = args["animation_name"].toString(); | |
| int targetKeyframes = args["target_keyframes"].toInt(0); | |
| int decimateStep = args["decimate_step"].toInt(0); | |
| if (targetKeyframes <= 0 && decimateStep <= 0) | |
| return makeErrorResult("Error: Specify 'target_keyframes' (>= 2) for resampling or 'decimate_step' (>= 2) for decimation"); | |
| bool isResample = targetKeyframes >= 2; | |
| if (isResample && targetKeyframes < 2) | |
| return makeErrorResult("Error: target_keyframes must be >= 2"); | |
| if (!isResample && decimateStep < 2) | |
| return makeErrorResult("Error: decimate_step must be >= 2"); | |
| QString animName = args["animation_name"].toString(); | |
| int targetKeyframes = args["target_keyframes"].toInt(0); | |
| int decimateStep = args["decimate_step"].toInt(0); | |
| bool hasTargetKeyframes = args.contains("target_keyframes"); | |
| bool hasDecimateStep = args.contains("decimate_step"); | |
| if (hasTargetKeyframes == hasDecimateStep) | |
| return makeErrorResult("Error: Specify exactly one of 'target_keyframes' or 'decimate_step'"); | |
| if (hasTargetKeyframes && targetKeyframes < 2) | |
| return makeErrorResult("Error: target_keyframes must be >= 2"); | |
| if (hasDecimateStep && decimateStep < 2) | |
| return makeErrorResult("Error: decimate_step must be >= 2"); | |
| bool isResample = hasTargetKeyframes; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer.cpp` around lines 2170 - 2182, The current logic infers mode
from values and accepts invalid combinations; change it to explicitly validate
presence so exactly one mode is chosen: check args.contains("target_keyframes")
and args.contains("decimate_step") (use animName/targetKeyframes/decimateStep
context) and return makeErrorResult if neither or both are provided; if
target_keyframes is present validate it >= 2 and set isResample=true, otherwise
validate decimate_step >= 2 and set isResample=false; keep the existing
makeErrorResult messages but ensure they are triggered based on presence checks
rather than inferred numeric defaults.
Comment on lines
+59
to
+61
| connect(transformOp, &TransformOperator::pivotModeChanged, this, [this]() { | ||
| emit pivotModeChanged(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if TransformOperator::setPivotMode and cyclePivotMode emit pivotModeChanged signal
ast-grep --pattern $'void TransformOperator::setPivotMode($$$) {
$$$
}'
ast-grep --pattern $'void TransformOperator::cyclePivotMode($$$) {
$$$
}'Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find TransformOperator header and implementation files
fd -t f TransformOperator
# Also search for pivotModeChanged signal definition
rg "pivotModeChanged" -t cpp -t hRepository: fernandotonon/QtMeshEditor
Length of output: 781
🏁 Script executed:
#!/bin/bash
# Read TransformOperator.cpp to find setPivotMode and cyclePivotMode implementations
rg "void TransformOperator::setPivotMode|void TransformOperator::cyclePivotMode" -A 20 src/TransformOperator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1260
🏁 Script executed:
#!/bin/bash
# Read PropertiesPanelController.cpp to see setPivotMode and cyclePivotMode implementations
rg "void PropertiesPanelController::setPivotMode|void PropertiesPanelController::cyclePivotMode" -A 10 src/PropertiesPanelController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 693
Remove redundant emit pivotModeChanged() calls to prevent double signal emissions.
The connection on line 59 already re-emits pivotModeChanged() whenever TransformOperator::pivotModeChanged fires. Both setPivotMode() and cyclePivotMode() explicitly emit the signal again, causing QML bindings to be notified twice per operation. Since cyclePivotMode() internally calls setPivotMode() (which emits via the TransformOperator), the connection handles all cases.
Remove the explicit emit calls in both methods:
Suggested fix
void PropertiesPanelController::setPivotMode(int mode)
{
if (mode >= TransformOperator::PIVOT_CENTER && mode <= TransformOperator::PIVOT_ORIGIN)
{
TransformOperator::getSingleton()->setPivotMode(
static_cast<TransformOperator::PivotMode>(mode));
- emit pivotModeChanged();
}
}
void PropertiesPanelController::cyclePivotMode()
{
TransformOperator::getSingleton()->cyclePivotMode();
- emit pivotModeChanged();
}🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 61-61: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PropertiesPanelController.cpp` around lines 59 - 61, The connection in
the constructor already forwards TransformOperator::pivotModeChanged to the
controller, so remove the redundant explicit emit pivotModeChanged() calls from
the controller methods setPivotMode() and cyclePivotMode() (cyclePivotMode
already calls setPivotMode), ensuring only the TransformOperator-triggered
signal is emitted and preventing double signals to QML.
Comment on lines
+295
to
+317
| // True geometric center of bounding box | ||
| if (sel->hasNodes()) | ||
| { | ||
| Ogre::Vector3 center = Ogre::Vector3::ZERO; | ||
| for (int i = 0; i < sel->getNodesCount(); ++i) | ||
| { | ||
| Ogre::SceneNode* node = sel->getSceneNode(i); | ||
| // Use world bounding box center if entities are attached, | ||
| // otherwise use node position | ||
| if (node->numAttachedObjects() > 0) | ||
| { | ||
| Ogre::AxisAlignedBox aabb; | ||
| for (auto& obj : node->getAttachedObjects()) | ||
| aabb.merge(obj->getWorldBoundingBox(true)); | ||
| if (aabb.isFinite()) | ||
| { | ||
| center += aabb.getCenter(); | ||
| continue; | ||
| } | ||
| } | ||
| center += node->getPosition(); | ||
| } | ||
| return center / static_cast<Ogre::Real>(sel->getNodesCount()); |
There was a problem hiding this comment.
Group nodes collapse to origin in center/bottom pivot modes.
These branches only look at objects attached directly to the selected node. A group node created by this PR has children but no attached mesh, so both modes fall back to node->getPosition() and ignore the grouped contents entirely. Merge descendant bounds before falling back to the node origin.
Also applies to: 351-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator.cpp` around lines 295 - 317, The center calculation
currently only considers objects attached directly to each SceneNode (using
node->numAttachedObjects() and node->getAttachedObjects()), then falls back to
node->getPosition(), which ignores meshes on child nodes; update the logic in
the block around TransformOperator::... (the sel->hasNodes() loop using
sel->getNodesCount(), sel->getSceneNode(i)) to compute an AABB that includes all
descendant attached objects (traverse node children or use a recursive helper to
collect getWorldBoundingBox(true) from all descendant attached objects and merge
into aabb via aabb.merge(...)) before falling back to node->getPosition(); apply
the same change to the analogous code at the other location mentioned (the
branch at lines 351-379) so grouped nodes’ true bounds are used for
center/bottom pivot modes.
Comment on lines
+582
to
+592
| Ogre::Vector3 pivotPoint = getPivotPoint(); | ||
| currentPosition = pivotPoint; | ||
| m_pTransformNode->setPosition(pivotPoint); | ||
| } | ||
| else if(SelectionSet::getSingleton()->hasEntities()) | ||
| { | ||
| currentPosition = SelectionSet::getSingleton()->getSelectionCenter()-SelectionSet::getSingleton()->getSelectionNodesCenter(); | ||
| currentOrientation = SelectionSet::getSingleton()->getSelectionOrientation(); | ||
| currentScale = SelectionSet::getSingleton()->getSelectionScale(); | ||
| m_pTransformNode->setPosition(currentPosition + SelectionSet::getSingleton()->getSelectionNodesCenter()); | ||
| Ogre::Vector3 pivotPoint = getPivotPoint(); | ||
| currentPosition = pivotPoint - SelectionSet::getSingleton()->getSelectionNodesCenter(); | ||
| m_pTransformNode->setPosition(pivotPoint); |
There was a problem hiding this comment.
Keep emitted position and edited position in the same coordinate space.
updateGizmoPosition() now publishes pivot-relative positions, but setSelectedPosition() still translates from the old selection center. With center or origin pivots, typed position edits will move the selection by the wrong delta.
🐛 Follow-up fix in setSelectedPosition()
void TransformOperator::setSelectedPosition(const Ogre::Vector3& newPosition)
{
if(SelectionSet::getSingleton()->hasNodes())
{
- Ogre::Vector3 translation = newPosition - SelectionSet::getSingleton()->getSelectionCenter();
+ Ogre::Vector3 translation = newPosition - getPivotPoint();
translateSelected(translation);
}
else if(SelectionSet::getSingleton()->hasEntities())
{
- Ogre::Vector3 translation = newPosition - SelectionSet::getSingleton()->getSelectionCenter()+SelectionSet::getSingleton()->getSelectionNodesCenter();
+ Ogre::Vector3 translation =
+ (newPosition + SelectionSet::getSingleton()->getSelectionNodesCenter()) - getPivotPoint();
translateSelected(translation);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator.cpp` around lines 582 - 592, updateGizmoPosition() now
emits positions relative to the pivot but setSelectedPosition() still computes
edits relative to the selection center, causing wrong deltas for center/origin
pivots; update setSelectedPosition() to interpret the incoming position as
pivot-relative: compute the world-space targetPosition = pivotPoint +
incomingPivotRelativePosition (using getPivotPoint()), compute delta =
targetPosition - (pivotPoint + currentPosition) or equivalently delta =
(incomingPivotRelativePosition - currentPosition), and apply that delta to the
selection nodes (use SelectionSet::getSingleton()->translateSelectionNodes or
the code path that moves nodes) so typed edits use the same coordinate space as
updateGizmoPosition(); ensure you reference
currentPosition/currentOrientation/currentScale and
SelectionSet::getSelectionNodesCenter() only when computing world transforms.
Drag scene nodes in the tree to reparent them under other nodes. World-space transforms are preserved by recomputing local transforms. Core: - Manager::reparentNode(): removes from old parent, adds to new parent, recalculates local transform to preserve world position. Validates against cycles (node into own subtree). - Manager::isDescendantOf(): static cycle detection helper - ReparentCommand: stores old/new parent names and local transforms for full undo/redo - SceneTreeModel: canReparent() validation, reparentNode() with undo - SceneTreeNode.qml: Drag/DropArea on Node and Group items, drag proxy label, blue highlight on valid drop targets - PropertiesPanel.qml: "Scene (Root)" drop zone for reparenting to root - MCP tool: reparent_node (node_name, new_parent_name) - Sentry breadcrumbs Part of #256 (Phase 1: Scene Editing Power Tools) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QML Drag.Internal mode doesn't pass mimeData to DropArea handlers. Replaced with a draggedNodeName property on PropertiesPanelController that's set on mouse press and read by DropAreas on enter/drop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MCPServer.h (1)
153-175:⚠️ Potential issue | 🟠 MajorBump
SERVER_VERSIONwith these new MCP tools.These declarations expand the MCP protocol surface, but the protocol version stays unchanged. MCP clients lose a reliable way to detect the new contract and gate tool usage safely.
As per coding guidelines,
src/MCPServer.{h,cpp}:MCPServer.h has a separate SERVER_VERSION ("1.0.0") for the MCP protocol; only bump SERVER_VERSION if the MCP interface changes, not for application version changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer.h` around lines 153 - 175, New MCP tool RPC methods were added (e.g., toolResampleAnimation, toolSaveScene, toolGenerateLods, toolGetCameraInfo, toolSetSnapSettings, toolExportPose, toolGroupNodes, etc.), which is a protocol change and requires bumping the MCP protocol version; update the SERVER_VERSION constant in MCPServer.h (and the corresponding definition/usage in MCPServer.cpp) to a new semantic version (e.g., from "1.0.0" to "1.1.0"), and ensure any codepaths that expose or check SERVER_VERSION (tests, client compatibility checks) are updated to the new value so clients can detect the expanded MCP interface.
🧹 Nitpick comments (1)
qml/SceneTreeNode.qml (1)
17-18: Expose TypeLabelRole to QML instead of hard-coding role259.The
SceneTreeModel::Rolesenum lacksQ_ENUMannotation, so the role constant is not accessible in QML. Hard-coding259(which equalsQt::UserRole + 3) couples the QML to the current enum ordering and will silently break if roles are reordered. AddQ_ENUM(Roles)toSceneTreeModelclass declaration or create a dedicated Q_PROPERTY that exposes the role ID.All occurrences at lines 18, 183, 194, 212, and 224 use this value to fetch
TypeLabelRole.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/SceneTreeNode.qml` around lines 17 - 18, The QML is hard-coded to role ID 259 when checking TypeLabelRole which will break if enum ordering changes; update the C++ side by adding Q_ENUM(Roles) to the SceneTreeModel::Roles enum (or add a Q_PROPERTY<int> TypeLabelRole exposing the role id) so QML can reference the role symbol instead of the magic number, then replace usages in QML (e.g., the isNodeType property and other treeModel.data(...) calls) to use the exposed SceneTreeModel.TypeLabelRole constant.
🤖 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/SceneTreeNode.qml`:
- Around line 80-88: The drop handler currently sets drop.accepted based on
cached canDrop instead of the actual result from
PropertiesPanelController.reparentNode; change onDropped to call
reparentNode(draggedName, treeNode.nodeName), capture its return (handle both
synchronous boolean or a Promise), and only set drop.accepted = true when that
call indicates success (and set drop.accepted = false and hide/clear
dropHighlight when it fails); reference onDropped, drop.accepted,
PropertiesPanelController.reparentNode, canDrop and dropHighlight to locate and
update the logic.
In `@src/commands/TransformCommands.h`:
- Around line 108-187: The header declares std::string members in GroupCommand
(mGroupNodeName, NodeParentInfo::nodeName/oldParentName), UngroupCommand
(mGroupNodeName, mGroupParentName, ChildInfo::childName) and ReparentCommand
(mNodeName, mOldParentName, mNewParentName) but doesn't include <string>; add a
direct `#include` <string> at the top of this header to avoid relying on
transitive includes and ensure these symbols compile reliably.
---
Outside diff comments:
In `@src/MCPServer.h`:
- Around line 153-175: New MCP tool RPC methods were added (e.g.,
toolResampleAnimation, toolSaveScene, toolGenerateLods, toolGetCameraInfo,
toolSetSnapSettings, toolExportPose, toolGroupNodes, etc.), which is a protocol
change and requires bumping the MCP protocol version; update the SERVER_VERSION
constant in MCPServer.h (and the corresponding definition/usage in
MCPServer.cpp) to a new semantic version (e.g., from "1.0.0" to "1.1.0"), and
ensure any codepaths that expose or check SERVER_VERSION (tests, client
compatibility checks) are updated to the new value so clients can detect the
expanded MCP interface.
---
Nitpick comments:
In `@qml/SceneTreeNode.qml`:
- Around line 17-18: The QML is hard-coded to role ID 259 when checking
TypeLabelRole which will break if enum ordering changes; update the C++ side by
adding Q_ENUM(Roles) to the SceneTreeModel::Roles enum (or add a Q_PROPERTY<int>
TypeLabelRole exposing the role id) so QML can reference the role symbol instead
of the magic number, then replace usages in QML (e.g., the isNodeType property
and other treeModel.data(...) calls) to use the exposed
SceneTreeModel.TypeLabelRole constant.
🪄 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: 767071da-ef2b-4467-814c-3adeb62b0d60
📒 Files selected for processing (12)
qml/PropertiesPanel.qmlqml/SceneTreeNode.qmlsrc/MCPServer.cppsrc/MCPServer.hsrc/Manager.cppsrc/Manager.hsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/SceneTreeModel.cppsrc/SceneTreeModel.hsrc/commands/TransformCommands.cppsrc/commands/TransformCommands.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SceneTreeModel.cpp
- src/PropertiesPanelController.h
👮 Files not reviewed due to content moderation or server errors (3)
- src/MCPServer.cpp
- src/commands/TransformCommands.cpp
- src/Manager.cpp
Comment on lines
+108
to
+187
| // Group selected nodes under a new parent node | ||
| class GroupCommand : public QUndoCommand | ||
| { | ||
| public: | ||
| GroupCommand(const QList<Ogre::SceneNode*>& nodes, | ||
| QUndoCommand* parent = nullptr); | ||
|
|
||
| void undo() override; | ||
| void redo() override; | ||
|
|
||
| private: | ||
| struct NodeParentInfo { | ||
| std::string nodeName; | ||
| std::string oldParentName; // empty string means root scene node | ||
| Ogre::Vector3 oldPosition; | ||
| Ogre::Quaternion oldOrientation; | ||
| Ogre::Vector3 oldScale; | ||
| }; | ||
| QList<NodeParentInfo> mNodeInfos; | ||
| std::string mGroupNodeName; | ||
| Ogre::Vector3 mGroupPosition; | ||
| bool mFirstRedo = true; | ||
| }; | ||
|
|
||
| // Ungroup: move children to group's parent, destroy group node | ||
| class UngroupCommand : public QUndoCommand | ||
| { | ||
| public: | ||
| UngroupCommand(Ogre::SceneNode* groupNode, | ||
| QUndoCommand* parent = nullptr); | ||
|
|
||
| void undo() override; | ||
| void redo() override; | ||
|
|
||
| private: | ||
| struct ChildInfo { | ||
| std::string childName; | ||
| Ogre::Vector3 localPosition; | ||
| Ogre::Quaternion localOrientation; | ||
| Ogre::Vector3 localScale; | ||
| }; | ||
| std::string mGroupNodeName; | ||
| std::string mGroupParentName; // empty string means root scene node | ||
| Ogre::Vector3 mGroupPosition; | ||
| Ogre::Quaternion mGroupOrientation; | ||
| Ogre::Vector3 mGroupScale; | ||
| QList<ChildInfo> mChildInfos; | ||
| bool mFirstRedo = true; | ||
| }; | ||
|
|
||
| // Reparent a scene node under a new parent, preserving world-space transform | ||
| class ReparentCommand : public QUndoCommand | ||
| { | ||
| public: | ||
| ReparentCommand(const QString& nodeName, | ||
| const QString& oldParentName, | ||
| const QString& newParentName, | ||
| const Ogre::Vector3& oldLocalPos, | ||
| const Ogre::Quaternion& oldLocalOrient, | ||
| const Ogre::Vector3& oldLocalScale, | ||
| const Ogre::Vector3& newLocalPos, | ||
| const Ogre::Quaternion& newLocalOrient, | ||
| const Ogre::Vector3& newLocalScale, | ||
| QUndoCommand* parent = nullptr); | ||
|
|
||
| void undo() override; | ||
| void redo() override; | ||
|
|
||
| private: | ||
| QString mNodeName; | ||
| QString mOldParentName; | ||
| QString mNewParentName; | ||
| Ogre::Vector3 mOldLocalPos; | ||
| Ogre::Quaternion mOldLocalOrient; | ||
| Ogre::Vector3 mOldLocalScale; | ||
| Ogre::Vector3 mNewLocalPos; | ||
| Ogre::Quaternion mNewLocalOrient; | ||
| Ogre::Vector3 mNewLocalScale; | ||
| bool mFirstRedo = true; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that TransformCommands.h uses std::string without including <string>.
sed -n '1,20p' src/commands/TransformCommands.h
echo
rg -n 'std::string' src/commands/TransformCommands.hRepository: fernandotonon/QtMeshEditor
Length of output: 767
Add the missing <string> include.
The new GroupCommand, UngroupCommand, and ReparentCommand classes use std::string members (lines 120, 121, 127, 144, 149, 150) but the header doesn't include <string> itself. This creates a brittle dependency on transitive includes.
Suggested fix
`#include` <QUndoCommand>
`#include` <QList>
+#include <string>
`#include` <vector>
`#include` <OgreVector.h>
`#include` <OgreQuaternion.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/TransformCommands.h` around lines 108 - 187, The header declares
std::string members in GroupCommand (mGroupNodeName,
NodeParentInfo::nodeName/oldParentName), UngroupCommand (mGroupNodeName,
mGroupParentName, ChildInfo::childName) and ReparentCommand (mNodeName,
mOldParentName, mNewParentName) but doesn't include <string>; add a direct
`#include` <string> at the top of this header to avoid relying on transitive
includes and ensure these symbols compile reliably.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 174-179: In the onDropped handler, use the boolean result from
PropertiesPanelController.reparentNode(...) to determine whether to accept the
drop instead of unconditionally setting drop.accepted; specifically, call
PropertiesPanelController.reparentNode(draggedName, "root"), capture its
returned success value, and set drop.accepted = success only when draggedName
and canDrop are true, while still hiding rootDropHighlight; refer to the
onDropped handler, PropertiesPanelController.draggedNodeName, canDrop,
PropertiesPanelController.reparentNode, rootDropHighlight, and drop.accepted
when making the change.
🪄 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: 9902ce4b-9d1e-45e7-84f4-88a6932f74d8
📒 Files selected for processing (3)
qml/PropertiesPanel.qmlqml/SceneTreeNode.qmlsrc/PropertiesPanelController.h
🚧 Files skipped from review as they are similar to previous changes (2)
- qml/SceneTreeNode.qml
- src/PropertiesPanelController.h
QML Drag/DropArea in QQuickWidget inside QDockWidget doesn't deliver events properly across recursive tree components. Replaced with: - "↑ to Root" button next to Scene (Root) header, visible when a nested node is selected - Grouping (Ctrl+G) for creating parent-child relationships - MCP reparent_node tool for programmatic access Removed all Drag/DropArea code from SceneTreeNode.qml and the root drop zone from PropertiesPanel.qml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the last child is moved out of a group via reparentNode(), the empty parent is now automatically destroyed instead of persisting as an orphaned "Node" in the scene tree. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When moving the last child out of a nested group, now walks up the parent chain and destroys ALL empty ancestors (not just the immediate parent). Fixes nested groups leaving orphaned empty nodes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Version 2.22.0 → 2.23.0 - 14 new tests: group_nodes, ungroup_node, reparent_node, set/get_pivot_mode, resample_animation (valid + error cases) - AllToolNamesAreRecognized updated to 51 tools - Winget fix already included (gh CLI approach from PR #254) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/Manager.cpp (1)
423-428:⚠️ Potential issue | 🟠 MajorProtect transform restoration from zero-scale ancestors.
All three code paths divide by derived scale components. A flattened parent/group (
scale.x/y/z == 0) will injectInf/NaNinto local position/scale during group, ungroup, or reparent, which corrupts the subtree. This is the same issue previously raised in the grouping path and it still exists here.🔧 Suggested pattern
+ auto safeScale = [](const Ogre::Vector3& s) { + constexpr Ogre::Real eps = 1e-6f; + return Ogre::Vector3( + std::abs(s.x) < eps ? eps : s.x, + std::abs(s.y) < eps ? eps : s.y, + std::abs(s.z) < eps ? eps : s.z); + }; + Ogre::Vector3 groupWorldScale = groupNode->_getDerivedScale(); + const Ogre::Vector3 safeGroupWorldScale = safeScale(groupWorldScale); Ogre::Vector3 groupDerivedPos = groupNode->_getDerivedPosition(); node->setOrientation(groupWorldOrient.Inverse() * worldOrient); - node->setScale(worldScale / groupWorldScale); + node->setScale(worldScale / safeGroupWorldScale); node->setPosition(groupWorldOrient.Inverse() * - ((worldPos - groupDerivedPos) / groupWorldScale)); + ((worldPos - groupDerivedPos) / safeGroupWorldScale));Also applies to: 451-454, 490-497, 563-570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Manager.cpp` around lines 423 - 428, The code computes local transforms by dividing by derived scale (parentWorldScale/groupWorldScale) which can be zero and produce Inf/NaN; update the transform-restoration logic around commonParent/_getDerivedScale() and any similar blocks (the groupLocalPos computation, and the other locations noted around lines 451-454, 490-497, 563-570) to guard against zero or near-zero scale components by testing each parentWorldScale (and groupWorldScale where used) against a small epsilon and substituting a safe non-zero value (e.g. 1.0 or epsilon) or using component-wise conditional division so you never divide by zero, then recompute groupLocalPos/groupLocalScale using those safe values so zeros in ancestor scales cannot corrupt the subtree.
🧹 Nitpick comments (2)
src/MCPServer_test.cpp (2)
5715-5739: These pivot-mode tests can pass without exercising the pivot logic.Both cases allow the generic
"not initialized"path, andSetPivotMode_Validonly checks for non-empty text. That means a broken parser or wrong success/error message can still pass here. If you want functional coverage, initializeTransformOperatorin this fixture; otherwise split the initialization failure into separate tests and assert the concrete expected strings in these ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 5715 - 5739, The pivot-mode tests (SetPivotMode_Valid, SetPivotMode_Invalid, GetPivotMode_ReturnsCurrentMode) are passing via the generic "not initialized" path and don't exercise TransformOperator; either initialize TransformOperator in the MCPServerTest fixture (so calls to server->callTool("set_pivot_mode"/"get_pivot_mode") exercise real pivot parsing/logic) or split the cases: add separate tests that explicitly simulate the "not initialized" error and make these three tests assert concrete expected responses (e.g., exact success message or specific "Invalid pivot mode" text) rather than only checking non-empty text; touch the MCPServerTest setup where TransformOperator is constructed/registered and update the expectations in SetPivotMode_Valid, SetPivotMode_Invalid, and GetPivotMode_ReturnsCurrentMode accordingly.
5746-5768: The resample tests don't currently validate resampling behavior.
ResampleAnimation_MissingArgscan fail before argument validation because there is no animated entity in scene, andResampleAnimation_Validaccepts any non-empty text even on error. These assertions won't catch a brokenresample_animationimplementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer_test.cpp` around lines 5746 - 5768, The tests are too weak: ResampleAnimation_MissingArgs may fail due to missing animated entity before argument validation, and ResampleAnimation_Valid only checks for non-empty text rather than correct behavior. For ResampleAnimation_MissingArgs, create an animated test entity (use createAnimatedTestEntity) before calling server->callTool("resample_animation", args) so the call reaches argument validation and then assert isError(result) and a helpful error text. For ResampleAnimation_Valid, ensure the test creates/uses the animated entity, assert that isError(result) is false for valid args (server->callTool("resample_animation", args)), and then verify the resample actually took place by inspecting the entity's animation/skeleton (use the returned Ogre::Entity* from createAnimatedTestEntity and check its animation keyframe count or other skeleton/animation state) rather than only checking getResultText(result).
🤖 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 117-145: The current Row positioner ignores the Layout.fillWidth
spacer so the "↑ to Root" button won't right-align; replace the Row with an Item
(or switch to RowLayout from QtQuick.Layouts) and use explicit anchors to
position children: keep the container width = outlinerColumn.width and height =
22, anchor the Text (the label with id toRootText's sibling) to the left
(anchors.left/verticalCenter), anchor the "Move to Root" Rectangle to the right
(anchors.right/verticalCenter) and remove the Layout.fillWidth spacer Item, and
ensure the Rectangle still references toRootMa for hover and calls
PropertiesPanelController.reparentNode(PropertiesPanelController.selectionName,
"root") on click so behavior remains unchanged.
In `@src/Manager.cpp`:
- Around line 405-418: The current grouping uses the parent of the first node
(commonParent) which breaks undo/ungroup for mixed-parent selections; change
GroupNodes behavior to detect if all items in nodes share the same parent
(iterate nodes and compare each getParent() to nodes.first()->getParent()) and
if they do proceed as now, otherwise either reject the operation with a clear
error/message to the user or create the group while preserving original parents
(i.e., create per-parent subgroup nodes or store each node's original parent and
restore them in ungroupNode()); update logic referencing commonParent,
groupName, groupNode and ensure ungroupNode() uses the stored original parent
references to restore the exact hierarchy.
In `@src/MCPServer_test.cpp`:
- Around line 5669-5672: The test calls to the RPC "reparent_node" use wrong
parameter names; update the QJsonObject keys to match the server's expected
names "child" and "parent" (not "node_name" / "new_parent_name") so
toolReparentNode() receives the correct fields; change the three occurrences
(the call around lines 5669, 5693, 5703) to set args["child"] and args["parent"]
accordingly so the tests exercise valid, cycle-prevention, and nonexistent-node
behavior.
- Around line 5625-5627: The tests call the ungroup tool with args["name"] but
toolUngroupNode() expects args["node"], so update the test invocations that
create QJsonObject args and call server->callTool("ungroup_node", args) to set
args["node"] = groupName (and similarly replace args["name"] with args["node"]
in the other test at the second occurrence) so the tests exercise the success
and "not a group node" branches rather than the missing-parameter path.
---
Duplicate comments:
In `@src/Manager.cpp`:
- Around line 423-428: The code computes local transforms by dividing by derived
scale (parentWorldScale/groupWorldScale) which can be zero and produce Inf/NaN;
update the transform-restoration logic around commonParent/_getDerivedScale()
and any similar blocks (the groupLocalPos computation, and the other locations
noted around lines 451-454, 490-497, 563-570) to guard against zero or near-zero
scale components by testing each parentWorldScale (and groupWorldScale where
used) against a small epsilon and substituting a safe non-zero value (e.g. 1.0
or epsilon) or using component-wise conditional division so you never divide by
zero, then recompute groupLocalPos/groupLocalScale using those safe values so
zeros in ancestor scales cannot corrupt the subtree.
---
Nitpick comments:
In `@src/MCPServer_test.cpp`:
- Around line 5715-5739: The pivot-mode tests (SetPivotMode_Valid,
SetPivotMode_Invalid, GetPivotMode_ReturnsCurrentMode) are passing via the
generic "not initialized" path and don't exercise TransformOperator; either
initialize TransformOperator in the MCPServerTest fixture (so calls to
server->callTool("set_pivot_mode"/"get_pivot_mode") exercise real pivot
parsing/logic) or split the cases: add separate tests that explicitly simulate
the "not initialized" error and make these three tests assert concrete expected
responses (e.g., exact success message or specific "Invalid pivot mode" text)
rather than only checking non-empty text; touch the MCPServerTest setup where
TransformOperator is constructed/registered and update the expectations in
SetPivotMode_Valid, SetPivotMode_Invalid, and GetPivotMode_ReturnsCurrentMode
accordingly.
- Around line 5746-5768: The tests are too weak: ResampleAnimation_MissingArgs
may fail due to missing animated entity before argument validation, and
ResampleAnimation_Valid only checks for non-empty text rather than correct
behavior. For ResampleAnimation_MissingArgs, create an animated test entity (use
createAnimatedTestEntity) before calling server->callTool("resample_animation",
args) so the call reaches argument validation and then assert isError(result)
and a helpful error text. For ResampleAnimation_Valid, ensure the test
creates/uses the animated entity, assert that isError(result) is false for valid
args (server->callTool("resample_animation", args)), and then verify the
resample actually took place by inspecting the entity's animation/skeleton (use
the returned Ogre::Entity* from createAnimatedTestEntity and check its animation
keyframe count or other skeleton/animation state) rather than only checking
getResultText(result).
🪄 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: ad51346f-79f2-42d1-8ce8-4456d2fd9a93
📒 Files selected for processing (5)
CMakeLists.txtqml/PropertiesPanel.qmlqml/SceneTreeNode.qmlsrc/MCPServer_test.cppsrc/Manager.cpp
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
Comment on lines
+117
to
+145
| // Scene header with reparent button | ||
| Row { | ||
| width: outlinerColumn.width | ||
| height: 22 | ||
| spacing: 4 | ||
|
|
||
| Text { | ||
| text: "\u25A1 Scene (Root)" | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 11; font.bold: true | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| leftPadding: 4 | ||
| } | ||
|
|
||
| Item { width: 1; height: 1; Layout.fillWidth: true } | ||
|
|
||
| // "Move to Root" button — visible when a non-root node is selected | ||
| Rectangle { | ||
| visible: PropertiesPanelController.selectionName !== "" && | ||
| PropertiesPanelController.canReparentNode(PropertiesPanelController.selectionName, "root") | ||
| width: toRootText.implicitWidth + 10; height: 18; radius: 3 | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| color: toRootMa.containsMouse ? PropertiesPanelController.highlightColor : PropertiesPanelController.headerColor | ||
| border.color: PropertiesPanelController.borderColor; border.width: 1 | ||
| Text { id: toRootText; anchors.centerIn: parent; text: "\u2191 to Root"; color: PropertiesPanelController.textColor; font.pixelSize: 9 } | ||
| MouseArea { id: toRootMa; anchors.fill: parent; hoverEnabled: true; cursorShape: Qt.PointingHandCursor | ||
| onClicked: PropertiesPanelController.reparentNode(PropertiesPanelController.selectionName, "root") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the relevant section of the file to see the current state
cat -n qml/PropertiesPanel.qml | sed -n '110,155p'Repository: fernandotonon/QtMeshEditor
Length of output: 2657
🏁 Script executed:
# Let's also check if there are any imports related to Layouts in the file
head -30 qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 810
🌐 Web query:
Qt Quick Row Layout.fillWidth behavior - does Row support Layout attached properties
💡 Result:
No. Row (QtQuick) is a positioner, not part of the Qt Quick Layouts system, so it does not honor Layout.* attached properties like Layout.fillWidth. Row just positions children; any “fill” behavior must be done via explicit width bindings/anchors (or spacers you size yourself). Row provides a different attached object (Positioner) for per-child positioning info, not Layout. [2]
If you want Layout.fillWidth / Layout.fillHeight behavior, use RowLayout (QtQuick.Layouts). In a RowLayout, child items support Layout.fillWidth and other Layout.* attached properties. [1]
Practical rule:
- Use
RowLayoutwhen you want automatic resizing viaLayout.*. [1] - Use
Rowwhen you only want simple positioning and will manage sizes yourself. [2]
Sources: RowLayout docs [1], Positioner/Row docs [2].
Layout.fillWidth won't work in Row—button won't right-align as intended.
Row is a positioner, not part of Qt Quick Layouts, so it ignores Layout.* attached properties. The spacer at line 131 won't expand, and the "↑ to Root" button stays packed next to the label.
Replace Row with Item and use explicit anchoring (or switch to RowLayout from QtQuick.Layouts):
Proposed fix
- Row {
+ Item {
width: outlinerColumn.width
height: 22
- spacing: 4
Text {
+ id: sceneRootLabel
text: "\u25A1 Scene (Root)"
color: PropertiesPanelController.textColor
font.pixelSize: 11; font.bold: true
+ anchors.left: parent.left
+ anchors.leftMargin: 4
anchors.verticalCenter: parent.verticalCenter
- leftPadding: 4
}
- Item { width: 1; height: 1; Layout.fillWidth: true }
-
// "Move to Root" button — visible when a non-root node is selected
Rectangle {
+ anchors.right: parent.right
visible: PropertiesPanelController.selectionName !== "" &&
PropertiesPanelController.canReparentNode(PropertiesPanelController.selectionName, "root")
width: toRootText.implicitWidth + 10; height: 18; radius: 3
anchors.verticalCenter: parent.verticalCenter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/PropertiesPanel.qml` around lines 117 - 145, The current Row positioner
ignores the Layout.fillWidth spacer so the "↑ to Root" button won't right-align;
replace the Row with an Item (or switch to RowLayout from QtQuick.Layouts) and
use explicit anchors to position children: keep the container width =
outlinerColumn.width and height = 22, anchor the Text (the label with id
toRootText's sibling) to the left (anchors.left/verticalCenter), anchor the
"Move to Root" Rectangle to the right (anchors.right/verticalCenter) and remove
the Layout.fillWidth spacer Item, and ensure the Rectangle still references
toRootMa for hover and calls
PropertiesPanelController.reparentNode(PropertiesPanelController.selectionName,
"root") on click so behavior remains unchanged.
Comment on lines
+405
to
+418
| // Find a common parent — use the parent of the first node | ||
| Ogre::SceneNode* commonParent = static_cast<Ogre::SceneNode*>(nodes.first()->getParent()); | ||
| if (!commonParent) | ||
| commonParent = mSceneMgr->getRootSceneNode(); | ||
|
|
||
| // Create the group node under the common parent | ||
| QString baseName = "Group"; | ||
| unsigned int number = 0; | ||
| while (mSceneMgr->hasSceneNode(QString(baseName + (number ? QString::number(number) : "")).toStdString())) | ||
| ++number; | ||
|
|
||
| QString groupName = baseName + (number ? QString::number(number) : ""); | ||
| Ogre::SceneNode* groupNode = commonParent->createChildSceneNode(groupName.toStdString()); | ||
|
|
There was a problem hiding this comment.
Reject mixed-parent selections or preserve original parents.
commonParent here is just the first node's parent, not a true common parent. If the selection spans multiple parents, grouping moves everything under that first parent, and ungroupNode() can only restore them there, so group → ungroup (and undo of group) stops being an inverse operation and silently rewrites the hierarchy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Manager.cpp` around lines 405 - 418, The current grouping uses the parent
of the first node (commonParent) which breaks undo/ungroup for mixed-parent
selections; change GroupNodes behavior to detect if all items in nodes share the
same parent (iterate nodes and compare each getParent() to
nodes.first()->getParent()) and if they do proceed as now, otherwise either
reject the operation with a clear error/message to the user or create the group
while preserving original parents (i.e., create per-parent subgroup nodes or
store each node's original parent and restore them in ungroupNode()); update
logic referencing commonParent, groupName, groupNode and ensure ungroupNode()
uses the stored original parent references to restore the exact hierarchy.
Comment on lines
+5625
to
+5627
| QJsonObject args; | ||
| args["name"] = groupName; | ||
| QJsonObject result = server->callTool("ungroup_node", args); |
There was a problem hiding this comment.
Use the tool's node argument in the ungroup tests.
toolUngroupNode() reads args["node"], not args["name"]. As written, both tests are only exercising the "missing parameter" path, so they won't cover either the success case or the "not a group node" branch.
🧪 Minimal fix
- args["name"] = groupName;
+ args["node"] = groupName;
...
- args["name"] = "UngroupNotGroupNode";
+ args["node"] = "UngroupNotGroupNode";Also applies to: 5646-5648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 5625 - 5627, The tests call the ungroup
tool with args["name"] but toolUngroupNode() expects args["node"], so update the
test invocations that create QJsonObject args and call
server->callTool("ungroup_node", args) to set args["node"] = groupName (and
similarly replace args["name"] with args["node"] in the other test at the second
occurrence) so the tests exercise the success and "not a group node" branches
rather than the missing-parameter path.
Comment on lines
+5669
to
+5672
| QJsonObject args; | ||
| args["node_name"] = "ReparentChild"; | ||
| args["new_parent_name"] = "ReparentParent"; | ||
| QJsonObject result = server->callTool("reparent_node", args); |
There was a problem hiding this comment.
Match the reparent_node parameter names used by the server.
toolReparentNode() expects child and parent. Using node_name / new_parent_name means all three tests stop at parameter validation and never exercise the valid, cycle-prevention, or nonexistent-node behavior they claim to cover.
🧪 Minimal fix
- args["node_name"] = "ReparentChild";
- args["new_parent_name"] = "ReparentParent";
+ args["child"] = "ReparentChild";
+ args["parent"] = "ReparentParent";
...
- args["node_name"] = "CycleParent";
- args["new_parent_name"] = "CycleChild";
+ args["child"] = "CycleParent";
+ args["parent"] = "CycleChild";
...
- args["node_name"] = "NonexistentReparentNode";
- args["new_parent_name"] = "root";
+ args["child"] = "NonexistentReparentNode";
+ args["parent"] = "root";Also applies to: 5693-5696, 5703-5706
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 5669 - 5672, The test calls to the RPC
"reparent_node" use wrong parameter names; update the QJsonObject keys to match
the server's expected names "child" and "parent" (not "node_name" /
"new_parent_name") so toolReparentNode() receives the correct fields; change the
three occurrences (the call around lines 5669, 5693, 5703) to set args["child"]
and args["parent"] accordingly so the tests exercise valid, cycle-prevention,
and nonexistent-node behavior.
|
12 tasks
This was referenced Apr 10, 2026
5 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
All remaining Phase 1 items from #256 (Scene Editing Power Tools).
Completed
qtmesh anim --resample 30/--decimate-step 5, MCP tool, 8 testsPhase 1 now 100% complete (9/9 items)
Test plan
qtmesh anim model.fbx --resample 30 -o out.fbxreduces keyframesSummary by CodeRabbit
New Features
Documentation
Tests