Prefer ImageBitmapLoader and dispose of textures after GPU upload #21
Conversation
There was a problem hiding this comment.
edit: I can't change ✅ to 💬 but I'd like to because of the question I posed about a possible bug. See comments.
Looks good. May want to remove logging before merge. Nice that ImageBitmapLoader existed for us to take advantage of. Too bad it's unavailable in safari.
src/loaders/TextureLoader.js
Outdated
| texture.onUpdate = function () { | ||
|
|
||
| console.info( "Removing texture", texture.id, url ); | ||
| Cache.remove( cacheKey ); |
There was a problem hiding this comment.
So here we remove the ImageBitmap we stored in the cache because we are going to dispose all of its associated resources. Should we instead just never put it in the cache? Perhaps it doesn't matter, since you can call texture.image.close() and delete texture.image twice without throwing an error, and if we happen to make use of the cache in the window between load and onUpdate that seems good. Do we expect that ever to happen?
There was a problem hiding this comment.
Related question: Is it possible for two textures to share a url to an image and have the following issue:
Texture 1 loads image, puts url: ImageBitmap into the cache.
Texture 2 loads image, pulls from the cache.
Texture 1 is uploaded to the GPU, uses texture.image.data, calls onUpdate to close the image and remove it from the cache.
Texture 2 is uploaded to the GPU, tries to use texture.image.data but fails, because the image was closed.
There was a problem hiding this comment.
Do we ever expect to have to upload a texture to the GPU multiple times?
There was a problem hiding this comment.
Re: "Should we instead just never push it in the cache", yeah probably, did it this way mostly to keep the patch small and localized to avoid future merge issues, but like we discussed (and as you pointed out above) there could be an issue with textures sharing the same Image.
There was a problem hiding this comment.
Resolved this possible race condition by removing the image from the cache righ as onLoad is called. This happens immediately after it is added to the cache so there is no chance for the cached image to get picked up. Might be slightly cleaner to just not add the image to the cache, but again trying to keep the diff small here since I expect this change to move into Hubs once GLTFLoader hooks land.
| @@ -1669,6 +1669,8 @@ THREE.GLTFLoader = ( function () { | |||
|
|
|||
| onLoad( result ); | |||
|
|
|||
| parser.cache.removeAll(); | |||
There was a problem hiding this comment.
The gltf parser also keeps a cache of things while its loading, don't see any reason we would want to keep it around after load, and it was retaining things.
This is built on top for ThreeJS 103 changes. It does 2 independent things that both touch similar code and seem easier to test together:
createImageBitmapis available. Since Firefox does not supportcreateImageBitmapoptions, we also turn off flipY on the textures we create. This is already the default in GLTFLoader but an option also needed to be exposed for the uvs generated in PlaneGeometry and PlaneBufferGeometry since Hubs uses those directly.imagefor a texture will be disposed of after upload to the GPU. This probably warrants a configurable option on the loader eventually, but nothing in Hubs currently cares about the image after the fact. This will likely change if we move our fonts or our 9patch images to actually using a TextureLoader instead of an a-asset-item, since both of these look at the texture's image to get dimensions.There are some discussions in flight in THREE mainline about better ways to configure loaders, so its likely we might change how we are doing this if/when those things land, but this seems like a pretty maintainable patch in the meantime.