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.lock({ level: 0 }): only upload a single mip level if locked explicitly #6004

Open
liamdon opened this issue Jan 27, 2024 · 5 comments
Assignees
Labels
area: graphics Graphics related issue enhancement

Comments

@liamdon
Copy link
Contributor

liamdon commented Jan 27, 2024

I also have some questions about future directions texture.lock() API, which I would be happy to help implement.

I'm asking these in the context of #6003 - I kept that PR deliberately small, but can open a followup PR depending on your thoughts.

Apologies in advance for length!

Questions for PlayCanvas team

Question 1

As mentioned in #5754, it would be good to be able to:

  1. Lock an individual slice of a 2D Texture Array
  2. Lock a specific mip level, and:
  3. Only upload the explicitly locked mip level instead of all of them

I'm excited to contribute an implementation for 1), but first I would like to clarify some issues around 2) and 3).

On 3): it looks to me like we always re-upload all mip levels regardless of whether we locked a specific one.

Q1: Is this correct, or have I missed something important in webgl/webgpu-texture.js?


Question 2

We could check texture._lockedLevel and use it to only upload the specified level when a level option was passed to texture.lock().

For example in webgl-texture.js:

        // Upload all existing mip levels. Initialize 0 mip anyway.
        while (texture._levels[mipLevel] || mipLevel === 0) {

+             // If we have locked a single mip level, only upload that mip
+            if (texture.lockedLevel !== -1 && mipLevel !== texture.lockedLevel) {
+                mipLevel++;
+                continue;
+            }

Q2: Does this seem like a good approach? Or should we be checking / updating texture._levelsUpdated instead?


Question 3

  • Currently, texture.lockedLevel defaults to 0 when no level option is passed.
  • This is because _lockedLevel !== -1 is overloaded to indicate whether a texture is locked.
  • With Don't upload texture if locked using TEXTURELOCK_READ #6003, we can use _lockedMode for this instead, and leave _lockedLevel at -1 if no explicit level was specified.

The default API would have the same behavior as now:

const data = texture.lock(); // Returns mip level 0 data

/** Internally, _lockedLevel remains -1, because user didn't set it explicitly */

texture.unlock(); // Uploads all mipmaps

But when the user specifies mip level 0 explicitly:

const data = texture.lock({ level: 0 }); // Returns mip level 0 data

/** Internally, _lockedLevel is now 0, because user set it explicitly */

texture.unlock(); // Uploads only mip level 0

So for most cases of lock(), the behavior is the same, but it could break expectations in code that is passing { level: 0 } and expecting all mips to update.

Additionally, anyone relying on the private texture._lockedLevel would not longer see it set to 0 when a texture is locked in the default way. But they could switch to the public texture.lockMode instead to check the locked state.

Q3: Is this level of API/behavior change acceptable?

Thank you, and let me know if there is a better venue for implementation questions like this!

@slimbuck
Copy link
Member

Hi @liamdon ,

Sorry it's taken so long to get back to you. This part of the engine sorely needs love and it's great to have you onboard!

  1. Nope you've not missed anything. We should only re-upload the levels that have changed. I believe re-uploading everything (when only the top level changed) is a bug.

  2. Texture upload doesn't happen immediately (check the implementation of texture.upload() - it's just setting dirty flags), but rather it happens on demand at render time (when the texture is about to be used). So a user can update multiple levels of a texture before the actual update is invoked. So tracking the dirty levels would require the use of levelsUpdated. (Also by render/upload time the textures should be unlocked anyway meaning _lockedLevel should always be -1).

  3. Actually I would suggest not trying to keep the old behaviour at all. Re-uploading mipmap data that hasn't changed is just wasted effort since the texture won't look any different anyway. I'd suggest keeping the code as simple and straightforward as possible - when mip level isn't specified by caller, it defaults to 0 and that's the only level that gets re-uploaded at render time.

cc @mvaligursky, who might have some things to add especially from restrictions on the WebGPU side.

@mvaligursky
Copy link
Contributor

  • WebGPU is similar, but batch uploads all dirty textures before rendering in WebGPUGraphicsDevice._uploadDirtyTextures. The main reason here is that mimmap generation is done using render passes, and we cannot do those while inside other rendering (render pass).
  • Another difference on WebGPU implementation is that we create the empty texture right away, and then can upload to it at any time. WebGL does those operations both at a later stage.
  • WebGL upload also has some behaviour where if some mip levels are missing, those are automatically generated. Nothing like this is done on WebGPU (at least at the moment, but I wasn't planning on adding it). On WebGPU, if the user manually uploads mipmaps and some are not specified, the simply contain no data.

@slimbuck
Copy link
Member

Nice, thanks.

Fortunately most of these differences are hidden inside the respective platform implementations (webgl-texture, webgl-device, webgpu-texture etc).

In terms of this public texture API, do you see any potential issues @mvaligursky?

@mvaligursky
Copy link
Contributor

Nope, the API suggestions sound great. Even though it probably not hurt to refactor the WebglTexture class a bit to make it more flexible, before implementing the changes - to make the code easier to follow.

@mvaligursky
Copy link
Contributor

related ticket: #4290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue enhancement
Projects
None yet
Development

No branches or pull requests

3 participants