Add 3D ViewCube navigation overlay - #193
Merged
Merged
Conversation
Adds a QML-rendered 3D navigation cube that tracks the active viewport's camera orientation in real-time. Supports click-to-snap for all 6 faces, 12 edges, and 8 corners, plus arcball drag rotation. The cube auto-hides when no viewport is active and reappears on focus, with a toggle in the Options menu. Also fixes a use-after-free crash when switching viewport layouts and ensures all child windows close with the main window. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ViewCube 3D navigation feature: new QML Window and QML-singleton C++ controller, SpaceCamera orientation animation, MainWindow/OgreWidget integration, build/resource/test additions, a View menu action, and duplicated ViewCube docs in CLAUDE.md. Changes
Sequence DiagramsequenceDiagram
participant User
participant ViewCubeWindow as ViewCubeWindow.qml
participant Controller as ViewCubeController
participant Camera as SpaceCamera
participant Widget as OgreWidget
User->>ViewCubeWindow: Click face / Drag to rotate
ViewCubeWindow->>ViewCubeWindow: Hit-test (corner/edge/face)
alt Snap (click)
ViewCubeWindow->>Controller: snapToView / snapToDirection
Controller->>Camera: animateToOrientation(targetQuat, duration)
Camera->>Camera: frameStarted — SLERP interpolate
Camera->>Widget: apply orientation
Widget->>ViewCubeWindow: render updated view
else Drag (rotate)
ViewCubeWindow->>Controller: rotateByDelta(dx,dy)
Controller->>Camera: set orientation immediately (cancel animation)
Camera->>Widget: apply orientation
Widget->>ViewCubeWindow: render updated view
end
rect rgba(100,150,200,0.5)
ViewCubeWindow->>ViewCubeWindow: Canvas2D render cube, labels, axis
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f7147d02e
ℹ️ 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
+54
to
+55
| { "Left", 0.7071, 0.0, -0.7071, 0.0 }, | ||
| { "Right", 0.7071, 0.0, 0.7071, 0.0 }, |
There was a problem hiding this comment.
The face mapping in snapToView has Left/Right reversed: the current "Left" quaternion rotates the reference view direction (0,0,-1) toward +X, while "Right" rotates toward -X (the opposite of the stated intent in the comment). In practice, clicking Left and Right on the cube will snap to opposite side views, which makes camera navigation incorrect.
Useful? React with 👍 / 👎.
Comment on lines
+134
to
+138
| emit visibilityChanged(m_visible); | ||
| }); | ||
| } | ||
|
|
||
| emit visibilityChanged(m_visible); |
There was a problem hiding this comment.
The visible property is computed by isVisible() (m_visible && m_activeWidget), but this path emits visibilityChanged(m_visible). When the active widget is cleared while the toggle remains enabled, the signal payload reports true even though the property value is false, so listeners that use the signal argument can become inconsistent with the actual ViewCube visibility.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/ViewCubeWindow.qml`:
- Around line 314-401: hitTest() and handleClick() currently only detect corners
and faces, so edge targets never get chosen; add an edge-detection step between
the corner check and face check that queries cubeCanvas.getEdgeDefs(), computes
each edge's projected midpoint (use the two vertex indices on
edge.vertIdxA/vertIdxB from projected[]), test a small hit radius and depth like
corners, and pick the nearest visible edge (z < 0) similar to how corners are
chosen; then update handleClick() to also iterate cubeCanvas.getEdgeDefs() and
when an edge name matches call ViewCubeController.snapToDirection(edge.dir[0],
edge.dir[1], edge.dir[2]) (or the appropriate snapping call for edges) so clicks
on edges snap to the 12 edge orientations.
In `@src/ViewCube/ViewCubeController.cpp`:
- Around line 11-18: Implement a proper destructor (~ViewCubeController) that
clears the global singleton by setting s_instance to nullptr when the object is
destroyed (e.g., if (s_instance == this) s_instance = nullptr), so instance() /
qmlInstance() won’t return freed memory; also update qmlInstance()’s fallback
allocation to avoid leaking raw memory (replace the raw new fallback with a
static local instance or a managed/smart-pointered singleton) so the fallback
path does not leak if ever used.
- Around line 173-190: The eventFilter branch for Hide/Close/Destroy leaves
m_activeWidget set for Hide events and never emits visibilityChanged(), causing
isVisible() to be wrong; in ViewCubeController::eventFilter, when obj ==
m_activeWidget and event type is QEvent::Hide, QEvent::Close or QEvent::Destroy,
set m_activeWidget = nullptr unconditionally, then emit visibilityChanged() (or
call the method that updates/hides the overlay) before returning; this ensures
isVisible() reflects the hidden state and the overlay is removed when the active
viewport hides/closes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5944b7d-9137-4d4f-a0f7-ed40c32fd73f
📒 Files selected for processing (15)
CLAUDE.mdCMakeLists.txtqml/ViewCubeWindow.qmlsrc/CMakeLists.txtsrc/OgreWidget.hsrc/SpaceCamera.cppsrc/SpaceCamera.hsrc/TransformOperator.cppsrc/ViewCube/ViewCubeController.cppsrc/ViewCube/ViewCubeController.hsrc/ViewCube/ViewCubeController_test.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrcui_files/mainwindow.ui
Comment on lines
+11
to
+18
| ViewCubeController::ViewCubeController(QWidget* mainWindow, QObject* parent) | ||
| : QObject(parent) | ||
| , m_mainWindow(mainWindow) | ||
| { | ||
| s_instance = this; | ||
| if (m_mainWindow) | ||
| m_mainWindow->installEventFilter(this); | ||
| } |
There was a problem hiding this comment.
Reset the singleton on teardown.
s_instance is overwritten on construction but never cleared again. After delete controller in src/ViewCube/ViewCubeController_test.cpp Line 21, instance() / qmlInstance() can hand out freed memory until another controller is constructed, and the fallback allocation in qmlInstance() leaks if that path ever runs first.
🧹 Proposed fix
--- a/src/ViewCube/ViewCubeController.h
+++ b/src/ViewCube/ViewCubeController.h
@@
public:
explicit ViewCubeController(QWidget* mainWindow, QObject* parent = nullptr);
+ ~ViewCubeController() override;
static ViewCubeController* qmlInstance(QQmlEngine* engine, QJSEngine* scriptEngine);--- a/src/ViewCube/ViewCubeController.cpp
+++ b/src/ViewCube/ViewCubeController.cpp
@@
ViewCubeController::ViewCubeController(QWidget* mainWindow, QObject* parent)
: QObject(parent)
, m_mainWindow(mainWindow)
{
s_instance = this;
if (m_mainWindow)
m_mainWindow->installEventFilter(this);
}
+
+ViewCubeController::~ViewCubeController()
+{
+ if (s_instance == this)
+ s_instance = nullptr;
+}
@@
ViewCubeController* ViewCubeController::qmlInstance(QQmlEngine* engine, QJSEngine* scriptEngine)
{
Q_UNUSED(engine)
Q_UNUSED(scriptEngine)
-
- if (!s_instance)
- s_instance = new ViewCubeController(nullptr);
- return s_instance;
+ Q_ASSERT_X(s_instance, "ViewCubeController::qmlInstance",
+ "MainWindow must create the ViewCubeController before QML loads");
+ return s_instance;
}Also applies to: 25-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ViewCube/ViewCubeController.cpp` around lines 11 - 18, Implement a proper
destructor (~ViewCubeController) that clears the global singleton by setting
s_instance to nullptr when the object is destroyed (e.g., if (s_instance ==
this) s_instance = nullptr), so instance() / qmlInstance() won’t return freed
memory; also update qmlInstance()’s fallback allocation to avoid leaking raw
memory (replace the raw new fallback with a static local instance or a
managed/smart-pointered singleton) so the fallback path does not leak if ever
used.
Comment on lines
+173
to
+190
| bool ViewCubeController::eventFilter(QObject* obj, QEvent* event) | ||
| { | ||
| auto type = event->type(); | ||
|
|
||
| if (obj == m_activeWidget && | ||
| (type == QEvent::Hide || type == QEvent::Close || type == QEvent::Destroy)) { | ||
| if (type != QEvent::Hide) | ||
| m_activeWidget = nullptr; | ||
| return QObject::eventFilter(obj, event); | ||
| } | ||
|
|
||
| // Reposition when active widget or main window moves/resizes | ||
| if (type == QEvent::Move || type == QEvent::Resize) { | ||
| if (m_visible && m_activeWidget && | ||
| (obj == m_activeWidget || obj == m_mainWindow)) | ||
| reposition(); | ||
| } | ||
| return QObject::eventFilter(obj, event); |
There was a problem hiding this comment.
Hide the overlay when the active viewport hides or closes.
This branch never emits visibilityChanged(), and isVisible() still treats any non-null widget as visible. During layout changes the ViewCube can stay rendered over a hidden/closing viewport until destruction, which breaks the advertised auto-hide behavior.
🙈 Proposed fix
--- a/src/ViewCube/ViewCubeController.h
+++ b/src/ViewCube/ViewCubeController.h
@@
- bool isVisible() const { return m_visible && m_activeWidget; }
+ bool isVisible() const;--- a/src/ViewCube/ViewCubeController.cpp
+++ b/src/ViewCube/ViewCubeController.cpp
@@
+bool ViewCubeController::isVisible() const
+{
+ return m_visible && m_activeWidget && m_activeWidget->isVisible();
+}
+
bool ViewCubeController::eventFilter(QObject* obj, QEvent* event)
{
auto type = event->type();
if (obj == m_activeWidget &&
- (type == QEvent::Hide || type == QEvent::Close || type == QEvent::Destroy)) {
- if (type != QEvent::Hide)
- m_activeWidget = nullptr;
+ (type == QEvent::Show || type == QEvent::Hide ||
+ type == QEvent::Close || type == QEvent::Destroy)) {
+ if (type == QEvent::Close || type == QEvent::Destroy)
+ m_activeWidget = nullptr;
+ emit visibilityChanged(m_visible);
return QObject::eventFilter(obj, event);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ViewCube/ViewCubeController.cpp` around lines 173 - 190, The eventFilter
branch for Hide/Close/Destroy leaves m_activeWidget set for Hide events and
never emits visibilityChanged(), causing isVisible() to be wrong; in
ViewCubeController::eventFilter, when obj == m_activeWidget and event type is
QEvent::Hide, QEvent::Close or QEvent::Destroy, set m_activeWidget = nullptr
unconditionally, then emit visibilityChanged() (or call the method that
updates/hides the overlay) before returning; this ensures isVisible() reflects
the hidden state and the overlay is removed when the active viewport
hides/closes.
- Add ViewCubeController source to tests/CMakeLists.txt (fixes undefined reference errors in MaterialEditorQML test targets) - Add destructor that clears s_instance to prevent dangling singleton - Emit isVisible() instead of m_visible in signals for payload consistency (isVisible requires both visible flag AND active widget) - Emit visibilityChanged in eventFilter on Hide/Close/Destroy events - Add edge snapping: 12 edge definitions with hit-testing between corner and face checks in QML, so clicking cube edges snaps to the corresponding diagonal view Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/ViewCube/ViewCubeController.h (1)
39-39:⚠️ Potential issue | 🟠 MajorMake
visibledepend on the widget's actual visibility.This accessor still reports
truefor a hidden-but-not-destroyed viewport, so theQEvent::Hidepath insrc/ViewCube/ViewCubeController.cppcan leave the QML window visible during layout switches. DeclareisVisible()here and implement it in the.cppagainstm_activeWidget->isVisible(); if the same widget can reappear, theQEvent::Showpath should notify again.🔧 Minimal header change
- bool isVisible() const { return m_visible && m_activeWidget; } + bool isVisible() const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController.h` at line 39, Update the inline isVisible() accessor to be a declaration in ViewCubeController.h and implement it in ViewCubeController.cpp so it checks the widget's actual visibility; specifically, change isVisible() to return m_visible && m_activeWidget && m_activeWidget->isVisible() in the .cpp (use the existing m_activeWidget and isVisible() call) and ensure the QEvent::Show path in ViewCubeController.cpp re-notifies when the widget becomes visible again after being hidden.src/ViewCube/ViewCubeController.cpp (1)
31-38:⚠️ Potential issue | 🟠 MajorDon't create a fallback controller in the singleton provider.
If QML resolves the singleton before MainWindow constructs the real controller, this branch hands the engine a
ViewCubeController(nullptr)with no main-window or active-widget wiring. Later overwritings_instancedoes not replace the object QML already cached, so the overlay can stay bound to the wrong controller for the lifetime of the engine.🔧 Safer provider
ViewCubeController* ViewCubeController::qmlInstance(QQmlEngine* engine, QJSEngine* scriptEngine) { Q_UNUSED(engine) Q_UNUSED(scriptEngine) - if (!s_instance) - s_instance = new ViewCubeController(nullptr); + Q_ASSERT_X(s_instance, "ViewCubeController::qmlInstance", + "Construct ViewCubeController before loading QML"); return s_instance; }#!/bin/bash # Verify whether the QML registration path can call ViewCubeController::qmlInstance # before the application-owned controller is constructed. rg -n -C3 -e 'ViewCubeController::qmlInstance|qmlRegisterSingleton(Type|Instance)|ViewCubeModule' src qml tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController.cpp` around lines 31 - 38, The qmlInstance provider currently creates a fallback ViewCubeController(nullptr) which can be cached by QML and never properly wired; change ViewCubeController::qmlInstance to NOT construct a new controller when s_instance is null — simply return nullptr (or qmlNullValue) so QML gets no stub; update any call-sites or QML registration to tolerate a null singleton or delay qmlRegisterSingleton until the real controller (s_instance) is constructed; target the ViewCubeController::qmlInstance function and the s_instance static so no new ViewCubeController(nullptr) is ever created in the provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/CMakeLists.txt`:
- Around line 62-63: The test file src/ViewCube/ViewCubeController_test.cpp is
not being added as a CTest target, so the 19 controller tests won't run; update
tests/CMakeLists.txt to create and register a real test target: add an
add_executable(ViewCubeController_test
src/ViewCube/ViewCubeController_test.cpp), link it with the appropriate
libraries via target_link_libraries(ViewCubeController_test PRIVATE gtest_main
<your_lib_target>), and register it with add_test(NAME ViewCubeController_test
COMMAND ViewCubeController_test) so CTest/CI discovers and runs the tests;
ensure the source path and target names match the existing
ViewCubeController.cpp inclusion.
---
Duplicate comments:
In `@src/ViewCube/ViewCubeController.cpp`:
- Around line 31-38: The qmlInstance provider currently creates a fallback
ViewCubeController(nullptr) which can be cached by QML and never properly wired;
change ViewCubeController::qmlInstance to NOT construct a new controller when
s_instance is null — simply return nullptr (or qmlNullValue) so QML gets no
stub; update any call-sites or QML registration to tolerate a null singleton or
delay qmlRegisterSingleton until the real controller (s_instance) is
constructed; target the ViewCubeController::qmlInstance function and the
s_instance static so no new ViewCubeController(nullptr) is ever created in the
provider.
In `@src/ViewCube/ViewCubeController.h`:
- Line 39: Update the inline isVisible() accessor to be a declaration in
ViewCubeController.h and implement it in ViewCubeController.cpp so it checks the
widget's actual visibility; specifically, change isVisible() to return m_visible
&& m_activeWidget && m_activeWidget->isVisible() in the .cpp (use the existing
m_activeWidget and isVisible() call) and ensure the QEvent::Show path in
ViewCubeController.cpp re-notifies when the widget becomes visible again after
being hidden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 668fcccd-6316-474a-ab9e-01953df93925
📒 Files selected for processing (4)
qml/ViewCubeWindow.qmlsrc/ViewCube/ViewCubeController.cppsrc/ViewCube/ViewCubeController.htests/CMakeLists.txt
Comment on lines
+62
to
63
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/ViewCube/ViewCubeController.cpp | ||
| ) |
There was a problem hiding this comment.
Register ViewCubeController_test.cpp as a real test target.
Adding the controller sources here fixes link inputs, but this file still never creates or discovers src/ViewCube/ViewCubeController_test.cpp. As written, the new 19 controller tests will not run under CTest/CI.
🔧 Minimal follow-up
+ if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/../src/ViewCube/ViewCubeController_test.cpp")
+ create_test_executable(ViewCubeController_test
+ "${CMAKE_CURRENT_SOURCE_DIR}/../src/ViewCube/ViewCubeController_test.cpp"
+ )
+ endif()
+
+ if(TARGET ViewCubeController_test)
+ gtest_discover_tests(ViewCubeController_test)
+ endif()
+
add_custom_target(run_all_materialeditorthml_tests
COMMAND ctest --verbose
DEPENDS MaterialEditorQML_qml_test_runner
+ $<$<TARGET_EXISTS:ViewCubeController_test>:ViewCubeController_test>
$<$<TARGET_EXISTS:MaterialEditorQML_test>:MaterialEditorQML_test>
$<$<TARGET_EXISTS:MaterialEditorQML_qml_test>:MaterialEditorQML_qml_test>
$<$<TARGET_EXISTS:MaterialEditorQML_perf_test>:MaterialEditorQML_perf_test>Based on learnings: Add Google Test unit tests for new functionality in the src/ directory with the _test.cpp suffix.
Also applies to: 113-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/CMakeLists.txt` around lines 62 - 63, The test file
src/ViewCube/ViewCubeController_test.cpp is not being added as a CTest target,
so the 19 controller tests won't run; update tests/CMakeLists.txt to create and
register a real test target: add an add_executable(ViewCubeController_test
src/ViewCube/ViewCubeController_test.cpp), link it with the appropriate
libraries via target_link_libraries(ViewCubeController_test PRIVATE gtest_main
<your_lib_target>), and register it with add_test(NAME ViewCubeController_test
COMMAND ViewCubeController_test) so CTest/CI discovers and runs the tests;
ensure the source path and target names match the existing
ViewCubeController.cpp inclusion.
The 50px window was clipping cube corners on diagonal orientations. Increased to 64px and locked dimensions with min/max constraints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/ViewCube/ViewCubeController.cpp (2)
179-198:⚠️ Potential issue | 🟠 MajorViewCube remains visible when viewport hides.
When
QEvent::Hidefires,m_activeWidgetstays non-null andisVisible()returnstrue(only checksm_visible && m_activeWidget). The ViewCube overlay remains rendered over a hidden viewport until the widget is closed or destroyed.🔧 Proposed fix in ViewCubeController.h
Update
isVisible()to also check the widget's visibility state:- bool isVisible() const { return m_visible && m_activeWidget; } + bool isVisible() const { return m_visible && m_activeWidget && m_activeWidget->isVisible(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController.cpp` around lines 179 - 198, The ViewCube remains visible on QEvent::Hide because isVisible() only checks m_visible and m_activeWidget but not the widget's actual visibility; update the visibility logic used by ViewCubeController::isVisible() so it also queries the current widget's visibility (e.g., ensure isVisible() returns m_visible && m_activeWidget && m_activeWidget->isVisible()), and keep the existing handling in ViewCubeController::eventFilter for QEvent::Hide/Close/Destroy so emit visibilityChanged(isVisible()) reflects the widget's true visible state.
31-39:⚠️ Potential issue | 🟠 MajorFallback allocation in
qmlInstance()risks memory leak and silent malfunction.If QML loads before
MainWindowcreates the controller, this path allocates a controller withnullptrmainWindow (no event filter installed), and the allocation is never freed. Replace with an assertion to fail fast during development.🔧 Proposed fix
ViewCubeController* ViewCubeController::qmlInstance(QQmlEngine* engine, QJSEngine* scriptEngine) { Q_UNUSED(engine) Q_UNUSED(scriptEngine) - if (!s_instance) - s_instance = new ViewCubeController(nullptr); + Q_ASSERT_X(s_instance, "ViewCubeController::qmlInstance", + "MainWindow must create the ViewCubeController before QML loads"); return s_instance; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController.cpp` around lines 31 - 39, The current QML instantiation in ViewCubeController::qmlInstance creates a leaked controller with a nullptr MainWindow; remove the fallback allocation (the new ViewCubeController(nullptr) path) and replace it with a failing assertion so the app fails fast during development. Specifically, in ViewCubeController::qmlInstance, do not assign s_instance = new ViewCubeController(nullptr); instead call a Q_ASSERT_X or qFatal (e.g., Q_ASSERT_X(false, "ViewCubeController::qmlInstance", "ViewCubeController must be created by MainWindow before QML loads")) and return s_instance, ensuring the only proper creation path is the MainWindow-created instance referenced by s_instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ViewCube/ViewCubeController.cpp`:
- Around line 179-198: The ViewCube remains visible on QEvent::Hide because
isVisible() only checks m_visible and m_activeWidget but not the widget's actual
visibility; update the visibility logic used by ViewCubeController::isVisible()
so it also queries the current widget's visibility (e.g., ensure isVisible()
returns m_visible && m_activeWidget && m_activeWidget->isVisible()), and keep
the existing handling in ViewCubeController::eventFilter for
QEvent::Hide/Close/Destroy so emit visibilityChanged(isVisible()) reflects the
widget's true visible state.
- Around line 31-39: The current QML instantiation in
ViewCubeController::qmlInstance creates a leaked controller with a nullptr
MainWindow; remove the fallback allocation (the new ViewCubeController(nullptr)
path) and replace it with a failing assertion so the app fails fast during
development. Specifically, in ViewCubeController::qmlInstance, do not assign
s_instance = new ViewCubeController(nullptr); instead call a Q_ASSERT_X or
qFatal (e.g., Q_ASSERT_X(false, "ViewCubeController::qmlInstance",
"ViewCubeController must be created by MainWindow before QML loads")) and return
s_instance, ensuring the only proper creation path is the MainWindow-created
instance referenced by s_instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1be7501a-53c9-4eec-864d-e9f1ccf57848
📒 Files selected for processing (2)
qml/ViewCubeWindow.qmlsrc/ViewCube/ViewCubeController.cpp
ViewCubeController (8 new tests): - Constructor with mainWindow: verifies event filter installation - Event filter with Move/Resize/unrelated events on mainWindow - Destructor preserving singleton when different instance exists - qmlInstance creating new controller when singleton is null - Signal count verification for visibility toggles - updateOrientation no-emit when orientation unchanged - Window position defaults with no active widget SpaceCamera (8 new tests): - isAnimating() default false - getCamera() returns null for mock - mousePressEvent sets animating false - frameStarted with zero/large/negative time deltas - Control key speed transition cycles - Extreme camera speed values Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/SpaceCamera_test.cpp (1)
687-708: Control-speed transition coverage is duplicatedThis test overlaps with existing assertions in Line 138-152 and Line 250-261. Consider consolidating into a single parameterized/control-transition test to reduce duplication and maintenance cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SpaceCamera_test.cpp` around lines 687 - 708, The ControlKey speed-transition assertions are duplicated across tests; consolidate them by removing this redundant TEST(SpaceCamera, ControlKeySpeedTransitionCycle) and instead add a single parameterized test (or extend the existing parameterized control-transition test) that covers the Control press/release cycle for MockSpaceCamera: use setCameraSpeed to set initial speed, call keyPressEvent and keyReleaseEvent on the same QKeyEvent instances, and assert getCameraSpeed values for precision and restored modes; update or reuse the existing test harness around the existing tests at (the ones covering Control key transitions) so all control-press/release scenarios are covered in one place rather than duplicated.src/ViewCube/ViewCubeController_test.cpp (2)
123-161: Consider usingEXPECT_NO_THROWfor crash-safety tests.The
EXPECT_TRUE(true)pattern works but doesn't clearly communicate intent. For "no crash" tests,EXPECT_NO_THROWorASSERT_NO_FATAL_FAILUREwould be more expressive.✨ Optional: More expressive crash-safety assertions
TEST_F(ViewCubeControllerTest, SnapToViewInvalidFaceNoOp) { - controller->snapToView("InvalidFace"); - controller->snapToView(""); - EXPECT_TRUE(true); + EXPECT_NO_THROW(controller->snapToView("InvalidFace")); + EXPECT_NO_THROW(controller->snapToView("")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController_test.cpp` around lines 123 - 161, These tests currently use EXPECT_TRUE(true) after calling controller methods which hides intent; wrap the call sequences in EXPECT_NO_THROW (or ASSERT_NO_FATAL_FAILURE if you need to stop on failure) so the test explicitly asserts "no exception/crash" — e.g. in tests SnapToViewInvalidFaceNoOp and SnapToViewValidFacesNoCamera wrap controller->snapToView(...) calls, in SnapToViewCaseInsensitive wrap the snapToView(...) calls, in SnapToDirectionWithoutCamera wrap controller->snapToDirection(...) calls, and in RotateByDeltaWithoutCamera wrap controller->rotateByDelta(...) calls with EXPECT_NO_THROW (or ASSERT_NO_FATAL_FAILURE) and remove the trailing EXPECT_TRUE(true).
308-319: Test has implicit ordering dependency.Line 311 asserts
instance() == nullptras a precondition. While this should hold afterViewCubeControllerTest::TearDownruns, the test relies on no other test having left a singleton instance alive.This is likely fine given Google Test's execution model, but consider adding a defensive cleanup at the start:
✨ Optional: Make precondition explicit and self-healing
TEST(ViewCubeControllerLifetime, QmlInstanceCreatesNewWhenSingletonIsNull) { - // Precondition: no singleton exists - EXPECT_EQ(ViewCubeController::instance(), nullptr); + // Ensure clean state (defensive against test ordering) + if (auto* existing = ViewCubeController::instance()) { + delete existing; + } + ASSERT_EQ(ViewCubeController::instance(), nullptr); auto* inst = ViewCubeController::qmlInstance(nullptr, nullptr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController_test.cpp` around lines 308 - 319, The test implicitly assumes no existing singleton; make it self-healing by checking ViewCubeController::instance() at the start of TEST(ViewCubeControllerLifetime, QmlInstanceCreatesNewWhenSingletonIsNull) and deleting/resetting it if non-null before the EXPECT_EQ precondition. Concretely, add a defensive cleanup at test start that calls delete on the existing ViewCubeController::instance() (or otherwise resets the singleton) so the subsequent EXPECT_EQ(ViewCubeController::instance(), nullptr) is guaranteed to hold, then proceed to call ViewCubeController::qmlInstance and the rest of the assertions.
🤖 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/SpaceCamera_test.cpp`:
- Around line 645-653: The test is vacuous because it never sets SpaceCamera
into an animating state; update TEST(SpaceCamera, MousePressSetsAnimatingFalse)
to first put the camera into animating mode (e.g., call
MockSpaceCamera::setAnimating(true) or use the real
startAnimation()/beginAnimation() method on MockSpaceCamera), assert that
spaceCamera.isAnimating() is true as a precondition, then send the QMouseEvent
via spaceCamera.mousePressEvent(&pressEvent) and finally
EXPECT_FALSE(spaceCamera.isAnimating()); ensure you reference MockSpaceCamera,
mousePressEvent, and isAnimating in the change.
---
Nitpick comments:
In `@src/SpaceCamera_test.cpp`:
- Around line 687-708: The ControlKey speed-transition assertions are duplicated
across tests; consolidate them by removing this redundant TEST(SpaceCamera,
ControlKeySpeedTransitionCycle) and instead add a single parameterized test (or
extend the existing parameterized control-transition test) that covers the
Control press/release cycle for MockSpaceCamera: use setCameraSpeed to set
initial speed, call keyPressEvent and keyReleaseEvent on the same QKeyEvent
instances, and assert getCameraSpeed values for precision and restored modes;
update or reuse the existing test harness around the existing tests at (the ones
covering Control key transitions) so all control-press/release scenarios are
covered in one place rather than duplicated.
In `@src/ViewCube/ViewCubeController_test.cpp`:
- Around line 123-161: These tests currently use EXPECT_TRUE(true) after calling
controller methods which hides intent; wrap the call sequences in
EXPECT_NO_THROW (or ASSERT_NO_FATAL_FAILURE if you need to stop on failure) so
the test explicitly asserts "no exception/crash" — e.g. in tests
SnapToViewInvalidFaceNoOp and SnapToViewValidFacesNoCamera wrap
controller->snapToView(...) calls, in SnapToViewCaseInsensitive wrap the
snapToView(...) calls, in SnapToDirectionWithoutCamera wrap
controller->snapToDirection(...) calls, and in RotateByDeltaWithoutCamera wrap
controller->rotateByDelta(...) calls with EXPECT_NO_THROW (or
ASSERT_NO_FATAL_FAILURE) and remove the trailing EXPECT_TRUE(true).
- Around line 308-319: The test implicitly assumes no existing singleton; make
it self-healing by checking ViewCubeController::instance() at the start of
TEST(ViewCubeControllerLifetime, QmlInstanceCreatesNewWhenSingletonIsNull) and
deleting/resetting it if non-null before the EXPECT_EQ precondition. Concretely,
add a defensive cleanup at test start that calls delete on the existing
ViewCubeController::instance() (or otherwise resets the singleton) so the
subsequent EXPECT_EQ(ViewCubeController::instance(), nullptr) is guaranteed to
hold, then proceed to call ViewCubeController::qmlInstance and the rest of the
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 067b4fdd-785c-44b9-8101-710ddc769b1f
📒 Files selected for processing (2)
src/SpaceCamera_test.cppsrc/ViewCube/ViewCubeController_test.cpp
Comment on lines
+645
to
+653
| TEST(SpaceCamera, MousePressSetsAnimatingFalse) | ||
| { | ||
| MockSpaceCamera spaceCamera; | ||
| // mousePressEvent always sets mAnimating = false (cancels animation) | ||
| QMouseEvent pressEvent(QEvent::MouseButtonPress, QPointF(50, 50), | ||
| Qt::MiddleButton, Qt::MiddleButton, Qt::NoModifier); | ||
| spaceCamera.mousePressEvent(&pressEvent); | ||
| EXPECT_FALSE(spaceCamera.isAnimating()); | ||
| } |
There was a problem hiding this comment.
MousePressSetsAnimatingFalse is currently a vacuous test
This test never puts SpaceCamera into an animating state before the press, so it can pass even if mouse press does not cancel an active animation.
✅ Proposed test fix
TEST(SpaceCamera, MousePressSetsAnimatingFalse)
{
MockSpaceCamera spaceCamera;
- // mousePressEvent always sets mAnimating = false (cancels animation)
+ // Arrange: start animation first
+ spaceCamera.animateToOrientation(Ogre::Quaternion::IDENTITY, 0.5f);
+ ASSERT_TRUE(spaceCamera.isAnimating());
+
+ // Act: mouse press should cancel animation
QMouseEvent pressEvent(QEvent::MouseButtonPress, QPointF(50, 50),
Qt::MiddleButton, Qt::MiddleButton, Qt::NoModifier);
spaceCamera.mousePressEvent(&pressEvent);
+
+ // Assert
EXPECT_FALSE(spaceCamera.isAnimating());
}📝 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
| TEST(SpaceCamera, MousePressSetsAnimatingFalse) | |
| { | |
| MockSpaceCamera spaceCamera; | |
| // mousePressEvent always sets mAnimating = false (cancels animation) | |
| QMouseEvent pressEvent(QEvent::MouseButtonPress, QPointF(50, 50), | |
| Qt::MiddleButton, Qt::MiddleButton, Qt::NoModifier); | |
| spaceCamera.mousePressEvent(&pressEvent); | |
| EXPECT_FALSE(spaceCamera.isAnimating()); | |
| } | |
| TEST(SpaceCamera, MousePressSetsAnimatingFalse) | |
| { | |
| MockSpaceCamera spaceCamera; | |
| // Arrange: start animation first | |
| spaceCamera.animateToOrientation(Ogre::Quaternion::IDENTITY, 0.5f); | |
| ASSERT_TRUE(spaceCamera.isAnimating()); | |
| // Act: mouse press should cancel animation | |
| QMouseEvent pressEvent(QEvent::MouseButtonPress, QPointF(50, 50), | |
| Qt::MiddleButton, Qt::MiddleButton, Qt::NoModifier); | |
| spaceCamera.mousePressEvent(&pressEvent); | |
| // Assert | |
| EXPECT_FALSE(spaceCamera.isAnimating()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SpaceCamera_test.cpp` around lines 645 - 653, The test is vacuous because
it never sets SpaceCamera into an animating state; update TEST(SpaceCamera,
MousePressSetsAnimatingFalse) to first put the camera into animating mode (e.g.,
call MockSpaceCamera::setAnimating(true) or use the real
startAnimation()/beginAnimation() method on MockSpaceCamera), assert that
spaceCamera.isAnimating() is true as a precondition, then send the QMouseEvent
via spaceCamera.mousePressEvent(&pressEvent) and finally
EXPECT_FALSE(spaceCamera.isAnimating()); ensure you reference MockSpaceCamera,
mousePressEvent, and isAnimating in the change.
Address code review feedback: - isVisible() now checks m_activeWidget->isVisible() so the ViewCube hides when the viewport widget is hidden (e.g., tab switch) - eventFilter handles QEvent::Show to restore ViewCube when viewport reappears - Clarify SpaceCamera test limitation comment (animateToOrientation requires Ogre mTarget, unavailable in MockSpaceCamera) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/ViewCube/ViewCubeController.cpp (1)
31-39:⚠️ Potential issue | 🟠 MajorFallback allocation in qmlInstance() can leak memory.
If QML loads before
MainWindowcreates the controller, this fallback creates an orphaned instance withnullptrmainWindow. WhenMainWindowlater creates its own instance,s_instanceis overwritten and the first instance leaks (no parent ownership).Consider asserting that the instance exists rather than creating a fallback:
🔧 Proposed fix
ViewCubeController* ViewCubeController::qmlInstance(QQmlEngine* engine, QJSEngine* scriptEngine) { Q_UNUSED(engine) Q_UNUSED(scriptEngine) - if (!s_instance) - s_instance = new ViewCubeController(nullptr); + Q_ASSERT_X(s_instance, "ViewCubeController::qmlInstance", + "MainWindow must create ViewCubeController before QML loads"); return s_instance; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ViewCube/ViewCubeController.cpp` around lines 31 - 39, The qmlInstance() fallback allocation (creating a new ViewCubeController(nullptr) when s_instance is null) can leak if MainWindow later creates the real controller; change ViewCubeController::qmlInstance to not allocate a fallback: instead reference the existing s_instance and assert it exists (e.g., Q_ASSERT(s_instance)) and return it, or return nullptr after logging a warning if you prefer a runtime-safe path—remove the "new ViewCubeController(nullptr)" code and any unconditional assignment to s_instance so MainWindow remains the sole creator/owner.src/SpaceCamera_test.cpp (1)
645-656:⚠️ Potential issue | 🟡 Minor
MousePressSetsAnimatingFalseis still vacuous for cancellation behavior.At Line 645, the test name implies canceling an active animation, but no animating precondition is established before the mouse press. This can pass even if cancellation regresses.
✅ Minimal fix to avoid misleading intent
-TEST(SpaceCamera, MousePressSetsAnimatingFalse) +TEST(SpaceCamera, MousePressKeepsAnimatingFalseWhenNotAnimating) { MockSpaceCamera spaceCamera; - // mousePressEvent always sets mAnimating = false (cancels animation). - // Note: we cannot set mAnimating=true first via animateToOrientation() - // because that method dereferences mTarget which is null in MockSpaceCamera. - // Full animation cancellation is tested in SpaceCameraOgreTest fixtures. + // This only verifies default non-animating state remains false after press. + // Add a separate Ogre-backed test that first enters an animating state, + // then verifies mousePressEvent cancels it. QMouseEvent pressEvent(QEvent::MouseButtonPress, QPointF(50, 50), Qt::MiddleButton, Qt::MiddleButton, Qt::NoModifier); spaceCamera.mousePressEvent(&pressEvent); EXPECT_FALSE(spaceCamera.isAnimating()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SpaceCamera_test.cpp` around lines 645 - 656, The test currently never puts the camera into an animating state, so make the precondition explicit: before calling spaceCamera.mousePressEvent(&pressEvent) set the camera to animating (e.g. call spaceCamera.setAnimating(true) or, if that setter doesn't exist, add a small accessor/mutator on MockSpaceCamera to set its mAnimating flag or make mAnimating public for the test), then assert EXPECT_TRUE(spaceCamera.isAnimating()) if desired and call mousePressEvent and EXPECT_FALSE(spaceCamera.isAnimating()); update MockSpaceCamera accordingly to allow the test to establish the animating precondition.
🧹 Nitpick comments (1)
src/SpaceCamera_test.cpp (1)
690-711: Clarify “restore” vs “reset” semantics in the control-speed cycle test.At Line 701, comments say speed is “restored,” but assertions expect
0.1fafter starting from1.0f. Either rename/comment this as reset-to-default behavior or assert restoration to the prior value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SpaceCamera_test.cpp` around lines 690 - 711, The test ControlKeySpeedTransitionCycle is ambiguous: update the test to reflect intended semantics of Control modifier on MockSpaceCamera by either (A) changing the comment and assertions to indicate a reset-to-default behavior (i.e., after releasing Control expect default speed 0.1f) or (B) changing the assertions to verify true restoration of the prior speed (i.e., after releasing Control expect the original 1.0f). Locate the TEST named ControlKeySpeedTransitionCycle and the calls to MockSpaceCamera::setCameraSpeed, getCameraSpeed, keyPressEvent, and keyReleaseEvent and modify the comments and expected values accordingly so they consistently match the chosen behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/SpaceCamera_test.cpp`:
- Around line 645-656: The test currently never puts the camera into an
animating state, so make the precondition explicit: before calling
spaceCamera.mousePressEvent(&pressEvent) set the camera to animating (e.g. call
spaceCamera.setAnimating(true) or, if that setter doesn't exist, add a small
accessor/mutator on MockSpaceCamera to set its mAnimating flag or make
mAnimating public for the test), then assert
EXPECT_TRUE(spaceCamera.isAnimating()) if desired and call mousePressEvent and
EXPECT_FALSE(spaceCamera.isAnimating()); update MockSpaceCamera accordingly to
allow the test to establish the animating precondition.
In `@src/ViewCube/ViewCubeController.cpp`:
- Around line 31-39: The qmlInstance() fallback allocation (creating a new
ViewCubeController(nullptr) when s_instance is null) can leak if MainWindow
later creates the real controller; change ViewCubeController::qmlInstance to not
allocate a fallback: instead reference the existing s_instance and assert it
exists (e.g., Q_ASSERT(s_instance)) and return it, or return nullptr after
logging a warning if you prefer a runtime-safe path—remove the "new
ViewCubeController(nullptr)" code and any unconditional assignment to s_instance
so MainWindow remains the sole creator/owner.
---
Nitpick comments:
In `@src/SpaceCamera_test.cpp`:
- Around line 690-711: The test ControlKeySpeedTransitionCycle is ambiguous:
update the test to reflect intended semantics of Control modifier on
MockSpaceCamera by either (A) changing the comment and assertions to indicate a
reset-to-default behavior (i.e., after releasing Control expect default speed
0.1f) or (B) changing the assertions to verify true restoration of the prior
speed (i.e., after releasing Control expect the original 1.0f). Locate the TEST
named ControlKeySpeedTransitionCycle and the calls to
MockSpaceCamera::setCameraSpeed, getCameraSpeed, keyPressEvent, and
keyReleaseEvent and modify the comments and expected values accordingly so they
consistently match the chosen behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41232f63-9b60-49dc-81e6-44969b6602bc
📒 Files selected for processing (3)
src/SpaceCamera_test.cppsrc/ViewCube/ViewCubeController.cppsrc/ViewCube/ViewCubeController.h
|
This was referenced Mar 23, 2026
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
SpaceCamera::animateToOrientation()deleteLater()instead ofdelete)TransformOperatorwhen viewport widgets are destroyedQApplication::quit()New Files
src/ViewCube/ViewCubeController.h/cpp— C++ controller bridging QML overlay with Ogre SpaceCamerasrc/ViewCube/ViewCubeController_test.cpp— 19 unit tests covering controller logicqml/ViewCubeWindow.qml— QML Canvas2D rendering with face/edge/corner hit-testingModified Files
src/SpaceCamera.h/cpp— AddedanimateToOrientation()andisAnimating()for smooth camera transitionssrc/mainwindow.h/cpp— ViewCube initialization, lifecycle,closeEventoverridesrc/TransformOperator.cpp— Safe widget destruction handling viaQObject::destroyedsrc/OgreWidget.h— AddedfocusOnWidgetsignalui_files/mainwindow.ui— Added "Show View Cube" menu actionsrc/qml_resources.qrc— ViewCube QML resource entryCLAUDE.md— ViewCube architecture documentationTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests
Chores