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

Broken model after using instance transform #1261

Closed harrycollin closed 9 months ago

harrycollin commented 9 months ago

Using the instance transform on the linked model results in misplaced objects. First discovered in Node and reproduced on https://gltf.report.

To Reproduce Steps to reproduce the behavior:

  1. Go to Sketchfab Model
  2. Download the model as gLTF (glb)
  3. Go to https://gltf.report
  4. Open the downloaded model
  5. Use the following code and apply to the model
    
    import { prune, dedup, resample, instance } from '@gltf-transform/functions';

await document.transform( // Remove duplicate vertex or texture data, if any. dedup(),

instance(),

// Losslessly resample animation frames.
resample(),

// Remove unused nodes, textures, or other data.
prune(),

);



6: The resulting model looks like this:
![image](https://github.com/donmccurdy/glTF-Transform/assets/10602138/b7bbdcba-20b7-4d7b-97a6-e99df77063de)

**Versions:**
 - Version: 3.5.1
 - Environment: Browser, Nodejs

**Additional context**
This doesn't seem to happen via the CLI for me.
donmccurdy commented 9 months ago

Confirmed, thanks @harrycollin! I'm seeing the same issue in the latest stable version v3.10.

donmccurdy commented 9 months ago

I've been able to reproduce this error in https://gltf.report, but I can't reproduce it in a Node.js script or in the CLI, and it's fairly difficult to debug in the web app. if you're able to observe the error in Node, do you mind sharing how you went about that? Things that might be important:

Thanks!

donmccurdy commented 9 months ago

Note to self — broken version of the file contains no accessors in the instance TRS transforms.

Correct:

    {
      "mesh": 30,
      "extensions": {
        "EXT_mesh_gpu_instancing": {
          "attributes": {
            "TRANSLATION": 0,
            "ROTATION": 1,
            "SCALE": 2
          }
        }
      }
    },

Broken:

        {
            "mesh": 30,
            "extensions": {
                "EXT_mesh_gpu_instancing": {
                    "attributes": {}
                }
            }
        },
harrycollin commented 9 months ago

Thanks for looking at this! I first noticed this in my Node environment. My bundled version of glTF-Transform is higher than that used on https://gltf.report.

Info:

harrycollin commented 9 months ago

if you're able to observe the error in Node

I created a very simple node script but couldn't reproduce the problem. However in my bundled version (info above) the problem still occurs. I can't think of anything else right now that would cause this to happen. I'll keep digging in the meantime.

donmccurdy commented 9 months ago

I have a hunch this might be a case of dual package hazard. Certain build configurations compiling ESM to CJS (for example) can cause two copies of a dependency to be loaded. If the @gltf-transform/functions package imports code from one copy of @gltf-transform/core but is given an input document from another copy, lots of unpredictable bugs can happen, like material instanceof Material returning false.

If that's the case, setting output to ESM in either your ESBuild or TypeScript configuration would likely fix it ... see https://github.com/donmccurdy/glTF-Transform/discussions/857#discussioncomment-5343345 for a similar bug and resolution.

Samsy commented 9 months ago

Actually reporting the same issue

On this model in particular : https://github.com/Samsy/glbs/blob/main/sample.glb

Using the command line CLI ( 3.4.0 ) to instance it results good : https://github.com/Samsy/glbs/blob/main/sample-instanced-good.glb

Screenshot 2024-02-08 at 15 30 28

But using GLTF report script to instance dont show anything, I opened the file on threejs editor, and does not seem to be correct. ( also tested on nodejs packages, no luck )

https://github.com/Samsy/glbs/blob/main/sample-instanced-broken.glb

Screenshot 2024-02-08 at 15 30 50

I also noticed, flatten does not seem to work on GLTF-report scripts

Flatten using cli ( 3.4.0 ) :

Screenshot 2024-02-08 at 15 44 46

Flatten using GLTF report scripts :

Screenshot 2024-02-08 at 15 44 24
donmccurdy commented 9 months ago

Would someone be able to share a Node.js script that reproduces this error, along with the specific bundlers and typescript configurations (if any) used to build and run that script? The script in the top post is fine, but I think the build processes compiling it are creating problems.

I'm fairly confident this has to do with dual package hazard. Presumably I can fix the issue in glTF Report, but if you're running into the same issues in your own code, your build process may also need a configuration change. Targeting ESM (whatever that means in your build system) tends to resolve this problem.

donmccurdy commented 9 months ago

I've found the cause — partial fix in https://github.com/donmccurdy/glTF-Transform/pull/1269, but there are a few pieces of the solution I still need to figure out before merging.

Doesn't look like build processes or dual package hazard were to blame.

donmccurdy commented 9 months ago

Fixed and published to 4.0.0-alpha.6. If you'd like to test that fix, you can use the @next tag, like npm install @gltf-transform/cli@next. glTF Report will be updated after the v4 stable release, which I'm hoping will happen by the end of March.

The issue was related to document.clone() leaving the scene graph in a subtly broken state, so if you need a workaround in v3 in the meantime, avoiding document.clone() should sidestep the issue.

Samsy commented 9 months ago

Thank you very much, I'm going to check this !

harrycollin commented 9 months ago

Thank you @donmccurdy. Sorry I missed your request above for more info. I'm not sure I can avoid using clone(), so I'll test in v4 :D

Samsy commented 9 months ago

I tested the next cli, confirms flatten / dedup / instance work as expected

Is there a way to install the "@next" version with node js ?

donmccurdy commented 9 months ago

@Samsy yes – if you're using the scripting API in a node.js environment you'd probably want:

npm install --save @gltf-transform/core@next @gltf-transform/extensions@next @gltf-transform/functions@next