UX Redesign: QML Inspector, Scale Gizmo, Undo/Redo#210
Merged
fernandotonon merged 8 commits intomasterfrom Mar 23, 2026
Merged
Conversation
Major UI modernization replacing the old 4-tab sidebar (Transform/Material/Edit/Animation) with a unified QML Inspector panel featuring collapsible sections, and adding several new features for a more professional 3D editor experience. Phase 1 - Complete Transform Tools: - Scale gizmo (ScaleGizmo.h/cpp) with cube handles at axis endpoints - Local/World transform space toggle (X key) - Unity-convention keyboard shortcuts: Q=Select, W=Translate, E=Rotate, R=Scale, F=Frame - SpaceCamera::frameSelection() zooms camera to fit selected objects Phase 2 - Scene Undo/Redo: - UndoManager singleton wrapping QUndoStack - TranslateCommand, RotateCommand, ScaleCommand, DeleteCommand - Ctrl+Z/Ctrl+Shift+Z with Edit menu - Node validity checks prevent crashes after deletion Phase 3 - QML Inspector Panel: - SceneTreeModel (QAbstractItemModel) with hierarchical Node/Mesh/Submesh tree - Recursive SceneTreeNode.qml with expand/collapse, multi-select, material typeahead - Transform spinboxes with up/down arrow keys and buttons - Per-primitive-type field configuration (size/radius/height/segments/UV) - Animation controls grouped per entity with enable/loop/skeleton/weights/rename Phase 4 - Toolbar & Layout: - View toolbar with Grid/Normals/MeshInfo/ViewCube toggles - Reorganized Tools toolbar with Scale + World/Local toggle - Right dock (Inspector) stretches full height - Animation Control auto-shows below viewports for animated entities - Improved status bar with transform space + selection count - Tooltip keyboard shortcut hints Additional: - ThemeManager QML singleton for canonical palette colors - BatchExporter and MaterialPresetLibrary stubs - Clean shutdown: LLM worker unload + _exit(0) to avoid ggml-metal assert - Version bump 2.15.1 → 2.16.0 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:
📝 WalkthroughWalkthroughReplaces the tabbed sidebar with a QML-based Properties Inspector, adds a scale gizmo and WORLD/LOCAL transform space, implements an Undo/Redo system with transform commands, adds batch export and material preset singletons, introduces a ThemeManager, and wires these into MainWindow, build files, resources, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant QML as QML PropertiesPanel (UI)
participant Controller as PropertiesPanelController (C++)
participant Model as SceneTreeModel (QAbstractItemModel)
participant Manager as Manager / Ogre Scene
participant Undo as UndoManager / QUndoStack
rect rgba(100,150,240,0.5)
QML->>Controller: request sceneTreeModel / read properties
Controller->>Model: provide model instance / role data
Model->>Manager: traverse scene nodes, entities, subentities
Manager-->>Model: node/entity data
Model-->>Controller: role/data updates (selection, material list)
Controller-->>QML: emits signals / exposes properties
end
sequenceDiagram
participant QML as QML PropertiesPanel (user action)
participant Controller as PropertiesPanelController
participant Undo as UndoManager
participant Cmd as TransformCommand (Translate/Rotate/Scale)
participant Manager as Manager / Ogre Scene
rect rgba(200,160,100,0.5)
QML->>Controller: user edits transform / triggers action
Controller->>Undo: push(new TransformCommand(...))
Undo->>Cmd: constructor captures state
Cmd->>Manager: redo() applies transform
QML->>Undo: user presses Ctrl+Z
Undo->>Cmd: undo() restores snapshot in Manager
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 97676c6944
ℹ️ 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
+638
to
+639
| // Per-axis scale | ||
| scaleFactor = Ogre::Vector3::UNIT_SCALE + (mTransformVector * (ratio - 1.0f)); |
There was a problem hiding this comment.
updateGizmo() now rotates the gizmo for SPACE_LOCAL, but the drag math here still uses the picked axis as raw UNIT_X/Y/Z. Since TranslationGizmo::highlightAxis, RotationGizmo::highlightCircle, and ScaleGizmo::highlightAxis all return world-space unit vectors, dragging a rotated node's local X/Y/Z handle still transforms along world axes. In practice the new World/Local toggle is only cosmetic for translated/rotated/scaled nodes that are not axis-aligned.
Useful? React with 👍 / 👎.
Comment on lines
+105
to
+109
| void PropertiesPanelController::setPosX(double v) { | ||
| if (mPosX != v) { | ||
| Ogre::Vector3 pos(v, mPosY, mPosZ); | ||
| TransformOperator::getSingleton()->setSelectedPosition(pos); | ||
| } |
There was a problem hiding this comment.
The new inspector fields call these setters directly, so position/rotation/scale changes are applied immediately from QML, but UndoManager::push still only happens in TransformOperator::mouseReleaseEvent for gizmo drags (src/TransformOperator.cpp:658-726). That means typing a value or using the up/down arrows in the redesigned inspector cannot be undone with Ctrl+Z, despite undo/redo being advertised for transform operations.
Useful? React with 👍 / 👎.
src/mainwindow.cpp
Outdated
Comment on lines
+603
to
+604
| auto* cam = mDockWidgetList.first()->getOgreWidget()->getSpaceCamera(); | ||
| if(cam) cam->frameSelection(); |
There was a problem hiding this comment.
QtMeshEditor supports multi-viewport layouts, and the rest of this change already tracks an active OgreWidget via focus signals, but F always calls frameSelection() on mDockWidgetList.first(). In any 2- or 4-viewport setup, pressing F while working in another viewport reframes the hidden first camera and leaves the viewport the user is actually using unchanged.
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui_files/mainwindow.ui (1)
106-122:⚠️ Potential issue | 🟠 MajorPlease keep the redesign controls out of
mainwindow.ui.These additions grow the widget-based shell with new menus, toolbars, and actions instead of keeping new UX in the QML layer. That conflicts with the repo rule for
ui_files/and keeps the redesign split across two UI stacks.As per coding guidelines,
ui_files/**/*.ui:Do not extend Widget-based UI. Existing ui_files/ remain but should not be extended.Also applies to: 155-180, 322-343, 510-551
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui_files/mainwindow.ui` around lines 106 - 122, The mainwindow.ui has new widget-based redesign controls that must be removed; delete the added QMenu/QAction entries (e.g., the QMenu elements named menuEdit and menuHelp and their addaction children like actionUndo, actionRedo, actionAbout, actionVerify_Update, and the top-level addaction entries menuFile, menuEdit, menuOp_es) so the widget UI is not extended, and move any new UX/features to the QML layer instead; repeat this removal for the other affected UI fragments referenced (around the other ranges) to keep ui_files/*.ui unchanged except for existing content.
🧹 Nitpick comments (6)
src/ThemeManager.cpp (1)
7-12: Consider renaminginstance()togetSingleton()for consistency.Other singletons in this codebase (Manager, SelectionSet, TransformOperator) use
getSingleton()andgetSingletonPtr()for access. Usinginstance()here introduces inconsistency. Based on learnings: "Access singletons viaClassName::getSingleton()orClassName::getSingletonPtr()."Proposed rename
-ThemeManager* ThemeManager::instance() +ThemeManager* ThemeManager::getSingleton() { if (!m_pSingleton) m_pSingleton = new ThemeManager(); return m_pSingleton; }Also update
qmlInstance()to callgetSingleton()instead ofinstance().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ThemeManager.cpp` around lines 7 - 12, ThemeManager::instance() breaks naming consistency with other singletons; rename the accessor to getSingleton() (and provide a getSingletonPtr() variant if needed) by changing the ThemeManager::instance() declaration/definition to ThemeManager::getSingleton(), update any forward declarations and all call sites to use getSingleton(), and update qmlInstance() to call getSingleton() instead of instance(); ensure the class header, implementation, and any references (including tests and QML registration) are updated together to avoid unresolved symbol errors.src/BatchExporter.cpp (1)
22-24: Consider validatingmOutputFormatbefore use.If
mOutputFormatis empty, the output path will end with a trailing dot (e.g.,file.), which may cause issues on some platforms or with CLIPipeline.Proposed validation
void BatchExporter::execute() { + if (mOutputFormat.isEmpty()) { + emit error(QString(), "Output format not specified"); + emit finished(0, mInputFiles.size()); + return; + } int success = 0, fail = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/BatchExporter.cpp` around lines 22 - 24, The output path construction uses mOutputFormat directly and can produce a trailing dot if mOutputFormat is empty; update the logic that builds outputPath (the expression using mOutputDir, fi.absolutePath(), fi.baseName(), and mOutputFormat) to validate mOutputFormat first — if mOutputFormat.isEmpty() either omit the dot and extension or substitute a safe default extension (e.g., "txt" or fi.completeSuffix()), and ensure the string concatenation only inserts the "." when a non-empty extension is present so outputPath never ends with a dangling dot.src/ScaleGizmo_test.cpp (1)
119-124: Minor: Reference address checks are always non-null.
ASSERT_NE(&mScaleGizmo->getXAxis(), nullptr)takes the address of a reference, which cannot be null in valid C++. IfcreateAxis()fails, the issue would manifest differently (e.g., crash or invalid object state). Consider checking a meaningful property instead, or this test can simply verify thatcreateAxis()doesn't throw/crash.Suggested alternative
TEST_F(ScaleGizmoTests, CreateAxis) { mScaleGizmo->createAxis(); - ASSERT_NE(&mScaleGizmo->getXAxis(), nullptr); - ASSERT_NE(&mScaleGizmo->getYAxis(), nullptr); - ASSERT_NE(&mScaleGizmo->getZAxis(), nullptr); + // Verify axes are valid by checking they have expected names or are attached + EXPECT_FALSE(mScaleGizmo->getXAxis().getName().empty()); + EXPECT_FALSE(mScaleGizmo->getYAxis().getName().empty()); + EXPECT_FALSE(mScaleGizmo->getZAxis().getName().empty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ScaleGizmo_test.cpp` around lines 119 - 124, The test ScaleGizmoTests::CreateAxis is asserting addresses of references (ASSERT_NE(&mScaleGizmo->getXAxis(), nullptr)) which is meaningless; change the test to assert meaningful state after calling createAxis(): call mScaleGizmo->createAxis() and then verify a real property or method on each axis returned by getXAxis/getYAxis/getZAxis (e.g., isVisible(), length() > 0, id() != 0, or a valid() method) or assert that createAxis() does not throw/crash (e.g., wrap in ASSERT_NO_THROW). Update references to getXAxis/getYAxis/getZAxis in the test to check those concrete properties rather than pointer addresses.src/UndoManager_test.cpp (1)
7-12: Consider full singleton reset for stricter test isolation.At Line 8 and Line 11,
clear()resets history but keeps the same singleton instance alive across tests. UsingUndoManager::kill()in fixture lifecycle would eliminate any residual QObject state between tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/UndoManager_test.cpp` around lines 7 - 12, Replace the current use of UndoManager::getSingleton()->clear() in the test fixture SetUp and TearDown with a full singleton teardown and recreation: call UndoManager::getSingleton()->kill() (or UndoManager::kill()) in TearDown to destroy the singleton instance and eliminate QObject residual state, and ensure SetUp obtains a fresh instance via UndoManager::getSingleton() (or reinitialize as needed) before each test so tests run with a clean singleton rather than only cleared history.src/SceneTreeModel_test.cpp (1)
17-17: Avoid fixed sleeps in test setup.At Line 17,
QThread::msleep(50)adds timing fragility and CI slowdown. Prefer waiting on a concrete condition/state transition instead of a hard delay.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SceneTreeModel_test.cpp` at line 17, Replace the brittle QThread::msleep(50) sleep with a deterministic wait for the actual condition or signal; instead of QThread::msleep(50) use a QSignalSpy on the relevant signal (e.g., rowsInserted(...) or a modelLoaded/changed signal) and call spy.wait(timeout) or use QTRY_VERIFY / QTRY_COMPARE with a small timeout loop checking the concrete state (e.g., treeModel->rowCount() > 0 or treeModel->isLoaded()). Locate the QThread::msleep(50) call in SceneTreeModel_test.cpp and swap it for one of those approaches so the test waits on the model's real state transition rather than a fixed sleep.src/TransformOperator.h (1)
123-126: IncludeOgreQuaternion.hdirectly for the new undo snapshot list.
mUndoStartOrientationsstoresQList<Ogre::Quaternion>, sosrc/TransformOperator.hnow depends on the full quaternion definition. Right now that only works if some transitive include happens to provide it.Minimal fix
`#include` <QPoint> `#include` <OgreRay.h> +#include <OgreQuaternion.h> `#include` "QtInputManager.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator.h` around lines 123 - 126, The header now stores QList<Ogre::Quaternion> (mUndoStartOrientations) but doesn't include the quaternion definition; add a direct include of OgreQuaternion.h in TransformOperator.h so the type is defined (avoid relying on transitive includes). Update the include block near the other Ogre includes in TransformOperator.h to `#include` "OgreQuaternion.h" (or the project-appropriate Ogre header) and ensure the header compiles standalone with mUndoStartOrientations present.
🤖 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/AnimationTimeline.qml`:
- Around line 51-96: The play/pause and stop Buttons are glyph-only and lack
accessible names or tooltips; update the two Button instances (the one using
AnimationTimelineController.playing and the stop Button that calls
AnimationTimelineController.stop()) to provide accessibility metadata and a
visible tooltip: attach Accessible.name (or
Accessible.name/Accessible.description) with a dynamic value for the play/pause
Button ("Pause" when AnimationTimelineController.playing is true, otherwise
"Play") and "Stop" for the stop Button, and add a ToolTip { text: ... } for each
to show the same label on hover; keep existing visuals/text unchanged and use
the same properties (contentItem/text) so assistive tech and mouse users get
clear labels.
In `@qml/CollapsibleSection.qml`:
- Around line 47-52: The header MouseArea (headerMouse) only toggles via mouse
click; make it keyboard-accessible by enabling focus and handling Enter/Space
keys: set headerMouse.focus = true (so it can receive keyboard events and be
tab-focusable) and add a Keys.onPressed handler (or Keys.onReturnPressed and
Keys.onSpacePressed) that toggles root.expanded the same way as onClicked;
ensure Keys.enabled is true if present and keep onClicked behavior unchanged so
both mouse and keyboard toggle the section.
In `@qml/PropertiesPanel.qml`:
- Around line 60-84: The delegates keep stale QModelIndex instances after
SceneTreeModel::rebuild() because Loader.item.nodeIndex is only assigned once;
update the Connections { function onModelReset() } handler to force the
Repeater/Loaders to rebuild by briefly clearing and restoring the delegates
(e.g., stash the current outlinerColumn.nodeCount, set outlinerColumn.nodeCount
= 0, then restore it) or by iterating the Repeater children and resetting each
Loader.source to "" and back to "qrc:/PropertiesPanel/SceneTreeNode.qml"; target
the outlinerColumn.nodeCount/Connections.onModelReset and
Loader.source/SceneTreeNode.qml symbols so every loader reassigns item.nodeIndex
from the fresh treeModel.
In `@qml/SceneTreeNode.qml`:
- Around line 20-25: The children can be assigned their nodeIndex and treeModel
after Component.onCompleted runs, so selection can appear out-of-sync; update
the code path that wires child items (the onLoaded / child creation block that
sets nodeIndex and treeModel) to call each child's refreshSelected() immediately
after assigning nodeIndex and treeModel so the child's refreshSelected() (the
function defined as refreshSelected) runs and updates selected right away; apply
the same fix to the other similar block (the occurrence around the
refreshSelected() duplicate at the second location).
In `@src/AnimationTimelineController.cpp`:
- Around line 103-125: onSelectionChanged currently assigns mCurrentAnimation
directly which bypasses setCurrentAnimation and leaves previous states enabled
and playback flags stale; update onSelectionChanged (and the similar block that
auto-selects the first animation) to call setCurrentAnimation(<name>) instead of
setting mCurrentAnimation directly, and when the selection becomes empty call
setCurrentAnimation(QString()) (or clear via setCurrentAnimation) and ensure
mPlaying is set to false so playback is reset; this uses
getCurrentAnimState()/setEnabled(...) behavior already implemented in
setCurrentAnimation to properly disable the previous state and enable/reset the
new one.
In `@src/commands/TransformCommands.cpp`:
- Around line 107-127: NodeSnapshot currently records visibility incorrectly
(snap.wasVisible is hard-coded true) and DeleteCommand::undo always calls
snap.node->setVisible(true, true), which re-shows nodes that were originally
hidden; update the snapshot creation (where NodeSnapshot snap is populated) to
set snap.wasVisible = node->isVisible() (or equivalent) instead of true, and
change DeleteCommand::undo to call snap.node->setVisible(snap.wasVisible, true)
so the node’s original visibility is restored; ensure you reference
NodeSnapshot, mSnapshots, and DeleteCommand::undo when making these edits.
In `@src/LLMManager.cpp`:
- Around line 81-87: The current flow risks a use-after-free because
initializeWorkerThread() connects m_workerThread::finished to
m_worker::deleteLater, but later code calls m_worker->unloadModel() after
m_workerThread->quit()/wait(); either remove that deleteLater connection if you
truly intend to leak the worker, or ensure unloadModel() runs on the worker
thread before the thread is stopped by invoking unloadModel via
QMetaObject::invokeMethod(m_worker, "unloadModel", Qt::BlockingQueuedConnection)
(or call unloadModel on the worker thread directly) prior to calling
m_workerThread->quit()/wait(); update the code around initializeWorkerThread(),
the deleteLater connection, and the shutdown sequence (where unloadModel(),
quit(), wait() are used) accordingly.
In `@src/mainwindow.cpp`:
- Around line 638-653: The closeEvent currently calls _exit(0) for all
non-Windows builds which prevents MainWindow::~MainWindow() and its cleanup
(timer/frame-listener/MCP) from running; change the platform guard so the
_exit(0) workaround is only applied on macOS (e.g., wrap the _exit(0) call with
the macOS-specific Qt define such as Q_OS_MAC) and leave Linux and other
non-Windows builds to run normal destruction; keep the surrounding comments and
ensure LLMManager::instance()->shutdownWorkerThread() and
QMainWindow::closeEvent(event) remain executed before the platform-specific
early-exit.
In `@src/MaterialPresetLibrary.cpp`:
- Around line 41-57: The applyPreset function accepts any arbitrary name and
creates a material "Preset/<name>" allowing invalid/unknown presets to be
created; update MaterialPresetLibrary::applyPreset to validate the incoming name
before creating or applying a material by checking it against the canonical
preset registry or an allowed-name pattern (e.g., look up name in the preset
list used by the library or reject names with illegal characters), and if the
name is not recognized return early without calling
Ogre::MaterialManager::create; apply the same validation logic to the other
preset-creation/apply code block referenced (the code covering the 60-96 range)
so materials are only created for known/validated presets and silent
creation/bloating is prevented, referencing matName,
Ogre::MaterialManager::getSingletonPtr/getByName/create, and SelectionSet usage
for where to place the guard.
In `@src/PropertiesPanelController.cpp`:
- Around line 198-207: getSelectedPrimitive() only checks a single SceneNode
selection, so when the user selects a Mesh or Submesh
(Ogre::Entity/Ogre::SubEntity) the function returns nullptr; update it to also
detect a single entity/subentity selection, resolve the owning SceneNode and
then call PrimitiveObject::getPrimitiveFromSceneNode. Concretely, inside
getSelectedPrimitive() after the existing SceneNode branch, check SelectionSet
for a single Ogre::Entity or Ogre::SubEntity selection (e.g. use the
SelectionSet entity/subentity accessors), obtain the corresponding Ogre::Entity
(for SubEntity get its parent), get its parent SceneNode, and return
PrimitiveObject::getPrimitiveFromSceneNode(node); keep the existing nullptr
return otherwise.
- Around line 443-468: The rename must be state-neutral until it succeeds: call
SkeletonTransform::renameAnimation(ent, oldName, newName) first and only if it
returns true perform the cleanup steps currently executed beforehand — disable
skeleton debug/bone weights via
mAnimationWidget->toggleSkeletonDebug/toggleBoneWeights, call setPlaying(false),
disable per-state playback via animSet->getAnimationStates() ->
state->setEnabled(false), and then refresh selection; when rebuilding selection
use the original nodes list from
SelectionSet::getSingleton()->getNodesSelectionList() and re-append them without
losing entity/subentity context (avoid dropping selection context by not
transforming rows into node-only items).
- Around line 39-53: Update PropertiesPanelController::onSelectionChanged to
refresh the cached transform fields before emitting transformChanged(): obtain
the newly selected object's transform (via SelectionSet::getSingleton() / the
selected entity's transform component or the TransformOperator API if
available), assign its position to mPosX/mPosY/mPosZ, its orientation to
mRotX/mRotY/mRotZ and its scale to mScaleX/mScaleY/mScaleZ, then emit
transformChanged(); this ensures the cached values reflect the new selection
rather than waiting for
selectedPositionChanged/selectedOrientationChanged/selectedScaleChanged signals.
In `@src/PropertiesPanelController.h`:
- Line 139: PropertiesPanelController stores a raw AnimationWidget*
(mAnimationWidget) and must use a guarded Qt pointer to avoid use-after-free
when the widget is destroyed; change mAnimationWidget's type to
QPointer<AnimationWidget> (or QPointer if AnimationWidget is a QObject) and
update setAnimationWidget(AnimationWidget* widget) to assign into that QPointer,
then replace raw nullptr checks with QPointer::isNull()/operator bool checks
where mAnimationWidget is used (references in methods around the previous checks
at lines noted), and apply the same conversion for the other raw widget
pointer(s) referenced in the 176–178 region so the singleton holds guarded
pointers only.
In `@src/SceneTreeModel.cpp`:
- Around line 329-332: The current SceneTreeModel::updateSelection emits
dataChanged with two invalid QModelIndex objects; change it to emit dataChanged
over a valid index range or use an appropriate alternative: compute the first
and last valid QModelIndex for the rows whose SelectedRole changed (e.g., use
index(0,0) and index(rowCount()-1,0) or track the specific changed rows) and
call emit dataChanged(firstIndex, lastIndex, {SelectedRole}); if every visible
row's selection changed prefer emit layoutChanged(); or if the entire model
changed use emit modelReset(); ensure you reference
SceneTreeModel::updateSelection and the SelectedRole when making the change.
- Around line 301-309: The MaterialNameRole update currently only emits
dataChanged for the edited row; modify the block in SceneTreeModel where you
handle SceneTreeItem::SubEntity and SceneTreeItem::Entity (the code that calls
setMaterialName on Ogre::SubEntity and Ogre::Entity) to also emit dataChanged
for all affected rows: when handling a SubEntity (static_cast<Ogre::SubEntity*>
on item->ogrePtr()), after emitting for the sub row also find the parent
SceneTreeItem (the Entity row) and emit dataChanged for that parent's
QModelIndex with {MaterialNameRole}; when handling an Entity, after calling
ent->setMaterialName(stdName) emit dataChanged for each child SubEntity row
(iterate child SceneTreeItem entries and emit dataChanged for each child index
with {MaterialNameRole}) so both parent and all children are notified.
---
Outside diff comments:
In `@ui_files/mainwindow.ui`:
- Around line 106-122: The mainwindow.ui has new widget-based redesign controls
that must be removed; delete the added QMenu/QAction entries (e.g., the QMenu
elements named menuEdit and menuHelp and their addaction children like
actionUndo, actionRedo, actionAbout, actionVerify_Update, and the top-level
addaction entries menuFile, menuEdit, menuOp_es) so the widget UI is not
extended, and move any new UX/features to the QML layer instead; repeat this
removal for the other affected UI fragments referenced (around the other ranges)
to keep ui_files/*.ui unchanged except for existing content.
---
Nitpick comments:
In `@src/BatchExporter.cpp`:
- Around line 22-24: The output path construction uses mOutputFormat directly
and can produce a trailing dot if mOutputFormat is empty; update the logic that
builds outputPath (the expression using mOutputDir, fi.absolutePath(),
fi.baseName(), and mOutputFormat) to validate mOutputFormat first — if
mOutputFormat.isEmpty() either omit the dot and extension or substitute a safe
default extension (e.g., "txt" or fi.completeSuffix()), and ensure the string
concatenation only inserts the "." when a non-empty extension is present so
outputPath never ends with a dangling dot.
In `@src/ScaleGizmo_test.cpp`:
- Around line 119-124: The test ScaleGizmoTests::CreateAxis is asserting
addresses of references (ASSERT_NE(&mScaleGizmo->getXAxis(), nullptr)) which is
meaningless; change the test to assert meaningful state after calling
createAxis(): call mScaleGizmo->createAxis() and then verify a real property or
method on each axis returned by getXAxis/getYAxis/getZAxis (e.g., isVisible(),
length() > 0, id() != 0, or a valid() method) or assert that createAxis() does
not throw/crash (e.g., wrap in ASSERT_NO_THROW). Update references to
getXAxis/getYAxis/getZAxis in the test to check those concrete properties rather
than pointer addresses.
In `@src/SceneTreeModel_test.cpp`:
- Line 17: Replace the brittle QThread::msleep(50) sleep with a deterministic
wait for the actual condition or signal; instead of QThread::msleep(50) use a
QSignalSpy on the relevant signal (e.g., rowsInserted(...) or a
modelLoaded/changed signal) and call spy.wait(timeout) or use QTRY_VERIFY /
QTRY_COMPARE with a small timeout loop checking the concrete state (e.g.,
treeModel->rowCount() > 0 or treeModel->isLoaded()). Locate the
QThread::msleep(50) call in SceneTreeModel_test.cpp and swap it for one of those
approaches so the test waits on the model's real state transition rather than a
fixed sleep.
In `@src/ThemeManager.cpp`:
- Around line 7-12: ThemeManager::instance() breaks naming consistency with
other singletons; rename the accessor to getSingleton() (and provide a
getSingletonPtr() variant if needed) by changing the ThemeManager::instance()
declaration/definition to ThemeManager::getSingleton(), update any forward
declarations and all call sites to use getSingleton(), and update qmlInstance()
to call getSingleton() instead of instance(); ensure the class header,
implementation, and any references (including tests and QML registration) are
updated together to avoid unresolved symbol errors.
In `@src/TransformOperator.h`:
- Around line 123-126: The header now stores QList<Ogre::Quaternion>
(mUndoStartOrientations) but doesn't include the quaternion definition; add a
direct include of OgreQuaternion.h in TransformOperator.h so the type is defined
(avoid relying on transitive includes). Update the include block near the other
Ogre includes in TransformOperator.h to `#include` "OgreQuaternion.h" (or the
project-appropriate Ogre header) and ensure the header compiles standalone with
mUndoStartOrientations present.
In `@src/UndoManager_test.cpp`:
- Around line 7-12: Replace the current use of
UndoManager::getSingleton()->clear() in the test fixture SetUp and TearDown with
a full singleton teardown and recreation: call
UndoManager::getSingleton()->kill() (or UndoManager::kill()) in TearDown to
destroy the singleton instance and eliminate QObject residual state, and ensure
SetUp obtains a fresh instance via UndoManager::getSingleton() (or reinitialize
as needed) before each test so tests run with a clean singleton rather than only
cleared history.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39aaef12-8210-48a6-b249-cf328f25814b
⛔ Files ignored due to path filters (1)
resources/scale.pngis excluded by!**/*.png
📒 Files selected for processing (43)
CLAUDE.mdCMakeLists.txtqml/AnimationTimeline.qmlqml/CollapsibleSection.qmlqml/PropertiesPanel.qmlqml/SceneTreeNode.qmlqml/TransformField.qmlresources/resource.qrcsrc/AnimationTimelineController.cppsrc/AnimationTimelineController.hsrc/BatchExporter.cppsrc/BatchExporter.hsrc/CMakeLists.txtsrc/LLMManager.cppsrc/LLMManager.hsrc/MaterialPresetLibrary.cppsrc/MaterialPresetLibrary.hsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/ScaleGizmo.cppsrc/ScaleGizmo.hsrc/ScaleGizmo_test.cppsrc/SceneTreeModel.cppsrc/SceneTreeModel.hsrc/SceneTreeModel_test.cppsrc/SpaceCamera.cppsrc/SpaceCamera.hsrc/ThemeManager.cppsrc/ThemeManager.hsrc/TransformOperator.cppsrc/TransformOperator.hsrc/UndoManager.cppsrc/UndoManager.hsrc/UndoManager_test.cppsrc/commands/TransformCommands.cppsrc/commands/TransformCommands.hsrc/main.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrctests/CMakeLists.txtui_files/animationcontrolwidget.uiui_files/mainwindow.ui
qml/AnimationTimeline.qml
Outdated
Comment on lines
+51
to
+96
| Button { | ||
| text: AnimationTimelineController.playing ? "\u275A\u275A" : "\u25B6" | ||
| onClicked: { | ||
| if (AnimationTimelineController.playing) | ||
| AnimationTimelineController.pause() | ||
| else | ||
| AnimationTimelineController.play() | ||
| } | ||
| implicitWidth: 32 | ||
| implicitHeight: 26 | ||
|
|
||
| background: Rectangle { | ||
| color: parent.pressed ? Qt.darker(PropertiesPanelController.headerColor, 1.2) | ||
| : PropertiesPanelController.headerColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| radius: 3 | ||
| } | ||
| contentItem: Text { | ||
| text: parent.text | ||
| color: PropertiesPanelController.textColor | ||
| horizontalAlignment: Text.AlignHCenter | ||
| verticalAlignment: Text.AlignVCenter | ||
| font.pixelSize: 12 | ||
| } | ||
| } | ||
|
|
||
| Button { | ||
| text: "\u25A0" | ||
| onClicked: AnimationTimelineController.stop() | ||
| implicitWidth: 32 | ||
| implicitHeight: 26 | ||
|
|
||
| background: Rectangle { | ||
| color: parent.pressed ? Qt.darker(PropertiesPanelController.headerColor, 1.2) | ||
| : PropertiesPanelController.headerColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| radius: 3 | ||
| } | ||
| contentItem: Text { | ||
| text: parent.text | ||
| color: PropertiesPanelController.textColor | ||
| horizontalAlignment: Text.AlignHCenter | ||
| verticalAlignment: Text.AlignVCenter | ||
| font.pixelSize: 12 | ||
| } | ||
| } |
There was a problem hiding this comment.
Add accessible labels/tooltips for icon-only playback controls.
At Line 51 and Line 77, the buttons are glyph-only (▶, ❚❚, ■) with no explicit accessible name/tooltip. This is an accessibility blocker for assistive-tech users.
Minimal accessibility enhancement
Button {
text: AnimationTimelineController.playing ? "\u275A\u275A" : "\u25B6"
+ Accessible.name: AnimationTimelineController.playing ? "Pause animation" : "Play animation"
+ ToolTip.visible: hovered
+ ToolTip.text: Accessible.name
onClicked: {
if (AnimationTimelineController.playing)
AnimationTimelineController.pause()
else
AnimationTimelineController.play()
} Button {
text: "\u25A0"
+ Accessible.name: "Stop animation"
+ ToolTip.visible: hovered
+ ToolTip.text: Accessible.name
onClicked: AnimationTimelineController.stop()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AnimationTimeline.qml` around lines 51 - 96, The play/pause and stop
Buttons are glyph-only and lack accessible names or tooltips; update the two
Button instances (the one using AnimationTimelineController.playing and the stop
Button that calls AnimationTimelineController.stop()) to provide accessibility
metadata and a visible tooltip: attach Accessible.name (or
Accessible.name/Accessible.description) with a dynamic value for the play/pause
Button ("Pause" when AnimationTimelineController.playing is true, otherwise
"Play") and "Stop" for the stop Button, and add a ToolTip { text: ... } for each
to show the same label on hover; keep existing visuals/text unchanged and use
the same properties (contentItem/text) so assistive tech and mouse users get
clear labels.
src/AnimationTimelineController.cpp
Outdated
Comment on lines
+103
to
+125
| void AnimationTimelineController::setCurrentAnimation(const QString& name) | ||
| { | ||
| if (mCurrentAnimation != name) | ||
| { | ||
| // Disable previous animation | ||
| auto* prevState = getCurrentAnimState(); | ||
| if (prevState) | ||
| prevState->setEnabled(false); | ||
|
|
||
| mCurrentAnimation = name; | ||
|
|
||
| // Enable new animation | ||
| auto* newState = getCurrentAnimState(); | ||
| if (newState) | ||
| { | ||
| newState->setEnabled(true); | ||
| newState->setLoop(true); | ||
| newState->setTimePosition(0); | ||
| } | ||
|
|
||
| emit currentAnimationChanged(); | ||
| emit progressChanged(); | ||
| } |
There was a problem hiding this comment.
Don't bypass setCurrentAnimation() when selection changes.
onSelectionChanged() assigns mCurrentAnimation directly, so the auto-selected first animation never gets enabled, mPlaying can survive onto an empty/non-animated selection, and any previously enabled state is left untouched. Route the default selection through setCurrentAnimation() and reset playback when the selection changes.
Also applies to: 163-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationTimelineController.cpp` around lines 103 - 125,
onSelectionChanged currently assigns mCurrentAnimation directly which bypasses
setCurrentAnimation and leaves previous states enabled and playback flags stale;
update onSelectionChanged (and the similar block that auto-selects the first
animation) to call setCurrentAnimation(<name>) instead of setting
mCurrentAnimation directly, and when the selection becomes empty call
setCurrentAnimation(QString()) (or clear via setCurrentAnimation) and ensure
mPlaying is set to false so playback is reset; this uses
getCurrentAnimState()/setEnabled(...) behavior already implemented in
setCurrentAnimation to properly disable the previous state and enable/reset the
new one.
Comment on lines
+443
to
+468
| // Disable skeleton debug/weights and stop playback before rename | ||
| // (rename recreates entity internals, stale pointers would crash) | ||
| if (mAnimationWidget) | ||
| { | ||
| // disableAllSkeletonDebug is private, but toggleSkeletonDebug(false) cleans up | ||
| if (mAnimationWidget->isSkeletonDebugActive(ent)) | ||
| mAnimationWidget->toggleSkeletonDebug(ent, false); | ||
| if (mAnimationWidget->isBoneWeightsShown(ent)) | ||
| mAnimationWidget->toggleBoneWeights(ent, false); | ||
| } | ||
| setPlaying(false); | ||
| if (auto* animSet = ent->getAllAnimationStates()) | ||
| { | ||
| for (const auto& [key, state] : animSet->getAnimationStates()) | ||
| state->setEnabled(false); | ||
| } | ||
|
|
||
| if (SkeletonTransform::renameAnimation(ent, oldName, newName)) | ||
| { | ||
| // Re-select current selection to force all widgets (including the | ||
| // old AnimationWidget) to refresh their tables with new entity data. | ||
| // This prevents stale pointers from crashing the poll timer. | ||
| auto nodes = SelectionSet::getSingleton()->getNodesSelectionList(); | ||
| SelectionSet::getSingleton()->clear(); | ||
| for (auto* node : nodes) | ||
| SelectionSet::getSingleton()->append(node); |
There was a problem hiding this comment.
renameAnimation() should stay state-neutral until the rename succeeds.
This disables playback/debug first and then rebuilds selection from node rows only. A failed rename leaves animations/debug off, and renaming from an entity/subentity row drops the current selection context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PropertiesPanelController.cpp` around lines 443 - 468, The rename must be
state-neutral until it succeeds: call SkeletonTransform::renameAnimation(ent,
oldName, newName) first and only if it returns true perform the cleanup steps
currently executed beforehand — disable skeleton debug/bone weights via
mAnimationWidget->toggleSkeletonDebug/toggleBoneWeights, call setPlaying(false),
disable per-state playback via animSet->getAnimationStates() ->
state->setEnabled(false), and then refresh selection; when rebuilding selection
use the original nodes list from
SelectionSet::getSingleton()->getNodesSelectionList() and re-append them without
losing entity/subentity context (avoid dropping selection context by not
transforming rows into node-only items).
| void setPrimVTile(double v); | ||
|
|
||
| Q_INVOKABLE void selectNodeByName(const QString& name); | ||
| void setAnimationWidget(class AnimationWidget* widget) { mAnimationWidget = widget; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "AnimationWidget declaration:"
fd 'AnimationWidget\.(h|hpp)$' -x sed -n '1,180p' {}
echo
echo "Controller-side lifetime handling:"
rg -n -C2 'setAnimationWidget\s*\(|mAnimationWidget\b|destroyed\s*\(' --glob '*.{cpp,h,hpp}'Repository: fernandotonon/QtMeshEditor
Length of output: 6176
Track AnimationWidget with a guarded pointer.
PropertiesPanelController is singleton-scoped, but mAnimationWidget is a raw external pointer. If the dock/widget is recreated or destroyed, later invokables can dereference freed memory. While null checks exist (lines 344–345, 405, 420, 445), they don't prevent use-after-free if the widget is destroyed without clearing the pointer.
Minimal Qt-side fix
`#include` <QColor>
+#include <QPointer>
`#include` <QVariantMap>
`#include` <QQmlEngine>
`#include` "SceneTreeModel.h"
@@
- class AnimationWidget* mAnimationWidget = nullptr;
+ QPointer<class AnimationWidget> mAnimationWidget;Also applies to: 176-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PropertiesPanelController.h` at line 139, PropertiesPanelController
stores a raw AnimationWidget* (mAnimationWidget) and must use a guarded Qt
pointer to avoid use-after-free when the widget is destroyed; change
mAnimationWidget's type to QPointer<AnimationWidget> (or QPointer if
AnimationWidget is a QObject) and update setAnimationWidget(AnimationWidget*
widget) to assign into that QPointer, then replace raw nullptr checks with
QPointer::isNull()/operator bool checks where mAnimationWidget is used
(references in methods around the previous checks at lines noted), and apply the
same conversion for the other raw widget pointer(s) referenced in the 176–178
region so the singleton holds guarded pointers only.
Comment on lines
+301
to
+309
| if (item->type() == SceneTreeItem::SubEntity) { | ||
| auto* sub = static_cast<Ogre::SubEntity*>(item->ogrePtr()); | ||
| sub->setMaterialName(stdName); | ||
| emit dataChanged(idx, idx, {MaterialNameRole}); | ||
| } | ||
| else if (item->type() == SceneTreeItem::Entity) { | ||
| auto* ent = static_cast<Ogre::Entity*>(item->ogrePtr()); | ||
| ent->setMaterialName(stdName); | ||
| emit dataChanged(idx, idx, {MaterialNameRole}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the SceneTreeModel.cpp file
find . -name "SceneTreeModel.cpp" -type fRepository: fernandotonon/QtMeshEditor
Length of output: 94
🏁 Script executed:
# Get the file to understand the context around lines 301-309
wc -l src/SceneTreeModel.cppRepository: fernandotonon/QtMeshEditor
Length of output: 96
🏁 Script executed:
# Read the relevant section around lines 301-309 with more context
sed -n '280,330p' src/SceneTreeModel.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1704
🏁 Script executed:
# Also check the SceneTreeModel.h header to understand the data structure
find . -name "SceneTreeModel.h" -type fRepository: fernandotonon/QtMeshEditor
Length of output: 92
🏁 Script executed:
# Read the header file to understand the structure
wc -l src/SceneTreeModel.hRepository: fernandotonon/QtMeshEditor
Length of output: 93
🏁 Script executed:
# Read SceneTreeModel.h to understand the data structure
sed -n '1,100p' src/SceneTreeModel.hRepository: fernandotonon/QtMeshEditor
Length of output: 2620
🏁 Script executed:
# Look for SceneTreeItem class definition to understand types
find . -name "SceneTreeItem.h" -o -name "SceneTreeItem.cpp" | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Search for SceneTreeItem class definition
rg "class SceneTreeItem" -A 20Repository: fernandotonon/QtMeshEditor
Length of output: 1088
🏁 Script executed:
# Understand how Entity and SubEntity rows are structured
rg "SubEntity" src/SceneTreeModel.cpp -B 3 -A 3Repository: fernandotonon/QtMeshEditor
Length of output: 2092
🏁 Script executed:
# Look at the data() method to understand how MaterialNameRole works
sed -n '1,333p' src/SceneTreeModel.cpp | grep -A 30 "QVariant.*data"Repository: fernandotonon/QtMeshEditor
Length of output: 1221
🏁 Script executed:
# Verify Entity-SubEntity material relationship by checking Ogre API usage
rg "setMaterialName|getMaterialName" src/ -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 14101
🏁 Script executed:
# Check how Entity and SubEntity interact with material setting in the codebase
rg "getNumSubEntities|getSubEntity" src/SceneTreeModel.cpp -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 705
🏁 Script executed:
# Examine the rebuild method to understand the full model structure
sed -n '150,250p' src/SceneTreeModel.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3097
🏁 Script executed:
# Verify how Ogre Entity::setMaterialName affects subentities
rg "Entity::setMaterialName" -A 5 -B 5Repository: fernandotonon/QtMeshEditor
Length of output: 939
🏁 Script executed:
# Check if there are other places in SceneTreeModel that emit dataChanged for multiple rows
rg "emit dataChanged" src/SceneTreeModel.cpp -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 603
🏁 Script executed:
# Look for any existing patterns of notifying parent/child rows
sed -n '200,333p' src/SceneTreeModel.cpp | grep -A 10 "dataChanged"Repository: fernandotonon/QtMeshEditor
Length of output: 683
Emit dataChanged for all affected rows when material names change.
The current code only notifies the edited row, but MaterialNameRole is shared across related items: Entity::setMaterialName() updates all SubEntity rows, and an Entity's MaterialNameRole displays its first SubEntity's material. Expanded trees will show stale material names until rebuild.
When a SubEntity at row 0 is changed, also notify the parent Entity. When an Entity is changed, notify all child SubEntity rows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SceneTreeModel.cpp` around lines 301 - 309, The MaterialNameRole update
currently only emits dataChanged for the edited row; modify the block in
SceneTreeModel where you handle SceneTreeItem::SubEntity and
SceneTreeItem::Entity (the code that calls setMaterialName on Ogre::SubEntity
and Ogre::Entity) to also emit dataChanged for all affected rows: when handling
a SubEntity (static_cast<Ogre::SubEntity*> on item->ogrePtr()), after emitting
for the sub row also find the parent SceneTreeItem (the Entity row) and emit
dataChanged for that parent's QModelIndex with {MaterialNameRole}; when handling
an Entity, after calling ent->setMaterialName(stdName) emit dataChanged for each
child SubEntity row (iterate child SceneTreeItem entries and emit dataChanged
for each child index with {MaterialNameRole}) so both parent and all children
are notified.
Comment on lines
+329
to
+332
| void SceneTreeModel::updateSelection() | ||
| { | ||
| emit dataChanged(QModelIndex(), QModelIndex(), {SelectedRole}); | ||
| emit selectionUpdated(); |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
cat -n src/SceneTreeModel.cpp | head -350 | tail -40Repository: fernandotonon/QtMeshEditor
Repository: fernandotonon/QtMeshEditor
Exit code: 0
stdout:
294 {
295 QModelIndex idx = index(row, 0, parentIndex);
296 auto* item = itemFromIndex(idx);
297 if (!item) return;
298
299 std::string stdName = matName.toStdString();
300
301 if (item->type() == SceneTreeItem::SubEntity) {
302 auto* sub = static_cast<Ogre::SubEntity*>(item->ogrePtr());
303 sub->setMaterialName(stdName);
304 emit dataChanged(idx, idx, {MaterialNameRole});
305 }
306 else if (item->type() == SceneTreeItem::Entity) {
307 auto* ent = static_cast<Ogre::Entity*>(item->ogrePtr());
308 ent->setMaterialName(stdName);
309 emit dataChanged(idx, idx, {MaterialNameRole});
310 }
311 }
312
313 QStringList SceneTreeModel::availableMaterials() const
314 {
315 QStringList names;
316 auto it = Ogre::MaterialManager::getSingleton().getResourceIterator();
317 while (it.hasMoreElements())
318 {
319 auto res = it.getNext();
320 QString name = QString::fromStdString(res->getName());
321 // Skip internal materials
322 if (!name.startsWith("Ogre/") && !name.startsWith("BaseWhite") && name != "GUI_Material")
323 names.append(name);
324 }
325 names.sort(Qt::CaseInsensitive);
326 return names;
327 }
328
329 void SceneTreeModel::updateSelection()
330 {
331 emit dataChanged(QModelIndex(), QModelIndex(), {SelectedRole});
332 emit selectionUpdated();
333 }
Script executed:
rg "dataChanged" src/SceneTreeModel.cpp -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Repository: fernandotonon/QtMeshEditor
Exit code: 0
stdout:
auto* sub = static_cast<Ogre::SubEntity*>(item->ogrePtr());
sub->setMaterialName(stdName);
emit dataChanged(idx, idx, {MaterialNameRole});
}
else if (item->type() == SceneTreeItem::Entity) {
auto* ent = static_cast<Ogre::Entity*>(item->ogrePtr());
ent->setMaterialName(stdName);
emit dataChanged(idx, idx, {MaterialNameRole});
}
}
--
void SceneTreeModel::updateSelection()
{
emit dataChanged(QModelIndex(), QModelIndex(), {SelectedRole});
emit selectionUpdated();
}
Script executed:
rg "updateSelection" src/SceneTreeModel.cpp -B 3 -A 3Repository: fernandotonon/QtMeshEditor
Repository: fernandotonon/QtMeshEditor
Exit code: 0
stdout:
connect(Manager::getSingleton(), &Manager::sceneNodeCreated, this, scheduleRebuild);
connect(Manager::getSingleton(), &Manager::sceneNodeDestroyed, this, scheduleRebuild);
connect(Manager::getSingleton(), &Manager::entityCreated, this, scheduleRebuild);
connect(SelectionSet::getSingleton(), &SelectionSet::selectionChanged, this, &SceneTreeModel::updateSelection);
}
SceneTreeModel::~SceneTreeModel()
--
return names;
}
void SceneTreeModel::updateSelection()
{
emit dataChanged(QModelIndex(), QModelIndex(), {SelectedRole});
emit selectionUpdated();
Qt QAbstractItemModel dataChanged QModelIndex documentation empty indices
In Qt's QAbstractItemModel::dataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight, const QVector<int> &roles = {}), the parameters topLeft and bottomRight define the range of changed items. Default-constructed QModelIndex objects are invalid/empty (QModelIndex). Official Qt documentation does not explicitly prohibit invalid indices but states that models must provide valid indices via createIndex and checkIndex validates them. Behavior with invalid indices is undefined per docs: "If the items do not have the same parent, the behavior is undefined" (from Qt 4 docs, similar implications in Qt 6). However, community sources and practical usage show: - Emitting dataChanged with two invalid indices (QModelIndex, QModelIndex) acts as a "nuclear option" to force a full view repaint of visible items without structural changes (no loss of selection/expansion). This is an implementation detail of QAbstractItemView::dataChanged, not officially supported, and less efficient. Used for small models or broad non-structural updates. - Single invalid index (e.g., dataChanged(invalid, valid)) typically fails to update views, as views check index.isValid in setData/data and ignore invalid ranges. - Proper usage requires valid indices with same parent, forming a rectangle: topLeft.row <= bottomRight.row, topLeft.column <= bottomRight.column. For empty/broad changes without structure mods, prefer layoutChanged or modelReset (resets selections). dataChanged assumes existing valid structure. Sources confirm via examples: invalid pair triggers full visible refresh (hacky), but docs imply valid indices required.
Citations:
- 1: https://doc.qt.io/qt-6/qabstractitemmodel.html
- 2: https://doc.qt.io/qt-6.3/qabstractitemmodel.html
- 3: https://runebook.dev/en/docs/qt/qabstractitemmodel/dataChanged
- 4: https://runebook.dev/en/articles/qt/qtreeview/dataChanged
- 5: https://forum.qt.io/topic/39357/solved-qabstractitemmodel-datachanged-question
- 6: https://stackoverflow.com/questions/76603422/update-a-qtableview-entirely-when-data-has-changed
- 7: https://stackoverflow.com/questions/16561974/when-to-emit-datachanged-from-a-qabstractitemmodel
Use valid QModelIndex ranges for dataChanged to ensure reliable view updates.
Line 331 emits dataChanged with two invalid QModelIndex objects. Per Qt documentation, behavior is undefined when indices are invalid or lack a common parent. While this may work as a side effect, it relies on undocumented implementation details. Views tracking SelectedRole will not consistently repaint on selection changes.
Use one of these approaches instead:
- Emit
dataChanged()for each valid row index that changed theSelectedRole - Use
layoutChanged()if all visible rows' selection state changed - Use
modelReset()if the entire model structure or all data changed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SceneTreeModel.cpp` around lines 329 - 332, The current
SceneTreeModel::updateSelection emits dataChanged with two invalid QModelIndex
objects; change it to emit dataChanged over a valid index range or use an
appropriate alternative: compute the first and last valid QModelIndex for the
rows whose SelectedRole changed (e.g., use index(0,0) and index(rowCount()-1,0)
or track the specific changed rows) and call emit dataChanged(firstIndex,
lastIndex, {SelectedRole}); if every visible row's selection changed prefer emit
layoutChanged(); or if the entire model changed use emit modelReset(); ensure
you reference SceneTreeModel::updateSelection and the SelectedRole when making
the change.
Comment on lines
+480
to
+492
| // Capture undo state for scene nodes | ||
| mUndoStartPositions.clear(); | ||
| mUndoStartOrientations.clear(); | ||
| mUndoStartScales.clear(); | ||
| if (SelectionSet::getSingleton()->hasNodes()) | ||
| { | ||
| for (Ogre::SceneNode* node : SelectionSet::getSingleton()->getNodesSelectionList()) | ||
| { | ||
| mUndoStartPositions.append(node->getPosition()); | ||
| mUndoStartOrientations.append(node->getOrientation()); | ||
| mUndoStartScales.append(node->getScale()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Undo is node-only right now.
Both the press-time snapshot and the release-time command push are gated by SelectionSet::hasNodes(), but the transform paths still mutate entity-backed selections through MeshTransform. That means gizmo/spinbox edits on mesh/entity selections bypass the undo stack entirely, so Ctrl+Z becomes a no-op for part of the new inspector workflow.
Also applies to: 657-728
This was
linked to
issues
Mar 23, 2026
Review fixes: - _exit(0) scoped to macOS only (was applying to Linux too) - Frame selection (F key) uses focused viewport, not always first - CollapsibleSection keyboard accessible (Tab/Enter/Space) - Outliner delegates rebuild on model reset (stale QModelIndex fix) - Child selection refreshed immediately after loading - LLM shutdown: unload model on worker thread before quit (use-after-free fix) - DeleteCommand preserves original node visibility SonarCloud fixes: - Removed const_cast in BatchExporter (C++17 string::data()) - Added explicit float casts for Ogre::Real precision loss - Fixed implicit integer precision loss New tests: - TransformOperator: TS_SCALE state, TransformSpace get/set/toggle/signal - ThemeManager: singleton, color validity, theme name, refresh signal Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The gizmo correctly oriented in LOCAL mode but drag math still used world-space UNIT_X/Y/Z vectors. Now translate and rotate drags convert the world-space delta into the gizmo's local coordinate system, apply the axis mask there, then convert back to world space. Added tests for local space translate and state cycling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents all shortcuts: transform tools (Q/W/E/R/X/Del), camera controls (F/middle/right/scroll/shift+middle/arrows), and edit shortcuts (Ctrl+Z/Shift+Z/O/S, Ctrl/Shift+click). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was
unlinked from
issues
Mar 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/PropertiesPanelController.cpp (3)
42-54:⚠️ Potential issue | 🔴 CriticalRefresh the cached transform fields on selection changes.
mPos*,mRot*, andmScale*are only refreshed fromTransformOperatorchange signals.onSelectionChanged()just re-emitstransformChanged(), so selecting a different object can still show — and then apply — the previous selection’s transform values.Also applies to: 477-487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 42 - 54, Selection changes don't refresh the cached transform fields (mPos*, mRot*, mScale*), so onSelectionChanged() can re-emit transformChanged() while still holding the previous object's values; update onSelectionChanged() to query TransformOperator::getSingleton() for the current selected position/orientation/scale (or call the existing lambda handlers) and assign mPosX/mPosY/mPosZ, mRotX/mRotY/mRotZ, mScaleX/mScaleY/mScaleZ from those current values before emitting transformChanged(); ensure the same fix is applied for the other occurrence mentioned (the block around the second onSelectionChanged handling).
198-207:⚠️ Potential issue | 🟠 MajorResolve primitives from mesh/submesh selections too.
The new scene tree can select Mesh/Submesh rows, but this helper only handles a single scene-node selection. Picking the Mesh/Submesh row of a primitive will return
nullptrhere and hide the primitive editor for the same object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 198 - 207, getSelectedPrimitive currently only checks a single scene-node selection and returns nullptr for Mesh/Submesh selections from the new tree; update it to also handle a single Mesh or SubMesh selection by resolving the mesh's SceneNode and returning the Primitive via PrimitiveObject::getPrimitiveFromSceneNode. Concretely: keep the existing SelectionSet node branch (SelectionSet::getSingleton(), hasNodes(), getSceneNode(0)), and add checks for a single mesh or submesh selection (e.g. hasMeshes()/getMeshesCount() or hasSubMeshes()/getSubMeshCount() on SelectionSet) — obtain the associated Ogre::SceneNode for the selected mesh/submesh and call PrimitiveObject::isPrimitive/PrimitiveObject::getPrimitiveFromSceneNode just like the node path; return nullptr otherwise.
433-468:⚠️ Potential issue | 🟠 MajorKeep animation rename state-neutral until it succeeds.
This disables playback/debug/weights before
SkeletonTransform::renameAnimation(...)returns, so a failed rename still leaves the UI mutated. On the success path, rebuilding the selection fromgetNodesSelectionList()also drops entity/subentity context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 433 - 468, The code mutates UI/animation state (toggleSkeletonDebug, toggleBoneWeights, setPlaying(false), disabling animation states) before calling SkeletonTransform::renameAnimation, and then rebuilds selection via getNodesSelectionList() which loses entity/sub-entity context; instead, move the debug/weights/playback disabling and iteration over ent->getAllAnimationStates() to after SkeletonTransform::renameAnimation returns true, and on success refresh selection without dropping entity/subentity context (avoid clearing and re-adding only nodes from SelectionSet::getNodesSelectionList(); preserve the original SelectionSet entries or reapply both node and entity/subentity identifiers when calling SelectionSet::clear and SelectionSet::append). Ensure you still check Manager::getSingleton()->hasAnimationName(ent, newName) before attempting rename and only mutate mAnimationWidget, setPlaying, and state->setEnabled when rename succeeded.src/AnimationTimelineController.cpp (1)
103-125:⚠️ Potential issue | 🟠 MajorSelection changes can leave the previous entity animating.
setCurrentAnimation()can only disable the “previous” state by resolving it through the current selection, andonSelectionChanged()bypasses that path entirely by assigningmCurrentAnimationdirectly. After a selection change, the old entity can stay enabled in the background while the new selection’s default animation never gets activated, andmPlayingalso stays stale.Also applies to: 163-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationTimelineController.cpp` around lines 103 - 125, The selection-change path currently assigns mCurrentAnimation directly (in onSelectionChanged) and bypasses the disable/enable logic in setCurrentAnimation, leaving the previous entity still animating and mPlaying stale; update onSelectionChanged (and any other selection-handling code that sets mCurrentAnimation directly) to call setCurrentAnimation(newName) instead of assigning mCurrentAnimation, ensure setCurrentAnimation first captures and disables the previous state via getCurrentAnimState() before updating mCurrentAnimation, and after enabling the new state's loop/time/enabled settings also update mPlaying to reflect the new state's playing status so no leftover animations remain active.
🤖 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`:
- Line 12: The current property "expanded" in SceneTreeNode.qml should default
to collapsed for all non-root nodes; change its initialization so only the root
node starts expanded (e.g., property bool expanded: indentLevel === 0), and
update the child-instantiation code so it only synchronously create child
objects when a node is actually expanded (move the loader.createObject / child
iteration into the expanded change handler or guard it with if (expanded)
instead of running unconditionally on creation). Target symbols: the "expanded"
property, "indentLevel", and the synchronous child loader loop that calls
createObject for children.
- Around line 111-123: The Text element id nameLabel currently has no explicit
width so elide never triggers; add a width or anchoring constraint (for example
set nameLabel.anchors.right to the material selector's left anchor or compute
width as parent.width minus the selector/spacing) so the Text has a bounded
width and elide: Text.ElideRight can take effect; update the Text block that
defines nameLabel accordingly.
In `@src/ThemeManager_test.cpp`:
- Around line 21-47: Tests dereference ThemeManager::instance() (assigned to tm)
without checking for nullptr, which can crash tests; update each test
(ColorsAreValid, ThemeNameNotEmpty, RefreshThemeEmitsSignal) to assert tm is
non-null before using it (e.g., use ASSERT_NE(tm, nullptr) or equivalent) so
failures are reported cleanly instead of causing a crash when
ThemeManager::instance() returns nullptr.
---
Duplicate comments:
In `@src/AnimationTimelineController.cpp`:
- Around line 103-125: The selection-change path currently assigns
mCurrentAnimation directly (in onSelectionChanged) and bypasses the
disable/enable logic in setCurrentAnimation, leaving the previous entity still
animating and mPlaying stale; update onSelectionChanged (and any other
selection-handling code that sets mCurrentAnimation directly) to call
setCurrentAnimation(newName) instead of assigning mCurrentAnimation, ensure
setCurrentAnimation first captures and disables the previous state via
getCurrentAnimState() before updating mCurrentAnimation, and after enabling the
new state's loop/time/enabled settings also update mPlaying to reflect the new
state's playing status so no leftover animations remain active.
In `@src/PropertiesPanelController.cpp`:
- Around line 42-54: Selection changes don't refresh the cached transform fields
(mPos*, mRot*, mScale*), so onSelectionChanged() can re-emit transformChanged()
while still holding the previous object's values; update onSelectionChanged() to
query TransformOperator::getSingleton() for the current selected
position/orientation/scale (or call the existing lambda handlers) and assign
mPosX/mPosY/mPosZ, mRotX/mRotY/mRotZ, mScaleX/mScaleY/mScaleZ from those current
values before emitting transformChanged(); ensure the same fix is applied for
the other occurrence mentioned (the block around the second onSelectionChanged
handling).
- Around line 198-207: getSelectedPrimitive currently only checks a single
scene-node selection and returns nullptr for Mesh/Submesh selections from the
new tree; update it to also handle a single Mesh or SubMesh selection by
resolving the mesh's SceneNode and returning the Primitive via
PrimitiveObject::getPrimitiveFromSceneNode. Concretely: keep the existing
SelectionSet node branch (SelectionSet::getSingleton(), hasNodes(),
getSceneNode(0)), and add checks for a single mesh or submesh selection (e.g.
hasMeshes()/getMeshesCount() or hasSubMeshes()/getSubMeshCount() on
SelectionSet) — obtain the associated Ogre::SceneNode for the selected
mesh/submesh and call
PrimitiveObject::isPrimitive/PrimitiveObject::getPrimitiveFromSceneNode just
like the node path; return nullptr otherwise.
- Around line 433-468: The code mutates UI/animation state (toggleSkeletonDebug,
toggleBoneWeights, setPlaying(false), disabling animation states) before calling
SkeletonTransform::renameAnimation, and then rebuilds selection via
getNodesSelectionList() which loses entity/sub-entity context; instead, move the
debug/weights/playback disabling and iteration over ent->getAllAnimationStates()
to after SkeletonTransform::renameAnimation returns true, and on success refresh
selection without dropping entity/subentity context (avoid clearing and
re-adding only nodes from SelectionSet::getNodesSelectionList(); preserve the
original SelectionSet entries or reapply both node and entity/subentity
identifiers when calling SelectionSet::clear and SelectionSet::append). Ensure
you still check Manager::getSingleton()->hasAnimationName(ent, newName) before
attempting rename and only mutate mAnimationWidget, setPlaying, and
state->setEnabled when rename succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e9ee15d-1f0e-4d92-9830-993efaf5829e
📒 Files selected for processing (11)
qml/CollapsibleSection.qmlqml/PropertiesPanel.qmlqml/SceneTreeNode.qmlsrc/AnimationTimelineController.cppsrc/BatchExporter.cppsrc/LLMManager.cppsrc/PropertiesPanelController.cppsrc/ThemeManager_test.cppsrc/TransformOperator_test.cppsrc/commands/TransformCommands.cppsrc/mainwindow.cpp
✅ Files skipped from review due to trivial changes (2)
- qml/CollapsibleSection.qml
- qml/PropertiesPanel.qml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/BatchExporter.cpp
- src/commands/TransformCommands.cpp
qml/SceneTreeNode.qml
Outdated
| property var treeModel | ||
| property int indentLevel: 0 | ||
|
|
||
| property bool expanded: indentLevel > 0 // Nodes start collapsed, children expanded |
There was a problem hiding this comment.
Keep descendants collapsed by default.
Line 12 makes every non-root row expanded as soon as it is created, so the synchronous loader chain in Lines 281-296 walks and instantiates the whole subtree on the first expand. On large scenes, that turns one click into a full recursive build on the UI thread.
⚡ Proposed fix
- property bool expanded: indentLevel > 0 // Nodes start collapsed, children expanded
+ property bool expanded: falseAlso applies to: 281-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/SceneTreeNode.qml` at line 12, The current property "expanded" in
SceneTreeNode.qml should default to collapsed for all non-root nodes; change its
initialization so only the root node starts expanded (e.g., property bool
expanded: indentLevel === 0), and update the child-instantiation code so it only
synchronously create child objects when a node is actually expanded (move the
loader.createObject / child iteration into the expanded change handler or guard
it with if (expanded) instead of running unconditionally on creation). Target
symbols: the "expanded" property, "indentLevel", and the synchronous child
loader loop that calls createObject for children.
Comment on lines
+111
to
+123
| Text { | ||
| id: nameLabel | ||
| text: { | ||
| if (!treeModel) return "" | ||
| var n = treeNode.nodeName | ||
| var t = treeModel.data(nodeIndex, 259) | ||
| return n + (t ? " (" + t + ")" : "") | ||
| } | ||
| color: treeNode.selected ? "white" : PropertiesPanelController.textColor | ||
| font.pixelSize: 11 | ||
| elide: Text.ElideRight | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "SceneTreeNode.qml" 2>/dev/null | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
git ls-files | grep -i "SceneTreeNode.qml"Repository: fernandotonon/QtMeshEditor
Length of output: 91
🏁 Script executed:
fd "SceneTreeNode.qml" --type fRepository: fernandotonon/QtMeshEditor
Length of output: 91
🏁 Script executed:
fd "\.qml$" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 529
🏁 Script executed:
cat -n qml/SceneTreeNode.qml | head -200Repository: fernandotonon/QtMeshEditor
Length of output: 8806
🏁 Script executed:
# Verify matSelector definition and visibility logic
sed -n '125,159p' qml/SceneTreeNode.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 1695
🏁 Script executed:
# Check if there are any tests or other files that reference this behavior
rg "nameLabel|SceneTreeNode" --type qml -A 3 -B 3 | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1451
Add width constraint to nameLabel so elide can take effect.
Without an explicit width, the Text element uses its implicit width and expands to fit the full text, making the elide: Text.ElideRight directive ineffective. This allows long node names to overflow the row and crowd out the material selector.
Proposed fix
Text {
id: nameLabel
+ width: Math.max(0, treeNode.width - x - (matSelector.visible ? matSelector.width + 12 : 8))
+ clip: true
text: {📝 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
| Text { | |
| id: nameLabel | |
| text: { | |
| if (!treeModel) return "" | |
| var n = treeNode.nodeName | |
| var t = treeModel.data(nodeIndex, 259) | |
| return n + (t ? " (" + t + ")" : "") | |
| } | |
| color: treeNode.selected ? "white" : PropertiesPanelController.textColor | |
| font.pixelSize: 11 | |
| elide: Text.ElideRight | |
| anchors.verticalCenter: parent.verticalCenter | |
| } | |
| Text { | |
| id: nameLabel | |
| width: Math.max(0, treeNode.width - x - (matSelector.visible ? matSelector.width + 12 : 8)) | |
| clip: true | |
| text: { | |
| if (!treeModel) return "" | |
| var n = treeNode.nodeName | |
| var t = treeModel.data(nodeIndex, 259) | |
| return n + (t ? " (" + t + ")" : "") | |
| } | |
| color: treeNode.selected ? "white" : PropertiesPanelController.textColor | |
| font.pixelSize: 11 | |
| elide: Text.ElideRight | |
| anchors.verticalCenter: parent.verticalCenter | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/SceneTreeNode.qml` around lines 111 - 123, The Text element id nameLabel
currently has no explicit width so elide never triggers; add a width or
anchoring constraint (for example set nameLabel.anchors.right to the material
selector's left anchor or compute width as parent.width minus the
selector/spacing) so the Text has a bounded width and elide: Text.ElideRight can
take effect; update the Text block that defines nameLabel accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/TransformOperator_test.cpp (1)
783-789:LocalSpaceWithAllStatesis currently a no-throw smoke test only.Consider adding a selected node and one observable assertion per state (translate/rotate/scale) so this test validates behavior, not just crash safety.
As per coding guidelines, "Add Google Test unit tests for new functionality."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator_test.cpp` around lines 783 - 789, The test LocalSpaceWithAllStates currently only verifies no-throw; modify it to create and select a test node, set TransformOperator::setTransformSpace(TransformOperator::SPACE_LOCAL), then for each state call TransformOperator::onTransformStateChange(TransformOperator::TS_TRANSLATE/TS_ROTATE/TS_SCALE) and add one observable assertion per state (for example assert the selected node is non-null via the selection API, assert TransformOperator::getCurrentState() equals the expected TS_* value or that the node's transform mode/flags changed, or assert transform handles/visibility through the inspector/handle query used in other tests); finally restore space to SPACE_WORLD as before. Use TransformOperator::getSingleton(), setTransformSpace, onTransformStateChange, TS_TRANSLATE, TS_ROTATE, TS_SCALE and any existing selection/assertion helpers to place the node in the scene and verify behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/TransformOperator_test.cpp`:
- Around line 775-780: The current assertion only checks that the position
changed and misses local-vs-world regressions; modify the test around
instance->translateSelected and node->getPosition so it first gives the node a
yawed orientation (use node->setOrientation / node->getOrientation()), call
instance->translateSelected(Ogre::Vector3(5,0,0)) with
TransformOperator::SPACE_LOCAL active, then compute the expected local
displacement by rotating the local translation by the node's orientation
(expectedDelta = node->getOrientation() * Ogre::Vector3(5,0,0)) and assert
endPos - startPos approximately equals expectedDelta (use component-wise near
comparisons with a small epsilon) and also assert it does not equal the
unrotated world delta to guard against world-space application; keep using the
existing symbols instance->translateSelected, node->getPosition,
node->getOrientation, and TransformOperator::SPACE_WORLD/SPACE_LOCAL to locate
code.
---
Nitpick comments:
In `@src/TransformOperator_test.cpp`:
- Around line 783-789: The test LocalSpaceWithAllStates currently only verifies
no-throw; modify it to create and select a test node, set
TransformOperator::setTransformSpace(TransformOperator::SPACE_LOCAL), then for
each state call
TransformOperator::onTransformStateChange(TransformOperator::TS_TRANSLATE/TS_ROTATE/TS_SCALE)
and add one observable assertion per state (for example assert the selected node
is non-null via the selection API, assert TransformOperator::getCurrentState()
equals the expected TS_* value or that the node's transform mode/flags changed,
or assert transform handles/visibility through the inspector/handle query used
in other tests); finally restore space to SPACE_WORLD as before. Use
TransformOperator::getSingleton(), setTransformSpace, onTransformStateChange,
TS_TRANSLATE, TS_ROTATE, TS_SCALE and any existing selection/assertion helpers
to place the node in the scene and verify behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 139bb624-3aa8-47c4-9c48-fe281eaa513a
📒 Files selected for processing (3)
docs/index.htmlsrc/TransformOperator.cppsrc/TransformOperator_test.cpp
✅ Files skipped from review due to trivial changes (1)
- docs/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- src/TransformOperator.cpp
Comment on lines
+775
to
+780
| Ogre::Vector3 startPos = node->getPosition(); | ||
| instance->translateSelected(Ogre::Vector3(5, 0, 0)); | ||
| Ogre::Vector3 endPos = node->getPosition(); | ||
|
|
||
| EXPECT_NE(startPos, endPos); | ||
| instance->setTransformSpace(TransformOperator::SPACE_WORLD); |
There was a problem hiding this comment.
LocalSpaceTranslate assertion is too weak to catch local/world math regressions.
EXPECT_NE(startPos, endPos) passes even if translation is incorrectly applied in world space. This test should assert the delta characteristics for a yawed node so the specific bug class is actually guarded.
Suggested assertion hardening
+ const Ogre::Vector3 delta = endPos - startPos;
- EXPECT_NE(startPos, endPos);
+ EXPECT_NEAR(delta.length(), 5.0f, 1e-4f);
+ // For a yawed node, local X movement should not remain pure world X.
+ EXPECT_NEAR(delta.x, 0.0f, 1e-4f);
+ EXPECT_NEAR(std::abs(delta.z), 5.0f, 1e-4f);+#include <cmath>As per coding guidelines, "Add Google Test unit tests for new functionality."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator_test.cpp` around lines 775 - 780, The current assertion
only checks that the position changed and misses local-vs-world regressions;
modify the test around instance->translateSelected and node->getPosition so it
first gives the node a yawed orientation (use node->setOrientation /
node->getOrientation()), call instance->translateSelected(Ogre::Vector3(5,0,0))
with TransformOperator::SPACE_LOCAL active, then compute the expected local
displacement by rotating the local translation by the node's orientation
(expectedDelta = node->getOrientation() * Ogre::Vector3(5,0,0)) and assert
endPos - startPos approximately equals expectedDelta (use component-wise near
comparisons with a small epsilon) and also assert it does not equal the
unrotated world delta to guard against world-space application; keep using the
existing symbols instance->translateSelected, node->getPosition,
node->getOrientation, and TransformOperator::SPACE_WORLD/SPACE_LOCAL to locate
code.
The QML Animation Timeline was replaced by the native AnimationControlWidget dock. Clean up the unused controller, QML file, and all references from main.cpp, mainwindow.cpp, CMakeLists.txt, qml_resources.qrc, tests, and CLAUDE.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- SceneTreeNode: all nodes start collapsed to avoid loading full subtree - Status bar: handle SubEntity-only selections - PropertiesPanelController: emit themeChanged on palette switch - ThemeManager tests: add ASSERT_NE null checks before dereferencing - LocalSpaceTranslate test: verify exact delta and space persistence 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/mainwindow.cpp (1)
567-574:⚠️ Potential issue | 🟡 MinorHandle submesh-only selections in the status bar.
With the new Node/Mesh/Submesh tree, a pure
SubEntityselection still falls through to"No selection"because this branch only checks nodes/entities. Add a submesh branch, or at least fall back to!sel->isEmpty()before reporting nothing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 567 - 574, SelectionSet-based status bar logic currently reports "No selection" for pure submesh/subentity selections because it only checks sel->hasNodes() and sel->hasEntities(); update the branch in mainwindow.cpp to handle submesh/subentity selections (e.g. check sel->hasSubEntities() or sel->hasSubMeshes() and use sel->getSubEntitiesCount()/getSubMeshesCount()) or at minimum replace the final else with a check like if (!sel->isEmpty()) to report a generic selection count; modify the block around SelectionSet::getSingleton() / sel to include that additional branch or fallback so submesh-only selections are reflected in statusMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mainwindow.cpp`:
- Around line 388-403: The action and dock are out of sync: wire
pAnimationControlWidget's visibilityChanged(bool) to update
ui->actionAnimation_Control->setChecked(visible) so the QAction reflects
user-closed docks, and also update the selection-change lambda to set the
QAction checked state instead of only calling
pAnimationControlWidget->setVisible; specifically, add a connect from
pAnimationControlWidget (the dock widget) visibilityChanged(bool) to a
slot/lambda that calls ui->actionAnimation_Control->setChecked(visible), and in
the SelectionSet::getSingleton() handler set both
ui->actionAnimation_Control->setChecked(hasAnims && toggled) (or
setChecked(hasAnims && ui->actionAnimation_Control->isChecked())) so the menu
and dock stay synchronized and user-closed docks are not reopened unexpectedly.
- Around line 367-381: The PropertiesPanelController is storing the local
pAnimationWidget raw pointer via setAnimationWidget(pAnimationWidget), which
will dangle when MainWindow destroys the widget; fix by either (A) making
pAnimationWidget a MainWindow member (e.g., mAnimationWidget) so its lifetime
matches the window and explicitly clear
PropertiesPanelController::instance()->setAnimationWidget(nullptr) in
MainWindow::~MainWindow(), or (B) change the controller to store a
QPointer<AnimationWidget> (mAnimationWidget) and adjust setAnimationWidget to
accept and keep a QPointer so it automatically becomes null when the widget is
deleted; update the connect usage to reference the member if you choose option
A.
- Around line 275-282: Remove the conditional that suppresses UI refresh when
undo/redo clears the selection: inside the QUndoStack::indexChanged lambda
(connected via UndoManager::getSingleton()->stack()), always trigger the
deferred QTimer singleShot callback to re-trigger the selection update from
SelectionSet::getSingleton(); do not skip emitting sel->selectionChanged() when
the selection is empty—ensure you still null-check sel before emitting so the
gizmo/Inspector/dock updates correctly even after the selection is cleared.
- Around line 290-294: Remove the duplicate QML singleton registration in
mainwindow.cpp: delete the
qmlRegisterSingletonType<PropertiesPanelController>(...) call that returns
PropertiesPanelController::qmlInstance(engine, nullptr) and rely on the existing
global registration in main.cpp; ensure no other local singleton registration
for PropertiesPanelController remains so the QML module-level registration (the
one in main.cpp) is the only registration used.
---
Duplicate comments:
In `@src/mainwindow.cpp`:
- Around line 567-574: SelectionSet-based status bar logic currently reports "No
selection" for pure submesh/subentity selections because it only checks
sel->hasNodes() and sel->hasEntities(); update the branch in mainwindow.cpp to
handle submesh/subentity selections (e.g. check sel->hasSubEntities() or
sel->hasSubMeshes() and use sel->getSubEntitiesCount()/getSubMeshesCount()) or
at minimum replace the final else with a check like if (!sel->isEmpty()) to
report a generic selection count; modify the block around
SelectionSet::getSingleton() / sel to include that additional branch or fallback
so submesh-only selections are reflected in statusMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bbcfe06-5a9f-4fae-8b28-35f65df8a867
📒 Files selected for processing (6)
CLAUDE.mdsrc/CMakeLists.txtsrc/main.cppsrc/mainwindow.cppsrc/qml_resources.qrctests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (4)
- src/main.cpp
- src/qml_resources.qrc
- src/CMakeLists.txt
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/CMakeLists.txt
Comment on lines
+275
to
+282
| // Refresh gizmo position after undo/redo (deferred to avoid re-entrant scene access) | ||
| connect(UndoManager::getSingleton()->stack(), &QUndoStack::indexChanged, this, [](int) { | ||
| QTimer::singleShot(0, []() { | ||
| // Re-trigger selection to update gizmo position and Inspector values | ||
| auto* sel = SelectionSet::getSingleton(); | ||
| if (!sel->isEmpty()) | ||
| emit sel->selectionChanged(); | ||
| }); |
There was a problem hiding this comment.
Refresh selection-dependent UI even when undo clears the selection.
The if (!sel->isEmpty()) guard skips the refresh path exactly when undo/redo leaves nothing selected, so the gizmo/Inspector/dock can stay stuck on the previous object state.
Suggested change
connect(UndoManager::getSingleton()->stack(), &QUndoStack::indexChanged, this, [](int) {
QTimer::singleShot(0, []() {
// Re-trigger selection to update gizmo position and Inspector values
auto* sel = SelectionSet::getSingleton();
- if (!sel->isEmpty())
- emit sel->selectionChanged();
+ emit sel->selectionChanged();
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 275 - 282, Remove the conditional that
suppresses UI refresh when undo/redo clears the selection: inside the
QUndoStack::indexChanged lambda (connected via
UndoManager::getSingleton()->stack()), always trigger the deferred QTimer
singleShot callback to re-trigger the selection update from
SelectionSet::getSingleton(); do not skip emitting sel->selectionChanged() when
the selection is empty—ensure you still null-check sel before emitting so the
gizmo/Inspector/dock updates correctly even after the selection is cleared.
Comment on lines
+290
to
+294
| // Register PropertiesPanelController in this widget's engine | ||
| qmlRegisterSingletonType<PropertiesPanelController>("PropertiesPanel", 1, 0, "PropertiesPanelController", | ||
| [](QQmlEngine* engine, QJSEngine*) -> QObject* { | ||
| return PropertiesPanelController::qmlInstance(engine, nullptr); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify there is a single registration site for the QML singleton.
rg -n -C2 'qmlRegisterSingletonType<PropertiesPanelController>|PropertiesPanelController::qmlInstance' \
src/main.cpp src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1030
Remove the duplicate singleton registration in mainwindow.cpp.
src/main.cpp already registers PropertiesPanelController as a QML singleton at module level (lines 213-217). The registration in mainwindow.cpp (lines 290-294) is a duplicate with a different factory signature (passing nullptr instead of the actual scriptEngine). Registering the same singleton twice globally causes conflicts and unpredictable behavior. Remove the duplicate registration here and rely on the global registration from main.cpp.
Suggested change
- // Register PropertiesPanelController in this widget's engine
- qmlRegisterSingletonType<PropertiesPanelController>("PropertiesPanel", 1, 0, "PropertiesPanelController",
- [](QQmlEngine* engine, QJSEngine*) -> QObject* {
- return PropertiesPanelController::qmlInstance(engine, nullptr);
- });
-
m_propertiesPanel->setSource(QUrl("qrc:/PropertiesPanel/PropertiesPanel.qml"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 290 - 294, Remove the duplicate QML
singleton registration in mainwindow.cpp: delete the
qmlRegisterSingletonType<PropertiesPanelController>(...) call that returns
PropertiesPanelController::qmlInstance(engine, nullptr) and rely on the existing
global registration in main.cpp; ensure no other local singleton registration
for PropertiesPanelController remains so the QML module-level registration (the
one in main.cpp) is the only registration used.
Comment on lines
+367
to
+381
| auto pAnimationWidget = new AnimationWidget(this); | ||
| pAnimationWidget->hide(); | ||
|
|
||
| // Add Animation Control Widget to the bottom of the main window | ||
| // Animation Control Widget — bottom dock, auto-shown for animated entities | ||
| auto pAnimationControlWidget = new AnimationControlWidget(this); | ||
| addDockWidget(Qt::BottomDockWidgetArea, pAnimationControlWidget); | ||
| // Constrain height so it doesn't eat viewport space | ||
| resizeDocks({pAnimationControlWidget}, {180}, Qt::Vertical); | ||
| pAnimationControlWidget->setVisible(false); | ||
| connect(pAnimationWidget,SIGNAL(changeAnimationName(const std::string&)),pAnimationControlWidget,SLOT(updateAnimationTree())); | ||
|
|
||
| //connect(m_pTransformWidget, SIGNAL(selectionChanged(QString)), pAnimationWidget, SLOT(updateAnimationTable())); | ||
| connect(pAnimationWidget,SIGNAL(changeAnimationState(bool)),this,SLOT(setPlaying(bool))); | ||
| connect(ui->tabWidget,&QTabWidget::currentChanged,this,[=](int index){ | ||
| if(index==3){ | ||
| // Add Animation Control Widget to the bottom of the main window | ||
|
|
||
| // Give PropertiesPanelController access to AnimationWidget for skeleton/weight toggles | ||
| PropertiesPanelController::instance()->setAnimationWidget(pAnimationWidget); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the controller stores a raw AnimationWidget pointer and that MainWindow
# never clears it before teardown.
rg -n -C3 'setAnimationWidget|mAnimationWidget' \
src/PropertiesPanelController.h src/mainwindow.cpp
rg -n -C3 '~MainWindow|Manager::kill|setAnimationWidget\(nullptr\)|AnimationWidget\(' \
src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3107
Don't let PropertiesPanelController keep a dead AnimationWidget*.
pAnimationWidget is created as a local variable and passed to PropertiesPanelController::instance()->setAnimationWidget(pAnimationWidget), where it's stored as a raw pointer mAnimationWidget. Since MainWindow::~MainWindow() never clears this pointer before the widget is destroyed, PropertiesPanelController will hold a dangling pointer after the window closes.
Either promote pAnimationWidget to a MainWindow member and clear it in the destructor, or use QPointer<AnimationWidget> in the controller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 367 - 381, The PropertiesPanelController is
storing the local pAnimationWidget raw pointer via
setAnimationWidget(pAnimationWidget), which will dangle when MainWindow destroys
the widget; fix by either (A) making pAnimationWidget a MainWindow member (e.g.,
mAnimationWidget) so its lifetime matches the window and explicitly clear
PropertiesPanelController::instance()->setAnimationWidget(nullptr) in
MainWindow::~MainWindow(), or (B) change the controller to store a
QPointer<AnimationWidget> (mAnimationWidget) and adjust setAnimationWidget to
accept and keep a QPointer so it automatically becomes null when the widget is
deleted; update the connect usage to reference the member if you choose option
A.
Comment on lines
+388
to
403
| // Toggle Animation Control visibility from menu | ||
| connect(ui->actionAnimation_Control, &QAction::toggled, this, [pAnimationControlWidget, this](bool checked) { | ||
| if (checked && PropertiesPanelController::instance()->hasAnimations()) | ||
| pAnimationControlWidget->setVisible(true); | ||
| addDockWidget(Qt::BottomDockWidgetArea, pAnimationControlWidget); | ||
| } else { | ||
| // Remove Animation Control Widget from the bottom of the main window | ||
| auto dockWidget = findChild<QDockWidget*>("AnimationControlWidget"); | ||
| if (dockWidget) { | ||
| pAnimationControlWidget->setVisible(false); | ||
| removeDockWidget(dockWidget); | ||
| } | ||
| } | ||
| else if (!checked) | ||
| pAnimationControlWidget->setVisible(false); | ||
| }); | ||
|
|
||
| // Auto-show/hide Animation Control based on selection (respects menu toggle) | ||
| connect(SelectionSet::getSingleton(), &SelectionSet::selectionChanged, this, [pAnimationControlWidget, this]() { | ||
| QTimer::singleShot(0, pAnimationControlWidget, [pAnimationControlWidget, this]() { | ||
| bool hasAnims = PropertiesPanelController::instance()->hasAnimations(); | ||
| bool toggled = ui->actionAnimation_Control->isChecked(); | ||
| pAnimationControlWidget->setVisible(hasAnims && toggled); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Keep the Animation Control action synced with dock visibility.
Right now the action only drives the dock. If the user closes the dock directly, the action stays checked, and the next selection change reopens it immediately.
Suggested change
connect(ui->actionAnimation_Control, &QAction::toggled, this, [pAnimationControlWidget, this](bool checked) {
if (checked && PropertiesPanelController::instance()->hasAnimations())
pAnimationControlWidget->setVisible(true);
else if (!checked)
pAnimationControlWidget->setVisible(false);
});
+ connect(pAnimationControlWidget, &QDockWidget::visibilityChanged,
+ ui->actionAnimation_Control, &QAction::setChecked);
// Auto-show/hide Animation Control based on selection (respects menu toggle)
connect(SelectionSet::getSingleton(), &SelectionSet::selectionChanged, this, [pAnimationControlWidget, this]() {📝 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
| // Toggle Animation Control visibility from menu | |
| connect(ui->actionAnimation_Control, &QAction::toggled, this, [pAnimationControlWidget, this](bool checked) { | |
| if (checked && PropertiesPanelController::instance()->hasAnimations()) | |
| pAnimationControlWidget->setVisible(true); | |
| addDockWidget(Qt::BottomDockWidgetArea, pAnimationControlWidget); | |
| } else { | |
| // Remove Animation Control Widget from the bottom of the main window | |
| auto dockWidget = findChild<QDockWidget*>("AnimationControlWidget"); | |
| if (dockWidget) { | |
| pAnimationControlWidget->setVisible(false); | |
| removeDockWidget(dockWidget); | |
| } | |
| } | |
| else if (!checked) | |
| pAnimationControlWidget->setVisible(false); | |
| }); | |
| // Auto-show/hide Animation Control based on selection (respects menu toggle) | |
| connect(SelectionSet::getSingleton(), &SelectionSet::selectionChanged, this, [pAnimationControlWidget, this]() { | |
| QTimer::singleShot(0, pAnimationControlWidget, [pAnimationControlWidget, this]() { | |
| bool hasAnims = PropertiesPanelController::instance()->hasAnimations(); | |
| bool toggled = ui->actionAnimation_Control->isChecked(); | |
| pAnimationControlWidget->setVisible(hasAnims && toggled); | |
| }); | |
| }); | |
| // Toggle Animation Control visibility from menu | |
| connect(ui->actionAnimation_Control, &QAction::toggled, this, [pAnimationControlWidget, this](bool checked) { | |
| if (checked && PropertiesPanelController::instance()->hasAnimations()) | |
| pAnimationControlWidget->setVisible(true); | |
| else if (!checked) | |
| pAnimationControlWidget->setVisible(false); | |
| }); | |
| connect(pAnimationControlWidget, &QDockWidget::visibilityChanged, | |
| ui->actionAnimation_Control, &QAction::setChecked); | |
| // Auto-show/hide Animation Control based on selection (respects menu toggle) | |
| connect(SelectionSet::getSingleton(), &SelectionSet::selectionChanged, this, [pAnimationControlWidget, this]() { | |
| QTimer::singleShot(0, pAnimationControlWidget, [pAnimationControlWidget, this]() { | |
| bool hasAnims = PropertiesPanelController::instance()->hasAnimations(); | |
| bool toggled = ui->actionAnimation_Control->isChecked(); | |
| pAnimationControlWidget->setVisible(hasAnims && toggled); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 388 - 403, The action and dock are out of
sync: wire pAnimationControlWidget's visibilityChanged(bool) to update
ui->actionAnimation_Control->setChecked(visible) so the QAction reflects
user-closed docks, and also update the selection-change lambda to set the
QAction checked state instead of only calling
pAnimationControlWidget->setVisible; specifically, add a connect from
pAnimationControlWidget (the dock widget) visibilityChanged(bool) to a
slot/lambda that calls ui->actionAnimation_Control->setChecked(visible), and in
the SelectionSet::getSingleton() handler set both
ui->actionAnimation_Control->setChecked(hasAnims && toggled) (or
setChecked(hasAnims && ui->actionAnimation_Control->isChecked())) so the menu
and dock stay synchronized and user-closed docks are not reopened unexpectedly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/mainwindow.cpp (4)
275-283:⚠️ Potential issue | 🟡 MinorUI not refreshed when undo/redo clears the selection.
The
if (!sel->isEmpty())guard skips emittingselectionChanged()when undo/redo leaves nothing selected. This can leave the gizmo, Inspector, or dock showing stale state.🐛 Proposed fix
connect(UndoManager::getSingleton()->stack(), &QUndoStack::indexChanged, this, [](int) { QTimer::singleShot(0, []() { // Re-trigger selection to update gizmo position and Inspector values auto* sel = SelectionSet::getSingleton(); - if (!sel->isEmpty()) - emit sel->selectionChanged(); + if (sel) + emit sel->selectionChanged(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 275 - 283, The current indexChanged handler defers and then only emits selectionChanged() when SelectionSet::getSingleton()->isEmpty() is false, which prevents UI refresh when undo/redo results in an empty selection; change the lambda used with QTimer::singleShot to always emit sel->selectionChanged() (after verifying sel is non-null) so the gizmo, Inspector and docks are updated even when the selection is cleared; update the anonymous lambda attached to UndoManager::getSingleton()->stack() accordingly.
290-294:⚠️ Potential issue | 🟠 MajorDuplicate QML singleton registration.
main.cppalready registersPropertiesPanelControlleras a QML singleton globally. This local registration inmainwindow.cppwith a different factory signature (passingnullptrforscriptEngine) creates a duplicate that can cause unpredictable behavior or conflicts.🐛 Proposed fix - remove the duplicate registration
m_propertiesPanel = new QQuickWidget(); m_propertiesPanel->setResizeMode(QQuickWidget::SizeRootObjectToView); - // Register PropertiesPanelController in this widget's engine - qmlRegisterSingletonType<PropertiesPanelController>("PropertiesPanel", 1, 0, "PropertiesPanelController", - [](QQmlEngine* engine, QJSEngine*) -> QObject* { - return PropertiesPanelController::qmlInstance(engine, nullptr); - }); - m_propertiesPanel->setSource(QUrl("qrc:/PropertiesPanel/PropertiesPanel.qml"));#!/bin/bash # Verify singleton registration exists in main.cpp rg -n 'qmlRegisterSingletonType.*PropertiesPanelController' src/main.cpp src/mainwindow.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 290 - 294, The local QML singleton registration in mainwindow.cpp using qmlRegisterSingletonType<PropertiesPanelController> and the lambda that calls PropertiesPanelController::qmlInstance(engine, nullptr) duplicates the global registration already done in main.cpp (different factory/signature) and must be removed; delete this qmlRegisterSingletonType call (the lambda block) from mainwindow.cpp so the global registration in main.cpp is the single source of truth and ensure any usages rely on that singleton.
367-381:⚠️ Potential issue | 🟠 Major
AnimationWidget*can become dangling.
pAnimationWidgetis passed toPropertiesPanelController::setAnimationWidget()and stored as a raw pointer. SincePropertiesPanelControlleris a QML singleton that persists afterMainWindowis destroyed, any subsequent calls tomAnimationWidget->isSkeletonDebugActive()etc. would dereference freed memory.🐛 Proposed fix - use QPointer in the controller
In
src/PropertiesPanelController.h:+#include <QPointer> // ... private: - class AnimationWidget* mAnimationWidget = nullptr; + QPointer<AnimationWidget> mAnimationWidget;The
QPointerwill automatically becomenullptrwhen the widget is deleted, making the existing null checks safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 367 - 381, The AnimationWidget pointer passed as pAnimationWidget into PropertiesPanelController::setAnimationWidget can dangle after MainWindow is destroyed; update PropertiesPanelController to store the widget as a QPointer<AnimationWidget> (add a member like QPointer<AnimationWidget> mAnimationWidget), keep setAnimationWidget(AnimationWidget* widget) but assign it into the QPointer, and ensure all uses of mAnimationWidget check for null before dereferencing (e.g., replace raw pointer checks with if (mAnimationWidget) ...). This makes the stored pointer automatically become nullptr when the widget is deleted and avoids use-after-free errors.
388-403:⚠️ Potential issue | 🟡 MinorAnimation Control action not synced with dock visibility.
If the user closes the dock directly (via the X button),
ui->actionAnimation_Controlstays checked, causing the next selection change to reopen the dock unexpectedly.🐛 Proposed fix - sync action with dock visibility
connect(ui->actionAnimation_Control, &QAction::toggled, this, [pAnimationControlWidget, this](bool checked) { if (checked && PropertiesPanelController::instance()->hasAnimations()) pAnimationControlWidget->setVisible(true); else if (!checked) pAnimationControlWidget->setVisible(false); }); + connect(pAnimationControlWidget, &QDockWidget::visibilityChanged, + ui->actionAnimation_Control, &QAction::setChecked); // Auto-show/hide Animation Control based on selection (respects menu toggle)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 388 - 403, The action and dock can get out of sync when the user closes the dock directly; hook the dock widget's visibilityChanged(bool) signal to update ui->actionAnimation_Control so the action reflects the dock state: add a connection from pAnimationControlWidget's visibilityChanged(bool) signal to a lambda that calls ui->actionAnimation_Control->setChecked(visible) (use pAnimationControlWidget and ui->actionAnimation_Control identifiers), ensuring the action is kept in sync when the dock is closed via the X button.src/PropertiesPanelController.cpp (3)
203-213:⚠️ Potential issue | 🟠 Major
getSelectedPrimitive()misses Entity/SubEntity selections.This function only handles
SceneNodeselections. When the user selects a Mesh or Submesh row in the scene tree (which resolves toOgre::Entity/Ogre::SubEntity), the function returnsnullptr, causing the primitive editor to disappear for the same object.🐛 Proposed fix to also resolve Entity/SubEntity selections
static PrimitiveObject* getSelectedPrimitive() { auto* sel = SelectionSet::getSingleton(); if (sel->hasNodes() && sel->getNodesCount() == 1) { Ogre::SceneNode* node = sel->getSceneNode(0); if (PrimitiveObject::isPrimitive(node)) return PrimitiveObject::getPrimitiveFromSceneNode(node); } + // Also handle single entity/subentity selection + if (sel->hasEntities() && sel->getEntitiesCount() == 1) + { + Ogre::Entity* ent = sel->getEntity(0); + if (Ogre::SceneNode* node = ent->getParentSceneNode()) + { + if (PrimitiveObject::isPrimitive(node)) + return PrimitiveObject::getPrimitiveFromSceneNode(node); + } + } + if (sel->hasSubEntities() && sel->getSubEntitiesCount() == 1) + { + Ogre::SubEntity* sub = sel->getSubEntity(0); + if (Ogre::SceneNode* node = sub->getParent()->getParentSceneNode()) + { + if (PrimitiveObject::isPrimitive(node)) + return PrimitiveObject::getPrimitiveFromSceneNode(node); + } + } return nullptr; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 203 - 213, getSelectedPrimitive currently only checks SelectionSet::getSceneNode(0) so Entity or SubEntity selections yield nullptr; update the function to also detect when the selected item is an Ogre::Entity or Ogre::SubEntity (via SelectionSet APIs that return the selected movable/object), obtain the associated SceneNode from the entity/sub-entity (e.g. parent scene node or the entity's getParentSceneNode), then call PrimitiveObject::isPrimitive and PrimitiveObject::getPrimitiveFromSceneNode on that node so entities/subentities resolve to the same PrimitiveObject as their SceneNode.
482-492:⚠️ Potential issue | 🟠 MajorTransform cache not refreshed on selection change.
onSelectionChanged()emitstransformChanged()without first updatingmPosX/Y/Z,mRotX/Y/Z, andmScaleX/Y/Zfrom the newly selected object. The cached values only update whenTransformOperatoremitsselectedPositionChanged/etc., but those signals may not fire on selection change. This causes the inspector to display stale transform values until the user edits them.🐛 Proposed fix
void PropertiesPanelController::onSelectionChanged() { + // Refresh cached transform values from the new selection + auto* sel = SelectionSet::getSingleton(); + if (sel->hasNodes() && sel->getNodesCount() == 1) + { + Ogre::SceneNode* node = sel->getSceneNode(0); + const Ogre::Vector3& pos = node->getPosition(); + mPosX = pos.x; mPosY = pos.y; mPosZ = pos.z; + const Ogre::Quaternion& q = node->getOrientation(); + Ogre::Vector3 rot; + Ogre::Matrix3 rotMat; + q.ToRotationMatrix(rotMat); + rotMat.ToEulerAnglesXYZ(Ogre::Radian(rot.x), Ogre::Radian(rot.y), Ogre::Radian(rot.z)); + mRotX = Ogre::Degree(rot.x).valueDegrees(); + mRotY = Ogre::Degree(rot.y).valueDegrees(); + mRotZ = Ogre::Degree(rot.z).valueDegrees(); + const Ogre::Vector3& scale = node->getScale(); + mScaleX = scale.x; mScaleY = scale.y; mScaleZ = scale.z; + } onTransformChanged(); emit selectionChanged(); emit primitiveChanged(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 482 - 492, onSelectionChanged() currently emits transformChanged() without refreshing the cached transform fields, so the inspector can show stale mPosX/mPosY/mPosZ, mRotX/mRotY/mRotZ, mScaleX/mScaleY/mScaleZ; update onSelectionChanged() to pull the newly selected object's transform into those member variables before calling onTransformChanged()/emitting transformChanged(); specifically locate PropertiesPanelController::onSelectionChanged and add logic to query the current selected primitive (or use TransformOperator's API) to set mPosX/Y/Z, mRotX/Y/Z and mScaleX/Y/Z from the selection, then call onTransformChanged()/emit transformChanged() so the UI gets the refreshed values.
448-477:⚠️ Potential issue | 🟠 Major
renameAnimation()clears state before rename succeeds.Lines 448-463 disable skeleton debug, bone weights, playback, and all animation states before calling
SkeletonTransform::renameAnimation(). If the rename fails (returnsfalse), these side effects are not rolled back, leaving the animation system in a disabled state.🐛 Proposed fix to defer cleanup until rename succeeds
bool PropertiesPanelController::renameAnimation(const QString& entityName, const QString& oldName, const QString& newName) { if (newName.isEmpty() || oldName == newName) return false; auto entities = SelectionSet::getSingleton()->getResolvedEntities(); for (Ogre::Entity* ent : entities) { if (QString::fromStdString(ent->getName()) != entityName) continue; if (Manager::getSingleton()->hasAnimationName(ent, newName)) return false; - // Disable skeleton debug/weights and stop playback before rename - // (rename recreates entity internals, stale pointers would crash) - if (mAnimationWidget) - { - // disableAllSkeletonDebug is private, but toggleSkeletonDebug(false) cleans up - if (mAnimationWidget->isSkeletonDebugActive(ent)) - mAnimationWidget->toggleSkeletonDebug(ent, false); - if (mAnimationWidget->isBoneWeightsShown(ent)) - mAnimationWidget->toggleBoneWeights(ent, false); - } - setPlaying(false); - if (auto* animSet = ent->getAllAnimationStates()) - { - for (const auto& [key, state] : animSet->getAnimationStates()) - state->setEnabled(false); - } - if (SkeletonTransform::renameAnimation(ent, oldName, newName)) { + // Disable skeleton debug/weights and stop playback after successful rename + // (rename recreates entity internals, stale pointers would crash) + if (mAnimationWidget) + { + if (mAnimationWidget->isSkeletonDebugActive(ent)) + mAnimationWidget->toggleSkeletonDebug(ent, false); + if (mAnimationWidget->isBoneWeightsShown(ent)) + mAnimationWidget->toggleBoneWeights(ent, false); + } + setPlaying(false); + if (auto* animSet = ent->getAllAnimationStates()) + { + for (const auto& [key, state] : animSet->getAnimationStates()) + state->setEnabled(false); + } + // Re-select to refresh widgets with new entity data auto nodes = SelectionSet::getSingleton()->getNodesSelectionList(); SelectionSet::getSingleton()->clear(); for (auto* node : nodes) SelectionSet::getSingleton()->append(node); emit animationStateChanged(); return true; } } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PropertiesPanelController.cpp` around lines 448 - 477, The current code disables skeleton debug/bone weights, stops playback (setPlaying(false)) and disables all animation states before calling SkeletonTransform::renameAnimation(ent, oldName, newName), which leaves the system disabled if the rename fails; move those side-effectful cleanup steps so they only run after SkeletonTransform::renameAnimation returns true (i.e., perform the rename first, and only if it succeeds then call mAnimationWidget->toggleSkeletonDebug/ toggleBoneWeights, setPlaying(false), iterate animSet->getAnimationStates() to setEnabled(false), refresh SelectionSet and emit animationStateChanged()), or alternatively capture and restore prior states on failure—refer to mAnimationWidget, setPlaying, animSet/getAnimationStates, SkeletonTransform::renameAnimation, SelectionSet::getSingleton and animationStateChanged to locate and update the logic.qml/SceneTreeNode.qml (1)
111-123:⚠️ Potential issue | 🟡 MinorAdd width constraint to
nameLabelsoelidecan take effect.Without an explicit width, the
Textelement expands to fit the full text, makingelide: Text.ElideRightineffective. Long node names can overflow and obscure the material selector.🐛 Proposed fix
// Name + type label Text { id: nameLabel + width: Math.max(0, treeNode.width - x - (matSelector.visible ? matSelector.width + 12 : 8)) + clip: true text: {Alternatively, use anchors:
anchors.right: matSelector.visible ? matSelector.left : parent.right anchors.rightMargin: matSelector.visible ? 4 : 8🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/SceneTreeNode.qml` around lines 111 - 123, The Text element id nameLabel currently has no width constraint so elide: Text.ElideRight never activates; constrain nameLabel's right edge (or set an explicit width) so long names are truncated instead of overflowing—update nameLabel to set anchors.right to matSelector.left when matSelector.visible (or parent.right when not), and add anchors.rightMargin (e.g., 4 when matSelector.visible, 8 otherwise) so the label respects available space; reference ids: nameLabel, treeModel, treeNode, nodeIndex, matSelector.
🧹 Nitpick comments (2)
src/TransformOperator_test.cpp (2)
729-742: Assert emittedtransformSpaceChangedpayload values, not only count.
spy.count()alone can pass even if the signal emits the wrong enum value. Add payload checks for both emissions.Suggested test hardening
TEST_F(TransformOperatorTestFixture, TransformSpaceChangedSignal) { TransformOperator* instance = TransformOperator::getSingleton(); QSignalSpy spy(instance, &TransformOperator::transformSpaceChanged); instance->setTransformSpace(TransformOperator::SPACE_LOCAL); - EXPECT_EQ(spy.count(), 1); + ASSERT_EQ(spy.count(), 1); + ASSERT_EQ(spy.at(0).size(), 1); + EXPECT_EQ(spy.at(0).at(0).toInt(), static_cast<int>(TransformOperator::SPACE_LOCAL)); // Setting same value should not emit instance->setTransformSpace(TransformOperator::SPACE_LOCAL); EXPECT_EQ(spy.count(), 1); instance->toggleTransformSpace(); - EXPECT_EQ(spy.count(), 2); + ASSERT_EQ(spy.count(), 2); + ASSERT_EQ(spy.at(1).size(), 1); + EXPECT_EQ(spy.at(1).at(0).toInt(), static_cast<int>(TransformOperator::SPACE_WORLD)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator_test.cpp` around lines 729 - 742, The test currently only checks QSignalSpy::count() which can miss incorrect payloads; update TransformOperatorTestFixture::TransformSpaceChangedSignal to inspect the signal arguments from the QSignalSpy (spy) after each emit: after instance->setTransformSpace(TransformOperator::SPACE_LOCAL) assert the first spy.takeFirst() (or spy[0]) contains the expected enum value TransformOperator::SPACE_LOCAL, ensure the second emission after instance->toggleTransformSpace() contains the toggled enum value (e.g., TransformOperator::SPACE_GLOBAL or the appropriate enum), and keep the existing count assertions to verify no extra signals were emitted when setting the same value twice.
758-789: Consider renaming this test to match asserted behavior.The test runs in
SPACE_LOCALbut explicitly validates world-space delta behavior fortranslateSelected. A more explicit name would reduce ambiguity for future maintainers.Possible rename for clarity
-TEST_F(TransformOperatorTestFixture, LocalSpaceTranslate) { +TEST_F(TransformOperatorTestFixture, TranslateSelected_UsesWorldDeltaEvenInLocalSpace) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator_test.cpp` around lines 758 - 789, Rename the test function to make its asserted behavior explicit: change TEST_F(TransformOperatorTestFixture, LocalSpaceTranslate) to a name reflecting that translateSelected applies a world-space delta even when the operator is in SPACE_LOCAL (e.g., TranslateSelected_WorldSpaceDeltaWhenSpaceLocal or LocalSpace_SetButTranslateSelectedUsesWorldSpace); update any references to this test name in the test suite or CI configs and keep the body intact (symbols to locate: TransformOperator::SPACE_LOCAL, translateSelected, getTransformSpace, TransformOperator::SPACE_WORLD, TEST_F(TransformOperatorTestFixture,...)).
🤖 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 84-108: Replace the hardcoded role number 259 in SceneTreeNode.qml
(treeModel.data(nodeIndex, 259)) with a named role exposed to QML: update the
C++ model that defines Roles to use Q_ENUM(Roles) (or add a Q_PROPERTY/static
int accessor) so QML can reference treeModel.TypeLabelRole, then change the QML
calls to treeModel.data(nodeIndex, treeModel.TypeLabelRole); alternatively, add
a Q_INVOKABLE on the model (e.g., getTypeLabel(int nodeIndex) similar to
materialName()) and call treeModel.getTypeLabel(nodeIndex) from the QML Text
element to remove the magic number.
---
Duplicate comments:
In `@qml/SceneTreeNode.qml`:
- Around line 111-123: The Text element id nameLabel currently has no width
constraint so elide: Text.ElideRight never activates; constrain nameLabel's
right edge (or set an explicit width) so long names are truncated instead of
overflowing—update nameLabel to set anchors.right to matSelector.left when
matSelector.visible (or parent.right when not), and add anchors.rightMargin
(e.g., 4 when matSelector.visible, 8 otherwise) so the label respects available
space; reference ids: nameLabel, treeModel, treeNode, nodeIndex, matSelector.
In `@src/mainwindow.cpp`:
- Around line 275-283: The current indexChanged handler defers and then only
emits selectionChanged() when SelectionSet::getSingleton()->isEmpty() is false,
which prevents UI refresh when undo/redo results in an empty selection; change
the lambda used with QTimer::singleShot to always emit sel->selectionChanged()
(after verifying sel is non-null) so the gizmo, Inspector and docks are updated
even when the selection is cleared; update the anonymous lambda attached to
UndoManager::getSingleton()->stack() accordingly.
- Around line 290-294: The local QML singleton registration in mainwindow.cpp
using qmlRegisterSingletonType<PropertiesPanelController> and the lambda that
calls PropertiesPanelController::qmlInstance(engine, nullptr) duplicates the
global registration already done in main.cpp (different factory/signature) and
must be removed; delete this qmlRegisterSingletonType call (the lambda block)
from mainwindow.cpp so the global registration in main.cpp is the single source
of truth and ensure any usages rely on that singleton.
- Around line 367-381: The AnimationWidget pointer passed as pAnimationWidget
into PropertiesPanelController::setAnimationWidget can dangle after MainWindow
is destroyed; update PropertiesPanelController to store the widget as a
QPointer<AnimationWidget> (add a member like QPointer<AnimationWidget>
mAnimationWidget), keep setAnimationWidget(AnimationWidget* widget) but assign
it into the QPointer, and ensure all uses of mAnimationWidget check for null
before dereferencing (e.g., replace raw pointer checks with if
(mAnimationWidget) ...). This makes the stored pointer automatically become
nullptr when the widget is deleted and avoids use-after-free errors.
- Around line 388-403: The action and dock can get out of sync when the user
closes the dock directly; hook the dock widget's visibilityChanged(bool) signal
to update ui->actionAnimation_Control so the action reflects the dock state: add
a connection from pAnimationControlWidget's visibilityChanged(bool) signal to a
lambda that calls ui->actionAnimation_Control->setChecked(visible) (use
pAnimationControlWidget and ui->actionAnimation_Control identifiers), ensuring
the action is kept in sync when the dock is closed via the X button.
In `@src/PropertiesPanelController.cpp`:
- Around line 203-213: getSelectedPrimitive currently only checks
SelectionSet::getSceneNode(0) so Entity or SubEntity selections yield nullptr;
update the function to also detect when the selected item is an Ogre::Entity or
Ogre::SubEntity (via SelectionSet APIs that return the selected movable/object),
obtain the associated SceneNode from the entity/sub-entity (e.g. parent scene
node or the entity's getParentSceneNode), then call PrimitiveObject::isPrimitive
and PrimitiveObject::getPrimitiveFromSceneNode on that node so
entities/subentities resolve to the same PrimitiveObject as their SceneNode.
- Around line 482-492: onSelectionChanged() currently emits transformChanged()
without refreshing the cached transform fields, so the inspector can show stale
mPosX/mPosY/mPosZ, mRotX/mRotY/mRotZ, mScaleX/mScaleY/mScaleZ; update
onSelectionChanged() to pull the newly selected object's transform into those
member variables before calling onTransformChanged()/emitting
transformChanged(); specifically locate
PropertiesPanelController::onSelectionChanged and add logic to query the current
selected primitive (or use TransformOperator's API) to set mPosX/Y/Z, mRotX/Y/Z
and mScaleX/Y/Z from the selection, then call onTransformChanged()/emit
transformChanged() so the UI gets the refreshed values.
- Around line 448-477: The current code disables skeleton debug/bone weights,
stops playback (setPlaying(false)) and disables all animation states before
calling SkeletonTransform::renameAnimation(ent, oldName, newName), which leaves
the system disabled if the rename fails; move those side-effectful cleanup steps
so they only run after SkeletonTransform::renameAnimation returns true (i.e.,
perform the rename first, and only if it succeeds then call
mAnimationWidget->toggleSkeletonDebug/ toggleBoneWeights, setPlaying(false),
iterate animSet->getAnimationStates() to setEnabled(false), refresh SelectionSet
and emit animationStateChanged()), or alternatively capture and restore prior
states on failure—refer to mAnimationWidget, setPlaying,
animSet/getAnimationStates, SkeletonTransform::renameAnimation,
SelectionSet::getSingleton and animationStateChanged to locate and update the
logic.
---
Nitpick comments:
In `@src/TransformOperator_test.cpp`:
- Around line 729-742: The test currently only checks QSignalSpy::count() which
can miss incorrect payloads; update
TransformOperatorTestFixture::TransformSpaceChangedSignal to inspect the signal
arguments from the QSignalSpy (spy) after each emit: after
instance->setTransformSpace(TransformOperator::SPACE_LOCAL) assert the first
spy.takeFirst() (or spy[0]) contains the expected enum value
TransformOperator::SPACE_LOCAL, ensure the second emission after
instance->toggleTransformSpace() contains the toggled enum value (e.g.,
TransformOperator::SPACE_GLOBAL or the appropriate enum), and keep the existing
count assertions to verify no extra signals were emitted when setting the same
value twice.
- Around line 758-789: Rename the test function to make its asserted behavior
explicit: change TEST_F(TransformOperatorTestFixture, LocalSpaceTranslate) to a
name reflecting that translateSelected applies a world-space delta even when the
operator is in SPACE_LOCAL (e.g.,
TranslateSelected_WorldSpaceDeltaWhenSpaceLocal or
LocalSpace_SetButTranslateSelectedUsesWorldSpace); update any references to this
test name in the test suite or CI configs and keep the body intact (symbols to
locate: TransformOperator::SPACE_LOCAL, translateSelected, getTransformSpace,
TransformOperator::SPACE_WORLD, TEST_F(TransformOperatorTestFixture,...)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88baac53-0a0d-4c8d-ae55-c3db264f59ed
📒 Files selected for processing (5)
qml/SceneTreeNode.qmlsrc/PropertiesPanelController.cppsrc/ThemeManager_test.cppsrc/TransformOperator_test.cppsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ThemeManager_test.cpp
Comment on lines
+84
to
+108
| Text { | ||
| text: { | ||
| if (!treeModel) return "" | ||
| var t = treeModel.data(nodeIndex, 259) | ||
| switch(t) { | ||
| case "Node": return "\u25A0" | ||
| case "Mesh": return "\u25C6" | ||
| case "Submesh": return "\u25CB" | ||
| default: return "\u25A1" | ||
| } | ||
| } | ||
| color: { | ||
| if (treeNode.selected) return "white" | ||
| if (!treeModel) return PropertiesPanelController.textColor | ||
| var t = treeModel.data(nodeIndex, 259) | ||
| switch(t) { | ||
| case "Node": return "#6ca0dc" | ||
| case "Mesh": return "#6cdc6c" | ||
| case "Submesh": return "#dcdc6c" | ||
| default: return PropertiesPanelController.textColor | ||
| } | ||
| } | ||
| font.pixelSize: 10 | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify what role value 259 corresponds to in SceneTreeModel
rg -n 'TypeLabelRole|Qt::UserRole' src/SceneTreeModel.h src/SceneTreeModel.cppRepository: fernandotonon/QtMeshEditor
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Get the full enum definition from SceneTreeModel.h around the TypeLabelRole
sed -n '45,60p' src/SceneTreeModel.hRepository: fernandotonon/QtMeshEditor
Length of output: 469
🏁 Script executed:
#!/bin/bash
# Check how other QML files access model roles to see if there's a pattern
rg -n 'treeModel\.data\(' qml/Repository: fernandotonon/QtMeshEditor
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Check if TypeLabelRole enum is exposed to QML via Q_ENUM
rg -n 'Q_ENUM|Q_PROPERTY.*Role|Q_INVOKABLE' src/SceneTreeModel.hRepository: fernandotonon/QtMeshEditor
Length of output: 551
🏁 Script executed:
#!/bin/bash
# Check how the treeModel property is defined in QML to understand its type
rg -n 'property.*treeModel' qml/SceneTreeNode.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 499
🏁 Script executed:
#!/bin/bash
# Get full SceneTreeModel.h to see complete enum and any Q_ENUM decorator
cat -n src/SceneTreeModel.h | head -80Repository: fernandotonon/QtMeshEditor
Length of output: 2915
🏁 Script executed:
#!/bin/bash
# Check for Qt version in project config files
rg -n 'QT_VERSION|TARGET Qt' . --type cmake --type proRepository: fernandotonon/QtMeshEditor
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check CMakeLists.txt for Qt version
find . -name "CMakeLists.txt" -o -name "*.pro" | xargs grep -l "qt\|Qt" 2>/dev/null | head -3 | xargs cat | grep -i "qt.*version\|find_package.*Qt"Repository: fernandotonon/QtMeshEditor
Length of output: 1947
Expose TypeLabelRole to QML instead of hardcoding 259.
The code uses treeModel.data(nodeIndex, 259) where 259 corresponds to TypeLabelRole. While the value is currently correct, hardcoding it is fragile—if the C++ enum order changes, this will silently break. With Qt 6, add Q_ENUM(Roles) to expose the enum to QML, then use treeModel.TypeLabelRole instead of the magic number. Alternatively, add a Q_INVOKABLE method like getTypeLabel(nodeIndex) following the pattern of the existing materialName() method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/SceneTreeNode.qml` around lines 84 - 108, Replace the hardcoded role
number 259 in SceneTreeNode.qml (treeModel.data(nodeIndex, 259)) with a named
role exposed to QML: update the C++ model that defines Roles to use
Q_ENUM(Roles) (or add a Q_PROPERTY/static int accessor) so QML can reference
treeModel.TypeLabelRole, then change the QML calls to treeModel.data(nodeIndex,
treeModel.TypeLabelRole); alternatively, add a Q_INVOKABLE on the model (e.g.,
getTypeLabel(int nodeIndex) similar to materialName()) and call
treeModel.getTypeLabel(nodeIndex) from the QML Text element to remove the magic
number.
cfg["showSegX"] had `(t != AP_SPRING ? true : true)` which always returns true regardless of condition. Simplified to just `true`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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
Major UI modernization (v2.16.0) replacing the old 4-tab sidebar with a unified QML Inspector panel, adding scale gizmo, undo/redo, and modern editor UX.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI Improvements
Documentation