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

Allow GLTFLoader to use ImageBitmapLoader #19518

Merged
merged 5 commits into from Jun 11, 2020
Merged

Conversation

toji
Copy link
Contributor

@toji toji commented Jun 1, 2020

Fixes #19511

This is one potential approach for allowing GLTFLoader to utilize ImageBitmaps, which significantly reduce stalls in rendering when uploading textures to the GPU. Alternatively, if allowing a custom ImageLoader to be supplied to the TextureLoader is not desirable, we could use ImageBitmapLoader and CanvasTexture in this class directly when ImageBitmaps are supported and fallback to TextureLoader otherwise.

The reason this is limited to GLTFLoader for now is that ImageBitmap-based textures don't support Y-flipping in the same way that the traditional approach does. If a workaround is figured out for that then ImageBitmaps could be used more widely.

Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

I think we should give this a try. When merged, I can update the docs and TS files.

@WestLangley WestLangley added this to the r118 milestone Jun 4, 2020
Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thanks you!

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jun 5, 2020

It's worth noting that ImageBitmapLoader depends on fetch() rather than XmlHttpRequest. I think that's OK in terms of browser support — browsers like IE11 that don't support fetch won't support createImageBitmap either, and so won't try to use this feature. But our use of fetch() does not include any HTTP request headers...

fetch( url ).then( function ( res ) {

...which could be a problem for some users who specify them. We should probably try to pass those request parameters into the fetch() call.

@mrdoob
Copy link
Owner

mrdoob commented Jun 5, 2020

Starting with GLTFLoader seemed like a good idea until I saw we have to add new API (setImageLoader)...

The reason this is limited to GLTFLoader for now is that ImageBitmap-based textures don't support Y-flipping in the same way that the traditional approach does. If a workaround is figured out for that then ImageBitmaps could be used more widely.

I think it may be better to try to implement glsl based Y-flipping and then implement this right in TextureLoader. Probably after taking care of #17949 though.

@toji
Copy link
Contributor Author

toji commented Jun 7, 2020

An alternate route to adding this would be to have the glTF loader internally use TextureLoader on browsers that don't support ImageBitmaps and ImageBitmapLoader + CanvasTextures on browsers that do. That felt messier to me, but if we're trying to keep new API surface off of TextureLoader at all costs it's a more self contained approach.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jun 9, 2020

I'd be OK with @toji's suggestion of just putting it on GLTFLoader for now, without a new API. Might be a safer place to start, and see if there are any browser support edge cases we've missed?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 9, 2020

Sounds good!

toji added 4 commits June 9, 2020 09:09
Fixes mrdoob#19511

This is one potential approach for allowing GLTFLoader to utilize ImageBitmaps, which significantly reduce stalls in rendering when uploading textures to the GPU. Alternatively, if allowing a custom ImageLoader to be supplied to the TextureLoader is not desirable, we could use ImageBitmapLoader and CanvasTexture in this class directly when ImageBitmaps are supported and fallback to TextureLoader otherwise.

The reason this is limited to GLTFLoader for now is that ImageBitmap-based textures don't support Y-flipping in the same way that the traditional approach does. If a workaround is figured out for that then ImageBitmaps could be used more widely.
Doesn't add additional API surface to TextureLoader
@toji
Copy link
Contributor Author

toji commented Jun 9, 2020

Okay, updated the PR to a variant that doesn't alter TextureLoader at all. Let me know how it looks!

@mrdoob
Copy link
Owner

mrdoob commented Jun 11, 2020

Okay!

@mrdoob mrdoob merged commit ff71bd0 into mrdoob:dev Jun 11, 2020
@mrdoob
Copy link
Owner

mrdoob commented Jun 11, 2020

Thanks!

@toji toji deleted the gltf-imagebitmap branch June 11, 2020 04:26
@mrdoob
Copy link
Owner

mrdoob commented Jun 20, 2020

@toji FYI, this PR broke model-viewer.

Screen Shot 2020-06-16 at 2 23 12 PM

Screen Shot 2020-06-16 at 2 23 25 PM

I've fixed it by passing { premultiplyAlpha: 'none' } to createImageBitmap by default: 7e5d148

@mrdoob
Copy link
Owner

mrdoob commented Jun 20, 2020

@jdashg FYI, I had to add Firefox UA sniffing because the current implementation of createImageBitmap breaks when passing { premultiplyAlpha: 'none' } as options: 70aed3c

Screen Shot 2020-06-19 at 6 22 54 PM

@donmccurdy
Copy link
Collaborator

Should have a user-facing option to disable the ImageBitmapLoader? I wonder if there may be other browser compatibility issues...

@mrdoob
Copy link
Owner

mrdoob commented Jun 20, 2020

I've tested Safari too. Seems like it's not shipping createImageBitmap yet.

@larsbergstrom
Copy link

Ooof, sorry about that! I know @fernandojsg and @daoshengmu and @takahirox were looking into this, e.g., for:
https://bugzilla.mozilla.org/show_bug.cgi?id=1367251
https://bugzilla.mozilla.org/show_bug.cgi?id=1367251

@toji
Copy link
Contributor Author

toji commented Jun 20, 2020

Blech. Sorry for missing the premultiplied alpha bit, but I'm glad the fix was trivial. And yes, Safari hasn't shipped createImageBitmap yet, which is 😢 but at least lends itself to clean feature detection.

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.

Use ImageBitmaps for GLTFLoader (and possibly others)
6 participants