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 non power-of-2 textures being flipped when resized #15904

Merged
merged 8 commits into from Mar 11, 2019

Conversation

Projects
None yet
4 participants
@merwaaan
Copy link
Contributor

merwaaan commented Mar 4, 2019

Fix for #15896

  • Uses an OffscreenCanvas only when document is not available (was the default choice before)
  • Flips the texture vertically in this case

merwaaan added some commits Mar 4, 2019

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 5, 2019

@Mugen87 does this look right to you?

@Mugen87

This comment has been minimized.

Copy link
Collaborator

Mugen87 commented Mar 5, 2019

I've tested the PR locally with the code from my test fiddle. Looks good now.

@Mugen87

Mugen87 approved these changes Mar 5, 2019

@@ -12,7 +12,7 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,

//

var useOffscreenCanvas = typeof OffscreenCanvas !== 'undefined';
var useOffscreenCanvas = typeof document === 'undefined';

This comment has been minimized.

@mrdoob

mrdoob Mar 5, 2019

Owner

Why are we changing this?

This comment has been minimized.

@Mugen87

Mugen87 Mar 5, 2019

Collaborator

OffscreenCanvas is also available in a non worker scenario. So testing for document seems more robust since it's definitely missing in a worker.

This comment has been minimized.

@mrdoob

mrdoob Mar 5, 2019

Owner

I see... How about changing the name of the variable then?

var isWorker = typeof document === 'undefined';

This comment has been minimized.

@merwaaan

merwaaan Mar 5, 2019

Author Contributor

Sure.

This comment has been minimized.

@mrdoob

mrdoob Mar 5, 2019

Owner

Thanks!

However, my original question still stands. We are no longer going to use OffscreenCanvas for resizing in non-worker scenarios?

This comment has been minimized.

@mrdoob

mrdoob Mar 5, 2019

Owner

I mean, the problem seems to be that ImageBitmap needs flipping, but I do not understand why this PR also reverts to not using OffscreenCanvas in non-worker scenarios.

This comment has been minimized.

@Mugen87

Mugen87 Mar 5, 2019

Collaborator

#15805 should enable texture resizing in workers. I though it's better to ensure the new code path is exclusively used in workers so the previous behavior of resizeImage() is not affected.

Well, maybe I'm too careful in that regard^^. I'm just not 100% sure of side effects if the method always returns resized images of type ImageBitmap when OffscreenCanvas is available.

This comment has been minimized.

@mrdoob

mrdoob Mar 5, 2019

Owner

I see. Yeah, lets not give up on OffscreenCanvas just yet. If we find problems with it we can report to browsers. Lets just flip the image for now.

This comment has been minimized.

@merwaaan

merwaaan Mar 6, 2019

Author Contributor

@mrdoob Ok, the latest commit switches back to OffscreenCanvas by default, and flips the texture in this case, which was the initial issue. Is that ok?

@@ -12,7 +12,7 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,

//

var useOffscreenCanvas = typeof OffscreenCanvas !== 'undefined';
var useOffscreenCanvas = typeof OffscreenCanvas !== 'undefined';

This comment has been minimized.

@mrdoob

mrdoob Mar 6, 2019

Owner

Minor: Extra spaces added.

@Mugen87

Mugen87 approved these changes Mar 6, 2019

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 11, 2019

I did more research on this over the weekend. Also in conversations with the WebGL team at Chrome. It's not clear if ImageBitmap being upside down is correct or not. Maybe it's a big in Chrome.

Until that is resolved, I realised we can just pass the OffscreenCanvas to gl directly.

Can you change this PR so it just changes this line from this:

return useOffscreenCanvas ? canvas.transferToImageBitmap() : canvas;

To this?

return canvas;

@mrdoob mrdoob added this to the r103 milestone Mar 11, 2019

@merwaaan

This comment has been minimized.

Copy link
Contributor Author

merwaaan commented Mar 11, 2019

@mrdoob Sure, I'll do that.

Is the conversation about ImageBitmap being upside down taking place in a public forum/issue tracker?

@merwaaan

This comment has been minimized.

Copy link
Contributor Author

merwaaan commented Mar 11, 2019

@mrdoob: I just tested @Mugen87 's fiddle again to make sure verything works and it seems like the image is not upside down anymore. Maybe fixed by a Chrome update?

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 11, 2019

Is the conversation about ImageBitmap being upside down taking place in a public forum/issue tracker?

Not yet.

@mrdoob: I just tested @Mugen87 's fiddle again to make sure verything works and it seems like the image is not upside down anymore. Maybe fixed by a Chrome update?

What browser/os did you try that on? I can still reproduce in Chrome/OSX.


// ImageBitmap is flipped vertically

if ( useOffscreenCanvas ) {

This comment has been minimized.

@mrdoob

mrdoob Mar 11, 2019

Owner

This block of code is not needed now because we're are now returning the canvas.

This comment has been minimized.

@merwaaan

merwaaan Mar 11, 2019

Author Contributor

Please ignore what I said about the issue disappearing. I got confused. What fixed it was just returning the canvas instead of the ImageBitmap.

@mrdoob mrdoob merged commit 7d387c3 into mrdoob:dev Mar 11, 2019

0 of 2 checks passed

LGTM analysis: JavaScript Fetching git commits
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 11, 2019

Thanks!

@takahirox

This comment has been minimized.

Copy link
Collaborator

takahirox commented Mar 11, 2019

I haven't perfectly read through the conversation here and in #15896 but I just want to comment that returning canvas may be safer as this PR finally goes than converting to ImageBitmap.

As I'm adding a note to ImageBitmapLoader document #15945, ImageBitmap flipY and premultiplyAlpha configuration need to be specified on bitmap creation, not on uploading to GPU. This means, texture.flipY/.premultiplyAlpha will be ignored. It may be confusing to users. If non pot regular image is implicitly converted to ImageBitmap inside of renderer, users will be surprised to see that changing texture.flipY/.premultiplyAlpha and then setting texture.needsUpdate=true has no effect.

The upside down problem mentioned above might come from this? I guess OffscreenCanvas.transferToImageBitmap() might generate ImageBitmap with imageOrientation = 'none' (non flipY) so if texture.flipY is true the texture will be upside down?

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 11, 2019

@takahirox That is indeed the problem. Thanks for confirming and clarifying 🙏

@takahirox

This comment has been minimized.

Copy link
Collaborator

takahirox commented Mar 11, 2019

Similarly saying, internally converting ImageBitmap to canvas might be confusing to users because of different texture.flipY/.premultiplyAlpha behaviors.

BTW, how ImageBitmap created with imageOrientation = 'flipY' is rendered to 2D canvas? Upside down? Or not? I need to confirm the specification but encouraging POT ImageBitmap maybe sounds reasonable and less problematic so far...

Update: perhaps it should be rendered upside down because ImageBitmap and its option isn't specific to WebGL. I'll look into more...

@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 11, 2019

@takahirox Just to clarify... The original reason we added OffscreenCanvas was to support Workers. No document in Workers.

@takahirox

This comment has been minimized.

Copy link
Collaborator

takahirox commented Mar 12, 2019

Yes, I know. I mean, the issue that non-POT ImageBitmap is implicitly converted to canvas in renderer has been around since before we introduced OffscreenCanvas for resizing in Worker. I'll clarify the problem and open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.