Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): conflicting resize logic when using non-default useDevicePixels #9326

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

Pessimistress
Copy link
Collaborator

Background

Canvas resize events are being responded to by two independent code paths:

  • CanvasContext with autoResize: true sets up resize observer which calls resize without the correct useDevicePixels setting
  • AnimationLoop with autoResizeDrawingBuffer: true checks canvas size before each render frame with the correct useDevicePixels setting

If the user sets useDevicePixels to a value that is not window.devicePixelRatio the canvas ends up getting resized twice to different dimensions.

Change List

  • Disable autoResize when creating the context

@Pessimistress Pessimistress requested a review from ibgreen January 4, 2025 01:29
@coveralls
Copy link

coveralls commented Jan 4, 2025

Coverage Status

coverage: 91.705%. remained the same
when pulling 73b6e9f on x/context-resize
into f13b96e on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

just some background.

  • There is an improved ResizeObserver API in browsers which allows the canvas context's ResizeObserver to determine the actual device pixel size of the canvas.
  • This hasn't been possible in the past, as CSS width * dpr is just an approximation (because of rounding errors) which can result in very noticeable and undesirable moire effects.
  • I recommend reading https://webgpufundamentals.org/webgpu/lessons/webgpu-resizing-the-canvas.html if interested.

CanvasContext now adopts this new method (at the cost of reduced support for arbitrary user specified DPRs in useDevicePixelRatio).

Even if the app does not use CanvasContext auto resize, the resize observer should update the pixelSize field on the canvas context, so that the app has access to the perfect size information for its own resizing calculations.

Longer term, I would avoid relying on any residual resize logic in the AnimationLoop. It is duplicative and confusing if resize is done (differently) in two places.

@Pessimistress
Copy link
Collaborator Author

My take away from reviewing luma.gl branches and discussion with @ibgreen:

  • CanvasContext's autoResize API will be the preferred way to handle resize, and AnimationLoop's autoResizeDrawingBuffer API will be deprecated
  • luma.gl v9.2 significantly improves pixel size handling from v9.1
  • As of the latest 9.1 alpha release, CanvasContextProps.autoResize cannot replace AnimationLoopProps.autoResizeDrawingBuffer, because resizeObserver is triggered outside of the render loop. This is intended to be fixed in 9.2 by the onResize callback.
  • Without last-minute changes to this essential code path, I am still disabling autoResize in this PR, but changed the comment to note that this is temporary.
    return new AnimationLoop({
      device: deviceOrPromise,
-      useDevicePixels,                 // v9.1
-      autoResizeDrawingBuffer: !gl,    // v9.1
+      autoResizeDrawingBuffer: false,  // v9.2
      autoResizeViewport: false,
      deviceOrPromise = luma.createDevice({
        createCanvasContext: {
          canvas: this._createCanvas(props),
          useDevicePixels: this.props.useDevicePixels,
-          autoResize: false   // v9.1
+          autoResize: true    // v9.2
        }
      });

Copy link
Collaborator

@felixpalmer felixpalmer left a comment

Choose a reason for hiding this comment

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

Agree with the approach 👍

@Pessimistress Pessimistress merged commit 1e8c2b2 into master Jan 6, 2025
2 checks passed
@Pessimistress Pessimistress deleted the x/context-resize branch January 6, 2025 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants