engine icon indicating copy to clipboard operation
engine copied to clipboard

Texture array and cubemaps single slice updates

Open heretique opened this issue 1 year ago • 8 comments

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.

heretique avatar Jun 13 '24 07:06 heretique

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.

mvaligursky avatar Jun 13 '24 09:06 mvaligursky

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 avatar Jun 13 '24 10:06 heretique

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 avatar Jun 17 '24 15:06 heretique

@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!

heretique avatar Jul 06 '24 09:07 heretique

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.

mvaligursky avatar Jul 09 '24 14:07 mvaligursky

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.

heretique avatar Sep 03 '24 10:09 heretique

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?

mvaligursky avatar Sep 03 '24 10:09 mvaligursky

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

heretique avatar Sep 03 '24 11:09 heretique