[v9] Fix GoogleMapsOverlay by not clearing canvas #8351
Conversation
zbigg
force-pushed
the
zbigg/fix-google-maps-overlay-interleaved
branch
from
b8c10bb to
1081deb
Compare
| onViewportActive, | ||
| clearStack = true, | ||
| clearCanvas = true | ||
| clearStack = true |
There was a problem hiding this comment.
clearCanvas was unused in this fn
zbigg
force-pushed
the
zbigg/fix-google-maps-overlay-interleaved
branch
from
1081deb to
d21ac1a
Compare
modules/core/src/lib/deck.ts
Outdated
|
|
||
| setGLParameters(device, this.props.parameters); | ||
|
|
||
| this.device?.canvasContext?.resize({useDevicePixels: this.props.useDevicePixels}); |
There was a problem hiding this comment.
Not sure if this is correct place, but we must do it in one place or another, when we're rendering to canvas we're not "managing" by ourselves.
There was a problem hiding this comment.
After some research, i think the core problem is that luma.gl CanvasContext.cssToDeviceRatio() returns invalid value if no-one called resize({useDevicePixels :true}) or setDevicePixelRatio and it's not called by animation loop for external contexts.
I think best fix would be to fix cssToDeviceRatio so it works even if user didn't call resize.
There was a problem hiding this comment.
The useDevicePixels is being passed down to luma in _createAnimationLoop, which should be enough. I also note that there we have a TODO regarding autoResizeDrawingBuffer which may be related to my other comment:
https://github.com/visgl/deck.gl/blob/master/modules/core/src/lib/deck.ts#L803-L804
There was a problem hiding this comment.
The CSS pixel API on the CanvasContext is a bit confusing, and I think it can be a source of subtle errors.
To create that API, I basically just took a bunch of global CSS pixel helper functions (that had been hacked on over the years) in luma.gl v8 and made them into members of the CanvasContext.
While they fit well there, I feel these functions as written and documented right now are not easy to understand and there are some pitfalls with how drawing buffers and canvases relate in WebGL that require special handling so one has to be careful when refactoring them.
The existing test cases were also too difficult to port as they relied on hacky mocking of WebGL context so we lost that safety net. Such subtle functions without test cases is a bad situation.
For the call in question, I wonder if useDevicePixels wouldn't be better as a prop on the CanvasContext rather than a parameter to resize(). It kind of looks confusing to not send in width and height but to have to remember to send in useDevicePixels.
There was a problem hiding this comment.
CanvasContext already has useDevicePixels property which is true as expected. It's misbehaving cssToDeviceRatio which expects resize to be called by AnimationLoop or by hand.
It kind of looks confusing to not send in width and height but to have to remember to send in useDevicePixels.
Actually it isn't mandatory. It's me overlooking that this.device?.canvasContext?.resize() is enough.
Nevertheless, question remains - who is responsible for setup like this? deck or GoogleMapsOverlay (seems best)
In latest version of patch this workaround is moved to GoogleMapsOverlay.
| scissor: [0, 0, gl.canvas.width, gl.canvas.height], | ||
| stencilFunc: [gl.ALWAYS, 0, 255, gl.ALWAYS, 0, 255] | ||
| }); | ||
|
|
There was a problem hiding this comment.
these are managed by beginRenderPass now i recon ... and i think they are not needed now
There was a problem hiding this comment.
Resizing the window results in the overlay being the wrong aspect. Could you check if this is due to removing this code?
There was a problem hiding this comment.
With this code back in place it happens too, but ... apparently, it's harder to reproduce.
Reverted those lines.
| scissor: [0, 0, gl.canvas.width, gl.canvas.height], | ||
| stencilFunc: [gl.ALWAYS, 0, 255, gl.ALWAYS, 0, 255] | ||
| }); | ||
|
|
There was a problem hiding this comment.
Resizing the window results in the overlay being the wrong aspect. Could you check if this is due to removing this code?
modules/core/src/lib/deck.ts
Outdated
|
|
||
| setGLParameters(device, this.props.parameters); | ||
|
|
||
| this.device?.canvasContext?.resize({useDevicePixels: this.props.useDevicePixels}); |
There was a problem hiding this comment.
The useDevicePixels is being passed down to luma in _createAnimationLoop, which should be enough. I also note that there we have a TODO regarding autoResizeDrawingBuffer which may be related to my other comment:
https://github.com/visgl/deck.gl/blob/master/modules/core/src/lib/deck.ts#L803-L804
zbigg
force-pushed
the
zbigg/fix-google-maps-overlay-interleaved
branch
from
8bea4db to
c61f2b1
Compare
Basic picking working useNormalizedColors Forward undefined/null log on null object Co-authored-by: Ib Green <ib@foursquare.com> Move uniformStore binding to _getModel Type uniformStore Tidy Tidy Bump luma API update chore(ci): Update CI dependencies (#8331) * chore(ci): Update CI to Node.js v18 * chore(ci): Update checkout, setup-node, and setup-python actions [website] Fix FirstPersonView broken video URL (#8330) Bump luma Improve types Tidy Remove unneeded state.uniformStore carto/fetchMap: fix support for quantile color scale in numeric columns for static quadbin/h3 tilesets (#8347) docs: fix source link for `fly-to-interpolator` (#8320) carto: add typedocs to source types (#8343) Remove use of deprecated BufferWithAccessor (#8345) [v9] Fix GoogleMapsOverlay by not clearing canvas (#8351) [website] GEOMATICO-371 | Taxo map biodiversity map showcase (#8169) Update Scatterplot to GLSL 300 (#8369) Update PathLayer to GLSL 300 (#8370) Update ArcLayer, LineLayer, PolygonLayer to GLSL 300 (#8371) Update BitmapLayer, IconLayer, PointCloudLayer, TripsLayer to GLSL 300 (#8372) Update remaining shaders to GLSL 300 (#8373) Co-authored-by: felixpalmer <felixpalmer@gmail.com> Bump vite from 4.4.9 to 4.5.1 (#8341) Bump luma Fix build Switch to ShaderInputs in ScatterployLayer Picking working again (without highlight)
For #8302
Background
Change List
clearCanvas: falseand not clear framebuffer