donmccurdy / glTF-Transform

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

Simplify breaks due to assertion failed #1263

Closed wayne-wu closed 4 months ago

wayne-wu commented 4 months ago

Describe the bug

I'm running into a bug where gltf-transform simplify from the command line breaks with an assertion failed error.

Upon closer examination, it looks like this error is coming from this line here (from the meshopt simplify function): https://github.com/zeux/meshoptimizer/blob/master/js/meshopt_simplifier.js#L178 , where targetCount > indices.Length after one iteration therefore failing the assertion.

One thing I did notice from the example of simplify here: https://github.com/zeux/meshoptimizer/blob/master/demo/simplify.html -- it looks like the targetCount is calculated based on index count and not the vertex position count as used by gltf-transform. I'm wondering if this is intended, and whether that relates to the bug?

To Reproduce I'm unable to share the model I was testing with, but I'll see if I can recreate the bug with another model.

Versions:

Thanks!

donmccurdy commented 4 months ago

Thanks @wayne-wu! Index count vs. vertex count sounds like a good guess on the cause. Perhaps happening when the source vertex attributes have many vertices, but the indices only use a smaller subset of those. The relevant code is here:

https://github.com/donmccurdy/glTF-Transform/blob/3901a3e178dabba16b1f6d73f7de5d20730065b1/packages/functions/src/simplify.ts#L162-L170

I'll look into reproducing the issue without the model, but if you'd like to try making the change and then testing on the model yourself, it would look something like:

yarn install
yarn dist
node packages/cli/bin/cli.js simplify in.glb out.glb

Or yarn watch will continuously watch and rebuild the code as you're making changes.

donmccurdy commented 4 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.

harrycollin commented 2 months ago

I encountered this using @gltf-transform/functions@4.0.0-alpha.15 package in node.js.

donmccurdy commented 2 months ago

@harrycollin possible to share a reproduction? The tests I have are passing. Meshoptimizer may have assertions for any number of things.

harrycollin commented 2 months ago

My mistake! It looks like I was passing undefined values to the ratio, error and lockBorder options.

donmccurdy commented 2 months ago

Ah, well that's a footgun! Should be fixed with: