GLTFExporter: decompress compressed textures using render targets instead of canvas

[Dadibom]

Description

Old texture decompress function left dangling contexts. Since the browser can only have a certain number of contexts, your application will eventually break.

This new approach does not leak contexts.

https://threejs.org/examples/?q=export#misc_exporter_gltf
go here and click export coffeemat three or so times, you will experience context loss

[Mugen87]

First is a fallback when an accessor has count=Infinity (this seems to happen for optimized glbs)

Do you mind sharing such a glTF asset in this PR? In this way, we can import the asset and then try the export with GLTFExporter.

[donmccurdy]

Or if the input is not a glTF file, it would be helpful to have some other way to reproduce the problem here. I don't think either of these issues are expected, so we just need to be sure we understand the problems for which we would be maintaining the code in this fix. :)

[Dadibom]

https://threejs.org/examples/?q=export#misc_exporter_gltf

go here and click export coffeemat three times, you will experience context loss. my PR seems to sometimes work, and sometimes i still get context losses unfortunately.

[Dadibom]

been having tons of issues with package linking which made me run incorrect/"random" versions, so was a bit confused. still haven't found a proper way to deep link three -> engine -> application for development so working with three fixes is insanely slow.

forcecontextloss works every time for me to prevent context leak (which in turn causes real context loss)
unfortunately the decompress function returns empty textures with this fix which is obviously not wanted.
need to find a way to drop the context in TextureUtils.decompress without losing the texture data

[Dadibom]

I haven't had the time to look for a repro case for count=Infinity
However the decompress update is vital when exporting a lot of compressed textures as shown with your example page
Would you like me to revert the count=Infinity change? I need it but at least it would be one less change for me to keep in sync with main.

note that the failed deepscan check is not related to these changes

[LeviPesin]

Could you please split these two fixes into two separate PRs?

[Mugen87]

@LeviPesin Please let the collaborators do this kind of issue and PR management. The first thing that was required here is to be able to reproduce what's going wrong.

[Mugen87]

I haven't had the time to look for a repro case for count=Infinity

@Dadibom Let's focus on this issue first. It would be good to know when and why BufferAttribute.count becomes Infinity in your app.

[Dadibom]

@Mugen87 I am not quite sure and unfortunately i can't share the project
I am doing some skin remapping, perhaps this is related

const newIndices = child.geometry.getAttribute('skinIndex').array.map(i => originalBoneIdMap[partBoneNameMap[i]]);
child.geometry.setAttribute('skinIndex', new Uint16BufferAttribute(newIndices, 4));

[Dadibom]

Note that its not BufferAttribute.count that is Infinity
but the count parameter passed to processAccessor

[Mugen87]

Does your geometries hold group data? AFAICS, this is the only way Infinity can be explicitly passed to processAccessor().

[Dadibom]

Hmm yes I am finding this:

child.geometry.clearGroups();
child.geometry.addGroup(0, Infinity, 0);
child.customDepthMaterial = new MeshDepthMaterial({ depthPacking: RGBADepthPacking });

I don't know exactly why this is done, will investigate further.

[Mugen87]

Using Infinity in addGroup() is actually valid. The entire groups API isn't nice though but still I think we should guarantee that the exporter doesn't break if Infinity is used.

Regarding your particular use case with MeshDepthMaterial, I think you can remove the line since I think it's the same like using no groups at all.

[Mugen87]

Do you mind removing all changes to TextureUtils? Let's focus this PR on just GLTFExporter.

The fix in the exporter looks good, imo!

[Dadibom]

Regarding your particular use case with MeshDepthMaterial, I think you can remove the line since I think it's the same like using no groups at all.

I think there was some reason to use customDepthMaterial but i don't remember, however the group is added because we use multiple materials/layers

[Mugen87]

however the group is added because we use multiple materials/layers

A single group does not add multi material support. You would need something like this:

child.geometry.addGroup(0, Infinity, 0);
child.geometry.addGroup(0, Infinity, 1);

[Dadibom]

A single group does not add multi material support. You would need something like this:

Yes, the other materials (and groups) are added in later stages :)

[Mugen87]

Instead of using the texture of the render target, do you mind creating a new instance of Texture instead?

I'm not sure who else is going to use the decompress() in the future and it seems more safe to use a fresh texture object instead of a render target texture.

[Dadibom]

Why create a new texture? WebGLRenderTarget is not reused and I'm gussing it already does new Texture internally?

[Mugen87]

Why create a new texture?

Textures of render targets have a flag that indicate they belong to render targets. When using these textures for rendering, they are treated differently by the renderer. Instead of setting the flag to a different value, it's more clear to just create a new instance.

[Dadibom]

Also note these lines:

renderTarget.texture.image = imageData;
renderTarget.dispose();

this works because the exporter doesnt actually check the texture data, only texture.image. if you tried to render the texture it wouldn't be visible as it is already disposed.

however the exporter itself does not dispose textures it creates with decompress so for now this is required

[LeviPesin]

Suggested change

	colorSpace: IS_SRGB ? SRGBColorSpace : LinearSRGBColorSpace,
	colorSpace: texture.colorSpace,

Wouldn't this be simpler?

[donmccurdy]

Agreed, if it works! If not we should probably include a comment explaining why texture.colorSpace is not used...

The texture for conversion may be non-color, using NoColorspace. Somewhat related:

• Roadmap for supporting wide-gamut color workflows #26479 (comment)

@WestLangley should we consider NoColorSpace a valid color space for a render target? I think the answer is probably yes, but I'm not sure whether it's fully supported now.

[WestLangley]

@donmccurdy I think this is an issue to be debated elsewhere, but FWIW...

1. One can render non-color data to a render target.

2. I don't think we should "honor" the color space of a render target when rendering to it. The color space is relevant only when reading from the render target.

[Mugen87]

Closing in favor of #27136 and #27137.