add Alt+Click drag to adjust viewport #433
Merged
+69
−2
Conversation
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
Reviewer's GuideImplements an Alt+Left-click drag interaction on the map canvas to rotate the 3D viewport camera (yaw and optionally pitch) with visual cursor feedback, and extends the horizontal camera angle configuration to support full 360° rotation. Sequence diagram for Alt+Left-drag camera rotation interactionsequenceDiagram
actor User
participant MapCanvas
participant CanvasAdvancedConfig as CanvasAdvancedConfig
User->>MapCanvas: mousePressEvent(Alt + Left, pos0)
MapCanvas->>MapCanvas: create m_altDragState(lastPos = pos0, originalCursor)
MapCanvas->>MapCanvas: setCursor(ClosedHandCursor)
MapCanvas-->>User: event.accept()
loop While Alt held and mouse moves
User->>MapCanvas: mouseMoveEvent(Alt + Left, posN)
MapCanvas->>MapCanvas: delta = posN - lastPos
MapCanvas->>CanvasAdvancedConfig: read horizontalAngle, verticalAngle, autoTilt
alt dx != 0
MapCanvas->>CanvasAdvancedConfig: horizontalAngle.set(horizontalAngle + dx * SENSITIVITY)
end
alt !autoTilt and dy != 0
MapCanvas->>CanvasAdvancedConfig: verticalAngle.set(verticalAngle - dy * SENSITIVITY)
end
alt angleChanged
MapCanvas->>MapCanvas: graphicsSettingsChanged()
end
MapCanvas->>MapCanvas: update lastPos
MapCanvas-->>User: event.accept()
end
User->>MapCanvas: mouseReleaseEvent(event)
MapCanvas->>MapCanvas: restore cursor from m_altDragState.originalCursor
MapCanvas->>MapCanvas: m_altDragState.reset()
MapCanvas-->>User: event.accept()
Updated class diagram for MapCanvas Alt-drag state and configurationclassDiagram
class MapCanvas {
+MapCanvas(MapData &mapData, PrespammedPath &prespammedPath)
+void mousePressEvent(QMouseEvent *event)
+void mouseMoveEvent(QMouseEvent *event)
+void mouseReleaseEvent(QMouseEvent *event)
-std::optional~AltDragState~ m_altDragState
}
class AltDragState {
+QPoint lastPos
+QCursor originalCursor
}
class Configuration {
}
class CanvasAdvancedConfig {
+FixedPoint_1_ verticalAngle
+FixedPoint_1_ horizontalAngle "-180.0 .. 180.0 degrees"
+FixedPoint_1_ fov
+FixedPoint_1_ layerHeight
+BoolSetting autoTilt
}
MapCanvas *-- AltDragState : uses
Configuration o-- CanvasAdvancedConfig : has
MapCanvas ..> CanvasAdvancedConfig : setConfig().canvas.advanced
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Alt-drag state is only cleared on mouseRelease, so if the user releases the Alt key while still holding the mouse button the canvas will stay in drag mode with the closed-hand cursor; consider listening for modifier changes or checking
event->modifiers()duringmouseMoveEventto cancel the drag when Alt is no longer pressed. - The
SENSITIVITYconstant inmouseMoveEventis hardcoded and its comment (1.0 degree per pixel) no longer matches the value (0.3f); consider either aligning the value with the comment or moving this into configuration so the mapping between pixels and degrees is explicit and easier to tune.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Alt-drag state is only cleared on mouseRelease, so if the user releases the Alt key while still holding the mouse button the canvas will stay in drag mode with the closed-hand cursor; consider listening for modifier changes or checking `event->modifiers()` during `mouseMoveEvent` to cancel the drag when Alt is no longer pressed.
- The `SENSITIVITY` constant in `mouseMoveEvent` is hardcoded and its comment (`1.0 degree per pixel`) no longer matches the value (`0.3f`); consider either aligning the value with the comment or moving this into configuration so the mapping between pixels and degrees is explicit and easier to tune.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas.cpp:603-604` </location>
<code_context>
+ const auto delta = pos - dragState.lastPos;
+ dragState.lastPos = pos;
+
+ // Camera rotation sensitivity: 1.0 degree per pixel
+ constexpr float SENSITIVITY = 0.3f;
+
+ // Horizontal movement adjusts yaw (horizontalAngle)
</code_context>
<issue_to_address>
**issue:** Sensitivity comment and constant value are inconsistent.
The code comment says `1.0 degree per pixel`, but `SENSITIVITY` is `0.3f`. Please update either the comment or the constant so they match the actual behavior.
</issue_to_address>
### Comment 2
<location> `src/display/mapcanvas.cpp:623` </location>
<code_context>
+ }
+ }
+
+ graphicsSettingsChanged();
+ event->accept();
+ return;
</code_context>
<issue_to_address>
**suggestion (performance):** graphicsSettingsChanged() is called even when no angle change occurs.
When dx == 0 and (auto-tilt is enabled or dy == 0), neither horizontalAngle nor verticalAngle changes, yet graphicsSettingsChanged() is still emitted. If this signal can trigger expensive work, add a guard so it’s only emitted when one of these angles actually changes.
Suggested implementation:
```cpp
void MapCanvas::mouseMoveEvent(QMouseEvent *const event)
{
if (m_altDragState.has_value()) {
auto &conf = setConfig().canvas.advanced;
bool angleChanged = false;
```
```cpp
// Vertical movement adjusts pitch (verticalAngle), if auto-tilt is off
if (!conf.autoTilt.get()) {
const int dy = delta.y();
if (dy != 0) {
// Negated to match intuitive up/down dragging
conf.verticalAngle.set(conf.verticalAngle.get()
+ static_cast<int>(static_cast<float>(-dy) * SENSITIVITY));
angleChanged = true;
}
}
```
```cpp
if (angleChanged) {
graphicsSettingsChanged();
}
event->accept();
return;
```
You should also set `angleChanged = true;` anywhere in this `mouseMoveEvent` branch where `conf.horizontalAngle` is modified (likely in the `dx != 0` handling). That ensures `graphicsSettingsChanged()` is emitted whenever either `horizontalAngle` or `verticalAngle` changes, and skipped when neither changes, matching the intent of the review comment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #433 +/- ##
==========================================
- Coverage 66.48% 65.53% -0.96%
==========================================
Files 85 86 +1
Lines 4186 3963 -223
Branches 255 264 +9
==========================================
- Hits 2783 2597 -186
+ Misses 1403 1366 -37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nschimme
force-pushed
the
alt-click-viewport
branch
from
fc0569e to
7957436
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider handling
m_altDragStatecleanup in additional paths likeleaveEvent/focusOutEvent(or widget destruction) so the cursor and drag state are always restored even if the mouse leaves the canvas or the widget loses focus mid-drag. - The
SENSITIVITYconstant and the comment// Camera rotation sensitivity: 3 degree per pixelappear inconsistent with theFixedPoint<1>angle units and integer cast; it would help to clarify the intended units and ensure the numeric value matches the comment (or vice versa).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider handling `m_altDragState` cleanup in additional paths like `leaveEvent`/`focusOutEvent` (or widget destruction) so the cursor and drag state are always restored even if the mouse leaves the canvas or the widget loses focus mid-drag.
- The `SENSITIVITY` constant and the comment `// Camera rotation sensitivity: 3 degree per pixel` appear inconsistent with the `FixedPoint<1>` angle units and integer cast; it would help to clarify the intended units and ensure the numeric value matches the comment (or vice versa).
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas.cpp:613-614` </location>
<code_context>
+ const auto delta = pos - dragState.lastPos;
+ dragState.lastPos = pos;
+
+ // Camera rotation sensitivity: 3 degree per pixel
+ constexpr float SENSITIVITY = 0.3f;
+
+ // Horizontal movement adjusts yaw (horizontalAngle)
</code_context>
<issue_to_address>
**issue:** The sensitivity comment appears inconsistent with the numeric value and the FixedPoint<1> units.
The code comment (`3 degree per pixel`) doesn’t match the effective rotation: with `SENSITIVITY = 0.3f` and `FixedPoint<1>` angles (tenths of a degree), a `dx` of 10 yields a change of 3 units = 0.3°. Please either change the constant to achieve the intended degrees-per-pixel, or update the comment to reflect the actual rotation per pixel.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add Alt+Left-click drag interaction to adjust the 3D viewport orientation and extend horizontal camera rotation range.
New Features:
Enhancements: