Remove mode option from texture.lock() and clarify types in JSDoc for getSource/setSource

[liamdon]

[Updated PR based on discussion below]

Texture.lock accepts a mode option, which can be either TEXTURELOCK_READ or TEXTURELOCK_WRITE. Currently, this option has no effect.

This (revised) PR removes the mode option from texture.lock() and clarifies the JSDoc types that can be passed to texture.getSource and texture.setSource.

[mvaligursky]

If you want to access texture data just for reading, and avoid the overhead of uploading the texture when you unlock, you could simply not call unlock at all. This would be consistent with how VertexBuffer.lock and IndexBuffer.lock works, even though it's not clearly documented. Perhaps we just need to improve the documentation here, and remove the options.mode.

[liamdon]

That makes sense to me. It looks like there is an existing way to get the source data for reading: getSource.

I skipped over this because the docs only mention that it returns an HTMLImageElement, but it would actually return whatever type was set on that level.

I will update this PR (and rename it) with the following changes:

• Remove the mode option from lock/unlock
• Change @param doc on getSource from HTMLImageElement to all the valid return types:

@param {Uint8Array|HTMLCanvasElement|HTMLImageElement|HTMLVideoElement|Uint8Array|HTMLCanvasElement[]|HTMLImageElement[]|HTMLVideoElement[]} source

• Also add Uint8Array to @param of setSource, because raw pixel data is also a valid source.

This should clarify the docs and avoid confusion over how to use lock: it's only if you want to write to the pixel data before uploading to the GPU.

[willeastcott]

@mvaligursky I disagree. The idea of lock and unlock is that they were designed to be called in pairs. Sure, not calling unlock is a hack that happens to work, but it's against the spirit of the original API design. Calling unlock signifies you're 100% done with accessing the contents of the texture. The correct behavior here is to only upload the texture on an unlock only if a lock mode of write was specified.

[liamdon]

@willeastcott If that's the way you want to go, I can bring back that behavior from the first commit. I think the JSDoc changes to setSource/getSource are still useful and accurate additions and could also stay.

While I'm in this file @willeastcott, what do you think about allowing locking for multiple mip levels at once?

For example:

const mip0Data = texture.lock({ level: 0 });
const mip1Data = texture.lock({ level: 1 });
const mip2ReadOnly = texture.lock({ level: 2, mode: TEXTURELOCK_READ });

mip0Data[0] = 1;
mip1Data[0] = 2;

const firstPixel = mip2ReadOnly[0];

// mip0Data and mip1Data are uploaded to GPU and locks are cleared. mip2ReadOnly is not uploaded
texture.unlock(); 

Most users won't need this, but it doesn't affect the existing API described in the docs:

const mip0Data = texture.lock(); // Locks mip 0 by default, writeable by default

mip0Data[0] = 1;

// mip0Data is uploaded to GPU
texture.unlock();

[willeastcott]

Good question. I would probably put something in the docs that you should avoid nesting locks. In reality, it should still work fine if you do. But it does look a bit weird. Like I mentioned before, I feel every lock should really be matched with a 'closing' unlock.

I'd also be interested for @slimbuck to chip in on all this too...

[slimbuck]

Hi @liamdon,

Firstly, sorry for not addressing this PR sooner. The texture lock/unlock and get/setSource API definitely needs work.

I've created an issue so we can discuss the changes we need in https://github.com/playcanvas/engine/issues/4290.

[mvaligursky]

Closing in favour of already merged https://github.com/playcanvas/engine/pull/6003