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

WebGLTexture: Basic support for gl.texStorage2D(). #22790

Merged
merged 5 commits into from
Nov 16, 2021
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 5, 2021

Related issue: #21874

Description

Introduces the usage of the WebGL 2 API texStorage2D(). Only normal textures are supported right now (so no data, compressed or cube textures). Before rolling out it everywhere I thought we could do it an incremental fashion and thus easier spot potential issues.

The PR helps to mitigate #22758.

@Mugen87 Mugen87 marked this pull request as draft November 5, 2021 16:19
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 5, 2021

It seems the implementation has problems with video textures.

When starting rendering, video textures have a 0 width and height. These are no valid values for texStorage2D(). Chrome reports:

[.WebGL-0x7ffd0c04f600]GL ERROR :GL_INVALID_VALUE : glTexStorage2D: dimensions out of range

Since textures defined via texStorage2D() are immutable, it's not possible to resize the texture at a later point.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 5, 2021

There is also issue when updating existing textures. texStorage2D() is only allowed to run once per texture. When setting Texture.needsUpdate o true multiple times, the engine is not allowed to call texStorage2D() again. Otherwise the following warning is reported:

[.WebGL-0x7ffd11025c00]GL ERROR :GL_INVALID_OPERATION : glTexStorage2D: texture is immutable

@mrdoob mrdoob added this to the r??? milestone Nov 5, 2021
@toji
Copy link
Contributor

toji commented Nov 5, 2021

Second commit with the fixes is more-or-less exactly what I would expect to see from an implementation of this feature. 👍

Does Three allow users to change a texture to an entirely different image after it's been created? As in: with different dimensions or format? If so then the way to handle that with texStorage is to simply delete the old GL texture handle when those mutable properties change and replace it with a new one. Sounds heavyweight, but it's essentially what texImage2D is doing behind the scenes when you reshape the texture.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 6, 2021

Does Three allow users to change a texture to an entirely different image after it's been created?

Technically it's at least possible. In the following fiddle, the initial 1024x1024 texture image is replaced with a 256x256 image: https://jsfiddle.net/ufyrwt5m/

However, I'm not sure if we ever defined some sort of policy about this. E.g. with buffer attributes we have stated that it's not possible to replace or resize the internal array buffer after the creation. We could define something similar for textures.

If we state that certain properties like the image (or at least its dimensions) and other structural data like texture format or type are considered to be immutable after the textures creation (or initial use), it should be easier to adapt texStorage2D(). As a consequence, users would have to create a new instance of Texture if fundamental image properties change.

Personally, I would prefer such a policy since we could benefit from certain assumptions in the engine that makes it easier to implement optimized code like the usage of texStorage().

Besides, why would someone suddenly change the data type or format of a texture? It seems more natural to me to create a new texture object if I want to use a different image.

@toji
Copy link
Contributor

toji commented Nov 6, 2021

Adopting a policy like that would definitely align better with the backend graphics APIs. With WebGPU, for example, all textures have immutable dimensions and formats. I'd encourage it!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 8, 2021

@mrdoob @WestLangley @donmccurdy @sunag Would you be okay if we add the following statement (or something similar) to the Texture documentation?

After the initial use of a texture it's not possible to change its dimensions, format or type. Please create a new texture for these use cases and call dispose() on the previous one.

I would also add this note to the migration guide. Having this policy in place would make it easier to build the engine on top of WebGPU and texStorage2D() (WebGL 2).

@WestLangley
Copy link
Collaborator

Something like

Note: After the initial use of a texture, its dimensions, format, and type cannot be changed. Instead, call dispose() on the texture and instantiate a new one.

@Mugen87 Mugen87 marked this pull request as ready for review November 10, 2021 09:01
@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 11, 2021

Would you be okay if we add the following statement (or something similar) to the Texture documentation?

Would this affect the pattern below?

material.map = textureLoader.load('image.png');
render();

Presumably the texture will be 'rendered' a few times before the image is downloaded and assigned to the texture, is that OK? Note this also applies to compressed textures. Otherwise I think I'm fine with saying that a texture cannot be resized.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 11, 2021

Presumably the texture will be 'rendered' a few times before the image is downloaded and assigned to the texture, is that OK?

Yes, that works. The PR does not affect this behavior.

WebGLTextures only uploads the texture when Texture.needsUpdate has been set at least once and thus the version is greater 0. The line below checks this requirement.

if ( texture.version > 0 && textureProperties.__version !== texture.version ) {

Only then uploadTexture() is executed which contains all the calls of texImage2D() or texStorage2D().

@mrdoob mrdoob modified the milestones: r???, r135 Nov 15, 2021
@mrdoob mrdoob merged commit deb5a9e into mrdoob:dev Nov 16, 2021
@mrdoob
Copy link
Owner

mrdoob commented Nov 16, 2021

Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 17, 2021

Before rolling out it everywhere I thought we could do it an incremental fashion and thus easier spot potential issues.

I suggest to expand the support of texStorage2D() with 136 to include other types of textures like data and cube textures.

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.

5 participants