microsoft / glTF-Toolkit

A collection of tools for modifying and optimizing glTF assets
MIT License
295 stars 41 forks source link

Improve handling of duplicate resources #9

Closed erikdahlstrom closed 6 years ago

erikdahlstrom commented 6 years ago

Given a set of gltf input LODs that have the same materials for each LOD:

  1. the images get packed multiple times (once per GLTF file), even though they could be packed just once if they're identical
  2. the merged GLTF json will contain redundant material information
  3. the exported GLB will as a result embed the same data (textures/images etc) once per LOD level, whereas it should really all share the one set of materials/textures/images

Some possible solutions:

  1. Add a flag to WindowsMRAssetConverter to specify whether the materials are shared between LODs, and if so, make the merger avoid adding new resources.
  2. Detect duplicate resources in the merger by binary comparision, and then resolve/remap all necessary places in the resulting GLTF json. Note that this wouldn't fix the issue with doing packing once per GLTF input file. Pack the images/textures after merging, assuming the merge step has eliminated all duplicates.
robertos commented 6 years ago

I like the duplicate detection, but that could be slow so a flag would be interesting here too. In general finding duplicate resources on a GLTF and simplifying could be a function on the toolkit.

As for performing LOD merging first, we could invert the order, but that also means we would have to perform changes in all affected LODs when packing, which can be a little tricky. I like having LOD merging be the last step.

I think my ideal solution would be to perform packing of LOD zero, then cache some hash of each resource when adding it to the final GLTF, and use these hashes to de-dupe when packing LODs. More specifically:

That way we won't have dupes + we only pack once, at the cost of hashing each file when packing (i.e. some optional param would be the map<resource hash, index of the final resource>.

What do you think @erikdahlstrom ?

erikdahlstrom commented 6 years ago

Sounds good to me. I think the merger needs a bit more work to deal with the case where only some resources are shared, especially wrt getting the right offsets for all the dependent fields in the glTF json.

I have the addition of the commandline flag on a branch, https://github.com/erikdahlstrom/glTF-Toolkit/tree/add_material_flag, I'd be happy to submit it as a pull-request if you want.

robertos commented 6 years ago

Thanks! I think you should send as a PR when we have the hashing described above.

robertos commented 6 years ago

Closed by #14