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

GLTFLoader: Allow textures which share the image source #23420

Merged
merged 2 commits into from
Feb 4, 2022

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Feb 3, 2022

Description

This PR allows GLTFLoader to create textures which share the same image source if multiple textures point to the same texture image source in glTF def.

Benefits:

  • Memory optimization: If multiple textures point to the same texture image source in glTF, texture image source will be shared and duplicated texture image source upload to GPU texture will be avoided.

Changes:

  • Breaking change: Change the second argument of parser.loadTextureImage() from texture source def object to source index. It can break external glTF loader plugins. Source index is needed for caching described below.
  • Add loadImageSource(sourceIndex, loader) to parser which caches textures with source index. If cache misses, it creates a new texture from a source def. If cache hits, it clones the cached texture (so that source is shared).
  • Some minor clean up

Side note:

  • If Texture allows instance creation from source, the code can be more straightforward? Like
const source = await parser.loadImageSource(sourceIndex, loader);
const texture = new Texture(source);
texture set up here...

/cc @donmccurdy

@takahirox takahirox changed the title GLTFLoader: Allow shared image source textures GLTFLoader: Allow textures which share the image source Feb 3, 2022
@takahirox
Copy link
Collaborator Author

Note: Two e2e tests fail

List of failed screenshots: webgl_materials_envmaps_hdr_nodes webgl_materials_envmaps_pmrem_nodes

But they don't use GLTFLoader so I think unrelated.

@takahirox takahirox force-pushed the GLTFLoaderShareImageSource branch from e4ced3b to e4baec9 Compare February 3, 2022 12:32
@takahirox takahirox force-pushed the GLTFLoaderShareImageSource branch from e4baec9 to 7aacf50 Compare February 3, 2022 12:45
@mrdoob mrdoob added this to the r138 milestone Feb 4, 2022
@mrdoob mrdoob merged commit 4e1e295 into mrdoob:dev Feb 4, 2022
@mrdoob
Copy link
Owner

mrdoob commented Feb 4, 2022

Thanks!

@takahirox takahirox deleted the GLTFLoaderShareImageSource branch February 4, 2022 04:27
@takahirox
Copy link
Collaborator Author

Added a note about the breaking change to the migration guide

https://github.com/mrdoob/three.js/wiki/Migration-Guide

Please feel free to rephrase what I wrote because English isn't my first language.

@0b5vr 0b5vr mentioned this pull request Mar 1, 2022
16 tasks
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Mar 1, 2022
joshuaellis pushed a commit to three-types/three-ts-types that referenced this pull request Mar 1, 2022
* change (GLTFLoader): add a new signature GLTFParser.loadImageSource

See: mrdoob/three.js#23420

* docs: remove an obsolete comment
donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* GLTFLoader: Allow shared image source textures

* Fix lint
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.

4 participants