feat: Refactored the vneinteraction to use quaternion based implementation. Remove Follow manipulator and Eular Orbit Behavior. Also, minor refactoring to organize the code.#37
Open
ajeetsinghyadav wants to merge 25 commits intomainfrom
Conversation
- Renamed and refactored OrbitCameraState to TrackballCameraState for improved clarity in camera manipulation. - Updated logging functions to reflect changes in state structure, including quaternion orientation instead of Euler angles. - Adjusted example code to demonstrate the new TrackballCameraState functionality, ensuring consistent behavior in camera state save/restore operations. - Modified FreeLookManipulator and OrbitalCameraManipulator to utilize quaternion-based orientation, enhancing stability and performance during interactions. - Updated tests to validate the new camera state management and ensure compatibility with existing functionalities.
…4f1d038cb1d4880e0bec95
- Adjusted logging output in the camera state save/restore example for better readability by formatting quaternion output. - Cleaned up code formatting in `camera_state.h`, `interaction_types.h`, and `free_look_manipulator.cpp` for consistency and clarity. - Enhanced comments in various files to improve understanding of camera manipulation functionalities and interactions.
… related files - Deleted `euler_orbit_strategy.h`, `orbit_behavior.cpp`, and `orbit_behavior.h` as they are no longer needed. - Updated `trackball_strategy.h` to remove dependencies on the deleted orbit behavior. - Refactored `OrbitalCameraManipulator` to exclusively use `TrackballStrategy` for rotation handling. - Removed rotation mode management from `Inspect3DController` and related logic. - Cleaned up `FollowController` and `FollowManipulator` implementations by removing unused code and ensuring proper camera manipulation. - Adjusted comments and documentation to reflect the removal of Euler-based rotation handling.
- Commented out tests in `orbital_camera_manipulator_test.cpp` and `manipulator_regression_test.cpp` to temporarily disable them. - Deleted the `orbit_behavior_test.cpp` file as it is no longer needed following recent refactoring of camera manipulation strategies.
- Revised `README.md` files for examples to reflect changes in controller functionalities and clarify usage. - Updated `01_library_info` to include new controller defaults and trackball projection modes. - Enhanced `02_medical_3d_inspect` to focus on trackball projection variants and inertia settings. - Simplified `05_robotic_simulator` by removing the FollowController, emphasizing Inspect3D and Navigation3D functionalities with moving pivots. - Adjusted example code to improve clarity and consistency in logging and comments across various files. - Removed references to deprecated FollowController in tests and examples, streamlining the codebase.
…applications - Streamlined logging statements in `01_example.cpp`, `05_example.cpp`, and `07_example.cpp` for better readability and uniformity. - Consolidated multi-line expressions into single lines where appropriate to enhance clarity. - Updated comments and formatting in `orbital_camera_manipulator_test.cpp` to maintain consistency across test cases.
…nipulator - Added `makeNavTrackball` method to `CameraRig` for creating a navigation rig with virtual trackball mouse look. - Enhanced `FreeLookManipulator` to support trackball rotation mode, allowing for more intuitive camera control. - Updated `Navigation3DController` to manage rotation modes, including the new trackball option. - Added tests to validate the functionality of the navigation trackball mode in both `CameraRig` and `Navigation3DController`.
…e trackball behavior handling
- Merged type headers `camera_action.h`, `camera_state.h`, and `input_binding.h` into a single `interaction_types.h` for better organization and accessibility. - Removed obsolete headers to streamline the codebase and reduce dependencies. - Updated documentation and examples to reflect the new structure and ensure compatibility with the consolidated types. - Introduced `interaction_utils.h` and `interaction_utils.cpp` to replace the functionality of the removed `camera_math.h` and `view_math.h`, providing internal helpers for camera manipulators.
…e autodoc - Changed the creation date from April 2026 to March 2026 in multiple header and implementation files. - Enabled autodoc for several files to ensure they are included in the documentation generation process. - Updated author information consistently across all modified files.
📝 WalkthroughWalkthroughConsolidates interaction type headers into Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
- Refactors 3D orbit interaction to a quaternion/virtual-trackball-only implementation and removes legacy Euler-orbit and Follow manipulator/controller paths.
- Consolidates public interaction type headers into
interaction_types.hand merges internal math helpers intointeraction_utils.*. - Updates tests/examples/docs to reflect the new APIs and removed features.
Changes:
- Orbital rotation: remove Euler/strategy abstraction; implement internal quaternion trackball rotation state in
OrbitalCameraManipulator. - API cleanup: remove
camera_action.h,camera_state.h,input_binding.h,Follow*, and add quaternion-based state/accessors +Navigation3DController::set/getRotationMode. - Documentation/tests/examples updated to the new type surface and trackball-only behavior.
Reviewed changes
Copilot reviewed 78 out of 78 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/trackball_behavior_test.cpp | Adds file header/autodoc + brief test doc comment. |
| tests/ortho_2d_manipulator_test.cpp | Adds file header/autodoc + brief test doc comment. |
| tests/ortho_2d_controller_test.cpp | Adds file header/autodoc + brief test doc comment. |
| tests/orbital_camera_manipulator_test.cpp | Updates rotate gesture payload ordering; leaves some rotation-mode tests commented out. |
| tests/orbit_behavior_test.cpp | Removes Euler OrbitBehavior unit tests (feature removed). |
| tests/navigation_3d_controller_test.cpp | Adds tests for new Navigation3DController rotation-mode API. |
| tests/manipulator_regression_test.cpp | Switches to interaction_utils.h; removes legacy Euler-orbit regression; adds a now-empty trackball regression test (commented out). |
| tests/inspect_3d_controller_test.cpp | Updates defaults test to trackball projection; removes rotation-mode assertions. |
| tests/input_mapper_test.cpp | Adds file header/autodoc + brief test doc comment. |
| tests/free_look_manipulator_test.cpp | Renames tests/docs to orientation/quaternion terminology. |
| tests/follow_manipulator_test.cpp | Removes FollowManipulator tests (feature removed). |
| tests/follow_controller_test.cpp | Removes FollowController tests (feature removed). |
| tests/controller_move_safety_test.cpp | Removes FollowController move-safety coverage (feature removed). |
| tests/CMakeLists.txt | Removes deleted test sources from build. |
| tests/camera_rig_test.cpp | Updates rig factory usage to trackball + adds makeNavTrackball test. |
| src/vertexnova/interaction/version.cpp | Enables autodoc in file header. |
| src/vertexnova/interaction/ortho_2d_manipulator.cpp | Enables autodoc in file header. |
| src/vertexnova/interaction/ortho_2d_controller.cpp | Enables autodoc in file header. |
| src/vertexnova/interaction/orbital_camera_manipulator.cpp | Replaces strategy-based orbit rotation with internal quaternion trackball rotation state + inertia. |
| src/vertexnova/interaction/navigation_3d_controller.cpp | Adds set/getRotationMode delegating to FreeLookManipulator. |
| src/vertexnova/interaction/interaction_utils.h | Merges former internal math headers into one internal helper header. |
| src/vertexnova/interaction/interaction_utils.cpp | Implements API-dependent helpers; replaces view_math include. |
| src/vertexnova/interaction/inspect_3d_controller.cpp | Removes rotation-mode state and API usage; defaults to trackball-only orbit manipulator. |
| src/vertexnova/interaction/input_mapper.cpp | Enables autodoc in file header. |
| src/vertexnova/interaction/input_event_translator.h | Enables autodoc in file header (internal header). |
| src/vertexnova/interaction/input_event_translator.cpp | Enables autodoc in file header. |
| src/vertexnova/interaction/free_look_manipulator.cpp | Moves FreeLook to quaternion orientation + adds optional trackball mouse-look mode. |
| src/vertexnova/interaction/follow_manipulator.cpp | Removes FollowManipulator implementation (feature removed). |
| src/vertexnova/interaction/follow_controller.cpp | Removes FollowController implementation (feature removed). |
| src/vertexnova/interaction/detail/trackball_strategy.h | Removes strategy abstraction header (no longer used). |
| src/vertexnova/interaction/detail/trackball_behavior.h | Updates documentation to reflect its narrowed role. |
| src/vertexnova/interaction/detail/trackball_behavior.cpp | Switches to interaction_utils.h; minor cleanups. |
| src/vertexnova/interaction/detail/rotation_strategy.h | Removes strategy interface header (no longer used). |
| src/vertexnova/interaction/detail/orbit_behavior.h | Removes Euler OrbitBehavior header (feature removed). |
| src/vertexnova/interaction/detail/orbit_behavior.cpp | Removes Euler OrbitBehavior implementation (feature removed). |
| src/vertexnova/interaction/detail/euler_orbit_strategy.h | Removes Euler strategy header (no longer used). |
| src/vertexnova/interaction/camera_rig.cpp | Removes makeOrbit/makeFollow; simplifies makeTrackball; adds makeNavTrackball. |
| src/vertexnova/interaction/camera_math.h | Removes legacy internal math header (replaced by interaction_utils). |
| src/vertexnova/interaction/camera_manipulator_base.cpp | Switches internal include from view_math to interaction_utils. |
| src/vertexnova/interaction/camera_controller_impl.h | Enables autodoc in file header (internal header). |
| src/CMakeLists.txt | Removes deleted sources; adds interaction_utils.cpp; removes deleted public headers. |
| include/vertexnova/interaction/ortho_2d_manipulator.h | Minor header metadata formatting. |
| include/vertexnova/interaction/ortho_2d_controller.h | Minor header metadata formatting. |
| include/vertexnova/interaction/orbital_camera_manipulator.h | Removes rotation-mode API; adds quaternion orientation API; updates docs. |
| include/vertexnova/interaction/navigation_3d_controller.h | Exposes set/getRotationMode in public API. |
| include/vertexnova/interaction/interaction.h | Umbrella header no longer includes Follow*; updates type-surface description. |
| include/vertexnova/interaction/interaction_types.h | Consolidates actions/state/bindings/enums into one header; adds FreeLookRotationMode. |
| include/vertexnova/interaction/inspect_3d_controller.h | Removes rotation-mode API from public controller contract; updates docs to trackball default. |
| include/vertexnova/interaction/input_binding.h | Removes shim header (consolidated into interaction_types.h). |
| include/vertexnova/interaction/free_look_manipulator.h | Moves to quaternion orientation API + adds trackball mouse-look mode. |
| include/vertexnova/interaction/follow_manipulator.h | Removes FollowManipulator public header (feature removed). |
| include/vertexnova/interaction/follow_controller.h | Removes FollowController public header (feature removed). |
| include/vertexnova/interaction/camera_state.h | Removes shim header (consolidated into interaction_types.h). |
| include/vertexnova/interaction/camera_rig.h | Removes makeOrbit/makeFollow; adds makeNavTrackball; updates docs. |
| include/vertexnova/interaction/camera_manipulator.h | Updates docs to remove FollowManipulator mention and Euler-orbit wording. |
| include/vertexnova/interaction/camera_manipulator_base.h | Minor header metadata formatting. |
| include/vertexnova/interaction/camera_controller.h | Updates docs to remove FollowController mention. |
| include/vertexnova/interaction/camera_action.h | Removes shim header (consolidated into interaction_types.h). |
| examples/README.md | Updates example descriptions to remove Follow and Euler-orbit references. |
| examples/08_camera_state_save_restore/README.md | Updates to reference interaction_types.h instead of removed camera_state.h. |
| examples/08_camera_state_save_restore/main.cpp | Updates top-level commentary to match consolidated types. |
| examples/08_camera_state_save_restore/08_example.cpp | Switches orbit bookmark sample to TrackballCameraState + quaternion restore. |
| examples/07_camera_rig_composition/README.md | Updates factory-method list and hybrid-rig explanation for trackball-only. |
| examples/07_camera_rig_composition/main.cpp | Updates example header comments for removed factories. |
| examples/07_camera_rig_composition/07_example.cpp | Removes orbit/follow factory usage; uses trackball + eRim projection rebuild example. |
| examples/06_custom_input_bindings/06_example.cpp | Switches to interaction_types.h; uses makeTrackball where needed. |
| examples/05_robotic_simulator/README.md | Removes Follow section; replaces with moving pivot + orbit damping. |
| examples/05_robotic_simulator/main.cpp | Updates example overview to remove FollowController. |
| examples/05_robotic_simulator/05_example.h | Updates example description to remove FollowController. |
| examples/05_robotic_simulator/05_example.cpp | Removes FollowController usage; demonstrates moving pivot and damping. |
| examples/02_medical_3d_inspect/README.md | Updates to trackball-only orbit + projection modes section. |
| examples/02_medical_3d_inspect/02_example.cpp | Removes rotation-mode switching; focuses on projection modes. |
| examples/01_library_info/README.md | Updates to remove FollowController and OrbitalRotationMode references. |
| examples/01_library_info/01_example.cpp | Removes FollowController logging; updates Inspect defaults output and rig factory list. |
| docs/vertexnova/interaction/interaction.md | Updates swimlane/type-surface text to reflect consolidated types and interaction_utils. |
| docs/vertexnova/interaction/diagrams/component.drawio | Updates public-type and math-helper labels (diagram content partially still references removed components). |
| CHANGELOG.md | Adds notes about consolidated headers/internal math merge, but unreleased notes still include now-inaccurate statements about OrbitBehavior/Follow*. |
Comments suppressed due to low confidence (1)
include/vertexnova/interaction/orbital_camera_manipulator.h:147
- This comment still references an "internal Euler path" for top/bottom pitch limits, but Euler orbit support was removed. Consider rewording to describe the actual trackball/quaternion implementation and the chosen polar-limit constants (±89°) without referring to Euler.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
include/vertexnova/interaction/camera_rig.h (1)
128-138:⚠️ Potential issue | 🟠 MajorKeep a deprecated
makeOrbit()bridge before hard-removing the old preset API.Lines 128-138 replace a public factory entry point with
makeTrackball(). That means existing integrations stop compiling even though the preset still maps to the same conceptual “orbit/trackball” use case. Please keepmakeOrbit()as a deprecated forwarding alias for at least one release, and document themakeFollow()removal in migration notes instead of making it a silent source break.Possible compatibility shim
/** Trackball rig: OrbitalCameraManipulator with quaternion virtual-trackball rotation. */ static CameraRig makeTrackball(); + [[deprecated("Use makeTrackball()")]] + static CameraRig makeOrbit() { return makeTrackball(); }As per coding guidelines, "Avoid surprising API changes; keep interfaces stable in C++ code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/vertexnova/interaction/camera_rig.h` around lines 128 - 138, Existing API removed silently: add a deprecated bridge named makeOrbit() that forwards to the new makeTrackball() to preserve source compatibility; specifically, declare static CameraRig makeOrbit() in the same header alongside makeTrackball() and mark it as deprecated (e.g., [[deprecated]] or PROJECT_DEPRECATED macro) with a short comment pointing users to use makeTrackball() and to the migration notes explaining makeFollow() removal, so existing callers continue to compile for at least one release.examples/08_camera_state_save_restore/main.cpp (1)
5-18:⚠️ Potential issue | 🟡 MinorUpdate the example banner to match the new quaternion state API.
The comment block still advertises
OrbitCameraState, a yaw/pitch-basedFreeCameraState, and a non-exportedOrbitalInteractionState, but this PR replaces those with quaternion-based state types ininteraction_types.h. Anyone using this example as migration guidance will get the wrong public surface.Doc fix
- * - OrbitCameraState — save/restore Euler orbit (COI, distance, yaw, pitch, up) - * - TrackballCameraState — save/restore quaternion trackball (COI, distance, quat, up) - * - FreeCameraState — save/restore free-look (position, yaw, pitch, up_hint) + * - TrackballCameraState — save/restore quaternion orbit/trackball state + * (COI, distance, orientation, world_up) + * - FreeCameraState — save/restore free-look state (position, orientation) * - Typical "undo/bookmark" pattern: save before interaction, restore to jump back * - Reading live state from OrbitalCameraManipulator fields * - Reading live state from FreeLookManipulator fields * - FreeLookInputState — querying held-key snapshot (diagnostic use) - * - OrbitalInteractionState — not exported as snapshot; shown via manipulator API + * - OrbitalInteractionState — drag/pan interaction snapshot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/08_camera_state_save_restore/main.cpp` around lines 5 - 18, Update the example banner to reflect the new quaternion-based camera state API: remove references to the old OrbitCameraState, yaw/pitch FreeCameraState and the non-exported OrbitalInteractionState, and instead describe the actual exported quaternion state types present in interaction_types.h (e.g., the quaternion-based trackball state and the quaternion-based free-look state) — update the bullet list under "Demonstrates:" to mention TrackballCameraState (quaternion orbit/trackball), the new quaternion-based free-look state type, and that interaction snapshot/serialize is the application's responsibility so readers consult interaction_types.h for exact struct names and fields.include/vertexnova/interaction/camera_controller.h (1)
67-68:⚠️ Potential issue | 🟡 MinorRemove stale
followreference fromonUpdatedocs.
ICameraController::onUpdatestill documents “follow smoothing” even though follow controller/manipulator were removed in this PR. Please update this line to avoid implying unsupported behavior.As per coding guidelines, public C++ headers should keep behavior documentation clear and unsurprising.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/vertexnova/interaction/camera_controller.h` around lines 67 - 68, The onUpdate documentation for ICameraController still mentions "follow smoothing" which is no longer supported; update the comment for ICameraController::onUpdate to remove the stale "follow smoothing" phrase and instead document only the active behaviors (e.g., inertia and damping) and the delta_time parameter so the header's public docs reflect current behavior.include/vertexnova/interaction/camera_manipulator.h (1)
15-16:⚠️ Potential issue | 🟡 MinorTop-level manipulator brief still mentions removed follow mode.
The
@briefstill says “orbit, fly, ortho, follow, …” while follow support was removed. Please sync this brief with the new supported set.As per coding guidelines, public APIs should avoid surprising or misleading interface documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/vertexnova/interaction/camera_manipulator.h` around lines 15 - 16, Update the `@brief` for ICameraManipulator to remove the outdated "follow" mode and reflect only the currently supported modes (e.g., "orbit, fly, ortho, …"); locate the ICameraManipulator declaration in camera_manipulator.h and edit the brief line so it no longer mentions "follow" and matches the actual supported manipulators.docs/vertexnova/interaction/diagrams/component.drawio (1)
17-50:⚠️ Potential issue | 🟡 MinorDiagram update is incomplete after follow/euler removal.
You updated
interaction_types.handinteraction_utils, but the same diagram still lists removed components (FollowController,FollowManipulator,EulerOrbitStrategy,OrbitBehavior). Please update these labels so the diagram matches the current architecture.Suggested drawio value updates
- <mxCell id="controllers_pub" value="Inspect3DController Navigation3DController Ortho2DController FollowController" ...> + <mxCell id="controllers_pub" value="Inspect3DController Navigation3DController Ortho2DController" ...> - <mxCell id="manipulators_pub" value="OrbitalCameraManipulator FreeLookManipulator Ortho2DManipulator FollowManipulator CameraManipulatorBase" ...> + <mxCell id="manipulators_pub" value="OrbitalCameraManipulator FreeLookManipulator Ortho2DManipulator CameraManipulatorBase" ...> - <mxCell id="strategies" value="detail/IRotationStrategy detail/EulerOrbitStrategy detail/TrackballStrategy" ...> + <mxCell id="strategies" value="detail/IRotationStrategy detail/TrackballStrategy" ...> - <mxCell id="behaviors" value="detail/OrbitBehavior detail/TrackballBehavior" ...> + <mxCell id="behaviors" value="detail/TrackballBehavior" ...>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/vertexnova/interaction/diagrams/component.drawio` around lines 17 - 50, The diagram still lists removed components (FollowController, FollowManipulator, EulerOrbitStrategy, OrbitBehavior) and must be updated to reflect the current code; open the component.drawio shapes with ids/controllers named controllers_pub, manipulators_pub, strategies and behaviors and remove the obsolete labels (FollowController, FollowManipulator, EulerOrbitStrategy, OrbitBehavior) and replace them with the current components present in interaction_types.h and interaction_utils (or simply remove those entries if no direct replacement exists), ensuring the values for interaction_types_h, controllers_pub, manipulators_pub, strategies, and behaviors match the updated source (interaction_types.h and interaction_utils).docs/vertexnova/interaction/interaction.md (1)
140-140:⚠️ Potential issue | 🟡 MinorRemove references to deleted factory methods
makeOrbit()andmakeFollow().The documentation lists
makeOrbit()andmakeFollow()as factory methods, but these no longer exist in the codebase. OnlymakeTrackball(),makeFps(),makeFly(), andmakeOrtho2D()are implemented.📝 Suggested fix
-- **Factories** — `makeOrbit()`, `makeTrackball()`, `makeFps()`, `makeFly()`, `makeOrtho2D()`, `makeFollow()` build rigs with a single default manipulator; you can still `addManipulator` for custom stacks. +- **Factories** — `makeTrackball()`, `makeFps()`, `makeFly()`, `makeOrtho2D()` build rigs with a single default manipulator; you can still `addManipulator` for custom stacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/vertexnova/interaction/interaction.md` at line 140, Update the docs line that lists factory functions by removing the deleted makeOrbit() and makeFollow() entries and only listing the implemented factories: makeTrackball(), makeFps(), makeFly(), and makeOrtho2D(); keep the note that you can still call addManipulator for custom stacks and ensure the sentence references the same symbols (makeTrackball, makeFps, makeFly, makeOrtho2D, addManipulator) so readers can find the valid API.
🧹 Nitpick comments (3)
examples/06_custom_input_bindings/06_example.cpp (1)
5-7: Remove the duplicateinteraction_types.hinclude.The same header is included on Lines 5 and 7.
#pragma oncemakes it harmless, but it adds noise to an example that is supposed to show the minimal wiring.Cleanup
`#include` "vertexnova/interaction/camera_rig.h" `#include` "vertexnova/interaction/inspect_3d_controller.h" -#include "vertexnova/interaction/interaction_types.h" `#include` "vertexnova/interaction/input_mapper.h" `#include` "vertexnova/interaction/interaction_types.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/06_custom_input_bindings/06_example.cpp` around lines 5 - 7, Remove the duplicate include of "interaction_types.h" in the example: keep a single `#include` "vertexnova/interaction/interaction_types.h" alongside the `#include` "vertexnova/interaction/input_mapper.h" in 06_example.cpp so the example is minimal and non-redundant.tests/free_look_manipulator_test.cpp (1)
225-256: Consider documenting the negative delta-time test intent.The test at lines 245-246 passes both zero and negative delta time to
onUpdate(). While this appears intentional for robustness testing, a brief inline comment explaining the expected behavior for negativedtwould improve maintainability.📝 Suggested documentation addition
b.markAnglesDirty(); + // Zero and negative dt should still trigger orientation sync (no movement, but state is updated). b.onUpdate(0.0); b.onUpdate(-1.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/free_look_manipulator_test.cpp` around lines 225 - 256, Add a brief inline comment in the test FreeLookManipulator.OnUpdateZeroDeltaTimeStillSyncsOrientationFromCamera explaining why onUpdate is called with zero and negative dt (e.g., to assert that ensureAnglesSynced()/syncOrientationFromCamera still run and that negative dt should be treated as no-op for movement or used for robustness), placing the comment immediately above the b.onUpdate(0.0); b.onUpdate(-1.0); calls near the markAnglesDirty() usage so future readers understand the intent of the negative dt check.src/vertexnova/interaction/free_look_manipulator.cpp (1)
43-63: Consider extracting shared quaternion scaling utility.This
scaleTrackballDeltaQuaternionimplementation is identical toOrbitalTrackballRotation::scaleQuaternioninorbital_camera_manipulator.cpp. Consider moving this tointeraction_utils.hto eliminate duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vertexnova/interaction/free_look_manipulator.cpp` around lines 43 - 63, The function scaleTrackballDeltaQuaternion duplicates OrbitalTrackballRotation::scaleQuaternion; extract the shared logic into a single utility (e.g., a free function named scaleQuaternion or scaleTrackballQuaternion) declared in interaction_utils.h and defined in the corresponding .cpp, then replace both scaleTrackballDeltaQuaternion and OrbitalTrackballRotation::scaleQuaternion bodies to call that utility; ensure the new utility preserves the existing behavior and noexcept/[[nodiscard]] semantics and uses the same vne::math::Quatf API so callers need only include interaction_utils.h.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/07_camera_rig_composition/README.md`:
- Around line 17-24: The fenced diagram block in README.md lacks a language tag
causing markdownlint MD040; update the triple-backtick fence around the
CameraRig diagram to include a language identifier (for example use "text") so
the block becomes ```text and keeps the same content; this targets the fenced
code block that contains "CameraRig", "OrbitalCameraManipulator", and
"FreeLookManipulator" so the linter stops warning.
In `@include/vertexnova/interaction/free_look_manipulator.h`:
- Around line 138-140: getOrientation() currently returns the cached
orientation_ inline and can bypass the dirty-sync path controlled by
orientation_dirty_ (toggled by markAnglesDirty()), so change getOrientation() to
be an out-of-line method that first performs the same "if (orientation_dirty_)
syncAnglesToOrientation()" (or whatever existing resync routine is used
elsewhere) before returning orientation_; update the header to declare
getOrientation() non-inline and implement it to clear orientation_dirty_ after
resync to ensure external camera changes are reflected immediately.
In `@include/vertexnova/interaction/interaction_types.h`:
- Around line 224-228: The documentation for TouchPinch::scale is inverted;
update the comment on the TouchPinch struct (symbol: TouchPinch, member: scale)
so it reads that values >1 zoom out and values <1 zoom in (matching the rest of
the API and Example 06), and scan/update any adjacent
documentation/comments/examples (e.g., Example 06) that referenced the old
direction to keep them consistent.
- Around line 15-30: Create thin forwarding headers for the old public headers
(e.g., camera_action.h, camera_state.h, input_binding.h) that simply include
interaction_types.h and re-export the old type names as deprecated aliases; for
each removed symbol referenced downstream (e.g., OrbitCameraState,
CameraActionType, CameraCommandPayload, GestureAction, TrackballCameraState,
FreeCameraState, FreeLookInputState, OrbitalInteractionState, InputRule,
MouseBinding, KeyBinding) add a using typedef or inline alias in those
forwarding headers annotated with the compiler deprecation attribute or a
static_assert-friendly deprecation macro, so existing code continues to compile
while emitting deprecation warnings and guiding users to include
interaction_types.h instead.
In `@src/vertexnova/interaction/orbital_camera_manipulator.cpp`:
- Around line 628-631: The aspect ratio calculation can divide by zero if
viewport().height is zero; update the block in orbital_camera_manipulator.cpp
(around perspCamera(), fov_y_rad, fov_x_rad) to guard against zero height by
using a safe denominator or fallback: compute a safeHeight =
std::max(viewport().height, tiny_positive_value) or branch if viewport().height
== 0 and then compute aspect = std::max(viewport().width / safeHeight,
CameraManipulatorBase::kMinOrthoExtent) (or simply set aspect =
CameraManipulatorBase::kMinOrthoExtent when height is zero) before computing
fov_x_rad so division by zero cannot occur.
In `@tests/manipulator_regression_test.cpp`:
- Around line 300-324: This test, OrbitalCamera_HorizontalDragMatchesEulerSign,
currently has its body commented out and provides no verification; either
re-enable and run the original checks against the quaternion-based
implementation or remove/disable the test to avoid false coverage. To fix:
restore the commented setup and actions for
vne::interaction::OrbitalCameraManipulator (construct b, call
b.setRotationMode(vne::interaction::OrbitalRotationMode::eTrackball),
b.setCamera(persp), b.onResize(...)), recreate the CameraCommandPayload moves
and call b.onAction(...) for eBeginRotate/eRotateDelta/eEndRotate, then compute
pos_before/pos_after from persp->getPosition() and re-add the
EXPECT_LT(pos_after.x(), pos_before.x()) assertion; alternatively, if the
behavior is intentionally pending, replace the test with
TEST(ManipulatorRegression,
DISABLED_OrbitalCamera_HorizontalDragMatchesEulerSign) to mark it as not
executed until the quaternion implementation is ready.
In `@tests/orbital_camera_manipulator_test.cpp`:
- Around line 29-41: Remove the commented-out legacy tests and add active unit
assertions that reflect the current API and defaults for
OrbitalCameraManipulator: instantiate vne::interaction::OrbitalCameraManipulator
and assert current default rotation mode via getRotationMode(), default pivot
via getPivotMode(), that getOrbitDistance() and getZoomSpeed() are > 0, and test
trackball inertia/rotation behavior through the current public methods (e.g.,
setRotationMode()/applyRotation()/simulateInertia() or their renamed
equivalents) to verify inertia is applied when trackball mode is active; update
or replace the two additional commented blocks at lines ~150-151 and ~181-182 in
the same way.
---
Outside diff comments:
In `@docs/vertexnova/interaction/diagrams/component.drawio`:
- Around line 17-50: The diagram still lists removed components
(FollowController, FollowManipulator, EulerOrbitStrategy, OrbitBehavior) and
must be updated to reflect the current code; open the component.drawio shapes
with ids/controllers named controllers_pub, manipulators_pub, strategies and
behaviors and remove the obsolete labels (FollowController, FollowManipulator,
EulerOrbitStrategy, OrbitBehavior) and replace them with the current components
present in interaction_types.h and interaction_utils (or simply remove those
entries if no direct replacement exists), ensuring the values for
interaction_types_h, controllers_pub, manipulators_pub, strategies, and
behaviors match the updated source (interaction_types.h and interaction_utils).
In `@docs/vertexnova/interaction/interaction.md`:
- Line 140: Update the docs line that lists factory functions by removing the
deleted makeOrbit() and makeFollow() entries and only listing the implemented
factories: makeTrackball(), makeFps(), makeFly(), and makeOrtho2D(); keep the
note that you can still call addManipulator for custom stacks and ensure the
sentence references the same symbols (makeTrackball, makeFps, makeFly,
makeOrtho2D, addManipulator) so readers can find the valid API.
In `@examples/08_camera_state_save_restore/main.cpp`:
- Around line 5-18: Update the example banner to reflect the new
quaternion-based camera state API: remove references to the old
OrbitCameraState, yaw/pitch FreeCameraState and the non-exported
OrbitalInteractionState, and instead describe the actual exported quaternion
state types present in interaction_types.h (e.g., the quaternion-based trackball
state and the quaternion-based free-look state) — update the bullet list under
"Demonstrates:" to mention TrackballCameraState (quaternion orbit/trackball),
the new quaternion-based free-look state type, and that interaction
snapshot/serialize is the application's responsibility so readers consult
interaction_types.h for exact struct names and fields.
In `@include/vertexnova/interaction/camera_controller.h`:
- Around line 67-68: The onUpdate documentation for ICameraController still
mentions "follow smoothing" which is no longer supported; update the comment for
ICameraController::onUpdate to remove the stale "follow smoothing" phrase and
instead document only the active behaviors (e.g., inertia and damping) and the
delta_time parameter so the header's public docs reflect current behavior.
In `@include/vertexnova/interaction/camera_manipulator.h`:
- Around line 15-16: Update the `@brief` for ICameraManipulator to remove the
outdated "follow" mode and reflect only the currently supported modes (e.g.,
"orbit, fly, ortho, …"); locate the ICameraManipulator declaration in
camera_manipulator.h and edit the brief line so it no longer mentions "follow"
and matches the actual supported manipulators.
In `@include/vertexnova/interaction/camera_rig.h`:
- Around line 128-138: Existing API removed silently: add a deprecated bridge
named makeOrbit() that forwards to the new makeTrackball() to preserve source
compatibility; specifically, declare static CameraRig makeOrbit() in the same
header alongside makeTrackball() and mark it as deprecated (e.g., [[deprecated]]
or PROJECT_DEPRECATED macro) with a short comment pointing users to use
makeTrackball() and to the migration notes explaining makeFollow() removal, so
existing callers continue to compile for at least one release.
---
Nitpick comments:
In `@examples/06_custom_input_bindings/06_example.cpp`:
- Around line 5-7: Remove the duplicate include of "interaction_types.h" in the
example: keep a single `#include` "vertexnova/interaction/interaction_types.h"
alongside the `#include` "vertexnova/interaction/input_mapper.h" in 06_example.cpp
so the example is minimal and non-redundant.
In `@src/vertexnova/interaction/free_look_manipulator.cpp`:
- Around line 43-63: The function scaleTrackballDeltaQuaternion duplicates
OrbitalTrackballRotation::scaleQuaternion; extract the shared logic into a
single utility (e.g., a free function named scaleQuaternion or
scaleTrackballQuaternion) declared in interaction_utils.h and defined in the
corresponding .cpp, then replace both scaleTrackballDeltaQuaternion and
OrbitalTrackballRotation::scaleQuaternion bodies to call that utility; ensure
the new utility preserves the existing behavior and noexcept/[[nodiscard]]
semantics and uses the same vne::math::Quatf API so callers need only include
interaction_utils.h.
In `@tests/free_look_manipulator_test.cpp`:
- Around line 225-256: Add a brief inline comment in the test
FreeLookManipulator.OnUpdateZeroDeltaTimeStillSyncsOrientationFromCamera
explaining why onUpdate is called with zero and negative dt (e.g., to assert
that ensureAnglesSynced()/syncOrientationFromCamera still run and that negative
dt should be treated as no-op for movement or used for robustness), placing the
comment immediately above the b.onUpdate(0.0); b.onUpdate(-1.0); calls near the
markAnglesDirty() usage so future readers understand the intent of the negative
dt check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40bacf8e-76d0-416a-b642-5e2bdf3de339
📒 Files selected for processing (78)
CHANGELOG.mddeps/internal/vnescenedocs/vertexnova/interaction/diagrams/component.drawiodocs/vertexnova/interaction/interaction.mdexamples/01_library_info/01_example.cppexamples/01_library_info/README.mdexamples/02_medical_3d_inspect/02_example.cppexamples/02_medical_3d_inspect/README.mdexamples/05_robotic_simulator/05_example.cppexamples/05_robotic_simulator/05_example.hexamples/05_robotic_simulator/README.mdexamples/05_robotic_simulator/main.cppexamples/06_custom_input_bindings/06_example.cppexamples/07_camera_rig_composition/07_example.cppexamples/07_camera_rig_composition/README.mdexamples/07_camera_rig_composition/main.cppexamples/08_camera_state_save_restore/08_example.cppexamples/08_camera_state_save_restore/README.mdexamples/08_camera_state_save_restore/main.cppexamples/README.mdinclude/vertexnova/interaction/camera_action.hinclude/vertexnova/interaction/camera_controller.hinclude/vertexnova/interaction/camera_manipulator.hinclude/vertexnova/interaction/camera_manipulator_base.hinclude/vertexnova/interaction/camera_rig.hinclude/vertexnova/interaction/camera_state.hinclude/vertexnova/interaction/follow_controller.hinclude/vertexnova/interaction/follow_manipulator.hinclude/vertexnova/interaction/free_look_manipulator.hinclude/vertexnova/interaction/input_binding.hinclude/vertexnova/interaction/inspect_3d_controller.hinclude/vertexnova/interaction/interaction.hinclude/vertexnova/interaction/interaction_types.hinclude/vertexnova/interaction/navigation_3d_controller.hinclude/vertexnova/interaction/orbital_camera_manipulator.hinclude/vertexnova/interaction/ortho_2d_controller.hinclude/vertexnova/interaction/ortho_2d_manipulator.hsrc/CMakeLists.txtsrc/vertexnova/interaction/camera_controller_impl.hsrc/vertexnova/interaction/camera_manipulator_base.cppsrc/vertexnova/interaction/camera_math.hsrc/vertexnova/interaction/camera_rig.cppsrc/vertexnova/interaction/detail/euler_orbit_strategy.hsrc/vertexnova/interaction/detail/orbit_behavior.cppsrc/vertexnova/interaction/detail/orbit_behavior.hsrc/vertexnova/interaction/detail/rotation_strategy.hsrc/vertexnova/interaction/detail/trackball_behavior.cppsrc/vertexnova/interaction/detail/trackball_behavior.hsrc/vertexnova/interaction/detail/trackball_strategy.hsrc/vertexnova/interaction/follow_controller.cppsrc/vertexnova/interaction/follow_manipulator.cppsrc/vertexnova/interaction/free_look_manipulator.cppsrc/vertexnova/interaction/input_event_translator.cppsrc/vertexnova/interaction/input_event_translator.hsrc/vertexnova/interaction/input_mapper.cppsrc/vertexnova/interaction/inspect_3d_controller.cppsrc/vertexnova/interaction/interaction_utils.cppsrc/vertexnova/interaction/interaction_utils.hsrc/vertexnova/interaction/navigation_3d_controller.cppsrc/vertexnova/interaction/orbital_camera_manipulator.cppsrc/vertexnova/interaction/ortho_2d_controller.cppsrc/vertexnova/interaction/ortho_2d_manipulator.cppsrc/vertexnova/interaction/version.cpptests/CMakeLists.txttests/camera_rig_test.cpptests/controller_move_safety_test.cpptests/follow_controller_test.cpptests/follow_manipulator_test.cpptests/free_look_manipulator_test.cpptests/input_mapper_test.cpptests/inspect_3d_controller_test.cpptests/manipulator_regression_test.cpptests/navigation_3d_controller_test.cpptests/orbit_behavior_test.cpptests/orbital_camera_manipulator_test.cpptests/ortho_2d_controller_test.cpptests/ortho_2d_manipulator_test.cpptests/trackball_behavior_test.cpp
💤 Files with no reviewable changes (17)
- include/vertexnova/interaction/camera_action.h
- tests/CMakeLists.txt
- tests/orbit_behavior_test.cpp
- include/vertexnova/interaction/follow_manipulator.h
- src/vertexnova/interaction/detail/orbit_behavior.cpp
- tests/follow_manipulator_test.cpp
- src/vertexnova/interaction/detail/euler_orbit_strategy.h
- src/vertexnova/interaction/follow_controller.cpp
- include/vertexnova/interaction/input_binding.h
- include/vertexnova/interaction/camera_state.h
- src/vertexnova/interaction/follow_manipulator.cpp
- src/vertexnova/interaction/camera_math.h
- include/vertexnova/interaction/follow_controller.h
- src/vertexnova/interaction/detail/orbit_behavior.h
- src/vertexnova/interaction/detail/rotation_strategy.h
- tests/follow_controller_test.cpp
- src/vertexnova/interaction/detail/trackball_strategy.h
…n presets in OrbitalCameraManipulator
…and implementation files
…eraManipulator tests - Improved the documentation for TrackballCameraState to clarify its purpose and components. - Reinstated and enhanced tests for OrbitalCameraManipulator, including checks for default values and trackball rotation scale. - Removed commented-out code in tests to improve readability and maintainability.
…lator functionality - Enhanced descriptions of the OrbitalCameraManipulator, emphasizing quaternion virtual trackball implementation and its internal inertia handling. - Revised interface and implementation sections to reflect changes in strategy and behavior components, specifically regarding the use of TrackballBehavior for screen mapping. - Updated diagrams to accurately represent the current structure and functionality of the interaction library.
- Added comments to explain the purpose of testing both zero and negative delta time in the onUpdate method, ensuring that angle synchronization occurs before movement integration.
…aternion - Moved the scaleTrackballDeltaQuaternion function to interaction_utils.cpp and renamed it to scaleTrackballQuaternion for better clarity and reusability. - Updated references in FreeLookManipulator and OrbitalCameraManipulator to use the new function, ensuring consistent quaternion scaling behavior across manipulators.
- Removed unnecessary blank lines and improved code formatting in the scaleTrackballQuaternion function for better readability. - Ensured consistent quaternion axis calculation by simplifying the axis definition without altering functionality.
- Updated aspect ratio calculations in fitToAABB to ensure correct handling of viewport dimensions, preventing potential division by zero. - Enhanced getWorldUnitsPerPixel method to use a local variable for viewport height, improving readability and maintainability.
- Updated comments in 06_example.cpp to specify that the pinch scale is used in the same multiplicative sense as the scroll zoom factor. - Enhanced documentation in input_mapper.h and interaction_types.h to clarify the relationship between pinch scale and zoom factor, ensuring better understanding of the touch pinch gesture's functionality. - Adjusted the implementation in input_mapper.cpp to correctly use the pinch scale as the zoom factor, aligning with the updated documentation.
…e documentation - Updated getOrientation method to ensure angle synchronization before returning the camera-to-world orientation. - Introduced a constant for pitch clamping to improve stability in pitch calculations. - Adjusted README.md to specify code block language for better clarity in documentation. - Updated pitch limits in OrbitalCameraManipulator to prevent singularity issues during top/bottom views.
…bitalCameraManipulator - Added `setOrbitAnimationEnabled` method to `Inspect3DController` to toggle animation for fit and view-direction transitions. - Enhanced `OrbitalCameraManipulator` to support disabling animation, allowing for instant fit and view-direction changes. - Updated documentation to reflect new functionality and usage examples for animated view presets. - Added tests to verify the behavior of orbit animation toggling and its impact on fit and view-direction methods.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/vertexnova/interaction/orbital_camera_manipulator.cpp (1)
800-809:⚠️ Potential issue | 🟠 MajorResync the cached quaternion after
setLandmark().This method retargets the attached camera and recomputes
orbit_distance_, but it leavesorbital_rot_->orientationon the old view. The next basis-driven call (getOrientation(),setOrbitDistance(), orthographicfitToAABB(), view-direction animation, etc.) can then jump because it starts from stale state.Minimal fix
void OrbitalCameraManipulator::setLandmark(const vne::math::Vec3f& world_pos) noexcept { anim_->stop(); coi_world_ = world_pos; pivot_mode_ = OrbitPivotMode::eFixed; if (camera_) { orbit_distance_ = std::max((camera_->getPosition() - coi_world_).length(), kMinOrbitDistance); camera_->setTarget(coi_world_); camera_->updateMatrices(); + onPivotChanged(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vertexnova/interaction/orbital_camera_manipulator.cpp` around lines 800 - 809, OrbitalCameraManipulator::setLandmark currently updates coi_world_, orbit_distance_ and the camera target but leaves the cached quaternion (orbital_rot_->orientation) stale; after anim_->stop(), after you call camera_->setTarget(coi_world_) and camera_->updateMatrices(), resynchronize the cached orientation by assigning orbital_rot_->orientation from the camera's current orientation (e.g. from camera_->getOrientation() or by building the quaternion from camera_->getViewBasis()/getOrientation() and normalize it) so subsequent calls like getOrientation(), setOrbitDistance(), fitToAABB(), or any view-direction animations start from the correct, up-to-date rotation state. Ensure you update the same orbital_rot_ object used elsewhere (not a new local) and preserve pivot_mode_/coi_world_/orbit_distance_ logic as-is.src/vertexnova/interaction/free_look_manipulator.cpp (1)
104-113:⚠️ Potential issue | 🟠 Major
setCamera()discards an orientation configured before attachment.Line 279 stores the quaternion even when
camera_is null, but Lines 111-112 immediately replace that state fromcamera_->getOrientation()on attach. The new setter is therefore a silent no-op for “configure first, attach camera later” flows. Either preserve a pending orientation acrosssetCamera(), or reject the no-camera case up front likesetYawPitchDegrees()does.As per coding guidelines, "Avoid surprising API changes; keep interfaces stable in C++ code."
Also applies to: 278-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/vertexnova/interaction/free_look_manipulator.cpp` around lines 104 - 113, FreeLookManipulator::setCamera currently overwrites any previously configured pending orientation when attaching a camera because it always calls syncOrientationFromCamera() if camera_ is non-null; preserve a pending orientation or reject the preconfigured case: modify setCamera (which calls CameraManipulatorBase::setCamera) so that if a stored pending orientation exists (e.g., set earlier while camera_ was null) you do not call syncOrientationFromCamera() and you keep orientation_dirty_ as set by the prior setter, otherwise (no pending orientation) continue to sync from camera_; alternatively mirror setYawPitchDegrees() behavior by validating/rejecting the no-camera case up-front. Ensure the logic references camera_, orientation_dirty_, syncOrientationFromCamera(), and the pending orientation storage used by the prior setters.
🧹 Nitpick comments (1)
include/vertexnova/interaction/orbital_camera_manipulator.h (1)
154-157: Document the input contract ofsetOrientation().This new public API does not say whether non-unit quaternions are normalized internally or whether zero-length input is invalid. A one-line precondition/result note here would make the new surface much less surprising for callers.
As per coding guidelines, "Ensure public APIs have brief doc comments if behavior isn't obvious in C++ code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/vertexnova/interaction/orbital_camera_manipulator.h` around lines 154 - 157, The setter setOrientation lacks a contract for non-unit or zero-length quaternions; update its doc comment to state whether inputs are normalized and how zero-length is handled. Specifically, document that setOrientation(const vne::math::Quatf& rotation) noexcept will normalize non-unit quaternions before applying them, treat a zero-length quaternion as the identity rotation (or alternatively assert/throw if you prefer a stricter policy—pick one consistent with project style), and clear rotation inertia and update the camera accordingly; reference setOrientation and getOrientation in the comment so callers know expected behavior and the resulting orientation value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/vertexnova/interaction/interaction.md`:
- Around line 35-42: The docs still list removed types—remove all references to
FollowManipulator and FollowController from the grouping table and any other
sections that mention them (e.g., the "Manipulators" and "Controllers" lists and
any later mentions of follow workflows); update the table row entries so only
existing symbols remain (e.g., keep OrbitalCameraManipulator,
FreeLookManipulator, Ortho2DManipulator under Manipulators and
Inspect3DController, Navigation3DController, Ortho2DController under
Controllers) and remove any explanatory text that describes follow-specific
behavior or usage (references to FollowManipulator, FollowController, or
"follow" workflows) so the documentation no longer presents stale API guidance.
In `@src/vertexnova/interaction/free_look_manipulator.cpp`:
- Around line 42-43: The constant kPitchAsinSinAbsMax (used by clampFpsPitch) is
too low (0.999f) so asin clamps to ≈87.4° and never reaches the intended 89°
guard; fix by raising the threshold to match sin(89°) (e.g., 0.99985f or
0.9999f) or even better compute it as the largest float < 1.0 via
std::nextafter(1.0f, 0.0f) and use that value in clampFpsPitch (and any other
uses in the 237-257 range) so over-pitch detection/correction behaves as
intended.
- Around line 259-270: getYawDegrees() and getPitchDegrees() read orientation_
indirectly via yawPitchFromOrientation() without ensuring the cached angles are
synced, so they can return stale values; before computing yaw/pitch call
ensureAnglesSynced() (or use getOrientation() which already calls
ensureAnglesSynced()) so the orientation/angles are up-to-date. Update
getYawDegrees(), getPitchDegrees() to resync (call ensureAnglesSynced() or
getOrientation()) then call yawPitchFromOrientation(y,p) and return the correct
component.
In `@src/vertexnova/interaction/orbital_camera_manipulator.cpp`:
- Around line 298-332: viewDirectionYawPitch currently forces eTop/eBottom to
±kViewDirPitchLimitDeg (89°) which prevents exact pole-facing views; change it
to set pitch to exact ±90° for ViewDirection::eTop and ::eBottom (keep yaw
as-is) and remove the hardcoded 89° here. Then ensure the camera-basis
construction that consumes yaw/pitch (the code that converts yaw/pitch ->
orientation in the orbital camera logic) detects the pole case (pitch == ±90°)
and uses a stable orthonormal fallback basis (e.g., choose a deterministic
right/forward vector such as world X axis) instead of perturbing the requested
direction; reference viewDirectionYawPitch and kViewDirPitchLimitDeg to locate
the change and the downstream basis construction that must handle the pole
robustly.
In `@tests/inspect_3d_controller_test.cpp`:
- Around line 55-74: The test currently only verifies the manipulator state
after calling Inspect3DController::fitToAABB, but must also assert the attached
camera was snapped immediately when orbit animation is disabled; after
ctrl.fitToAABB(...), add assertions that the controller's attached camera (the
cam set via ctrl.setCamera(cam)) has its position and look-at/target updated to
the expected snap values (matching the computed center-of-interest/fit) rather
than waiting for onUpdate(), using the camera getter methods you have (e.g., the
cam instance's position and target accessors) and comparing them to the expected
values.
---
Outside diff comments:
In `@src/vertexnova/interaction/free_look_manipulator.cpp`:
- Around line 104-113: FreeLookManipulator::setCamera currently overwrites any
previously configured pending orientation when attaching a camera because it
always calls syncOrientationFromCamera() if camera_ is non-null; preserve a
pending orientation or reject the preconfigured case: modify setCamera (which
calls CameraManipulatorBase::setCamera) so that if a stored pending orientation
exists (e.g., set earlier while camera_ was null) you do not call
syncOrientationFromCamera() and you keep orientation_dirty_ as set by the prior
setter, otherwise (no pending orientation) continue to sync from camera_;
alternatively mirror setYawPitchDegrees() behavior by validating/rejecting the
no-camera case up-front. Ensure the logic references camera_,
orientation_dirty_, syncOrientationFromCamera(), and the pending orientation
storage used by the prior setters.
In `@src/vertexnova/interaction/orbital_camera_manipulator.cpp`:
- Around line 800-809: OrbitalCameraManipulator::setLandmark currently updates
coi_world_, orbit_distance_ and the camera target but leaves the cached
quaternion (orbital_rot_->orientation) stale; after anim_->stop(), after you
call camera_->setTarget(coi_world_) and camera_->updateMatrices(), resynchronize
the cached orientation by assigning orbital_rot_->orientation from the camera's
current orientation (e.g. from camera_->getOrientation() or by building the
quaternion from camera_->getViewBasis()/getOrientation() and normalize it) so
subsequent calls like getOrientation(), setOrbitDistance(), fitToAABB(), or any
view-direction animations start from the correct, up-to-date rotation state.
Ensure you update the same orbital_rot_ object used elsewhere (not a new local)
and preserve pivot_mode_/coi_world_/orbit_distance_ logic as-is.
---
Nitpick comments:
In `@include/vertexnova/interaction/orbital_camera_manipulator.h`:
- Around line 154-157: The setter setOrientation lacks a contract for non-unit
or zero-length quaternions; update its doc comment to state whether inputs are
normalized and how zero-length is handled. Specifically, document that
setOrientation(const vne::math::Quatf& rotation) noexcept will normalize
non-unit quaternions before applying them, treat a zero-length quaternion as the
identity rotation (or alternatively assert/throw if you prefer a stricter
policy—pick one consistent with project style), and clear rotation inertia and
update the camera accordingly; reference setOrientation and getOrientation in
the comment so callers know expected behavior and the resulting orientation
value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82e76d50-602e-4681-ba6a-477eca7d0d90
📒 Files selected for processing (22)
docs/vertexnova/interaction/diagrams/component.drawiodocs/vertexnova/interaction/interaction.mdexamples/02_medical_3d_inspect/02_example.cppexamples/02_medical_3d_inspect/README.mdexamples/06_custom_input_bindings/06_example.cppexamples/07_camera_rig_composition/README.mdinclude/vertexnova/interaction/camera_manipulator_base.hinclude/vertexnova/interaction/free_look_manipulator.hinclude/vertexnova/interaction/input_mapper.hinclude/vertexnova/interaction/inspect_3d_controller.hinclude/vertexnova/interaction/interaction_types.hinclude/vertexnova/interaction/orbital_camera_manipulator.hsrc/vertexnova/interaction/free_look_manipulator.cppsrc/vertexnova/interaction/input_mapper.cppsrc/vertexnova/interaction/inspect_3d_controller.cppsrc/vertexnova/interaction/interaction_utils.cppsrc/vertexnova/interaction/interaction_utils.hsrc/vertexnova/interaction/orbital_camera_manipulator.cpptests/free_look_manipulator_test.cpptests/inspect_3d_controller_test.cpptests/manipulator_regression_test.cpptests/orbital_camera_manipulator_test.cpp
✅ Files skipped from review due to trivial changes (3)
- include/vertexnova/interaction/camera_manipulator_base.h
- include/vertexnova/interaction/input_mapper.h
- examples/02_medical_3d_inspect/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/06_custom_input_bindings/06_example.cpp
- docs/vertexnova/interaction/diagrams/component.drawio
- examples/07_camera_rig_composition/README.md
- src/vertexnova/interaction/interaction_utils.cpp
- examples/02_medical_3d_inspect/02_example.cpp
- tests/orbital_camera_manipulator_test.cpp
- include/vertexnova/interaction/inspect_3d_controller.h
Comment on lines
+42
to
+43
| /** Clamp |forward·worldUp| below 1 for stable @c asin in @ref FreeLookManipulator::clampFpsPitch. */ | ||
| constexpr float kPitchAsinSinAbsMax = 0.999f; |
There was a problem hiding this comment.
clampFpsPitch() never reaches the 89° guard.
kPitchAsinSinAbsMax is 0.999f, but sin(89°) is about 0.99985. After the clamp on Line 243, asin(s) tops out around 87.4°, so the early return on Lines 246-247 is always taken and over-pitch rotations are never corrected in FPS mode.
🐛 Proposed fix
-/** Clamp |forward·worldUp| below 1 for stable `@c` asin in `@ref` FreeLookManipulator::clampFpsPitch. */
-constexpr float kPitchAsinSinAbsMax = 0.999f;
+/** Clamp into the valid `@c` asin domain while still allowing the 89° pitch guard to trigger. */
+constexpr float kPitchAsinSinAbsMax = 1.0f;Also applies to: 237-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/vertexnova/interaction/free_look_manipulator.cpp` around lines 42 - 43,
The constant kPitchAsinSinAbsMax (used by clampFpsPitch) is too low (0.999f) so
asin clamps to ≈87.4° and never reaches the intended 89° guard; fix by raising
the threshold to match sin(89°) (e.g., 0.99985f or 0.9999f) or even better
compute it as the largest float < 1.0 via std::nextafter(1.0f, 0.0f) and use
that value in clampFpsPitch (and any other uses in the 237-257 range) so
over-pitch detection/correction behaves as intended.
Comment on lines
+259
to
+270
| float FreeLookManipulator::getYawDegrees() const noexcept { | ||
| float y = 0.0f; | ||
| float p = 0.0f; | ||
| yawPitchFromOrientation(y, p); | ||
| return y; | ||
| } | ||
|
|
||
| float FreeLookManipulator::getPitchDegrees() const noexcept { | ||
| float y = 0.0f; | ||
| float p = 0.0f; | ||
| yawPitchFromOrientation(y, p); | ||
| return p; |
There was a problem hiding this comment.
Resync before returning yaw/pitch getters.
getOrientation() already calls ensureAnglesSynced(), but these accessors read orientation_ directly. That leaves them stale after paths that only mark the state dirty, such as the zoom flow in src/vertexnova/interaction/navigation_3d_controller.cpp.
💡 Proposed fix
float FreeLookManipulator::getYawDegrees() const noexcept {
+ const_cast<FreeLookManipulator*>(this)->ensureAnglesSynced();
float y = 0.0f;
float p = 0.0f;
yawPitchFromOrientation(y, p);
return y;
}
float FreeLookManipulator::getPitchDegrees() const noexcept {
+ const_cast<FreeLookManipulator*>(this)->ensureAnglesSynced();
float y = 0.0f;
float p = 0.0f;
yawPitchFromOrientation(y, p);
return p;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/vertexnova/interaction/free_look_manipulator.cpp` around lines 259 - 270,
getYawDegrees() and getPitchDegrees() read orientation_ indirectly via
yawPitchFromOrientation() without ensuring the cached angles are synced, so they
can return stale values; before computing yaw/pitch call ensureAnglesSynced()
(or use getOrientation() which already calls ensureAnglesSynced()) so the
orientation/angles are up-to-date. Update getYawDegrees(), getPitchDegrees() to
resync (call ensureAnglesSynced() or getOrientation()) then call
yawPitchFromOrientation(y,p) and return the correct component.
Comment on lines
+298
to
+332
| /** Top/bottom view presets: magnitude below 90° to stay off the polar singularity with world up. */ | ||
| constexpr float kViewDirPitchLimitDeg = 89.0f; | ||
|
|
||
| void viewDirectionYawPitch(ViewDirection dir, float& yaw, float& pitch) noexcept { | ||
| switch (dir) { | ||
| case ViewDirection::eFront: | ||
| yaw = 0.0f; | ||
| pitch = 0.0f; | ||
| break; | ||
| case ViewDirection::eBack: | ||
| yaw = kYawDegBack; | ||
| pitch = 0.0f; | ||
| break; | ||
| case ViewDirection::eLeft: | ||
| yaw = kYawDegLeft; | ||
| pitch = 0.0f; | ||
| break; | ||
| case ViewDirection::eRight: | ||
| yaw = kYawDegRight; | ||
| pitch = 0.0f; | ||
| break; | ||
| case ViewDirection::eTop: | ||
| yaw = 0.0f; | ||
| pitch = -kViewDirPitchLimitDeg; | ||
| break; | ||
| case ViewDirection::eBottom: | ||
| yaw = 0.0f; | ||
| pitch = kViewDirPitchLimitDeg; | ||
| break; | ||
| case ViewDirection::eIso: | ||
| yaw = kYawDegIso; | ||
| pitch = kPitchDegIso; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
eTop / eBottom no longer produce exact axis-aligned views.
viewDirectionYawPitch() hard-codes ±89°, so those presets always keep a 1° tilt instead of looking exactly along ±world-up. That contradicts the public ViewDirection contract and makes the preset views unreliable for exact CAD/inspection alignment. Please handle the pole case with a stable fallback basis instead of changing the requested direction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/vertexnova/interaction/orbital_camera_manipulator.cpp` around lines 298 -
332, viewDirectionYawPitch currently forces eTop/eBottom to
±kViewDirPitchLimitDeg (89°) which prevents exact pole-facing views; change it
to set pitch to exact ±90° for ViewDirection::eTop and ::eBottom (keep yaw
as-is) and remove the hardcoded 89° here. Then ensure the camera-basis
construction that consumes yaw/pitch (the code that converts yaw/pitch ->
orientation in the orbital camera logic) detects the pole case (pitch == ±90°)
and uses a stable orthonormal fallback basis (e.g., choose a deterministic
right/forward vector such as world X axis) instead of perturbing the requested
direction; reference viewDirectionYawPitch and kViewDirPitchLimitDeg to locate
the change and the downstream basis construction that must handle the pole
robustly.
Comment on lines
+55
to
74
| TEST(Inspect3DController, SetOrbitAnimationEnabledFalseSnapFitWithoutSteppingUpdate) { | ||
| vne::interaction::Inspect3DController ctrl; | ||
| auto cam = makePerspCamera(); | ||
| cam->setPosition(vne::math::Vec3f(0.0f, 0.0f, 5.0f)); | ||
| cam->lookAt(vne::math::Vec3f(0.0f, 0.0f, 0.0f), vne::math::Vec3f(0.0f, 1.0f, 0.0f)); | ||
| ctrl.setCamera(cam); | ||
| ctrl.onResize(1280.0f, 720.0f); | ||
|
|
||
| ctrl.orbitalCameraManipulator().setFitAnimationDuration(0.5f); | ||
| ctrl.setOrbitAnimationEnabled(false); | ||
| EXPECT_FALSE(ctrl.isOrbitAnimationEnabled()); | ||
|
|
||
| ctrl.fitToAABB(vne::math::Vec3f(9.0f, -1.0f, -1.0f), vne::math::Vec3f(11.0f, 1.0f, 1.0f)); | ||
| const auto coi = ctrl.orbitalCameraManipulator().getCenterOfInterestWorld(); | ||
| EXPECT_NEAR(coi.x(), 10.0f, 0.02f); | ||
| EXPECT_FLOAT_EQ(ctrl.orbitalCameraManipulator().getFitAnimationDuration(), 0.5f); | ||
|
|
||
| ctrl.setOrbitAnimationEnabled(true); | ||
| EXPECT_TRUE(ctrl.isOrbitAnimationEnabled()); | ||
| } |
There was a problem hiding this comment.
Assert the camera snap, not just the manipulator state.
This test is meant to prove fitToAABB() applies immediately when orbit animation is disabled, but it only checks getCenterOfInterestWorld() and the stored duration. It would still pass if the controller deferred the actual camera pose update until onUpdate(). Please assert the attached camera’s target/position right after ctrl.fitToAABB(...).
Suggested assertion tightening
ctrl.fitToAABB(vne::math::Vec3f(9.0f, -1.0f, -1.0f), vne::math::Vec3f(11.0f, 1.0f, 1.0f));
const auto coi = ctrl.orbitalCameraManipulator().getCenterOfInterestWorld();
EXPECT_NEAR(coi.x(), 10.0f, 0.02f);
+ EXPECT_NEAR(cam->getTarget().x(), 10.0f, 0.02f);
+ EXPECT_GT((cam->getPosition() - vne::math::Vec3f(0.0f, 0.0f, 5.0f)).length(), 1e-4f);
EXPECT_FLOAT_EQ(ctrl.orbitalCameraManipulator().getFitAnimationDuration(), 0.5f);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/inspect_3d_controller_test.cpp` around lines 55 - 74, The test
currently only verifies the manipulator state after calling
Inspect3DController::fitToAABB, but must also assert the attached camera was
snapped immediately when orbit animation is disabled; after ctrl.fitToAABB(...),
add assertions that the controller's attached camera (the cam set via
ctrl.setCamera(cam)) has its position and look-at/target updated to the expected
snap values (matching the computed center-of-interest/fit) rather than waiting
for onUpdate(), using the camera getter methods you have (e.g., the cam
instance's position and target accessors) and comparing them to the expected
values.
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
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.
Description
Release notes: Use a Conventional Commits–style PR title (e.g.
feat: add X,fix: resolve Y,docs: update Z) so release-please can include this change in the changelog. If you squash-merge, use the PR title as the commit message.Checklist
cmake -B build/sharedandcmake --build build/shared, or platform script with-l shared/-l static).ctest --test-dir build/sharedor script-a test).clang-formatas configured for this repo); CI clang-format will check.Additional notes
Summary by CodeRabbit
New Features
Changed
Removed
Tests