-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: main
Are you sure you want to change the base?
Conversation
a784bff
to
e60b546
Compare
e60b546
to
29b1ce6
Compare
src/platform/graphics/texture.js
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
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
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. |
Yeah, with regards to mipmaps I played with texture destruction on the alternate branch. 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. |
29b1ce6
to
5b01b5a
Compare
5b01b5a
to
ffed60e
Compare
A small update: |
29fefc0
to
8d1f719
Compare
8d1f719
to
887568c
Compare
887568c
to
b986ced
Compare
b986ced
to
8cbcafd
Compare
8cbcafd
to
50cbd9b
Compare
@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 and how it should look properly: Doing some Spector.js captures I'm also noticing differences between textures bound to Any help is appreciated, thank you! |
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. |
43c5e2a
to
fa85fe1
Compare
b6a950f
to
39bfdd2
Compare
1d012de
to
43b99d0
Compare
I finally managed to figure out the light baking issue. It was fixed with commit 43b99d0. |
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 |
…mebufferTextureLayer
43b99d0
to
bdb2d40
Compare
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:
_dimension
private field to use for identifying the "type" of the texture._type
is already used for something else_slices
field to indicate the number of faces for a cubemap, slices/layers for a texture 2d array, depth for a 3d texturearrayLength
,_volume
,_cubemap
internal fields but based the API getters and setters on the newly introduced_dimension
and_slices
fieldsimmediate
option at texture creation,setSource
andunlock
to force an immediate upload of texture data to GPUsetSource
. 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 uselock
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 usinglock
worked before because_levelsUpdated
was not being set at all and there is a check in thewebgl
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 usinggl.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 reachsetTexture
a part of the GL state is already set, shaders bound, etc. If I do my ownWebglMipmapRenderere.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 thedraw
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.