Skip to content

Deduplicate images, samplers, textures - #48

Merged
EliCDavis merged 5 commits into
EliCDavis:mainfrom
alexykot:feature/dedupe-images-samplers-textures
Dec 16, 2024
Merged

Deduplicate images, samplers, textures#48
EliCDavis merged 5 commits into
EliCDavis:mainfrom
alexykot:feature/dedupe-images-samplers-textures

Conversation

@alexykot

Copy link
Copy Markdown
Contributor

This PR adds logic to writer.AddTexture() method to check and reuse existing resources if they are identical to newly added, instead of adding more.

Images are compared by URIs only, since byte buffer embedding is not supported yet.
Samplers are compared by four numeric values they have.
Textures are compared by source (image) and sampler indices, and by the extensions map.

ChildOfRootProperty values are not checked for any objects.


This change of behaviour required minor changes in the extension tests - the textures in the extensions now need to be actually different so they are given different indices. An empty texture is identical to any other empty texture, so I made them non-empty.

@alexykot alexykot mentioned this pull request Dec 13, 2024
Comment thread formats/gltf/writer.go Outdated
Comment thread formats/gltf/writer.go Outdated
Comment thread formats/gltf/writer.go Outdated
@alexykot
alexykot force-pushed the feature/dedupe-images-samplers-textures branch from a915d11 to eb3e9fd Compare December 16, 2024 16:21
@alexykot
alexykot requested a review from EliCDavis December 16, 2024 16:25
@EliCDavis
EliCDavis merged commit 4ab31c8 into EliCDavis:main Dec 16, 2024
@alexykot
alexykot deleted the feature/dedupe-images-samplers-textures branch December 16, 2024 20:35
@alexykot

Copy link
Copy Markdown
Contributor Author

Hmm, nope, using pointers here for texture dedupe was a bad idea.

Effectively, what was done here in the earlier version of the PR - I now have to do on the client side. have to deduplicate the textures myself before sending them to polyform.

I think this using pointers is a wrong approach in principle. This approach actually means no real deduplication within the library, because if the pointers are pointing to the same object - deduplication was already done by the client. So the heavy lifting was done, and the all that library is doing is not screwing it all up.

I think the library should in fact do this heavy lifting and do real deduplication - object comparison and reuse the same object index in GLTF if the values are the same, even if the actual instances in memory are different.

With meshes the approach was different purely because of technical limitations - comparing large meshes directly by vector attribute values is prohibitively expensive. So that was pushed onto the client where the logic and metadata around the scene creation allow to avoid direct comparisons.

For all other objects - comparison is not technically difficult and should be done by the library. I'll make another PR.

kalebpace pushed a commit to kalebpace/polyform that referenced this pull request Jan 20, 2025
* Deduplicate images, samplers and textures when adding a tex

* Fix tests

* Address comments

* Change test to check for no-empty-sampler state

* Rollback unnecessary changes
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.

2 participants