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

Texture array and cubemaps single slice updates #6693

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

heretique
Copy link
Contributor

Hello PC team,
this PR adds single slice updates for texture 2d arrays and cubemaps.

Please note this is a draft, work in progress PR but I wanted to get it out so that I can get feedback early on.
Please also note that I have another branch where I was a little more ambitious with larger internal changes for the textures but that proved to generate more chained changes because there are a lot of internal places where the engine uses the internals ("private" fields) of the textures bypassing the provided API. So after discussions on #4290 I went with the least possible changes, that I'll list bellow:

  • added a _dimension private field to use for identifying the "type" of the texture. _type is already used for something else
  • added a single _slices field to indicate the number of faces for a cubemap, slices/layers for a texture 2d array, depth for a 3d texture
  • removed arrayLength, _volume, _cubemap internal fields but based the API getters and setters on the newly introduced _dimension and _slices fields
  • added an immediate option at texture creation, setSource and unlock to force an immediate upload of texture data to GPU
  • this may be debatable but I removed the possibility of setting any mipmap level from setSource. Given the name and the fact that it is supposed to set the texture source from a "browser interface" it doesn't make much sense, plus in all the codebase only level 0 it is used. This should force users to use lock API which is more low level.
  • lock now allows you to update mip level 0 for single slices of a texture 2d array or cubemap face. Why this limitation? because of the way the _levelsUpdated were used before. In the first place I don't believe updating a single face using lock worked before because _levelsUpdated was not being set at all and there is a check in the webgl implementation that specifically checks for _levelsUpdated[0][face]. I decided to do a similar thing for texture 2 arrays as well and allow setting mip 0 and fixed it as well for cubemap. I believe it makes more sense as if any mip level is missing they still need to be generated using gl.generateMipmaps on WebGL.

This last point brings me to the big issue that I encountered, gl.generateMipmap does a full mipmap chain generation on all types of textures, this may make sense for 2D and 3D textures but not so much for cubemaps and even less for texure 2d arrays. It does make sense at texture creation time, but this last type can contain hundreds of textures and re-generating the full mipmap chain after a single slice upload is wasteful and can block the main loop.
The solution, implement a mipmap renderer for WebGL like the one we have for WebGPU. I already started with that, although not pushed yet because there's a big issue with that. For WebGL most of the texture data is lazily uploaded during main render loop right inside the draw method. By the time we reach setTexture a part of the GL state is already set, shaders bound, etc. If I do my own WebglMipmapRenderere.generate call it will probably mess up the state pretty bad as I need to bind framebuffers, shaders, etc. for rendering the mipmaps. We will probably need to upload dirty textures before entering the draw method and here's where I'm kindly asking for guidance/suggestions on how to do this without breaking anything 😁. I'll update the PR with the mipmaprenderer in the following days when I have some time to polish it.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@@ -91,7 +102,15 @@ class Texture {
* @param {string} [options.name] - The name of the texture. Defaults to null.
* @param {number} [options.width] - The width of the texture in pixels. Defaults to 4.
* @param {number} [options.height] - The height of the texture in pixels. Defaults to 4.
* @param {number} [options.depth] - The number of depth slices in a 3D texture.
* @param {number} [options.slices] - The number of depth slices in a 3D texture, the number of textures
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to design the API to support cubemap arrays for when we get to adding those

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably look at WebGPU API for some inspirations and even naming

https://www.w3.org/TR/webgpu/#dictdef-gputextureviewdescriptor

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a look at the WebGPU API you mentioned but I'd leave the cubemap arrays support for a separate PR. I'm afraid of too many cascading changes if I change too much of the internals. See my other branch I mentioned in the description

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep not suggesting to add a support, just to make sure the changes to the API will support it (slices specifically)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the "slices" to "layers" to make it more consistent with WebGPU naming but also WebGL (framebufferTextureLayer)

@mvaligursky
Copy link
Contributor

With regards to uploading texture during the rendering. I'm very keen to remove this. WebGPU implementation does not do it and uploads dirty textures at the start of the render pass - see _uploadDirtyTextures. Might not be the best option, but works for now. Textures are all created at the start time, but data is uploaded here. There's a problem with this though that sometimes a texture properties are changed during runtime, and a create texture would need to be re-created on WebGPU, but that's not handled yet, we just print warning:

Debug.warn("Texture#mipmaps: mipmap property is currently not allowed to be changed on WebGPU, create the texture appropriately.", this);

So I need to solve this at some point.

WebGL supports this and that must be preserved even if the texture is create ahead of time.

@heretique
Copy link
Contributor Author

heretique commented Jun 13, 2024

Yeah, with regards to mipmaps I played with texture destruction on the alternate branch.
There I introduced the concept of texture operation with a new field _operation and these values where available:
const TEXTURE_OPERATION_NONE = 0;
const TEXTURE_OPERATION_UPLOAD = 1;
const TEXTURE_OPERATION_UPLOAD_PARTIAL = 2;
const TEXTURE_OPERATION_CLEAR_MIPS = 4;

This allowed me to easily separate full data upload vs partial plus clear the mips if previously enabled by destroying the underlaying texture and creating it anew.
I would still leave those as a separate PR to enhance texture API and behavior and only focus on texture 2d and cubemaps partial updates with this PR.

@heretique
Copy link
Contributor Author

A small update:
I moved the texture data upload outside of WebglGraphicsDevice.setTexture to WebglGraphicsDevice._uploadDirtyTextures and it is now called inside WebglGraphicsDevice.startRenderPass taking a hint from how WebGPU does this. This will allow me to properly use the custom mipmap renderer that is going to be used when updating a single texture layer without the fear of affecting current WebGL state or the need to implement a state tracker for rolling back state.
The move does seem to affect only two examples (I've tested almost all of them): "Light Baked" and "Lights Baked AO". The colored lights in both examples are missing. I'm currently investigating the issue.

@heretique
Copy link
Contributor Author

@mvaligursky I'm having a hard time trying to figure out why lightmapping doesn't properly work after I moved the texture GPU uploads to WebglGraphicsDevice._uploadDirtyTextures inside WebglGraphicsDevice.startRenderPass, the lightmapper stops working properly. When texture upload is in WebglGraphicsDevice.setTexture it works fine. So must be some order of initialization thing but I spent a couple of day on it and I couldn't figure it out.
Do you have any suggestion where I should look? I ran the lightmapper step by step but probably I'm overlooking something.
Here's how the example looks with my changes:

image

and how it should look properly:

image

Doing some Spector.js captures I'm also noticing differences between textures bound to texture_lightMap and texture_dirLightMap uniforms:

image

Any help is appreciated, thank you!

@mvaligursky
Copy link
Contributor

No clue what the problem could be. From the screenshot, it seems baking of directional light works, but omni does not work. But not sure what the problem could be.
Maybe try reversing the array of the lights to see if omni works and directional does not.

@heretique
Copy link
Contributor Author

I finally managed to figure out the light baking issue. It was fixed with commit 43b99d0.
Now the thing that remains is to implement a mipmap renderer like the one WebGPU uses, when updating a single or only a few of the textures (layers) of the 2D texture array.

@mvaligursky
Copy link
Contributor

Cool! I think perhaps the part where textures are uploaded before rendering, instead of during rendering, is very standalong, similar to what is done on WebGPU platform. It should perhaps be extracted to a separate PR?

@heretique
Copy link
Contributor Author

Cool! I think perhaps the part where textures are uploaded before rendering, instead of during rendering, is very standalong, similar to what is done on WebGPU platform. It should perhaps be extracted to a separate PR?

Sure, I could try and make it a separate PR. There's a similar standalone one that I extracted from this #6698

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.

2 participants