donmccurdy / glTF-Transform

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

Running 'instance()' creates incorrect transforms on some nodes #1505

Closed donmccurdy closed 2 months ago

donmccurdy commented 2 months ago

Describe the bug

Originally reported at https://github.com/pmndrs/gltfjsx/issues/277, running optimize on the attached models causes the window panels to be rotated 90º and out of position. Disabling instancing with --no-instance prevents the issue, which suggests that instance() may have a bug.

https://github.com/user-attachments/files/17029993/venice-transformed.zip

To Reproduce

# ❌ broken
gltf-transform optimize venice.glb --no-compress

# ✅ ok
gltf-transform optimize venice.glb --no-compress --no-instance

Expected behavior

Visual consistency in the instanced model.

Versions:

zeux commented 2 months ago

Likely unrelated to the transform issue, but also want to note that in the transformed glb, at least one node (Window.021.0) has no mesh but has a EXT_mesh_gpu_instancing extension with some number of instances. I flagged this in https://github.com/KhronosGroup/glTF/pull/2404 but that PR did not reach a resolution so I still don't know if this is supposed to be valid or not. At the very least it seems non-optimal wrt output file size.

donmccurdy commented 2 months ago

Thanks @zeux! It was indeed related. The original file already used EXT_mesh_gpu_instancing, and the implementation of the instance() function was not prepared for that, detaching the existing meshes but leaving EXT_mesh_gpu_instancing extensions behind. Will be fixed by #1507.