glTF-Transform icon indicating copy to clipboard operation
glTF-Transform copied to clipboard
verified publisher icon donmccurdy

Reorder dedupe property types

Open robertlong opened this issue 3 years ago • 2 comments

By deduping meshes after textures and materials you're more likely to be able to deduplicate more meshes.

robertlong avatar Aug 17 '22 23:08 robertlong

Thanks @robertlong! I think the unit tests for this feature predate the addition of material deduplication (in https://github.com/donmccurdy/glTF-Transform/commit/f69517b8fd30a05ddc728ad5b28c5d59ff16c5c6), and the unit test expects mesh deduplication to fail for two materials that differ only in name:

https://github.com/donmccurdy/glTF-Transform/blob/0ffa610a82594df3394b3f0aed4d2cb7dce5d2bf/packages/functions/test/dedup.test.ts#L82-L100

Do mind updating the test to ensure that (1) meshes sharing materials that differ only in name WILL be deduplicated, and (2) meshes sharing materials that are meaningfully different WILL NOT be deduplicated?

donmccurdy avatar Aug 18 '22 18:08 donmccurdy

Sounds good, I'm also noticing that there's a bug de-duping materials+meshes in Third Room right now so I'm looking into that first

robertlong avatar Aug 19 '22 18:08 robertlong

@robertlong Would it be ok for me to merge this? Or do you think the bug in Third Room is related to this change?

donmccurdy avatar Dec 21 '22 02:12 donmccurdy

The bug was unrelated. This should be safe to merge 👍

robertlong avatar Dec 21 '22 02:12 robertlong

Ok, thank you!

donmccurdy avatar Dec 21 '22 03:12 donmccurdy