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

gltfpack: Merge identical texture objects together #602

Merged
merged 2 commits into from
Sep 25, 2023
Merged

Conversation

zeux
Copy link
Owner

@zeux zeux commented Sep 24, 2023

Blender glTF exporter (and possibly others) output duplicate texture objects in some exports. This blocks material merging which can result in more draw calls than necessary, and in some loaders (such as Three.JS) may result in extra texture decoding during loading and extra memory spent.

For simplicity we still assume that images and samplers in the source document are unique, but deduplicate and remove redundant texture objects now.

The process is a little different from how materials are deduplicated, as we need to maintain remap for unused textures to avoid traversing all texture views again as they are part of a complicated material structure.

Blender glTF exporter (and possibly others) output duplicate texture objects
in some exports. This blocks material merging which can result in more draw
calls than necessary, and in some loaders (such as Three.JS) may result in
extra texture decoding during loading and extra memory spent.

For simplicity we still assume that images in the source document are
unique, but deduplicate and remove redundant texture objects now.

The process is a little different from how materials are deduplicated, as
we need to maintain remap for unused textures to avoid traversing all views
again as they are part of a complicated material structure.
Also include minor refactoring of mergeTextures.
@zeux
Copy link
Owner Author

zeux commented Sep 24, 2023

Looks like there's no memory savings because three.js uses uri+sampler as a cache key, so this only affects material merge optimizations (or other non-three.js renderers?).

@zeux zeux merged commit 65cec2f into master Sep 25, 2023
11 checks passed
@zeux zeux deleted the gltf-texmerge branch September 25, 2023 16:42
@donmccurdy
Copy link
Contributor

@zeux in case it's relevant — I currently merge identical texture objects in glTF Transform, but have been asked not to do that in certain cases where the material uses different parts of the same texture for different purposes, e.g. color vs. normal map. In that situation three.js has difficulty sharing the texture resource while associating it with sRGB and non-sRGB data.

I'm inclined to continue merging textures, and to consider mixed colorspaces a pathological case, but I would be interested if you have any thoughts on what the glTF spec should recommend:

@zeux
Copy link
Owner Author

zeux commented Sep 25, 2023

@donmccurdy Thanks! I forgot to mention this but this was partly motivated by the example from donmccurdy/glTF-Transform#1051 which requires texture merging for efficiency, as skipping it blocked materials from being merged, and as a result significantly limited draw call optimizations:

Input: 19308 draw calls
Before texture object merge: output 17789 draw calls (default options), 4625 draw calls (-mi), 4394 draw calls (-mm)
After texture object merge: output 753 draw calls (default options), 2645 draw calls (-mi), 21 draw calls (-mm)

Before this I didn't realize that Blender exports can contain a lot of duplicate objects; after this I ran this on some glTF-Sample-Assets examples and did find other examples of scenes with duplicate texture objects, e.g. ABeatifulGame (although that scene doesn't get more efficient from the draw call perspective as a result of texture object merging).

I'll need to look at the issue more closely; it's certainly possible to prevent merging when color spaces are different. gltfpack currently infers image color space based on how the image is used, and this information is necessary for compression, so assets with conflicts like this may not get compressed very cleanly.

@donmccurdy
Copy link
Contributor

That all lines up with what I've experienced, yes! Trying to make reasonable choices about a texture associated with multiple color spaces was more than I wanted to deal with though hah - hoping that logging a warning could be enough.

@zeux
Copy link
Owner Author

zeux commented Sep 30, 2023

Yeah I looked at the motivating discussion in glTF-Transform and that asset just feels broken so absent a clear use case with a proper asset it feels like a pathological case that doesn't need to be supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants