Conversation
cdata
left a comment
There was a problem hiding this comment.
So awesome to see this finally landing in <model-viewer> 😻
| export const $updateSource = Symbol('updateSource'); | ||
| export const $markLoaded = Symbol('markLoaded'); | ||
| export const $container = Symbol('container'); | ||
| export const $input = Symbol('input'); |
There was a problem hiding this comment.
Kind of minor, but this immediately made me think of the <input> element.
There was a problem hiding this comment.
Bump: I know it will be a pain, but let's pick a slightly less ambiguous name for this. userInputElement or something.
| } | ||
| } | ||
| const canvas = this[$displayCanvas]; | ||
| const blobCanvas = document.createElement('canvas'); |
There was a problem hiding this comment.
Let's hold a global reference to this so that we don't create it over and over again.
| return true; | ||
| } | ||
|
|
||
| [$selectCanvas]() { |
There was a problem hiding this comment.
Would you mind leaving a small comment about why we have this canvas swapping technique here?
| width: 100%; | ||
| height: 100%; | ||
| display: block; | ||
| overflow: hidden; |
| aria-label="A depiction of a 3D model" | ||
| aria-live="polite"> | ||
| </canvas> | ||
| <div class="input" tabindex="1" |
There was a problem hiding this comment.
It just occurred to me that one side-effect of this change is that right-click -> save image will not work anymore 😟 kind of minor, but still lame....
There was a problem hiding this comment.
Yeah, although with the transparency, you often don't get the image you expect anyway...
There was a problem hiding this comment.
That's fair, although the delta between no image and potentially surprising image is somewhat large 😛
| } | ||
| } | ||
|
|
||
| get onlyOneScene(): boolean { |
There was a problem hiding this comment.
minor, but the name here would read more naturally to me as hasOnlyOneScene
| return this[$arRenderer] != null && this[$arRenderer].isPresenting; | ||
| } | ||
|
|
||
| expandTo(width: number, height: number) { |
There was a problem hiding this comment.
Sort of thought the method signature said it all, but I'll add something.
| expandTo(width: number, height: number) { | ||
| if (width > this.width || height > this.height) { | ||
| const maxWidth = Math.max(width, this.width); | ||
| const maxHeight = Math.max(height, this.height); |
There was a problem hiding this comment.
We may want to explore shrinking these dimensions back down after some time has passed since scenes have been removed...
There was a problem hiding this comment.
I guess; we never did before. How about I file an issue on it?
| this.threeRenderer.setPixelRatio(dpr); | ||
| this.canvasElement.style.width = `${this.width}px`; | ||
| this.canvasElement.style.height = `${this.height}px`; | ||
| for (let scene of this.scenes) { |
There was a problem hiding this comment.
const (incidentally, linter will eventually correct this on your behalf)
| const contextBitmap = context as ImageBitmapRenderingContext; | ||
| const bitmap = | ||
| (this.canvas3D as OffscreenCanvas).transferToImageBitmap(); | ||
| contextBitmap.transferFromImageBitmap(bitmap); |
| return true; | ||
| } | ||
|
|
||
| [$selectCanvas]() { |
There was a problem hiding this comment.
Would you mind leaving a small comment about why we have this canvas swapping technique here?
| aria-label="A depiction of a 3D model" | ||
| aria-live="polite"> | ||
| </canvas> | ||
| <div class="input" tabindex="1" |
There was a problem hiding this comment.
That's fair, although the delta between no image and potentially surprising image is somewhat large 😛
| }); | ||
|
|
||
| test( | ||
| // TODO(elalish) This test is flaky on iOS and Safari. |
There was a problem hiding this comment.
File an issue 🙏 TODO($ISSUENUM)
| suite.skip('when losing the GL context', () => { | ||
| // We're skipping this test for now, as losing the context that was | ||
| // created with transferControlToOffscreen() causes the Chrome tab to | ||
| // crash. |
| await this.preload(url, progressCallback); | ||
|
|
||
| const gltf = await cache.get(url)!; | ||
| if (gltf == null) { |
There was a problem hiding this comment.
This part of the code changed significantly in #1072 . Maybe it made this issue go away?
| export const $updateSource = Symbol('updateSource'); | ||
| export const $markLoaded = Symbol('markLoaded'); | ||
| export const $container = Symbol('container'); | ||
| export const $input = Symbol('input'); |
There was a problem hiding this comment.
Bump: I know it will be a pain, but let's pick a slightly less ambiguous name for this. userInputElement or something.
…into offscreenCanvas
Fixes #845
Fixes #862
Fixes #37
Fixes #216
This replaces #985, though we could probably do it as a two-step PR if you really want to. We now use OffscreenCanvas and BitmapRenderer when available, and with a single instance of model-viewer we only use a single canvas, so these two things help performance considerably. Also, all canvases are now the same size, which is equal to the largest any canvas was during a session. This keeps reallocation down. I also updated toBlob to use its own canvas instead of trying to resize what's existing. Found one Chrome bug (I think), which is that context loss of an offscreenCanvas that was created with transferControlToOffscreen() seems to crash the Chrome tab.
Most of this change is just a big rename, since I wrapped an input div around our canvas (since we have two now). This separates the interaction element from the display element.