-
-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Update meshoptimizer to 0.22 #98529
Update meshoptimizer to 0.22 #98529
Conversation
For posterity, summary of future work that I think would be valuable; I want to try to get to some of this this year but no promises...
|
Curious question, does this pr resolve the outstanding issues with animation bone weights? |
See also #94108 where the merge angle is currently hardcoded to 60 degrees (matching the 3D scene import default). It could be exposed as an option still. What would be a more reasonable default? Intuitively, I would say either 45 degrees or even 44 degrees (to keep 45-degree angles intact). |
No - as the simplifier is not aware of the bone influences, it will not make different decisions. Ideally, fixing that issue is predicated on a) use of combined distance+attribute metric (see above), b) actually giving simplifier some sort of bone influence data like we discussed. Without (b), the simplifier is still going to collapse side edges of a cylinder as it just doesn't know anything about bone structure. With (b), it will be able to produce intermediate LOD levels that preserve this deformation, but crucially all of them will have very small positional error (and high attribute error!), so unless the renderer uses the combined error, the LOD switches will still break the mesh visually under deformation. |
Yeah I was thinking something like 30-40 degrees but this would need to be experimented with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are split into three sections:
- Update mesh optimizer
- Update patch
- Update godot interface.
The changes seem to be upstream changes to mesh optimizer.
The interface changes seem minor.
The photos given by zeux show improvement. Haven't spent time trying to antagonize the simplifier, so that's a good research question if it degrades anywhere.
Looks good to me considering the changes to the godot engine are interface changes. We're following major versions of mesh optimizer so any bugs in the upstream should be filed there first.
Our policy is to merge the commits into one to be ready for the merge. Thanks!
The Godot-specific patch is just a single line now; removing this patch will likely require adjusting Godot importer code to handle error limits better. This also adds new SIMPLIFY_ options; Godot is currently not using any of these but might use SIMPLIFY_PRUNE and SIMPLIFY_SPARSE in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I'm all for trimming down the patches and getting things closer to upstream as much as we can.
Thank you! |
This change updates meshoptimizer to 0.22 (from version 0.20 that Godot was using). This mostly affects simplification as other components Godot is using are unchanged.
Simplifier has been significantly improved over the last two releases; attribute weighting is now more consistent and works better with discontinuities, a class of topological restrictions has been lifted which for meshes with complex topology results in higher quality, smaller triangle count, or sometimes both; a set of new options allow to use some extra features in the future.
Godot is currently using a patch on top of meshoptimizer 0.20 that reports distance-only error from
meshopt_simplifyWithAttributes
instead of combined distance+attribute error. This PR reproduces this patch (which is now just a single line due to changes in the internal code structure); my goal over the meshoptimizer 0.23 timeframe is to either rework Godot LOD generation to not need this patch (see below), or if that doesn't work add an official option to no longer require a patch. To make individual commits in this PR cleaner, the patch is reintroduced in a separate commit.In general, simplifier behavior should be expected to be different. It's usually producing fewer triangles at better quality now, but because of this sometimes it's hard to compare 1-1 (e.g. previously the simplifier could be stuck at a higher triangle count, so comparing the lowest LOD is not always 1-1 - the new LODs could look "worse" if you just force the lowest to display). Simplifier changes have been tested extensively during development but I've looked at a couple assets to compare, screenshots below (left = godot master, right = this PR - all screenshots include triangle count as well, and all screenshots use aggressive LOD bias to make the LODs show up at a much closer distance).
I've also looked at the "800 models" scene from an earlier Godot issue; this PR doesn't significantly change the quality there at the same triangle count but manages to generate fewer triangles at lowest LOD level. In general, sometimes this PR will generate an extra level of detail as the simplifier can proceed further, and as such the import time might be a tiny bit longer -- as we discussed previously, the current flow for LOD generation in the importer is bad for import performance. I intend to rework this; the newly added
SIMPLIFY_SPARSE
option will also be helpful here.Note that this PR tries to limit the number of code changes to absolute minimum. In particular, the new
PRUNE
option is likely to be helpful for some assets and should improve the LOD quality for complex assets as it will reject small details that the simplifier might not be able to remove currently - but this can be investigated and enabled separately.Some of the attribute metric changes in this release result in a slightly different behavior depending on the triangle sizes and normal deviation. In general, this may require increasing the normal weight when it was previously small (below 1). Since Godot is already using normal weight 2.0, and the attribute changes are mostly tested on normals with weight range of 0.5-2, for now I've kept the weight as is. If we get any reports about regressions in normal quality, we can bump the weight up to mitigate this.
The Godot patch to the metric is generally problematic: on assets with significant normal variance after simplification, the simplifier is currently not really restricted from making the normal quality much worse (error limit is FLT_MAX and resulting attribute error is ignored for the purposes of LOD selection). Many of the updates to attribute metric in this release are motivated to fix this and make the combined distance+attribute metric useful for LOD selection. However for this to work, the simplification should be done with a finite error limit (to avoid getting LODs that have an abnormally high limit and will never be selected; in current Godot master, and after this PR, these LODs may still be selected even though the simplifier knows the normal error is too high). It's because of this that I'd like to rework the simplifier integration to remove the need for the patch, but this is not done in this PR to make this one more compatible and easier to review. This also may require changing the default normal merge limit - Godot currently uses 60 degree cutoff to merge normal vertices, which is really not great for normal quality on meshes with creases. So I'd want to update that default to be much smaller, but again - not part of this PR. Just mentioning all of this here so that future work is more obviously outlined.
Some comparison screenshots (only including ones where there's some difference between quality and triangle counts, I've omitted a bunch of screenshots where behavior is basically similar).