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

fix(simplify): Correct usage of 'target_index_count' #1268

Closed donmccurdy closed 9 months ago

donmccurdy commented 9 months ago
donmccurdy commented 9 months ago

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

marwie commented 8 months ago

@donmccurdy do you know if this is related and fixes https://github.com/zeux/meshoptimizer/issues/661 ?

donmccurdy commented 8 months ago

@marwie very likely this would fix the discrepancy in the ratio (50% vs 90%), yes. I think it is unrelated to the error metric, though. This is published to v4 under the @next tag on npm.

marwie commented 8 months ago

Thank you. I've updated to latest 3 release today but perhaps it's worth updating to 4 already then. From the roadmap it seems like there are no breaking changes https://github.com/donmccurdy/glTF-Transform/issues/1136

donmccurdy commented 8 months ago

I haven't written the changelog for v4 yet, but so far the breaking changes are minimal. Some options renamed, or with different default values. If you're using TypeScript, it should catch things. The larger breaking change in v4 will be #1141, which requires some changes to extension implementations. See the changes to EXT_mesh_gpu_instancing in that PR, for an example.

This fix could be patched to a v3.10.x release safely, if that's better.

marwie commented 8 months ago

If you can patch 3 that would be great. Simplified meshes look much better with this I'd love to make a release with a first version of automatic LOD generation without having to worry breaking other parts of the pipeline because I didnt test enough.

Is 1141 already part of the current alpha.10 version?

I tested with 4 alpha.10 just now - on first sight it seems to be OK but I'd need to test more in other projects to be confident.

donmccurdy commented 8 months ago

Published to published v3.10.1. ✅

1141 is not on the alpha release yet — everything on the alpha release has already been merged to the main branch. See https://github.com/donmccurdy/glTF-Transform/milestone/28 for the full list of what's in the alpha vs. still planned. It's entirely possible that I'll have some 'broken' releases go out on the alpha before it's stable, so if you did switch to that early, I'd recommend pinning the version rather than using a ^ prefix.

marwie commented 8 months ago

Thanks a lot! Time to update then 😎