Open
Conversation
2. no async error
Reviewer's GuideAdds Qt 6.10.2-compatible WebAssembly support for the map canvas by introducing a QOpenGLWindow-based WASM backend, tracking WebGL context loss, adapting async mesh generation and gestures, and updating CMake/build/deployment and OpenGL error handling for Qt and platform quirks. Sequence diagram for pinch-to-zoom handling via MapWindow and MapCanvassequenceDiagram
actor User
participant InputDevice
participant MapWindow
participant QGestureEvent
participant QPinchGesture
participant MapCanvas
User->>InputDevice: Perform pinch gesture
InputDevice->>MapWindow: QEvent_Gesture
MapWindow->>MapWindow: event(event)
MapWindow->>QGestureEvent: dynamic_cast
QGestureEvent-->>MapWindow: gesture(Qt_PinchGesture)
MapWindow->>QPinchGesture: dynamic_cast
QPinchGesture-->>MapWindow: pinch
alt ScaleFactorChanged
MapWindow->>MapCanvas: setPinchZoom(pinchFactor)
MapCanvas->>MapCanvas: update()
end
alt GestureFinished
MapWindow->>MapCanvas: endPinchZoom()
MapCanvas->>MapCanvas: zoomChanged()
MapCanvas->>MapCanvas: update()
end
MapWindow-->>InputDevice: event_handled
Class diagram for updated MapCanvas, viewport abstraction, and MapWindowclassDiagram
class MapCanvas {
+static bool isWasmContextLost()
+static void resetWasmContextState()
+explicit MapCanvas(MapData &mapData, PrespammedPath &prespammedPath, Mmapper2Group &groupManager)
+explicit MapCanvas(MapData &mapData, PrespammedPath &prespammedPath, Mmapper2Group &groupManager, QWidget *parent)
+void initializeGL()
+void paintGL()
+void resizeGL(int width, int height)
+void setPinchZoom(float pinchFactor)
+void endPinchZoom()
+QWidget* getContainerWidget()
+void setContainerWidget(QWidget *widget)
-static bool s_wasmInitialized
-static atomic_bool s_wasmContextLost
-QWidget* m_containerWidget
}
class MapCanvasViewport {
-SizeSource m_sizeSource
glm~mat4~ m_viewProj
glm~vec2~ m_worldMouse
int m_currentLayer
+MapCanvasViewport(QWidget &sizeSource)
+MapCanvasViewport(QWindow &sizeSource)
+int width() const
+int height() const
+Viewport getViewport() const
+float getTotalScaleFactor() const
}
class SizeSource {
-QWidget* m_widget
-QWindow* m_window
+SizeSource(QWidget &widget)
+SizeSource(QWindow &window)
+int width() const
+int height() const
+QRect rect() const
}
class MapCanvasInputState {
+ScaleFactor m_scaleFactor
+void setZoom(float zoom)
+float getRawZoom() const
}
class MapWindow {
-std~unique_ptr~<MapCanvas> m_canvas
+MapWindow(MapData &mapData, PrespammedPath &pp, Mmapper2Group &gm, QWidget *parent)
+MapCanvas* getCanvas() const
+void resizeEvent(QResizeEvent *event)
+bool event(QEvent *event)
+void hideSplashImage()
}
class QWidget
class QWindow
class QOpenGLWidget
class QOpenGLWindow
MapCanvas ..|> MapCanvasViewport
MapCanvas ..|> MapCanvasInputState
MapCanvas --|> QOpenGLWidget
MapCanvas --|> QOpenGLWindow
MapCanvasViewport *-- SizeSource
MapWindow o--> MapCanvas
MapWindow --|> QWidget
SizeSource --> QWidget
SizeSource --> QWindow
MapCanvas --> QWidget : uses_as_container
MapWindow --> QOpenGLWindow : embeds_via_createWindowContainer
Class diagram for updated OpenGL error handling and context trackingclassDiagram
class MapCanvas {
+static bool isWasmContextLost()
+static void resetWasmContextState()
-static bool s_wasmInitialized
-static atomic_bool s_wasmContextLost
+void initializeGL()
+void resizeGL(int width, int height)
+void paintGL()
+void updateMapBatches()
}
class LegacyFunctions {
+void checkError()
+int clearErrors()
+void configureFbo(int samples)
+void glGenVertexArrays(GLsizei n, GLuint *arrays)
}
class VAO {
-GLuint m_vao
+void emplace(SharedFunctions &sharedFunctions)
}
class OpenGL {
-bool m_rendererInitialized
+void initializeRenderer(float devicePixelRatio)
+LegacyFunctions& getFunctions()
+LegacyFunctions& getSharedFunctions(Badge_MapCanvas)
}
class MapCanvasRoomDrawer {
+FutureSharedMapBatchFinisher generateMapDataFinisher(mctp_MapCanvasTextures textures, GLFont &font, Map &map)
}
class FutureSharedMapBatchFinisher
class SharedMapBatchFinisher
class std_abort
MapCanvas --> OpenGL : uses
OpenGL --> LegacyFunctions : owns
VAO --> LegacyFunctions : uses_for_VAO_creation
MapCanvasRoomDrawer --> MapCanvas : calls_isWasmContextLost
MapCanvasRoomDrawer --> FutureSharedMapBatchFinisher
FutureSharedMapBatchFinisher --> SharedMapBatchFinisher
LegacyFunctions ..> std_abort : used_on_desktop_errors
LegacyFunctions ..> MapCanvas : clearErrors_used_from_initializeGL
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 EMSCRIPTEN-specific global state for WebGL context tracking in mapcanvas_gl.cpp (g_wasmInitialized, g_wasmContextLost, g_wasmInitAttempts) would be easier to reason about and test if it lived in a dedicated struct/namespace or as MapCanvas static members instead of file-level globals.
- There is a lot of duplicated logic between the QOpenGLWidget and QOpenGLWindow code paths (e.g. MakeCurrentRaii, width/height/rect wrappers, event forwarding); consider factoring out common behavior behind small helpers or templates to reduce the number of #ifdef EMSCRIPTEN branches.
- The new scripts/build-wasm.sh hardcodes local paths for emsdk and Qt; making these configurable via environment variables or script arguments would avoid breakage for other environments and CI.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The EMSCRIPTEN-specific global state for WebGL context tracking in mapcanvas_gl.cpp (g_wasmInitialized, g_wasmContextLost, g_wasmInitAttempts) would be easier to reason about and test if it lived in a dedicated struct/namespace or as MapCanvas static members instead of file-level globals.
- There is a lot of duplicated logic between the QOpenGLWidget and QOpenGLWindow code paths (e.g. MakeCurrentRaii, width/height/rect wrappers, event forwarding); consider factoring out common behavior behind small helpers or templates to reduce the number of #ifdef __EMSCRIPTEN__ branches.
- The new scripts/build-wasm.sh hardcodes local paths for emsdk and Qt; making these configurable via environment variables or script arguments would avoid breakage for other environments and CI.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas_gl.cpp:237-239` </location>
<code_context>
+// WASM: Track initialization and context state globally
+#ifdef __EMSCRIPTEN__
+static bool g_wasmInitialized = false;
+static std::atomic<bool> g_wasmContextLost{false};
+static std::atomic<int> g_wasmInitAttempts{0};
+#endif
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Global WASM context flags are shared across all MapCanvas instances and never reset.
These globals are process-wide. If more than one MapCanvas is created over the app’s lifetime, later instances will see `g_wasmInitialized == true` and skip `initializeGL()`, remaining uninitialized. Likewise, a single context loss sets `g_wasmContextLost` for all instances with no way to recover. Please scope these flags to MapCanvas (as members) or add a lifecycle-tied reset mechanism.
</issue_to_address>
### Comment 2
<location> `scripts/build-wasm.sh:4-11` </location>
<code_context>
+
+# Source Emscripten environment
+# IMPORTANT: Change this path to match your emsdk installation location
+source "$HOME/dev/emsdk/emsdk_env.sh"
+
+# Paths - automatically detect project root (parent of scripts directory)
+MMAPPER_SRC="$(cd "$(dirname "$0")/.." && pwd)"
+QT_WASM="$HOME/Qt/6.10.2/wasm_multithread"
+QT_HOST="$HOME/Qt/6.10.2/macos"
+
+# Configure with qt-cmake
</code_context>
<issue_to_address>
**suggestion:** Hardcoded local paths in build-wasm.sh reduce portability and maintainability.
The script hardcodes `$HOME/dev/emsdk/emsdk_env.sh` and specific Qt versions/paths, which hurts reuse across machines and CI. Consider making these paths configurable via environment variables or script arguments (with reasonable defaults) so users don’t need to edit the script locally.
```suggestion
# Source Emscripten environment
# You can override EMSDK_ENV to point to your emsdk_env.sh, otherwise a default under $HOME is used.
: "${EMSDK_ENV:="$HOME/dev/emsdk/emsdk_env.sh"}"
if [ ! -f "$EMSDK_ENV" ]; then
echo "ERROR: Emscripten environment script not found at '$EMSDK_ENV'." >&2
echo "Set EMSDK_ENV to your emsdk_env.sh path before running this script." >&2
exit 1
fi
# shellcheck source=/dev/null
source "$EMSDK_ENV"
# Paths - automatically detect project root (parent of scripts directory)
MMAPPER_SRC="$(cd "$(dirname "$0")/.." && pwd)"
# You can override QT_WASM and QT_HOST to match your Qt installation.
: "${QT_WASM:="$HOME/Qt/6.10.2/wasm_multithread"}"
: "${QT_HOST:="$HOME/Qt/6.10.2/macos"}"
```
</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 #455 +/- ##
===========================================
- Coverage 66.48% 25.85% -40.63%
===========================================
Files 85 492 +407
Lines 4186 40725 +36539
Branches 255 4415 +4160
===========================================
+ Hits 2783 10531 +7748
- Misses 1403 30194 +28791 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
Could we move the gesture handling to the MapWindow as it continues to be a widget? |
nschimme
reviewed
Feb 12, 2026
nschimme
reviewed
Feb 12, 2026
nschimme
reviewed
Feb 12, 2026
Contributor
Author
Moved pinch-to-zoom gesture handling from MapCanvas to MapWindow. |
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
MapWindow::hideCanvasunder__EMSCRIPTEN__thehideargument is ignored, so callers lose the ability to hide/show the canvas; consider forwarding this to the container widget (e.g.,getCanvas()->getContainerWidget()->setVisible(!hide)) or documenting this limitation explicitly. - The new
SizeSourcecan silently return zero sizes or an emptyQRectwhen neither aQWidgetnorQWindowis set; adding an assertion or explicit guard when both pointers are null would make misuse easier to detect. - For consistency with the rest of the codebase, the new
qDebug/qWarninglogging in WASM-specific branches (e.g.,VAO::emplace,OpenGL::initializeRenderer,generateMapDataFinisher) might be better routed through the existing logging utilities so logging behavior stays uniform across platforms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MapWindow::hideCanvas` under `__EMSCRIPTEN__` the `hide` argument is ignored, so callers lose the ability to hide/show the canvas; consider forwarding this to the container widget (e.g., `getCanvas()->getContainerWidget()->setVisible(!hide)`) or documenting this limitation explicitly.
- The new `SizeSource` can silently return zero sizes or an empty `QRect` when neither a `QWidget` nor `QWindow` is set; adding an assertion or explicit guard when both pointers are null would make misuse easier to detect.
- For consistency with the rest of the codebase, the new `qDebug`/`qWarning` logging in WASM-specific branches (e.g., `VAO::emplace`, `OpenGL::initializeRenderer`, `generateMapDataFinisher`) might be better routed through the existing logging utilities so logging behavior stays uniform across platforms.
## Individual Comments
### Comment 1
<location> `src/display/mapcanvas_gl.cpp:286-287` </location>
<code_context>
+#endif
doneCurrent();
- QMessageBox::critical(this,
+ QMessageBox::critical(nullptr,
"Unable to initialize OpenGL",
"Upgrade your video card drivers");
if constexpr (CURRENT_PLATFORM == PlatformEnum::Windows) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using nullptr as QMessageBox parent changes modality/positioning on desktop; consider keeping `this` except where it cannot be used (e.g. WASM).
This change makes the dialog a top-level window, which can end up behind other windows or on another screen, regressing the existing UX. If this is for WASM, consider keeping `this` on native builds and only using `nullptr` (or omitting the dialog) under `__EMSCRIPTEN__` so desktop behavior remains unchanged.
</issue_to_address>
### Comment 2
<location> `src/display/mapcanvas_gl.cpp:52-59` </location>
<code_context>
+ : m_sizeSource{sizeSource}
+ {}
+
+#ifdef __EMSCRIPTEN__
+ explicit MapCanvasViewport(QWindow &sizeSource)
+ : m_sizeSource{sizeSource}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Reinitialization path on WASM sets context-lost flag but does not log or attempt any recovery, which may make failures opaque.
On WASM, when `initializeGL()` sees `s_wasmInitialized` already true, it just sets `s_wasmContextLost` and returns. This effectively becomes a silent no-op: no log and no call to something like `resetWasmContextState()`. That can make post–context-loss failures hard to diagnose. Please add at least a warning log here and consider a documented recovery path that safely calls `resetWasmContextState()` before retrying initialization.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+286
to
287
| QMessageBox::critical(nullptr, | ||
| "Unable to initialize OpenGL", |
There was a problem hiding this comment.
issue (bug_risk): Using nullptr as QMessageBox parent changes modality/positioning on desktop; consider keeping this except where it cannot be used (e.g. WASM).
This change makes the dialog a top-level window, which can end up behind other windows or on another screen, regressing the existing UX. If this is for WASM, consider keeping this on native builds and only using nullptr (or omitting the dialog) under __EMSCRIPTEN__ so desktop behavior remains unchanged.
Comment on lines
+52
to
59
| #ifdef __EMSCRIPTEN__ | ||
| #include <QOpenGLWindow> | ||
| #else | ||
| #include <QOpenGLWidget> | ||
| #endif | ||
| #include <QtCore> | ||
| #include <QtGui/qopengl.h> | ||
| #include <QtGui> |
There was a problem hiding this comment.
suggestion (bug_risk): Reinitialization path on WASM sets context-lost flag but does not log or attempt any recovery, which may make failures opaque.
On WASM, when initializeGL() sees s_wasmInitialized already true, it just sets s_wasmContextLost and returns. This effectively becomes a silent no-op: no log and no call to something like resetWasmContextState(). That can make post–context-loss failures hard to diagnose. Please add at least a warning log here and consider a documented recovery path that safely calls resetWasmContextState() before retrying initialization.
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
4 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.
Summary by Sourcery
Add WebAssembly (WASM) support for the Qt 6.10.2 map canvas by switching to QOpenGLWindow on WASM, improving WebGL context handling, and updating build and platform-specific integration.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Summary by Sourcery
Add robust Qt 6.10.2 WebAssembly support for the OpenGL map canvas while preserving desktop behavior and updating build/tooling for newer Qt, GLM, and platform quirks.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment: