feat: Integrated new vneintraction code. Also, make vneinteraction by default on for the vnetestbed. - #43
Merged
Conversation
…improved compatibility
- Replaced references to behavior classes with manipulator classes for improved clarity and consistency in interaction management. - Updated navigation mode handling to utilize FreeLookMode instead of NavigateMode, enhancing the user experience. - Adjusted UI elements and documentation to reflect these changes, ensuring accurate representation of available controls and settings.
…action - Moved trackball projection settings UI elements to the correct conditional block based on the rotation mode, ensuring proper display and functionality. - Enhanced clarity in the interaction settings by maintaining consistent UI behavior for trackball projection options.
- Changed the default option for VNE_WITH_VNEINTRACTION to ON, indicating that the vneinteraction dependency is now expected for this repository. - Updated comments in VNEPrivateDeps.cmake to clarify the behavior of the vneinteraction dependency management, ensuring better understanding of its integration and testing options.
📝 WalkthroughWalkthroughThis PR flips the CMake option Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the GLFW/OpenGL interaction sample code to the newer vneinteraction API (mode/type renames and behavior→manipulator accessors) and makes vneinteraction enabled by default in the repo build.
Changes:
- Switch sample interaction code from
*Behavior()APIs /NavigateMode/OrbitRotationModeto*Manipulator()APIs /FreeLookMode/OrbitalRotationMode. - Default
VNE_WITH_VNEINTRACTIONtoONso thevneinteractionsubmodule is expected for typical builds. - Extend
VNEPrivateDeps.cmakecache var wiring to keepvneinteractiontests/examples/dev options disabled by default.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| samples/glfw_opengl/common/base_scene_layer.cpp | Update Inspect controller rotation mode enum name to match new interaction API. |
| samples/glfw_opengl/03_test_interaction/README.md | Update documentation to reference FreeLookMode (needs a small consistency fix). |
| samples/glfw_opengl/03_test_interaction/demo_test_interaction.h | Replace NavigateMode with FreeLookMode in the interaction demo interface/state. |
| samples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp | Migrate demo implementation to manipulator APIs and updated interaction types. |
| CMakeLists.txt | Enable vneinteraction by default via VNE_WITH_VNEINTRACTION=ON. |
| cmake/VNEPrivateDeps.cmake | Ensure vneinteraction tests/examples/dev options stay disabled unless explicitly enabled. |
- Updated comments in VNEPrivateDeps.cmake to clarify the behavior of the SAVE_RESTORE mechanism for VNE_INTERACTION_* variables, addressing issues seen in CI with stale cache values. - Set VNETESTBED_VNEINTERACTION_EMBEDDED to ON to ensure proper integration of the vneinteraction tests while keeping its gtest suite out of vnetestbed.
…test_interaction - Introduced a new constant for the default camera up vector to improve camera orientation handling. - Updated the resetCamera method to set the camera's up direction, enhancing the overall interaction experience.
- Updated the vneinteraction subproject to the latest commit for improved compatibility. - Enhanced comments in VNEPrivateDeps.cmake to clarify the handling of test configurations and prevent stale cache issues during CI builds.
Comment on lines
+101
to
+102
| # vneinteraction is expected for this repo; still overridable with -DVNE_WITH_VNEINTRACTION=OFF if needed. | ||
| option(VNE_WITH_VNEINTRACTION "Enable private dependency vneinteraction" ON) |
There was a problem hiding this comment.
The CMake option name is misspelled (VNE_WITH_VNEINTRACTION vs vneinteraction). Since this is user-facing and now default-ON, it’s easy for consumers to guess the (correctly spelled) flag and get confused. Consider introducing a correctly spelled VNE_WITH_VNEINTERACTION option and treating VNE_WITH_VNEINTRACTION as a deprecated alias (copy value over + emit a deprecation message), then use the correctly spelled name throughout the build logic.
Suggested change
| # vneinteraction is expected for this repo; still overridable with -DVNE_WITH_VNEINTRACTION=OFF if needed. | |
| option(VNE_WITH_VNEINTRACTION "Enable private dependency vneinteraction" ON) | |
| # vneinteraction is expected for this repo; use -DVNE_WITH_VNEINTERACTION=OFF to disable it. | |
| # Backward compatibility: VNE_WITH_VNEINTRACTION is a deprecated misspelled alias. | |
| option(VNE_WITH_VNEINTERACTION "Enable private dependency vneinteraction" ON) | |
| if(DEFINED VNE_WITH_VNEINTRACTION) | |
| message(DEPRECATION | |
| "CMake option VNE_WITH_VNEINTRACTION is deprecated and will be removed in a future release. " | |
| "Use VNE_WITH_VNEINTERACTION instead.") | |
| set(VNE_WITH_VNEINTERACTION "${VNE_WITH_VNEINTRACTION}" CACHE BOOL | |
| "Enable private dependency vneinteraction" FORCE) | |
| endif() |
Comment on lines
+12
to
+13
| set(VNETESTBED_VNEINTERACTION_EMBEDDED ON) | ||
| # vneinteraction uses VNE_INTERACTION_TESTS (not BUILD_TESTS). CACHE_VARS turn tests off before |
There was a problem hiding this comment.
VNETESTBED_VNEINTERACTION_EMBEDDED is set here but (in this repo) is not referenced anywhere else. If it’s meant to control behavior inside the vneinteraction subproject, consider passing it explicitly via CACHE_VARS (or documenting where it’s consumed); otherwise remove the assignment to avoid dead configuration state.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
samples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp (1)
838-866:⚠️ Potential issue | 🔴 CriticalThis code will fail to compile —
TrackballProjectionModeenum and the getter/setter methods do not exist.The trackball projection UI block (lines 858–865) references:
vne::interaction::TrackballProjectionModeenum (undefined)orb.getTrackballProjectionMode()method (undefined)orb.setTrackballProjectionMode()method (undefined)These APIs are not present in the interaction library. Either they need to be implemented in the
OrbitalCameraManipulatorclass, or this entire block should be removed if this functionality is not yet supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp` around lines 838 - 866, The Trackball projection UI references nonexistent APIs (vne::interaction::TrackballProjectionMode, orb.getTrackballProjectionMode(), orb.setTrackballProjectionMode()); remove the inner block guarded by insp->getRotationMode() == vne::interaction::OrbitalRotationMode::eTrackball that declares TPM, proj_idx, proj_names, the ImGui::Combo call, and the ImGui::TextDisabled line, or alternatively implement the missing enum and orb getter/setter in OrbitalCameraManipulator (implement TrackballProjectionMode, getTrackballProjectionMode(), setTrackballProjectionMode()) if projection modes are intended to be supported; update references accordingly (TrackballProjectionMode, getTrackballProjectionMode, setTrackballProjectionMode).
🧹 Nitpick comments (1)
samples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp (1)
67-92: Consider adding an explicitdefaultcase to suppress compiler warnings.The switch covers all
ControllerKindvalues with a fallback return after the switch, but some compilers may still warn about a missingdefault. Adding an explicit default case would silence these warnings and clarify intent.♻️ Suggested fix
case ControllerKind::eFollow: { return vne::interaction::FollowController{}; } + default: + break; } return vne::interaction::Inspect3DController{}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp` around lines 67 - 92, Add an explicit default case to the switch inside makeController to silence compiler warnings and make intent clear: inside makeController(ControllerKind kind, vne::interaction::FreeLookMode nav_mode) add a default: branch that returns a sensible fallback (e.g., vne::interaction::Inspect3DController{}) or asserts/throws if unreachable; update/remove the redundant return after the switch if desired. Reference: makeController, ControllerKind, Inspect3DController, Navigation3DController, Ortho2DController, FollowController.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@samples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp`:
- Around line 838-866: The Trackball projection UI references nonexistent APIs
(vne::interaction::TrackballProjectionMode, orb.getTrackballProjectionMode(),
orb.setTrackballProjectionMode()); remove the inner block guarded by
insp->getRotationMode() == vne::interaction::OrbitalRotationMode::eTrackball
that declares TPM, proj_idx, proj_names, the ImGui::Combo call, and the
ImGui::TextDisabled line, or alternatively implement the missing enum and orb
getter/setter in OrbitalCameraManipulator (implement TrackballProjectionMode,
getTrackballProjectionMode(), setTrackballProjectionMode()) if projection modes
are intended to be supported; update references accordingly
(TrackballProjectionMode, getTrackballProjectionMode,
setTrackballProjectionMode).
---
Nitpick comments:
In `@samples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp`:
- Around line 67-92: Add an explicit default case to the switch inside
makeController to silence compiler warnings and make intent clear: inside
makeController(ControllerKind kind, vne::interaction::FreeLookMode nav_mode) add
a default: branch that returns a sensible fallback (e.g.,
vne::interaction::Inspect3DController{}) or asserts/throws if unreachable;
update/remove the redundant return after the switch if desired. Reference:
makeController, ControllerKind, Inspect3DController, Navigation3DController,
Ortho2DController, FollowController.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d56380d1-f610-43df-bcda-f0241d23969e
📒 Files selected for processing (3)
cmake/VNEPrivateDeps.cmakedeps/internal/vnescenesamples/glfw_opengl/03_test_interaction/demo_test_interaction.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cmake/VNEPrivateDeps.cmake
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 buildandcmake --build build, or platform script).ctest --test-dir buildor script-a test).clang-formatas configured for this repo); CI clang-format will check.Additional notes
Summary by CodeRabbit
Note
Medium Risk
Moderate risk because it changes build defaults to require the
vneinteractionsubmodule and updates sample code to new interaction API types/manipulators, which could break builds if dependencies or API versions mismatch.Overview
Enables
vneinteractionby default in the top-level CMake config (still overridable), making the submodule an expected dependency.Hardens private-dep integration in
VNEPrivateDeps.cmakeby explicitly disablingvneinteraction’s own tests/examples/dev/CI options when embedded, reducing the chance of stale CMake cache re-enabling extra targets.Updates the
03_test_interactionsample (andBaseInteractionLayer) to the newer manipulator-based interaction API: swaps*Behavioraccessors for*Manipulator, renames types (NavigateMode→FreeLookMode,OrbitRotationMode→OrbitalRotationMode, trackball projection type), tweaks UI strings accordingly, and resets camera up-vector on reset.Reviewed by Cursor Bugbot for commit 749b066. Bugbot is set up for automated code reviews on this repo. Configure here.