refactor MapCanvas to QOpenGLWindow for better WebGL2 support#458
Merged
nschimme merged 1 commit intoMUME:masterfrom Feb 14, 2026
Merged
refactor MapCanvas to QOpenGLWindow for better WebGL2 support#458nschimme merged 1 commit intoMUME:masterfrom
nschimme merged 1 commit intoMUME:masterfrom
Conversation
Reviewer's GuideRefactors MapCanvas from a QOpenGLWidget to a QOpenGLWindow embedded via a window container, reworks zoom/drag interaction to operate in world coordinates with unified scale handling, and adds richer gesture, tooltip, and context-menu plumbing between MapCanvas, MapWindow, and MainWindow. Sequence diagram for zoom gestures and world-centered zoomingsequenceDiagram
actor User
participant TouchSystem
participant MapCanvas
participant MapCanvasViewport
participant MapWindow
participant HorzScrollBar
participant VertScrollBar
User->>TouchSystem: pinch gesture
TouchSystem->>MapCanvas: QTouchEvent (TouchBegin/Update)
MapCanvas->>MapCanvas: touchEvent(event)
MapCanvas->>MapCanvas: update m_initialPinchDistance, m_lastPinchFactor
MapCanvas->>MapCanvas: handleZoomAtEvent(event, deltaFactor)
MapCanvas->>MapCanvasViewport: getMouseCoords(event)
MapCanvasViewport-->>MapCanvas: optional glm::vec2 mousePos
MapCanvas->>MapCanvas: zoomAt(factor, mousePos)
MapCanvas->>MapCanvasViewport: unproject(mousePos)
MapCanvasViewport-->>MapCanvas: optional worldPos
MapCanvas->>MapCanvas: update m_scaleFactor *= factor
MapCanvas->>MapCanvas: zoomChanged()
MapCanvas->>MapCanvas: setViewportAndMvp(width, height)
MapCanvas->>MapCanvasViewport: unproject(mousePos) with new m_viewProj
MapCanvasViewport-->>MapCanvas: optional newWorldPos
MapCanvas->>MapCanvas: adjust m_scroll to keep worldPos under cursor
MapCanvas-->>MapWindow: sig_onCenter(newScroll)
MapWindow->>MapWindow: slot_centerOnWorldPos(worldPos)
MapWindow->>HorzScrollBar: setValue(scrollPos.x)
MapWindow->>VertScrollBar: setValue(scrollPos.y)
MapCanvas->>MapCanvas: update()
Sequence diagram for context menu and tooltip handlingsequenceDiagram
actor User
participant MapCanvas
participant MapWindow
participant MainWindow
participant QToolTip
User->>MapCanvas: Right-click mousePressEvent
MapCanvas->>MapCanvas: mousePressEvent(event)
MapCanvas-->>MainWindow: sig_customContextMenuRequested(pos)
MainWindow->>MainWindow: slot_showContextMenu(pos)
MainWindow->>MainWindow: slot_closeContextMenu()
MainWindow->>MainWindow: create QMenu and populate actions
MainWindow->>MapCanvas: getCanvas()
MainWindow->>MainWindow: contextMenu.popup(mapToGlobal(pos))
User->>MapCanvas: Left-click release on room
MapCanvas->>MapCanvas: mouseReleaseEvent(event)
MapCanvas->>MapCanvas: detect click without movement
MapCanvas-->>MapWindow: sig_showTooltip(text, pos)
MapWindow->>MapWindow: slot_showTooltip(text, pos)
MapWindow->>QToolTip: showText(canvasContainer.mapToGlobal(pos), text)
User->>MapCanvas: Any non-right-click press or touch begin
MapCanvas-->>MainWindow: sig_dismissContextMenu()
MainWindow->>MainWindow: slot_closeContextMenu()
MainWindow->>MainWindow: if m_contextMenu close()
Class diagram for refactored MapCanvas and MapWindow interactionclassDiagram
class MapCanvas {
+MapCanvas(MapData &mapData, PrespammedPath &prespammedPath, Mmapper2Group &groupManager, QWindow *parent)
+~MapCanvas()
+void initializeGL()
+void resizeGL(int width, int height)
+void paintGL()
+void cleanupOpenGL()
+void setZoom(float zoom)
+float getRawZoom() const
+int width() const
+int height() const
+QRect rect() const
+void slot_onForcedPositionChange()
+void slot_createRoom()
+void slot_setScroll(const glm::vec2 &worldPos)
+void slot_setHorizontalScroll(float worldX)
+void slot_setVerticalScroll(float worldY)
+void graphicsSettingsChanged()
+void userPressedEscape(bool pressed)
+signals void sig_onCenter(const glm::vec2 &worldPos)
+signals void sig_setScrollBars(const glm::vec2 &worldDims)
+signals void sig_continuousScroll(int hStep, int vStep)
+signals void sig_mapMove(int dx, int dy)
+signals void sig_zoomChanged(float zoom)
+signals void sig_showTooltip(const QString &text, const QPoint &pos)
+signals void sig_customContextMenuRequested(const QPoint &pos)
+signals void sig_dismissContextMenu()
-std::optional<AltDragState> m_altDragState
-std::optional<DragState> m_dragState
-float m_initialPinchDistance
-float m_lastPinchFactor
-float m_lastMagnification
-glm::vec2 m_scroll
-glm::mat4 m_viewProj
-ScaleFactor m_scaleFactor
-void mousePressEvent(QMouseEvent *event)
-void mouseReleaseEvent(QMouseEvent *event)
-void mouseMoveEvent(QMouseEvent *event)
-void wheelEvent(QWheelEvent *event)
-void touchEvent(QTouchEvent *event)
-bool event(QEvent *event)
-void startMoving(const MouseSel &startPos)
-void stopMoving()
-void zoomAt(float factor, const glm::vec2 &mousePos)
-void handleZoomAtEvent(const QInputEvent *event, float deltaFactor)
-void setViewportAndMvp(int width, int height)
-void onMovement()
}
class MapCanvasViewport {
+MapCanvasViewport(QWindow &window)
+int width() const
+int height() const
+Viewport getViewport() const
+float getTotalScaleFactor() const
+std::optional<glm::vec3> project(const glm::vec3 &v) const
+glm::vec3 unproject_raw(const glm::vec3 &mouseDepth) const
+glm::vec3 unproject_raw(const glm::vec3 &mouseDepth, const glm::mat4 &viewProj) const
+glm::vec3 unproject_clamped(const glm::vec2 &mouse) const
+glm::vec3 unproject_clamped(const glm::vec2 &mouse, const glm::mat4 &viewProj) const
+std::optional<glm::vec3> unproject(const QInputEvent *event) const
+std::optional<glm::vec3> unproject(const glm::vec2 &xy) const
+std::optional<MouseSel> getUnprojectedMouseSel(const QInputEvent *event) const
+std::optional<MouseSel> getUnprojectedMouseSel(const glm::vec2 &xy) const
+std::optional<glm::vec2> getMouseCoords(const QInputEvent *event) const
-QWindow &m_window
-glm::mat4 m_viewProj
-glm::vec2 m_scroll
-ScaleFactor m_scaleFactor
-int m_currentLayer
}
class MapCanvasInputState {
+MapCanvasInputState(PrespammedPath &prespammedPath)
+void startMoving(const MouseSel &startPos)
+void stopMoving()
+bool hasSel1() const
+bool hasSel2() const
+MouseSel getSel1() const
+MouseSel getSel2() const
+bool hasBackup() const
+MouseSel getBackup() const
+CanvasMouseModeEnum m_canvasMouseMode
-std::optional<MouseSel> m_sel1
-std::optional<MouseSel> m_sel2
-std::optional<MouseSel> m_backup
}
class ScaleFactor {
+float getRaw() const
+float getTotal() const
+void set(float scale)
+void reset()
+void logStep(int n)
+ScaleFactor operator*(float factor) const
+ScaleFactor &operator*=(float factor)
-float m_scaleFactor
-static float clamp(float x)
+static const float ZOOM_STEP
}
class MapWindow {
+MapWindow(MapData &mapData, PrespammedPath &pp, Mmapper2Group &gm, QWidget *parent)
+~MapWindow()
+void hideSplashImage()
+void keyPressEvent(QKeyEvent *event)
+void keyReleaseEvent(QKeyEvent *event)
+MapCanvas *getCanvas() const
+void setZoom(float zoom)
+float getZoom() const
+void setCanvasEnabled(bool enabled)
+signals void sig_setScroll(const glm::vec2 &worldPos)
+signals void sig_zoomChanged(float zoom)
+slots void slot_mapMove(int dx, int dy)
+slots void slot_continuousScroll(int hStep, int vStep)
+slots void slot_scrollTimerTimeout()
+slots void slot_graphicsSettingsChanged()
+slots void slot_centerOnWorldPos(const glm::vec2 &worldPos)
+slots void slot_setScrollBars(const glm::vec2 &worldDims)
+slots void slot_zoomChanged(const float zoom)
+slots void slot_showTooltip(const QString &text, const QPoint &pos)
+void updateScrollBars()
-QPointer<QGridLayout> m_gridLayout
-QPointer<QScrollBar> m_horizontalScrollBar
-QPointer<QScrollBar> m_verticalScrollBar
-QPointer<MapCanvas> m_canvas
-QPointer<QWidget> m_canvasContainer
-QPointer<QLabel> m_splashLabel
-QPointer<QTimer> m_scrollTimer
-KnownMapSize m_knownMapSize
-int m_verticalScrollStep
-int m_horizontalScrollStep
-void centerOnScrollPos(const glm::ivec2 &scrollPos)
}
class MainWindow {
+void wireConnections()
+void setupMenuBar()
+slots void slot_showContextMenu(const QPoint &pos)
+slots void slot_closeContextMenu()
-QPointer<QMenu> m_contextMenu
-MapWindow *m_mapWindow
-void slot_setShowMenuBar()
}
class CanvasDisabler {
+CanvasDisabler(MapWindow &in_window)
+~CanvasDisabler()
-MapWindow &window
}
MapCanvas --|> QOpenGLWindow
MapCanvas ..|> MapCanvasViewport
MapCanvas ..|> MapCanvasInputState
MapCanvasViewport --> ScaleFactor
MapCanvas --> ScaleFactor
MapWindow --> MapCanvas
MapWindow --> QScrollBar
MapWindow --> QTimer
MainWindow --> MapWindow
MainWindow --> MapCanvas
CanvasDisabler --> MapWindow
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 1 issue, and left some high level feedback:
- The new getMouseCoords/unproject path relies on throwing/catching std::invalid_argument for non-mouse events in hot input paths; consider refactoring to a non-exceptional flow (e.g., return std::optionalglm::vec2 or early-type checks) to avoid overhead and make control flow clearer.
- Several QPointer members (e.g., scrollTimer, m_splashLabel, m_canvasContainer) are dereferenced without null checks after deleteLater or conditional creation; adding defensive null checks at use sites would make the code more robust against lifetime/ownership changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new getMouseCoords/unproject path relies on throwing/catching std::invalid_argument for non-mouse events in hot input paths; consider refactoring to a non-exceptional flow (e.g., return std::optional<glm::vec2> or early-type checks) to avoid overhead and make control flow clearer.
- Several QPointer members (e.g., scrollTimer, m_splashLabel, m_canvasContainer) are dereferenced without null checks after deleteLater or conditional creation; adding defensive null checks at use sites would make the code more robust against lifetime/ownership changes.
## Individual Comments
### Comment 1
<location> `src/display/mapwindow.cpp:297-300` </location>
<code_context>
return m_canvas->getRawZoom();
}
+void MapWindow::slot_showTooltip(const QString &text, const QPoint &pos)
+{
+ QToolTip::showText(m_canvasContainer->mapToGlobal(pos), text, m_canvasContainer);
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tooltip lifetime behavior changed by dropping the timeout parameter.
Previously we called `QToolTip::showText(..., rect(), 5000)`, so the tooltip auto‑hid after 5 seconds. This overload has no timeout, so tooltips now remain until another tooltip appears or focus changes. If that behavior isn’t desired, use the timeout overload here to match the prior UX.
```suggestion
void MapWindow::slot_showTooltip(const QString &text, const QPoint &pos)
{
QToolTip::showText(
m_canvasContainer->mapToGlobal(pos),
text,
m_canvasContainer,
m_canvasContainer->rect(),
5000);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #458 +/- ##
===========================================
- Coverage 66.48% 25.77% -40.71%
===========================================
Files 85 492 +407
Lines 4186 40839 +36653
Branches 255 4436 +4181
===========================================
+ Hits 2783 10528 +7745
- Misses 1403 30311 +28908 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nschimme
force-pushed
the
migrate-qopenglwindow
branch
from
f6a1c55 to
1ab2845
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
sig_customContextMenuRequestedsignal onMapCanvasis never emitted andsetContextMenuPolicy(Qt::CustomContextMenu)was removed, so the context menu wiring fromMainWindow::wireConnections()will no longer fire; consider explicitly handling the context-menu event inMapCanvas::event/mouse handlers and emitting this signal. - In
MapCanvas::touchEvent, touch handling is only active for exactly two points and otherwise falls back toQOpenGLWindow::touchEvent; if you intend to support single-finger drags or more general touch interactions, it may be worth explicitly handling those cases or documenting that only two-finger pinch is honored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `sig_customContextMenuRequested` signal on `MapCanvas` is never emitted and `setContextMenuPolicy(Qt::CustomContextMenu)` was removed, so the context menu wiring from `MainWindow::wireConnections()` will no longer fire; consider explicitly handling the context-menu event in `MapCanvas::event`/mouse handlers and emitting this signal.
- In `MapCanvas::touchEvent`, touch handling is only active for exactly two points and otherwise falls back to `QOpenGLWindow::touchEvent`; if you intend to support single-finger drags or more general touch interactions, it may be worth explicitly handling those cases or documenting that only two-finger pinch is honored.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nschimme
force-pushed
the
migrate-qopenglwindow
branch
from
1ab2845 to
8489295
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Now that
MapCanvasViewport::getMouseCoordsreturnsstd::optional, some paths (e.g. theCanvasMouseModeEnum::RAYPICK_ROOMSbranch inMapCanvas::mousePressEvent) still appear to usexywithout checking fornullopt; consider auditing allgetMouseCoords/unprojectcall sites to guard against empty results before using the coordinates. - The new touch and native zoom gesture handling assumes exactly two touch points in
touchEventand specific platform semantics forQNativeGestureEvent::value(); it would be helpful to centralize these assumptions (e.g. named constants or helper functions) and handle or explicitly ignore edge cases like >2 touch points to make the behavior and limitations clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `MapCanvasViewport::getMouseCoords` returns `std::optional`, some paths (e.g. the `CanvasMouseModeEnum::RAYPICK_ROOMS` branch in `MapCanvas::mousePressEvent`) still appear to use `xy` without checking for `nullopt`; consider auditing all `getMouseCoords`/`unproject` call sites to guard against empty results before using the coordinates.
- The new touch and native zoom gesture handling assumes exactly two touch points in `touchEvent` and specific platform semantics for `QNativeGestureEvent::value()`; it would be helpful to centralize these assumptions (e.g. named constants or helper functions) and handle or explicitly ignore edge cases like >2 touch points to make the behavior and limitations clearer.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas_gl.cpp:218` </location>
<code_context>
} catch (const std::exception &) {
hide();
doneCurrent();
- QMessageBox::critical(this,
+ QMessageBox::critical(nullptr,
"Unable to initialize OpenGL",
"Upgrade your video card drivers");
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a null parent for the OpenGL init error dialog may degrade UX (wrong window stacking / screen / modality).
Previously this dialog was correctly parented to the canvas widget, which kept stacking, modality, and screen placement consistent. With a nullptr parent it may show behind the main window, on another screen, or look disconnected from the app on some platforms. If the issue is that MapCanvas is now a QWindow, consider parenting the message box to the appropriate top-level QWidget (e.g., MapWindow/MainWindow) instead of nullptr to preserve this behavior.
</issue_to_address>
### Comment 2
<location> `src/display/MapCanvasData.cpp:107-116` </location>
<code_context>
+std::optional<glm::vec2> MapCanvasViewport::getMouseCoords(const QInputEvent *const event) const
</code_context>
<issue_to_address>
**suggestion:** Returning std::nullopt for unhandled event types can silently drop input; consider making failure more explicit or exhaustive.
Previously this would throw on unsupported event types, making incorrect usage obvious. With std::nullopt, unexpected QInputEvent subtypes now fail silently (e.g., zoom/selection just don’t happen). Consider asserting or logging in the final else branch (at least in debug builds) while still returning std::nullopt in release to catch misuse early without changing the public API.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nschimme
force-pushed
the
migrate-qopenglwindow
branch
from
8489295 to
7b6ca51
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:
- The declaration of
calculateNativeZoomDelta(const QNativeGestureEvent *event)inmapcanvas.his never defined or used; either remove it or hook it up fromMapCanvas::event()to keep the interface consistent and avoid dead declarations. - In
MapCanvasViewport::getMouseCoordstheassert(false)on unhandled event types can cause unexpected crashes; consider returningstd::nulloptwithout asserting so future/new input event types fail gracefully rather than aborting the application.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The declaration of `calculateNativeZoomDelta(const QNativeGestureEvent *event)` in `mapcanvas.h` is never defined or used; either remove it or hook it up from `MapCanvas::event()` to keep the interface consistent and avoid dead declarations.
- In `MapCanvasViewport::getMouseCoords` the `assert(false)` on unhandled event types can cause unexpected crashes; consider returning `std::nullopt` without asserting so future/new input event types fail gracefully rather than aborting the application.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas.h:243-245` </location>
<code_context>
void setMvp(const glm::mat4 &viewProj);
void setViewportAndMvp(int width, int height);
+ void zoomAt(float factor, const glm::vec2 &mousePos);
+ void handleZoomAtEvent(const QInputEvent *event, float deltaFactor);
+ NODISCARD float calculateNativeZoomDelta(const QNativeGestureEvent *event);
+
NODISCARD BatchedInfomarksMeshes getInfomarksMeshes();
</code_context>
<issue_to_address>
**nitpick:** Remove or implement the unused calculateNativeZoomDelta() declaration.
`calculateNativeZoomDelta(const QNativeGestureEvent *event)` is declared here but never defined or used—`deltaFactor` is computed inline in `MapCanvas::event()`. To avoid misleading API surface and potential compiler warnings about an unused/undefined function, either implement and call this helper from `event()`, or remove the declaration entirely.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nschimme
force-pushed
the
migrate-qopenglwindow
branch
from
7b6ca51 to
626b26f
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- MapCanvas::calculateNativeZoomDelta is declared in the header but never defined or used; either remove the declaration or provide an implementation to avoid dead API and potential ODR/link issues.
- MapCanvasViewport::getMouseCoords now asserts(false) for unhandled event types; consider replacing this with a non-fatal path (e.g., qWarning and returning std::nullopt) so unexpected QInputEvent subtypes in production builds don't trigger assertions.
- MapCanvas::touchEvent handles only the two-finger pinch case and otherwise defers to the base implementation; if single-finger or multi-finger panning/dragging is expected on touch devices, you may want to extend this to support consistent MOVE behavior for touch input similar to the mouse path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- MapCanvas::calculateNativeZoomDelta is declared in the header but never defined or used; either remove the declaration or provide an implementation to avoid dead API and potential ODR/link issues.
- MapCanvasViewport::getMouseCoords now asserts(false) for unhandled event types; consider replacing this with a non-fatal path (e.g., qWarning and returning std::nullopt) so unexpected QInputEvent subtypes in production builds don't trigger assertions.
- MapCanvas::touchEvent handles only the two-finger pinch case and otherwise defers to the base implementation; if single-finger or multi-finger panning/dragging is expected on touch devices, you may want to extend this to support consistent MOVE behavior for touch input similar to the mouse path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Added platform-specific gesture support for macOS and touch devices - Implemented smooth world-coordinate dragging and zoom Co-authored-by: KasparMetsa <kasparmetsa@gmail.com>
nschimme
force-pushed
the
migrate-qopenglwindow
branch
from
626b26f to
db9467a
Compare
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
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.
based upon #455
Summary by Sourcery
Refactor the map rendering canvas to use QOpenGLWindow and improve input handling for smoother, world-coordinate-aware navigation and better cross‑platform gesture support.
New Features:
Enhancements: