Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 24 additions & 28 deletions modules/core/src/lib/deck-picker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,10 @@ export default class DeckPicker {
}
}

// Resize it to current canvas size (this is a noop if size hasn't changed)
const {canvas} = this.device.getDefaultCanvasContext();
this.pickingFBO?.resize({width: canvas.width, height: canvas.height});
this.depthFBO?.resize({width: canvas.width, height: canvas.height});
// Picking buffers must track the active drawing buffer, not raw canvas element dimensions.
Comment thread
ibgreen-openai marked this conversation as resolved.
Outdated
const [width, height] = this.device.getDefaultCanvasContext().getDrawingBufferSize();
this.pickingFBO?.resize({width, height});
this.depthFBO?.resize({width, height});
}

/** Preliminary filtering of the layers list. Skid picking pass if no layer is pickable. */
Expand Down Expand Up @@ -223,8 +223,9 @@ export default class DeckPicker {
result: PickingInfo[];
emptyInfo: PickingInfo;
}> {
// @ts-expect-error TODO - assuming WebGL context
const pixelRatio = this.device.canvasContext.cssToDeviceRatio();
const canvasContext = this.device.getDefaultCanvasContext();
// Picking starts in CSS pixels, so use the canvas context's current conversion ratio.
const pixelRatio = canvasContext.cssToDeviceRatio();

const pickableLayers = this._getPickable(layers);

Expand All @@ -239,9 +240,8 @@ export default class DeckPicker {

// Convert from canvas top-left to WebGL bottom-left coordinates
// Top-left coordinates [x, y] to bottom-left coordinates [deviceX, deviceY]
// And compensate for pixelRatio
// @ts-expect-error TODO - assuming WebGL context
const devicePixelRange = this.device.canvasContext.cssToDevicePixels([x, y], true);
// And compensate for the context's current CSS-to-device ratio.
const devicePixelRange = canvasContext.cssToDevicePixels([x, y], true);
const devicePixel = [
devicePixelRange.x + Math.floor(devicePixelRange.width / 2),
devicePixelRange.y + Math.floor(devicePixelRange.height / 2)
Expand Down Expand Up @@ -387,8 +387,9 @@ export default class DeckPicker {
result: PickingInfo[];
emptyInfo: PickingInfo;
} {
// @ts-expect-error TODO - assuming WebGL context
const pixelRatio = this.device.canvasContext.cssToDeviceRatio();
const canvasContext = this.device.getDefaultCanvasContext();
// Keep the sync picking path aligned with the same canvas context state used for drawing.
const pixelRatio = canvasContext.cssToDeviceRatio();

const pickableLayers = this._getPickable(layers);

Expand All @@ -403,9 +404,8 @@ export default class DeckPicker {

// Convert from canvas top-left to WebGL bottom-left coordinates
// Top-left coordinates [x, y] to bottom-left coordinates [deviceX, deviceY]
// And compensate for pixelRatio
// @ts-expect-error TODO - assuming WebGL context
const devicePixelRange = this.device.canvasContext.cssToDevicePixels([x, y], true);
// And compensate for the context's current CSS-to-device ratio.
const devicePixelRange = canvasContext.cssToDevicePixels([x, y], true);
const devicePixel = [
devicePixelRange.x + Math.floor(devicePixelRange.width / 2),
devicePixelRange.y + Math.floor(devicePixelRange.height / 2)
Expand Down Expand Up @@ -556,19 +556,17 @@ export default class DeckPicker {
this._resizeBuffer();

// Convert from canvas top-left to WebGL bottom-left coordinates
// And compensate for pixelRatio
// @ts-expect-error TODO - assuming WebGL context
const pixelRatio = this.device.canvasContext.cssToDeviceRatio();
// @ts-expect-error TODO - assuming WebGL context
const leftTop = this.device.canvasContext.cssToDevicePixels([x, y], true);
// And compensate for the context's current CSS-to-device ratio.
const canvasContext = this.device.getDefaultCanvasContext();
const pixelRatio = canvasContext.cssToDeviceRatio();
const leftTop = canvasContext.cssToDevicePixels([x, y], true);

// take left and top (y inverted in device pixels) from start location
const deviceLeft = leftTop.x;
const deviceTop = leftTop.y + leftTop.height;

// take right and bottom (y inverted in device pixels) from end location
// @ts-expect-error TODO - assuming WebGL context
const rightBottom = this.device.canvasContext.cssToDevicePixels([x + width, y + height], true);
const rightBottom = canvasContext.cssToDevicePixels([x + width, y + height], true);
const deviceRight = rightBottom.x + rightBottom.width;
const deviceBottom = rightBottom.y;

Expand Down Expand Up @@ -663,19 +661,17 @@ export default class DeckPicker {
this._resizeBuffer();

// Convert from canvas top-left to WebGL bottom-left coordinates
// And compensate for pixelRatio
// @ts-expect-error TODO - assuming WebGL context
const pixelRatio = this.device.canvasContext.cssToDeviceRatio();
// @ts-expect-error TODO - assuming WebGL context
const leftTop = this.device.canvasContext.cssToDevicePixels([x, y], true);
// And compensate for the context's current CSS-to-device ratio.
const canvasContext = this.device.getDefaultCanvasContext();
const pixelRatio = canvasContext.cssToDeviceRatio();
const leftTop = canvasContext.cssToDevicePixels([x, y], true);

// take left and top (y inverted in device pixels) from start location
const deviceLeft = leftTop.x;
const deviceTop = leftTop.y + leftTop.height;

// take right and bottom (y inverted in device pixels) from end location
// @ts-expect-error TODO - assuming WebGL context
const rightBottom = this.device.canvasContext.cssToDevicePixels([x + width, y + height], true);
const rightBottom = canvasContext.cssToDevicePixels([x + width, y + height], true);
const deviceRight = rightBottom.x + rightBottom.width;
const deviceBottom = rightBottom.y;

Expand Down
58 changes: 39 additions & 19 deletions modules/core/src/lib/deck.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ export default class Deck<ViewsT extends ViewOrViews = null> {
private _metricsCounter: number = 0;
private _hoverPickSequence: number = 0;
private _pointerDownPickSequence: number = 0;
private _pollCanvasContextSize: boolean = false;

private _needsRedraw: false | string = 'Initial render';
private _pickRequest: {
Expand Down Expand Up @@ -386,6 +387,9 @@ export default class Deck<ViewsT extends ViewOrViews = null> {
// See if we already have a device
if (props.device) {
this.device = props.device;
// External Device ownership means Deck cannot wrap luma's onResize callback.
// Keep a render-loop poll so viewport dimensions still follow CanvasContext state.
this._pollCanvasContextSize = true;
}

let deviceOrPromise: Device | Promise<Device> | null = this.device;
Expand All @@ -405,11 +409,9 @@ export default class Deck<ViewsT extends ViewOrViews = null> {
_cachePipelines: true,
...this.props.deviceProps,
onResize: (canvasContext, info) => {
// Sync drawing buffer dimensions with externally-managed canvas
const {width, height} = canvasContext.canvas;
canvasContext.setDrawingBufferSize(width, height);

this._needsRedraw = 'Canvas resized';
// Attached contexts still emit resize through luma's CanvasContext.
// Deck only mirrors that state into viewport dimensions and redraw flags.
this._onCanvasContextResize(canvasContext);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of setDrawingBufferSize here causes a resize regression in interleaved modes (MapLibre, Mapbox, etc).

webgl2Adapter.attach() sets autoResize: false on the canvas context (webgl-adapter.ts:73), which means luma's _updateDrawingBufferSize() in canvas-surface.ts skips updating the drawing buffer when the ResizeObserver fires. The CSS size tracking (cssWidth/cssHeight) updates correctly, but luma's internal drawingBufferWidth/drawingBufferHeight never sync to the new canvas dimensions.

So while CanvasContext is indeed the authority for observing the resize, it doesn't act on it for attached contexts - someone still has to call setDrawingBufferSize. On master that was Deck.. this PR removes that call assuming luma handles it, but luma explicitly doesn't for autoResize: false.

Possible paths forward:

  1. Restore the setDrawingBufferSize call here (what master does, see fix(core): restore drawing buffer sync for attached GL contexts on resize #10281)
  2. Change luma's attach() to not disable autoResize, or add a separate flag that skips only the canvas element resize but still syncs the drawing buffer tracking
  3. Add an "installable resize listener" on CanvasContext (as you mentioned in the "Future Opportunity" section) that handles this automatically

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrisgervang I tried to address this concern can you take another look?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just did a full side-by-side comparison between this PR and the behavior on master, and they behave identically.

The only remaining issue I found was a "flashing" bug across environments in interleaved mode, but I observed this before and after this change, so it's not a regression.. something to follow up on separately.

flashing.short.mov

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall doing a fix for flashing during resize in the main canvas, might be worth reviewing:

userOnResize?.(canvasContext, info);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if a user passes deviceProps.onResize, it gets spread onto the device here but is then unconditionally overwritten in _setDeviceResizeHandler (line 1082) without forwarding. The old code preserved the user callback. Probably not blocking since none of our integrations use it, but worth noting as an intentional public API change or adding a forward call back.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I added documentation for this overwrite in deck docs.
My overall assessment was that it was not really worth the extra complexity to try to handle this.
We can add it back later if needed.

}
});
Expand Down Expand Up @@ -1054,15 +1056,22 @@ export default class Deck<ViewsT extends ViewOrViews = null> {
}
}

/** If canvas size has changed, reads out the new size and update */
private _updateCanvasSize(): void {
/**
* Sync Deck viewport dimensions from the active canvas context.
* luma.gl owns resize observation, DPR tracking and drawing buffer sizing.
*/
private _updateCanvasSize(
canvasContext: {
getCSSSize(): [number, number];
} | null = this.device?.getDefaultCanvasContext?.() || null
): void {
const {canvas} = this;
if (!canvas) {
return;
}
// Fallback to width/height when clientWidth/clientHeight are undefined (OffscreenCanvas).
const newWidth = canvas.clientWidth ?? canvas.width;
const newHeight = canvas.clientHeight ?? canvas.height;
const [newWidth, newHeight] = canvasContext
? // The canvas context owns the authoritative CSS size after resize/DPR observation.
canvasContext.getCSSSize()
: // Fallback to width/height when there is no default canvas context available yet.
[canvas?.clientWidth ?? canvas?.width ?? 0, canvas?.clientHeight ?? canvas?.height ?? 0];

if (newWidth !== this.width || newHeight !== this.height) {
// @ts-expect-error private assign to read-only property
this.width = newWidth;
Expand All @@ -1075,6 +1084,12 @@ export default class Deck<ViewsT extends ViewOrViews = null> {
}
}

private _onCanvasContextResize(canvasContext: {getCSSSize(): [number, number]}): void {
// luma owns resize detection; Deck reacts by invalidating redraw and updating view state.
this._needsRedraw = 'Canvas resized';
this._updateCanvasSize(canvasContext);
}

private _createAnimationLoop(
deviceOrPromise: Device | Promise<Device>,
props: DeckProps<ViewsT>
Expand Down Expand Up @@ -1149,10 +1164,9 @@ export default class Deck<ViewsT extends ViewOrViews = null> {
autoResize: true
},
onResize: (canvasContext, info) => {
// Set redraw flag when luma.gl's CanvasContext detects a resize
// This restores pre-9.2 behavior where resize automatically triggered redraws
this._needsRedraw = 'Canvas resized';
// Call user's onResize if provided
// Deck-created canvases follow the same contract as attached canvases:
// luma updates canvas state, Deck updates viewport bookkeeping and callbacks.
this._onCanvasContextResize(canvasContext);
userOnResize?.(canvasContext, info);
}
});
Expand Down Expand Up @@ -1383,7 +1397,8 @@ export default class Deck<ViewsT extends ViewOrViews = null> {

this.setProps(this.props);

this._updateCanvasSize();
// Seed the initial Deck width/height from the current canvas context before onLoad fires.
this._updateCanvasSize(this.device.getDefaultCanvasContext());
this.props.onLoad();
}

Expand Down Expand Up @@ -1447,7 +1462,12 @@ export default class Deck<ViewsT extends ViewOrViews = null> {
}
}

this._updateCanvasSize();
if (this._pollCanvasContextSize) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this primarily intended to be a performance optimization to remove unnecessary syntonization when the device is internal and deck owns the onResize wiring?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This separate path between internal and externally created devices is the main remaining wart.
My current understanding is that we need some changes in luma to get around this.
I updated the implementation to be more CanvasContext centric and added a second infographic to the PR description to illustrate the two flows.

// Callers that hand Deck an existing Device keep luma's CanvasContext as the source
// of truth, but Deck does not own that context's onResize wiring. Poll the context
// once per frame so width/height stay in sync without falling back to DOM reads.
this._updateCanvasSize();
}

this._updateCursor();
Comment thread
ibgreen marked this conversation as resolved.

Expand Down
3 changes: 2 additions & 1 deletion modules/google-maps/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ export function createDeckInstance(

const newDeck = new Deck({
...props,
// Default to true for high-DPI displays, but allow user override
// The basemap owns the shared canvas in interleaved mode; Deck only forwards the preferred DPR.
// In non-interleaved mode this still feeds the luma canvas context that Deck creates.
useDevicePixels: props.useDevicePixels ?? true,
style: props.interleaved ? null : {pointerEvents: 'none'},
parent: getContainer(overlay, props.style),
Expand Down
5 changes: 3 additions & 2 deletions modules/mapbox/src/deck-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ export function getDeckInstance({
};
deckProps.views ||= getDefaultView(map);

// deck is using the WebGLContext created by mapbox,
// block deck from setting the canvas size, and use the map's viewState to drive deck.
// deck is using the WebGLContext created by mapbox.
// The map and its attached luma canvas context own canvas sizing and DPR state here.
// Deck only follows view state and avoids trying to size the shared canvas itself.
Object.assign(deckProps, {
width: null,
height: null,
Expand Down
104 changes: 104 additions & 0 deletions test/modules/core/lib/deck.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,110 @@ test('Deck#abort', async () => {
console.log('Deck initialization aborted');
});

test('Deck#canvas context resize drives Deck dimensions', async () => {
const resizeEvents: Array<{width: number; height: number}> = [];
const deck = new Deck({
device,
width: 1,
height: 1,
viewState: {longitude: 0, latitude: 0, zoom: 0},
layers: [],
onResize: dimensions => resizeEvents.push(dimensions)
});

await waitForRender(deck);

const canvasContext = device.getDefaultCanvasContext();
const originalGetCSSSize = canvasContext.getCSSSize.bind(canvasContext);
const nextSize: [number, number] = [17, 19];

try {
canvasContext.getCSSSize = () => nextSize;
resizeEvents.length = 0;

// Call the internal resize hook directly so the test verifies Deck's reaction to luma state.
// @ts-expect-error testing private resize hook
deck._onCanvasContextResize(canvasContext);

expect(deck.width, 'Deck width comes from canvas context CSS size').toBe(nextSize[0]);
expect(deck.height, 'Deck height comes from canvas context CSS size').toBe(nextSize[1]);
expect(resizeEvents, 'Deck onResize fires from canvas context resize').toEqual([
{width: nextSize[0], height: nextSize[1]}
]);
expect(deck.needsRedraw(), 'resize invalidates redraw').toBeTruthy();
} finally {
canvasContext.getCSSSize = originalGetCSSSize;
deck.finalize();
}
});

test('Deck#useDevicePixels forwards to canvas context', async () => {
const deck = new Deck({
device,
width: 1,
height: 1,
viewState: {longitude: 0, latitude: 0, zoom: 0},
layers: []
});

await waitForRender(deck);

const canvasContext = device.getDefaultCanvasContext();
const initialUseDevicePixels = canvasContext.props.useDevicePixels;

try {
// Deck.setProps should only forward the preference into luma's canvas context.
deck.setProps({useDevicePixels: false});
expect(canvasContext.props.useDevicePixels, 'canvas context useDevicePixels updated').toBe(
false
);

// Numeric overrides should flow through unchanged so luma can size the drawing buffer.
deck.setProps({useDevicePixels: 2});
expect(canvasContext.props.useDevicePixels, 'numeric DPR override is forwarded').toBe(2);
} finally {
canvasContext.setProps({useDevicePixels: initialUseDevicePixels});
deck.finalize();
}
});

test('Deck#render frame syncs provided device canvas context size', async () => {
const resizeEvents: Array<{width: number; height: number}> = [];
const deck = new Deck({
device,
width: 1,
height: 1,
viewState: {longitude: 0, latitude: 0, zoom: 0},
layers: [],
onResize: dimensions => resizeEvents.push(dimensions)
});

await waitForRender(deck);

const canvasContext = device.getDefaultCanvasContext();
const originalGetCSSSize = canvasContext.getCSSSize.bind(canvasContext);
const nextSize: [number, number] = [23, 29];

try {
canvasContext.getCSSSize = () => nextSize;
resizeEvents.length = 0;

// Provided Device instances do not route luma's onResize callback through Deck.
// The render loop still refreshes dimensions from CanvasContext so view state stays current.
// @ts-expect-error testing private render loop
deck._onRenderFrame();

expect(deck.width, 'Deck width is refreshed during render frame').toBe(nextSize[0]);
expect(deck.height, 'Deck height is refreshed during render frame').toBe(nextSize[1]);
expect(resizeEvents, 'Deck onResize fires from render-frame canvas context sync').toEqual([
{width: nextSize[0], height: nextSize[1]}
]);
} finally {
canvasContext.getCSSSize = originalGetCSSSize;
deck.finalize();
}
});

test('Deck#no views', async () => {
await new Promise<void>((resolve, reject) => {
const deck = new Deck({
Expand Down
Loading