Add Alt+Middle Mouse camera rotation #426
Closed
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
Camera Control Enhancement: - Added camera rotation using Alt + Middle Mouse Button - Works only when auto-tilt is disabled (user manual camera mode) - Smooth rotation with mouse drag sensitivity - Extended horizontal angle range to full -180..180 degrees (was -45..45) Implementation: - New CameraRotationState struct to track rotation session - Stores initial mouse position and camera angles - Calculates delta movement for smooth camera updates - SizeAll cursor feedback during rotation - Configuration update for extended angle range User Experience: - Intuitive drag-to-rotate camera control - Complements existing camera navigation tools - Enables full 360-degree horizontal rotation - Vertical angle remains clamped to safe range (0-90 degrees) This gives users more precise control over the 3D camera view.
Reviewer's GuideImplements Alt+Middle Mouse camera rotation in manual camera mode, refactors cursor handling for canvas mouse modes, and adds texture/season handling and batch reset hooks to MapCanvas. Sequence diagram for Alt+Middle Mouse camera rotation interactionsequenceDiagram
actor User
participant QtEventLoop
participant MapCanvas
participant MapCanvasConfig
participant Configuration as CanvasConfiguration
User->>QtEventLoop: Alt + Middle mouse press
QtEventLoop->>MapCanvas: mousePressEvent(event)
MapCanvas->>MapCanvas: read buttons/modifiers
MapCanvas->>MapCanvasConfig: isAutoTilt()
MapCanvasConfig-->>MapCanvas: autoTiltDisabled
alt Alt+Middle and auto tilt disabled
MapCanvas->>CanvasConfiguration: setConfig().canvas.advanced
MapCanvas->>MapCanvas: create CameraRotationState
MapCanvas->>MapCanvas: store initialMousePos and camera angles
MapCanvas->>MapCanvas: setCursor(SizeAllCursor)
MapCanvas-->>QtEventLoop: event->accept()
else other mouse handling
MapCanvas->>MapCanvas: existing press handling
end
loop while middle button held and mouse moves
User->>QtEventLoop: mouse drag
QtEventLoop->>MapCanvas: mouseMoveEvent(event)
MapCanvas->>MapCanvas: isCameraRotating() and m_mouseMiddlePressed
alt camera rotating
MapCanvas->>MapCanvas: compute delta from initialMousePos
MapCanvas->>CanvasConfiguration: update horizontalAngle
MapCanvas->>CanvasConfiguration: update verticalAngle
MapCanvas->>MapCanvas: slot_requestUpdate()
MapCanvas-->>QtEventLoop: event->accept()
else normal move handling
MapCanvas->>MapCanvas: existing move handling
end
end
User->>QtEventLoop: middle button release
QtEventLoop->>MapCanvas: mouseReleaseEvent(event)
MapCanvas->>MapCanvas: isCameraRotating()
alt releasing camera rotation
MapCanvas->>MapCanvas: m_cameraRotation.reset()
MapCanvas->>MapCanvas: m_mouseMiddlePressed = false
MapCanvas->>MapCanvas: restoreCursorForMouseMode()
MapCanvas-->>QtEventLoop: event->accept()
else other release handling
MapCanvas->>MapCanvas: existing release handling
end
Class diagram for updated MapCanvas camera and texture featuresclassDiagram
class MapCanvas {
// Existing responsibilities omitted
+slot_setCanvasMouseMode(mode: CanvasMouseModeEnum) void
+slot_onSeasonChanged(newSeason: MumeSeasonEnum) void
+slot_reloadTextures() void
+slot_rebuildMeshes() void
+infomarksChanged() void
+mapBatchesChanged() void
+layerChanged() void
+slot_mapChanged() void
+slot_requestUpdate() void
// Mouse handling
-mousePressEvent(event: QMouseEvent*) void
-mouseMoveEvent(event: QMouseEvent*) void
-mouseReleaseEvent(event: QMouseEvent*) void
-restoreCursorForMouseMode() void
// Rendering and textures
-actuallyPaintGL() void
-renderBackgroundImage() void
-loadBackgroundImageIfNeeded() void
-paintMap() void
-renderMapBatches() void
-paintBatchedInfomarks() void
// Data members
-Batches m_batches
-MapCanvasTextures m_textures
-SharedMMTexture m_backgroundTexture
-QString m_backgroundImagePath
-CanvasMouseModeEnum m_canvasMouseMode
-bool m_mouseLeftPressed
-bool m_mouseRightPressed
-bool m_mouseMiddlePressed
-optional~CameraRotationState~ m_cameraRotation
}
class CameraRotationState {
+CameraRotationState(initialMousePos: glm_vec2, initialVerticalAngle: float, initialHorizontalAngle: float)
+glm_vec2 initialMousePos
+float initialVerticalAngle
+float initialHorizontalAngle
}
class MapCanvasConfig {
+isAutoTilt() bool
}
class CanvasConfigurationAdvanced {
+FloatOption verticalAngle
+FloatOption horizontalAngle
+setFloat(value: float) void
+getFloat() float
}
class Batches {
+resetExistingMeshesButKeepPendingRemesh() void
+mapBatches: MapBatchContainer
}
class MapDiff {
+resetExistingMeshesAndIgnorePendingRemesh() void
}
class MapCanvasTextures {
+destroyAll() void
}
MapCanvas o-- CameraRotationState : uses
MapCanvas --> MapCanvasConfig : queries_auto_tilt
MapCanvas --> CanvasConfigurationAdvanced : updates_camera_angles
MapCanvas --> Batches : manages_meshes
MapCanvas --> MapDiff : manages_mesh_diff
MapCanvas --> MapCanvasTextures : manages_textures
Flow diagram for seasonal texture reload and batch resettingflowchart TD
A[Texture or season change detected] --> B{Change source}
B -->|Season change event| C["slot_onSeasonChanged(newSeason)"]
B -->|Texture settings changed| D["slot_reloadTextures()"]
C --> E[Check config.canvas.enableSeasonalTextures]
E -->|disabled| F[Return without reload]
E -->|enabled| G["setCurrentSeason(newSeason)"]
G --> H["makeCurrent()"] --> I["m_textures.destroyAll()"] --> J["initTextures()"] --> K["doneCurrent()"]
K --> L["m_diff.resetExistingMeshesAndIgnorePendingRemesh()"]
L --> M["slot_requestUpdate()"]
D --> N["makeCurrent()"] --> O["m_textures.destroyAll()"] --> P["initTextures()"] --> Q["doneCurrent()"]
Q --> R["m_batches.resetExistingMeshesButKeepPendingRemesh()"]
R --> S["m_diff.resetExistingMeshesAndIgnorePendingRemesh()"]
S --> T["graphicsSettingsChanged()"]
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 there - I've reviewed your changes - here's some feedback:
- In
mouseMoveEvent, the vertical rotation comment says the motion is inverted (moving the mouse down should decrease the pitch), but the implementation addsdelta.ytoinitialVerticalAngle; either invert the sign or adjust the comment so behavior and documentation match. - The camera rotation path only checks
MapCanvasConfig::isAutoTilt()and the Alt modifier on mouse press; consider re-validating these conditions inmouseMoveEvent(and possibly cancelling rotation if they change mid-drag) to avoid leaving the user in a rotation state that no longer matches the current mode/modifiers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `mouseMoveEvent`, the vertical rotation comment says the motion is inverted (moving the mouse down should decrease the pitch), but the implementation adds `delta.y` to `initialVerticalAngle`; either invert the sign or adjust the comment so behavior and documentation match.
- The camera rotation path only checks `MapCanvasConfig::isAutoTilt()` and the Alt modifier on mouse press; consider re-validating these conditions in `mouseMoveEvent` (and possibly cancelling rotation if they change mid-drag) to avoid leaving the user in a rotation state that no longer matches the current mode/modifiers.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas.cpp:479-478` </location>
<code_context>
} else if (event->button() == Qt::BackButton) {
slot_layerDown();
return event->accept();
+ } else if (hasMiddleButton && hasAlt && !MapCanvasConfig::isAutoTilt()) {
+ // Alt + Middle Mouse: Camera rotation (only when auto-tilt is disabled)
+ auto &advanced = setConfig().canvas.advanced;
+ m_cameraRotation.emplace(CameraRotationState{getMouseCoords(event),
+ advanced.verticalAngle.getFloat(),
+ advanced.horizontalAngle.getFloat()});
+ setCursor(Qt::SizeAllCursor);
+ return event->accept();
} else if (!m_mouseLeftPressed && m_mouseRightPressed) {
if (m_canvasMouseMode == CanvasMouseModeEnum::MOVE && hasSel1()) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard camera-rotation start on the actual middle-button press rather than any event while middle is held.
This branch should check `event->button() == Qt::MiddleButton` instead of `hasMiddleButton` (`event->buttons()`). As written, if the middle button is already held and the user presses another button with Alt down, we can incorrectly re-enter the camera-rotation path on a non-middle-button event and reinitialize `m_cameraRotation`. Using the specific button check avoids this unintended re-entrancy.
</issue_to_address>
### Comment 2
<location> `src/display/mapcanvas.cpp:653-654` </location>
<code_context>
+ const glm::vec2 currentMousePos = getMouseCoords(event);
+ const glm::vec2 delta = currentMousePos - m_cameraRotation->initialMousePos;
+
+ // Camera rotation sensitivity: 1.0 degree per pixel
+ constexpr float ROTATION_SENSITIVITY = 0.3f;
+
+ auto &advanced = setConfig().canvas.advanced;
</code_context>
<issue_to_address>
**nitpick:** Fix the discrepancy between the sensitivity comment and the actual constant value.
The comment states `1.0 degree per pixel`, but the constant is `0.3f`. Please either update the comment to match the current value or change the constant to `1.0f` so they stay consistent for future tuning.
</issue_to_address>
### Comment 3
<location> `src/display/mapcanvas.cpp:663-667` </location>
<code_context>
+ + (delta.x * ROTATION_SENSITIVITY);
+ advanced.horizontalAngle.setFloat(newHorizontalAngle);
+
+ // Vertical mouse movement controls vertical angle (pitch)
+ // Inverted: moving mouse down should decrease pitch angle
+ const float newVerticalAngle = m_cameraRotation->initialVerticalAngle
+ + (delta.y * ROTATION_SENSITIVITY);
+ advanced.verticalAngle.setFloat(newVerticalAngle);
+
+ slot_requestUpdate();
</code_context>
<issue_to_address>
**issue (bug_risk):** Invert the vertical delta or adjust the comment to match the actual pitch behavior.
Given Qt’s coordinate system (Y increases downward), moving the mouse down makes `delta.y` positive, so `initialVerticalAngle + (delta.y * ROTATION_SENSITIVITY)` actually *increases* the pitch angle. If the control is meant to be inverted as the comment states, this should be `initialVerticalAngle - (delta.y * ROTATION_SENSITIVITY)`. Otherwise, update the comment to describe the current non-inverted behavior accurately.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return event->accept(); | ||
| } else if (event->button() == Qt::BackButton) { | ||
| slot_layerDown(); | ||
| return event->accept(); |
There was a problem hiding this comment.
issue (bug_risk): Guard camera-rotation start on the actual middle-button press rather than any event while middle is held.
This branch should check event->button() == Qt::MiddleButton instead of hasMiddleButton (event->buttons()). As written, if the middle button is already held and the user presses another button with Alt down, we can incorrectly re-enter the camera-rotation path on a non-middle-button event and reinitialize m_cameraRotation. Using the specific button check avoids this unintended re-entrancy.
Comment on lines
+653
to
+654
| // Camera rotation sensitivity: 1.0 degree per pixel | ||
| constexpr float ROTATION_SENSITIVITY = 0.3f; |
There was a problem hiding this comment.
nitpick: Fix the discrepancy between the sensitivity comment and the actual constant value.
The comment states 1.0 degree per pixel, but the constant is 0.3f. Please either update the comment to match the current value or change the constant to 1.0f so they stay consistent for future tuning.
Comment on lines
+663
to
+667
| // Vertical mouse movement controls vertical angle (pitch) | ||
| // Inverted: moving mouse down should decrease pitch angle | ||
| const float newVerticalAngle = m_cameraRotation->initialVerticalAngle | ||
| + (delta.y * ROTATION_SENSITIVITY); | ||
| advanced.verticalAngle.setFloat(newVerticalAngle); |
There was a problem hiding this comment.
issue (bug_risk): Invert the vertical delta or adjust the comment to match the actual pitch behavior.
Given Qt’s coordinate system (Y increases downward), moving the mouse down makes delta.y positive, so initialVerticalAngle + (delta.y * ROTATION_SENSITIVITY) actually increases the pitch angle. If the control is meant to be inverted as the comment states, this should be initialVerticalAngle - (delta.y * ROTATION_SENSITIVITY). Otherwise, update the comment to describe the current non-inverted behavior accurately.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Camera Control Enhancement:
Implementation:
User Experience:
This gives users more precise control over the 3D camera view.
Summary by Sourcery
Add manual camera rotation controls and improve map canvas texture and batch handling.
New Features:
Enhancements: