donmccurdy / glTF-Transform

glTF 2.0 SDK for JavaScript and TypeScript, on Web and Node.js.
https://gltf-transform.dev
MIT License
1.37k stars 149 forks source link

Duplicate image URIs are blank #1513

Open aaaidan opened 1 day ago

aaaidan commented 1 day ago

Hi! First time caller. glTF-Transform is just wonderful. But i have found what seems like it might be a bug.

Describe the bug

Since 4.0.9, when loading a GLTF with textures that use duplicate URIs, all textures (except the first duplicate) parse the URI as an empty string.

It looks like this is intended behavior in 4.0.9, but it has "bug-like" effects for our client code.

To Reproduce Test cases demonstrate the behavior.

Expected behavior Textures with duplicate URIs present the URI as it appears in the gltf data, rather than an empty string.

Versions:

Additional context

I'm bringing this issue for discussion knowing only the "client" side of the story, and not fully understanding why this change may be necessary. I sheepishly, but fully, agree that duplicate URIs are bad form, at best. But as you clarified duplicate URIs are technically valid. I suggest that the API should, if possible, present the duplicates "as they exist" in the gltf data, because I think that's the least surprising way to handle this strange edge case.

For our pipeline, 4.0.9 is a breaking change from 4.0.8. One of our scripts processes all textures serially, updating each texture (data, URI, and type) based on the existing URI, regardless of its possible duplicate status. This means duplicates are processed multiple times, which is wasteful but otherwise harmless: duplicate URI textures get identical processing. In 4.0.8, when the transformed GLTF is written out, it (presumably) writes out to the same file multiple times for duplicate textures. Inelegant, but it works.

In 4.0.9, the duplicate URIs appear as blank strings, the script doesn't run the required process on that texture, leading to only a partially processed gltf.

Should we be doing it like this? Absolutely not, I'm honestly I little embarrassed to admit this breaks our code, and we'll fix it. We should be deduplicating these textures, or just making sure the input data is free of duplicates in the first place.

I raise this issue for a few reasons:

donmccurdy commented 1 day ago

Hi @aaaidan, and thanks for the report! In any case — it sounds like #1511 should not have gone out in a patch release, I've reverted that in https://github.com/donmccurdy/glTF-Transform/commit/84e77f58d321702dba486fc07ed1ce7279d4897d and published as v4.0.10. I'll think a bit more about the questions and what to do next.

aaaidan commented 1 day ago

Amazing, thank you! (That's a wild turnaround, by the way. 😅)

It won't surprise you, but the dedup transform was super easy to add and solved our compatibility with 4.0.9.