search

donmccurdy / glTF-Transform

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

Draft: Remove side effects from quantize() #1186

Closed donmccurdy closed 7 months ago

donmccurdy commented 12 months ago

Still a draft. So far this PR tracks which properties have been created or cloned during the quantization process, and considers only those properties for pruning.

We'll still need to consider how to implement deduplication without actually running the whole dedup() transform. Currently the implementation of dedup is fairly complicated — it was written before a.equals(b) was available I think. Maybe it could be simpler by using that. But we need to be pretty careful to not introduce performance regressions for models that may have many thousands of accessors. My rough ideas for next steps would be:

  1. expose separate functions like
    • getDuplicateMaterials(materials)
    • getDuplicateAccessors(accessors)
    • ...
  2. call those functions with all materials/accessors/... in dedup() and only with modified properties in quantize()
  3. clean things up and simplify as much as possible

If anyone is interested in improving dedup() in a separate PR please let me know, I'm not sure yet when I'll get to that.

donmccurdy commented 11 months ago

I think https://github.com/donmccurdy/glTF-Transform/pull/1188 mostly removes the need for this PR, but will take a closer look to see if dedup() can be cleaned up a bit anyway.

donmccurdy commented 7 months ago

See also, https://github.com/donmccurdy/glTF-Transform/pull/1300.