Support for OffscreenCanvas in Web Workers #5168
Conversation
willeastcott
added
feature request
area: graphics
| @@ -444,7 +444,7 @@ class GraphicsDevice extends EventHandler { | |||
| } | |||
|
|
|||
| updateClientRect() { | |||
| this.clientRect = this.canvas.getBoundingClientRect(); | |||
| this.clientRect = new DOMRect(0, 0, this.canvas.width, this.canvas.height); | |||
There was a problem hiding this comment.
Couple of issues here:
DOMRectisn't available in Node.js so this causes the unit tests to fail.- This function is called every frame so this will be generating garbage. Probably not a significant amount - just pointing it out.
There was a problem hiding this comment.
I just pushed a commit with an alternative implementation which will generate no garbage and also works in Node.js: It's now a public class field (better for performance because "hidden class"). It's a simple object with properties width and height. From reviewing the code base, I can say that only width and height are used, and not x and y. The only reference is in src/framework/components/camera/component.js .
src/core/platform.js
Outdated
| @@ -87,6 +87,13 @@ const platform = { | |||
| */ | |||
| browser: environment === 'browser', | |||
|
|
|||
| /** | |||
| * Convenience boolean indicating whether we're running in a Web Worker | |||
There was a problem hiding this comment.
| * Convenience boolean indicating whether we're running in a Web Worker | |
| * Convenience boolean indicating whether we're running in a Web Worker. |
| this.clientRect.width = this.canvas.width; | ||
| this.clientRect.height = this.canvas.height; |
There was a problem hiding this comment.
My guess is that the canvas.width and canvas.height properties are the pixel resolution of the canvas, whereas getBoundingClientRect would return the CSS width and height (see devicePixelRatio for more details). So I would imagine CameraComponent#worldToScreen and CameraComponent#screenToWorld would break if the canvas pixel resolution does not match the CSS resolution. Maybe needs this.canvas.style.width instead? So parseInt(this.canvas.style.width, 10). But I'm not sure if that would work in all circumstances...
There was a problem hiding this comment.
Unfortunately it would not work. Because Web Workers don't do the page layout, they are unaware of el.getBoundingClientRect(), el.style, getComputedStyle(el) et al. Only the main/rendering thread knows about layout dimensions and coordinates of mouse button clicks.
Therefore I can think right now of the following independent options:
- Change
CameraComponent#screenToWorldandCameraComponent#worldToScreento accept normalized coordinates for the mouse clicks. i.e. let the user do the division of the mouse click coordinates by the canvas screen dimensions, of which they have full knowledge. - Pass the canvas screen dimensions into the
Applicationconstructor (and require that the canvas is not scaled after). - Leave the implementation as it is and require that
OffscreenCanvasnot be scaled by CSS whenCameraComponent#screenToWorldandCameraComponent#worldToScreenare to be used.
I can't judge what would be suitable for the project, but option 3 would be least amount of work while still being correct for the general use case.
There was a problem hiding this comment.
option 3 seems to be the best fit for now. also, may I enquiry if there's anything else prevent this from being merged to main?
GSterbrant
added
the
open source contribution
mvaligursky
removed
the
release: next minor
Fixes #2481
This probably needs the development of a minimal example (and perhaps automatic tests). Since this is my first contribution, please advise on how to proceed.
I'm happy to discuss and improve the implementation if needed.
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.