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

WebGLRenderer.copyTextureToTexture3D() support Texture and CompressedTexture #21942

Merged
merged 2 commits into from
Jun 10, 2021

Conversation

mbredif
Copy link
Contributor

@mbredif mbredif commented Jun 2, 2021

Description

copyTextureToTexture3D currently assumes that srcTexture is a THREE.DataTexture.

Bug

The bug may be reproduced using the webgl2_materials_texture2darray.html example of this PR against r128 or dev.
It fails because srcTexture.image is undefined.

Workaround

Without this PR, a THREE.Texture may be passed to copyTextureToTexture3D after the following workaround (so that it mimicks a THREE.DataTexture)
texture.image.data = texture.image;

Fix
This PR enables passing a THREE.Texture or a THREE.CompressedTexture as a source to copyTextureToTexture3D using an approach similar to the implementation of copyTextureToTexture .
It has not been thoroughly tested on compressed textures though.

Alternative/temporary fix
Change the docs to state that srcTexture should be a DataTexture rather than a Texture.

This contribution is funded by the ALEGORIA ANR project.

@mbredif mbredif force-pushed the copyTextureToTexture3D branch from a7cc089 to 00a18e2 Compare June 2, 2021 15:54
@mbredif mbredif force-pushed the copyTextureToTexture3D branch from 00a18e2 to 1b23d11 Compare June 2, 2021 16:21
@mrdoob
Copy link
Owner

mrdoob commented Jun 2, 2021

@DavidPeicho Looks good?

@mrdoob mrdoob added this to the r130 milestone Jun 2, 2021
@DavidPeicho
Copy link
Contributor

Sounds good to me!

@mbredif
Copy link
Contributor Author

mbredif commented Jun 4, 2021

@DavidPeicho , thanks for your feedback
Have you tested it with compressed textures ? I did not test it much. But if you have, we could remove the warning !

@DavidPeicho
Copy link
Contributor

I haven't. I use it for medical texture 3D and and most of the texture I use as sources are slices I generated.
I think we can try with any compressed texture just see if it works

@mbredif
Copy link
Contributor Author

mbredif commented Jun 4, 2021

Maybe it should be discussed in another PR, but I find it weird that the UNPACK parameters come from the dstTexture rather than the srcTexture, especially for FLIPY. What do you think ?

@DavidPeicho
Copy link
Contributor

It's a bit of both to be honest. You need to unpack based on the source and dest texture. For the FLIP_Y that's a really good question, I am pretty sure the current version will fail with force instance if source is flipped and dest is flipped as well.
I don't really know what should be the value, but by intuition I would say it should also be based on both source flipY and dest flipY?

@mbredif
Copy link
Contributor Author

mbredif commented Jun 4, 2021

I was wanting to propose either _gl.pixelStorei( _gl.UNPACK_FLIP_Y_WEBGL, srcTexture.flipY != dstTexture.flipY ); (as you propose) or _gl.pixelStorei( _gl.UNPACK_FLIP_Y_WEBGL, srcTexture.flipY); (considering that flipping only affects the data loading and has no effect afterward)

  • should the box and positions be affected by the flipY values ?
  • what about UNPACK_PREMULTIPLY_ALPHA_WEBGL and UNPACK_ALIGNMENT ?

(copyTextureToTexture and copyTextureToTexture3D are equally affected btw)

@DavidPeicho
Copy link
Contributor

Maybe let's open another issue for those concerns! I think if we change the current behavior we might break some code for people already expecting it to work in the way it currently is.

@mbredif
Copy link
Contributor Author

mbredif commented Jun 10, 2021

Agreed, let's merge it now and open 2 follow up issues (one for unpacking parameters, notably FLIPY, one for compressed texture support).
@mrdoob, can you remove the changes in the example during the merge or do I have to revert it first ?

@mrdoob
Copy link
Owner

mrdoob commented Jun 10, 2021

@mrdoob, can you remove the changes in the example during the merge or do I have to revert it first ?

@mbredif it'll be cleaner if you do it 🙏

@mbredif mbredif mentioned this pull request Jun 10, 2021
@mrdoob mrdoob changed the title copyTextureToTexture3D when !srcTexture.isDataTexture WebGLRenderer.copyTextureToTexture3D() support Texture and CompressedTexture Jun 10, 2021
@mrdoob mrdoob merged commit 4b22bc5 into mrdoob:dev Jun 10, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jun 10, 2021

Thanks!

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.

3 participants