-
Notifications
You must be signed in to change notification settings - Fork 480
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
Add API to specify which vertices to lock when simplifying #601
Conversation
Two quick questions:
Obviously as an experimental feature this could be adjusted in the future, but ideally I try not to break experimental APIs so it would be good to settle on something that won't need to be adjusted. |
Indeed. In our use case, the mesh is divided up into multiple meshlets and repeatedly joined and simplified. We want the borders of these meshlets to stay intact to avoid visible seams when adjacent meshlets are rendered at different lod levels. However, the border edges of the original mesh can be simplified because they're never shared. So we need to be able to explicitely specify which edges (or vertices) can't be touched.
Well that's definitely a possibility. Our own usecase doesn't have much use for a weight though, so I'll leave that judgement up to you 😉. In terms of interface itself, I would agree that the number of parameters is already huge. Perhaps group them all together into a struct and pass that instead? If you let the struct start with bitflags denoting which properties are filled in, you can add other members later on and still be ABI compatible. |
I was wondering, would a similar feature be able to work on |
It's a completely different algorithm but it should be possible to implement. I think if you change |
@zeux do you think this PR (or some kind of refactored form of this) will be merged? |
Yes, I plan to look into this in the future. This PR currently can't be merged as is - there's API questions that we haven't resolved and it breaks various builds in its current form. |
I'll definitely be up for refactoring this PR if we can flesh out a favorable API. |
This feature looks awesome! I was looking to something similar, thanks @oisyn |
@oisyn out of curiosity did you already tried just playing with the meshopt_SimplifyLockBorder flag working on meshlets and "merged meshlets"? |
@gents83 Yes, the thing is that the original mesh might not be a closed manifold and has borders of its own. It's perfectly fine, even required, that this original border gets simplified along with the rest of the mesh. I only want the actual edges that join the meshlets together to stay intact. With |
@zeux @oisyn any plans to push this forward? I would love to use this for generating hierarchical LODs for my meshlet renderer: bevyengine/bevy#10164, and it would be awesome if the API I needed could be part of upstream. |
Yes I plan to look into this further soon; you can probably use SimplifyLockBorder flag in the meantime (it will restrict simplification of meshes with open borders, but it will be sufficient for correctness). |
Adding link to this discussion, that can bring some ideas using SimplifyLockBorder flag: #569 |
Coming back to this, my proposal would be to adjust the change as follows:
Rationale:
In JS interface we can probably just pass 0 for now to keep tests running. @oisyn let me know if this would work for you and whether you'd be up for adjusting the PR accordingly; alternatively I can also take a look at making this change myself in the next week or so. |
Hi @zeux, yes this would work for me. I'll pick this up right now. |
bb4c7e6
to
2aa04ff
Compare
Ok, I've made the necessary changes
EDIT: hold on, fixing the tests... 😉 |
Might want to allow passing a nullptr for vertex_lock to avoid allocation for cases where users don't want to lock anything. |
@JMS55 That is in fact allowed, but I see I have not specified that in the comments 👍 |
Alright I have fixed all the native tests. As for JS, I implemented it to the best of my abilities. I think the missing part is the WASM binary generation that needs to go in the |
I'm not seeing that checkbox 😬 |
According to this github issue: https://github.com/orgs/community/discussions/5634, it won't work for cross-organizational repos 😐 |
git was built for email patches after all. |
Thanks! All tests appear to pass. |
Thanks! |
Hi,
We are using the
meshoptimizer
to simplify parts of a mesh, but I needed to be able to manually specify which vertices to lock. From #432 I gathered that the ABI ofmesh_simplify
was finalized, so I added amesh_simplifyWithLocks
variant that takes an extra array of vertex indices of vertices to lock. I've also added it tomesh_simplifyWithAttributes
since that one is still experimental.