Improve camera zoom and pan for smoother navigation#198
Conversation
Zoom: - Proportional zoom: each scroll tick moves 15% of current distance, so zooming feels consistent regardless of model size - Minimum distance clamp prevents zooming through the target - Pinch gesture uses float delta for fine-grained control Pan: - WASD pan is now proportional to camera distance (slower when close) - Two-finger trackpad swipe always pans (both axes), no longer zooms on vertical swipe — uses Qt::MouseEventSynthesizedBySystem to distinguish trackpad from mouse wheel - Mouse scroll wheel still zooms (Ctrl+scroll also zooms) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughZoom and pan functionality refactored from fixed integer deltas to real-valued, distance-aware motion calculations. Input handling branches on source (Ctrl modifier, system events, mouse wheel) with proportional scaling. Gesture handling simplified for consistency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
📝 Coding Plan
Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/OgreWidget.cpp (1)
271-273: Consider replacing pinch scale magic number with a named constant.Line 272 uses
5.0inline; promoting it to a named constant (or shared camera-input tuning value) will make future sensitivity tuning safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OgreWidget.cpp` around lines 271 - 273, Replace the magic literal 5.0 used when computing pinch delta with a named constant (e.g., PINCH_SCALE or kPinchScale) and use that constant in the computation of delta in the method where gestureEvent is handled (the code calling mCamera->zoomByDelta). Define the constant in a visible scope for tuning (file-level static const, anonymous-namespace const, or a static/class member on OgreWidget) so it can be adjusted centrally, keep the same static_cast to Ogre::Real when multiplying gestureEvent->value(), and update any references to use the new named constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SpaceCamera.cpp`:
- Around line 229-234: The touchpad detection using event->source() ==
Qt::MouseEventSynthesizedBySystem is unreliable; change the logic in the scroll
handling branch (the block that currently calls pan(Ogre::Vector2(xDelta *
panScale, yDelta * panScale)) and later falls through to zoom) to also check
event->pixelDelta() (Qt 6 high-resolution scrolling) as a fallback: treat
non-zero pixelDelta() (or a sufficiently large pixelDelta magnitude) as a
touchpad/trackpad scroll and call pan(...) with the scaled pixel deltas,
otherwise fall back to angleDelta() behavior (wheel steps) and call zoom(...).
Update references around MouseEventSynthesizedBySystem, pixelDelta(),
angleDelta(), pan(), and zoom() accordingly so touchpad scrolling consistently
triggers panning across platforms.
---
Nitpick comments:
In `@src/OgreWidget.cpp`:
- Around line 271-273: Replace the magic literal 5.0 used when computing pinch
delta with a named constant (e.g., PINCH_SCALE or kPinchScale) and use that
constant in the computation of delta in the method where gestureEvent is handled
(the code calling mCamera->zoomByDelta). Define the constant in a visible scope
for tuning (file-level static const, anonymous-namespace const, or a
static/class member on OgreWidget) so it can be adjusted centrally, keep the
same static_cast to Ogre::Real when multiplying gestureEvent->value(), and
update any references to use the new named constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c1bd35a-d566-499a-9aca-ddedf137ab7f
📒 Files selected for processing (3)
src/OgreWidget.cppsrc/SpaceCamera.cppsrc/SpaceCamera.h
| else if (event->source() == Qt::MouseEventSynthesizedBySystem) | ||
| { | ||
| // Trackpad two-finger swipe = pan in both axes | ||
| Ogre::Real panScale = 3.0f; | ||
| pan(Ogre::Vector2(xDelta * panScale, yDelta * panScale)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Qt (5 and 6), is QWheelEvent::source() == Qt::MouseEventSynthesizedBySystem a reliable way to detect two-finger touchpad scroll across macOS, Windows, and Linux? What is the recommended cross-platform detection signal (e.g., pixelDelta, pointingDevice type, phase)?
💡 Result:
No. QWheelEvent::source() == Qt::MouseEventSynthesizedBySystem is not a reliable (or portable) “two‑finger touchpad scroll” detector across macOS/Windows/Linux (and not even reliably on macOS): Qt explicitly warns that it was “tempting and wrong” to equate “synthesized” mouse events with “from touch”, and in practice both mouse-wheel and touchpad wheel events can report MouseEventSynthesizedBySystem. [1], [2]
Recommended cross‑platform approach (Qt 5 & Qt 6): don’t detect “touchpad”, detect high‑resolution scrolling
Use the wheel event properties that describe how the scroll is delivered, and handle both smoothly:
- Prefer
pixelDelta()when non-null (high‑resolution / touch-like scrolling); otherwise fall back toangleDelta()(traditional wheel “steps”). [3] - Use
phase()(when provided) to know whether you’re starting/updating/ending a scroll sequence; for non‑phase‑aware mice it isNoScrollPhase. [4] - Use
inverted()to respect “natural scrolling” direction (trackpads, some mice). (Documented alongside wheel event construction in Qt docs.) [3]
Qt 6: if you truly need device-based detection, use the pointing device (not source())
In Qt 6, QWheelEvent carries a QPointingDevice* device, and Qt documents that for touchpad-originated wheel events device()->type() will be QInputDevice::DeviceType::Touchpad. [3]
That’s the closest Qt provides to an explicit “this was a touchpad” classification; availability/accuracy still depends on platform backend/device discovery, but it’s the intended API in Qt 6—not source(). [1], [3]
Practical rule of thumb (best portability):
- Treat
pixelDelta()!=QPoint(0,0)as “smooth/pixel scrolling” (often touchpad / precision scrolling), and otherwise treat it as a wheel. [3] - Use
device()->type()==Touchpadonly as an optional refinement in Qt 6. [3]
Sources:
[1] Qt blog (“Input Events in Qt 6”) on why QMouseEvent::source() was misleading for inferring touch origin (qt.io)
[2] StackOverflow report that both real mouse wheel and touchpad can show MouseEventSynthesizedBySystem (stackoverflow.com)
[3] QWheelEvent docs (Qt 6) including pixelDelta/angleDelta/phase/source/device() and note about device()->type()==Touchpad (doc.qt.io)
[4] Qt 5.7 changelog note: QWheelEvent::phase() returns NoScrollPhase for non-phase-aware mice (sources.debian.org)
Citations:
- 1: https://www.qt.io/blog/input-events-in-qt-6?utm_source=openai
- 2: https://stackoverflow.com/questions/72789280/how-to-distinguish-real-mouse-wheel-events-in-qt6?utm_source=openai
- 3: https://doc.qt.io/qt-6/qwheelevent.html?utm_source=openai
- 4: https://sources.debian.org/src/qtbase-opensource-src-gles/5.15.8%2Bdfsg-3/dist/changes-5.7.0/?utm_source=openai
🏁 Script executed:
# Find and examine SpaceCamera.cpp around lines 229-234
fd SpaceCamera.cpp --type fRepository: fernandotonon/QtMeshEditor
Length of output: 89
🏁 Script executed:
# Get the size and examine the wheel event handling context
wc -l src/SpaceCamera.cppRepository: fernandotonon/QtMeshEditor
Length of output: 93
🏁 Script executed:
# Read the wheel event handling context
cat -n src/SpaceCamera.cpp | sed -n '200,260p'Repository: fernandotonon/QtMeshEditor
Length of output: 1976
🏁 Script executed:
# Check Qt version and includes to understand which Qt version is being used
rg "QT_VERSION|include.*Qt.*Core|QWheelEvent" src/SpaceCamera.cpp -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 226
🏁 Script executed:
# Check Qt version in CMakeLists.txt or project configuration
fd "CMakeLists.txt|\.pro|\.pri|conanfile" --type f | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 187
🏁 Script executed:
# Check Qt version in CMakeLists.txt
rg "find_package.*Qt|QT_VERSION|qt_version" CMakeLists.txt -iRepository: fernandotonon/QtMeshEditor
Length of output: 1502
Touchpad detection via MouseEventSynthesizedBySystem is unreliable; use pixel-delta as fallback.
Qt's MouseEventSynthesizedBySystem flag is not reliable for distinguishing touchpad events across macOS, Windows, and Linux—both real mouse wheel and touchpad scroll can report it, and some platforms may not set it at all. This causes touchpad scrolling to incorrectly fall through to zoom (line 238) instead of panning.
Harden detection by checking for high-resolution scrolling:
Fix: Add pixel-delta fallback
- else if (event->source() == Qt::MouseEventSynthesizedBySystem)
+ else if (event->source() == Qt::MouseEventSynthesizedBySystem
+ || !event->pixelDelta().isNull())
{
// Trackpad two-finger swipe = pan in both axes
Ogre::Real panScale = 3.0f;
pan(Ogre::Vector2(xDelta * panScale, yDelta * panScale));
}Qt 6 (which this project uses) documents that pixelDelta() indicates high-resolution/touch-like scrolling (typically trackpad), while angleDelta() is traditional wheel "steps." Falling back to pixel-delta detection ensures portable touchpad handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SpaceCamera.cpp` around lines 229 - 234, The touchpad detection using
event->source() == Qt::MouseEventSynthesizedBySystem is unreliable; change the
logic in the scroll handling branch (the block that currently calls
pan(Ogre::Vector2(xDelta * panScale, yDelta * panScale)) and later falls through
to zoom) to also check event->pixelDelta() (Qt 6 high-resolution scrolling) as a
fallback: treat non-zero pixelDelta() (or a sufficiently large pixelDelta
magnitude) as a touchpad/trackpad scroll and call pan(...) with the scaled pixel
deltas, otherwise fall back to angleDelta() behavior (wheel steps) and call
zoom(...). Update references around MouseEventSynthesizedBySystem, pixelDelta(),
angleDelta(), pan(), and zoom() accordingly so touchpad scrolling consistently
triggers panning across platforms.
![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: c71a24485e
ℹ️ 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".
| // Proportional zoom: step size scales with distance to target, | ||
| // so zooming feels consistent regardless of model size. | ||
| Ogre::Real distance = std::abs(mCameraNode->getPosition().z); | ||
| Ogre::Real minDistance = 0.01f; |
There was a problem hiding this comment.
Using a hardcoded minDistance = 0.01f lets the camera stop much closer to the target than the configured near clip plane (setNearClipDistance(1.0f) in SpaceCamera::SpaceCamera), so zooming in can place the focal region inside the near plane and make geometry disappear/clamp oddly at close range. This regression comes from the new minimum-distance clamp; it should be derived from the active near clip distance (or another scene-scale-aware lower bound) instead of 0.01.
Useful? React with 👍 / 👎.
|
1 participant
Summary
Controls summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes