WebGLTextures: Avoid unnecessary texture uploads.

[Mugen87]

Fixed #17766
Fixed #5821

This is a draft PR that should demonstrate what kind of changes are necessary to avoid unnecessary texture uploads in three.js.

Background

three.js is currently not able to detect shared images in textures. For example when you clone a texture multiple times, three.js is going to create an internal WebGLTexture object for each texture. And that means unnecessary processing and memory allocation.

The Current Solution

I've tried to approach this problem in different ways. This PR represents a change with the least impact on the code base. However, it requires a hack in the THREE.Texture. This hack clearly shows what kind of new logic is required to avoid multiple uploads.

The idea is to add a version property to Texture.image. That makes it possible to determine whether a texture upload is required or not. Besides, WebGLTextures only creates raw WebGL texture objects if necessary now. This is done by introducing a new weak map that maps WebGL texture objects to images. The problem is that a single image can have multiple raw WebGL textures because of different texture properties. Some of these properties are mostly static and closely coupled to the actual texture data (like format, type or encoding). Other parameters like wrapS/wrapT might be different from texture to texture. To accommodate this issue, WebGLTextures now computes a key via getTextureCacheKey() based on the texture properties and uses it to identify raw WebGL texture objects. It's similar to how three.js uses material parameters to reuse existing shader programs.

In any event, the enhancement of Texture is a hack and thus not a clean solution. However, introducing THREE.Image will be a breaking change because the semantics of Texture.image is going to change.

The Better Solution

I think I would rename THREE.Image to something like THREE.TextureImage in order to avoid name conflicts with the native Image ctor. The type of Texture.image would be changed to THREE.TextureImage or a new property introduced (so it's easier to ensure backwards compatibility). Both cases will require refactoring in loaders and serialization/deserialization logic however hacks like this won't be necessary anymore:

three.js/src/textures/Texture.js

Line 186 in 7680fb5

image.uuid = _Math.generateUUID(); // UGH

Note that the same type of hack (pollute objects with custom properties) is used to assign uuids to images. Introducing THREE.TextureImage would avoid this by wrapping the image into a separate object. Something like:

function TextureImage( image ) {

    this.image = image;
    this.version = 0;

}

Object.assign( TextureImage.prototype, {

    toJSON( meta ) {

        // ...

    }

} );

I'm trying to experiment with this approach in the next time to better understand the impact on the code base and if it's possible to introduce TextureImage without breaking code. Fortunately, I can reuse code from this PR, especially the changes in WebGLTextures might be useful.

[Mugen87]

Updated the PR by introducing TextureImage (c934e4d). The code does not pollute raw image objects anymore^^.

I could also think of a more radical change: In such a scenario, ImageLoader and ImageBitmapLoader would already return TextureImage objects. Instead of introducing a new property Texture.textureImage, the existing Texture.image would be changed to type TextureImage. Right now, I think it's a bit "odd" to have both. Besides, it would not be possible to pass raw image objects to Texture's ctor anymore but only TextureImage objects. That would led to a more consistent architecture but also breaking user level and example code en masse. So...this might be something if we could start from scratch^^. IMO, the current state of the PR seems to be a good compromise between avoiding unnecessary texture uploads and an easy to use/consistent API.

[donmccurdy]

@Mugen87 Thank you for working on this!

You claim that your previous solution was a hack — do you see anything wrong with it other than pollution of the Image objects? I'm not opposed to adding a new TextureImage class, but if modifying Image properties was the only problem (I don't see any other issues) there may be an option that could be hidden from the public API:

Instead of using WeakMap<Image, WebglTexture>, what about WeakMap<Image, UploadedTextureCache>, similar to:

uploadedTextureCache = _images.get( image );

uploadedTextureCache[ textureCacheKey ] = {
  uuid: ...,
  version: ...,
  glTexture: ...
};

_images.set( image, uploadedTextureCache );

All else being equal, it might be nice to hide this implementation detail from the public API.

[Mugen87]

do you see anything wrong with it other than pollution of the Image objects?

A new class would be the ideal place for the respective toJSON() logic. The current implementation in Texture is a bit of a hack.

[donmccurdy]

Could you express what makes it seem like a hack? BufferAttribute does not require a separate subclass to represent and serialize its .array property, for example. The additional class is a breaking change, and an increase in user-facing complexity, so I think it would be helpful to justify that more clearly.

[Mugen87]

The additional class is a breaking change,

Can you please explain why it's a breaking change? Since the image property is still available, user level code should not notice the changes of this PR.

Could you express what makes it seem like a hack?

I just think the image-related code perfectly fits into TextureImage. The Texture class can now call TextureImage.toJSON() and does not care about image serialization anymore.

[donmccurdy]

Oops I see. Agreed, it's not a breaking change.

It's still a bit complex; the meanings of Texture, TextureImage, and Image classes are not self-explanatory given the names. Alternative names for TextureImage might improve that:

• GPUImage
• GLImage
• UploadedImage
• InternalImage

^Arguably the word "Image" could be replaced with "Texture" for any of those.

Or the toJSON code could be put in a utility function without requiring a wrapper class. ¯_(ツ)_/¯

[Mugen87]

That sounds everything very reasonable 😊. Let's see what @mrdoob and the others prefer from these options.

[DavidPeicho]

It think the name PixelBuffer / PixelData makes a lot of sense. Textures use a PixelBuffer / PixelData that is uploaded on the GPU. The only bad thing is that it makes a lot of sense for the DataTexture.data field, but sounds weird for a HTML image element.

[stevesan]

+1 to this! I encountered a similar problem when using GLTFLoader with KHR_texture_transform. Although there are other issues to be fixed there, having this PR would be crucial for a proper solution.

[zeux]

Just noting that this is important to fix for KHR_mesh_quantization extension as well since it relies on KHR_texture_transform for providing dequantization data.

[mrdoob]

More ideas on this subject...

Instead of adding a new class under THREE.Texture, we could add a new class on top of it.
We could introduce THREE.Sampler and move the relevant properties from THREE.Texture.

const texture = new THREE.TextureLoader().load( 'foo.png' );
const material = new THREE.MeshBasicMaterial();
material.map = new THREE.Sampler( texture );
material.map.offset.x = 0.5;

One could still do material.map = texture but they would lose the transforms so things wouldn't completely break.

Another name option would be THREE.TextureSampler.

[donmccurdy]

Adding a class on top might also fit better with the node material pattern (like how UVTransformNode works now).

[Mugen87]

Closing. The PR is full of merge conflicts and the THREE.Sampler looks so promising that it's worth giving it a try. If it turns out the approach is problematic for some reasons, we can still introduce THREE.TextureImage to fix the issue.

[mrdoob]

@Mugen87 Would you like to give it a go at implementing THREE.Sampler?

[Mugen87]

Yes 😊 . Already thinking about it^^. It's actually more complex to introduce Sampler above Texture than TextureImage (which only holds the image property) below Texture.

[mrdoob]

Seeing that the THREE.Sampler idea is way more complicated than the approach here...

How about renaming THREE.TextureImage to THREE.ImageSource (which could be followed by THREE.DataSource, THREE.ImageArraySource, ...)?

We can then introduce a new source property to THREE.Texture to transition from the legacy approach.

[mrdoob]

Or, to keep things more open it could just be THREE.Source with uuid and data as properties:

const image = new THREE.ImageLoader().load( 'foo.png' );
const source = new THREE.Source( image ); // source.data = image

const material = new THREE.MeshBasicMaterial();
material.map = new THREE.Texture( source );
material.map.offset.x = 0.5;

Sorry for the back and forth 😅

[Mugen87]

Let me create a new PR with this approach.