Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

donmccurdy
Copy link
Owner

@donmccurdy
Copy link
Owner Author

donmccurdy commented Feb 16, 2024

@donmccurdy donmccurdy changed the base branch from chore/torus-test-utils to main February 16, 2024 02:39
@donmccurdy donmccurdy merged commit 30a7f27 into main Feb 16, 2024
6 checks passed
@donmccurdy donmccurdy deleted the fix/simplify-index-count branch February 16, 2024 02:42
@donmccurdy donmccurdy added this to the v4.0 milestone Feb 16, 2024
@donmccurdy donmccurdy added bug Something isn't working package:functions labels Feb 16, 2024
@marwie
Copy link
Contributor

marwie commented Mar 13, 2024

@donmccurdy do you know if this is related and fixes zeux/meshoptimizer#661 ?

@donmccurdy
Copy link
Owner Author

donmccurdy commented Mar 13, 2024

@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
Copy link
Contributor

marwie commented Mar 13, 2024

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 #1136

@donmccurdy
Copy link
Owner Author

donmccurdy commented Mar 13, 2024

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
Copy link
Contributor

marwie commented Mar 14, 2024

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
Copy link
Owner Author

donmccurdy commented Mar 15, 2024

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
Copy link
Contributor

marwie commented Mar 15, 2024

Thanks a lot! Time to update then 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working package:functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify breaks due to assertion failed
2 participants