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

meshoptimizer: Update to v0.20 (with a reduced patch) #84384

Merged
merged 4 commits into from
Dec 12, 2023
Merged

meshoptimizer: Update to v0.20 (with a reduced patch) #84384

merged 4 commits into from
Dec 12, 2023

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Nov 2, 2023

This change updates meshoptimizer dependency to v0.20 with a small (currently) Godot-specific patch on top.

There's a bunch of performance improvements to various optimization algorithms in this release, but also this release incorporates the first experimental version of attribute-aware simplifier.

Godot was already using a much earlier version of this simplifier (which was implemented by meshoptimizer author, myself, on a branch), so this update catches up and dramatically reduces the patch size on top of meshoptimizer upstream version.

We still need a patch: meshoptimizer v0.20 currently incorporates attribute error into the resulting error returned by the function; Godot had a patch that computes and tracks a distance-based error separately and returns it. This is critical because Godot uses a very large normal weight, and so without this patch the resulting error, which is used for LOD selection, is really large. Fortunately, the new code structure of upstream simplifier makes it much much easier to keep track of this error. I plan to figure this out somehow in a future meshoptimizer version (I was aware of this discrepancy when working on simplifier, but wanted to reconcile how the error is getting tracked so decided to avoid adding this for the time being). This patch was updated in a separate commit to simplify reviewing.

In addition to updating the third-party code, I've also fixed the code that passed normal_weights - we only ever needed 3 values here, but passed a value per vertex - and tweaked the function signature to match the updated one.

Now, the simplifier metric has been adjusted from what Godot used previously. It's difficult to explain fully but in brief, the original Godot patch normalized attribute error in a way that significantly distorted the geometric error. The updated version that is part of v0.20 handles this better although still needs updates; I expect future improvements here to make things better, but even this patch should reduce the geometry deformation due to the very high normal weight here.

Screenshots for comparison on a particular high-poly mesh with forced low LOD that demonstrates the improvement; please ignore the "texture" deformation - this isn't texture, it's vertex colors, and since Godot doesn't pass them to the metric, they get distorted - but the shape & lighting difference should be fairly apparent:

Default LOD settings:

image

LOD bias 0.01, current Godot master:

Screenshot from 2023-11-02 14-56-40

LOD bias 0.01, this PR:

Screenshot from 2023-11-02 14-57-54

As the comment in the importer says, we should probably expose the normal weight as an import parameter :) but that definitely doesn't belong in this PR. Let me know if you'd be up for me following up with that separately.

Note: this change completely overwrites the meshoptimizer library source
(from git SHA c21d3be6ddf627f8ca852ba4b6db9903b0557858)
without including any patches; a distance error metric patch is still
needed and will be reapplied in the next commit.

The changes elsewhere are due to a signature change for
meshopt_simplifyWithAttributes.
This change replicates the distance-only metric patch which is now much
smaller and cleaner, as upstream simplifier already tracks distance and
attribute quadrics separately - it just doesn't store both errors in the
collapse structures.

The old two patches were removed as they are no longer needed.
The weight is per-attribute scalar (X, Y, Z), not per-vertex; this was
the case even before the library update so this appears to be an
oversight.
@zeux zeux requested a review from a team as a code owner November 2, 2023 22:17
@fire fire requested review from a team November 2, 2023 22:30
@fire
Copy link
Member

fire commented Nov 2, 2023

I'll be up for pull requests following exposing the normal weight as an import parameter separately. As feature freeze is in effect, I'll review as much as I can and then we'll aim for 4.3. Also backing porting from 4.3 to 4.2 is a option too.

As far as I know Godot Engine 4.2 release is "soon".

@zeux
Copy link
Contributor Author

zeux commented Nov 2, 2023

fwiw I am not familiar with the release schedule but I don't think this needs to go into 4.2 - because this changes the import results (they aren't 1 to 1 because of metric differences), if the 4.2 release is close it could be better to wait as I think this code hasn't changed much between 4.1 and 4.2.

@akien-mga
Copy link
Member

Thanks a lot for contributing downstream @zeux, that's really valuable! The changes are looking great.

@fire
Copy link
Member

fire commented Nov 2, 2023

Someone asked, so here's a more obvious comparison.

Recording.2023-11-02.163855.mp4

https://imgsli.com/MjE4MDgw

@jcostello
Copy link
Contributor

Someone asked, so here's a more obvious comparison.

What about the polycount of the comparison?

@zeux
Copy link
Contributor Author

zeux commented Nov 3, 2023

What about the polycount of the comparison?

It's noted in the bottom right and is about the same (the number includes ~800 triangles or so used to render the rest of the scene, namely, the grid); in general the new version should produce approximately the same number of triangles when the error is unlimited (which is the case for Godot's usage), the difference will be in which vertices get selected.

@fire
Copy link
Member

fire commented Nov 7, 2023

Since this is a LOD related topic, do you think we should cap the error? If I remember correctly, this decision was chosen so that we would get more lod levels to use.

Trying to debug: #84479 (comment)

Moving this discussion to 84479.

@zeux
Copy link
Contributor Author

zeux commented Nov 8, 2023

Replied on that thread re: skinning, which is a difficult and unrelated problem although after this PR it might be easier to tackle this long term.

I feel like the way error is being treated should probably be reworked:

  • Uncapped error means that the simplifier can never stop before reaching a fairly large deformation, since it doesn't have a non-topological limit
  • While this will generally speaking result in a large geometric error, this means the last LOD might be unusable because in practice it won't be selected; a large-but-not-infinite error may allow for a higher quality last LOD
  • I feel like the error probably had to be uncapped due to the combination of metrics issues (this PR improves that but doesn't fully fix it), and a fairly large weight that skews the combined error pretty significantly. The error limit applies to the combined error, so large weights or metrics issues would limit how far the simplifier can go of course.
  • Because of the metrics issues, the combined error wasn't usable for use in rendering selection (it's still not usable without significantly reducing the normal weight). Because of this, we aren't using attribute error for LOD selection, Because of that, it's difficult to solve visual degradation that is purely attribute based, like the skinning issue in the linked PR, because the LOD level that has unacceptably large visual issues might still get selected

With this PR I tried to mostly reduce the custom changes Godot had, and it won't be possible to cap the error without reconfiguring the simplifier (it would be possible to only cap the geometric error, which would require further Godot-specific patches... this can't really fix skinning issues though so I'm not sure it's a good route). I'm hoping that we can improve the LOD quality in a series of small steps - for example, start from this PR, make normal weight configurable, make the default much smaller, reintegrate attribute error into the LOD selection criteria, make the maximum error configurable, make the default smaller and finite. But in my ideal world that should all happen in small PRs after this - as it stands, it's hard for me to understand the reason for some details here as there's not enough granularity in the commits.

One final caveat is that we're ultimately talking about tuning the metrics, and the metrics are still subject to change. So some steps above I'm fully confident in, but some others may need to be reworked if the metric changes in future releases (I don't have a specific set of metric changes in mind but I'm mildly unsatisifed with the current one and plan to try to rework it).

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Godot Engine 4.2 was released. Let's start testing this in 4.3.

@fire
Copy link
Member

fire commented Dec 7, 2023

Let me check if this updated the copyright hash and version number.

Edited:

Note for the production team to check.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look great to me. Its nice to see us reducing the size of our patches.

Tested locally on the TPS demo and this PR appears to both improve quality and reduce triangle count

Master
Screenshot from 2023-12-07 10-35-31
This PR
Screenshot from 2023-12-07 10-34-05

@YuriSizov
Copy link
Contributor

Seems like we're almost ready, but I would like @akien-mga to check the buildsystem/thirdparty integration part and sign off.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks so much for helping us with this update @zeux!

Could you apply this diff to update our documentation for thirdparty code?

diff --git a/COPYRIGHT.txt b/COPYRIGHT.txt
index fe046863fc..0cbbbc7dec 100644
--- a/COPYRIGHT.txt
+++ b/COPYRIGHT.txt
@@ -325,7 +325,7 @@ License: Apache-2.0
 
 Files: ./thirdparty/meshoptimizer/
 Comment: meshoptimizer
-Copyright: 2016-2022, Arseny Kapoulkine
+Copyright: 2016-2023, Arseny Kapoulkine
 License: Expat
 
 Files: ./thirdparty/minimp3/
diff --git a/thirdparty/README.md b/thirdparty/README.md
index be0693bc34..edf143cad2 100644
--- a/thirdparty/README.md
+++ b/thirdparty/README.md
@@ -501,7 +501,7 @@ File extracted from upstream release tarball:
 ## meshoptimizer
 
 - Upstream: https://github.com/zeux/meshoptimizer
-- Version: git (4a287848fd664ae1c3fc8e5e008560534ceeb526, 2022)
+- Version: git (c21d3be6ddf627f8ca852ba4b6db9903b0557858, 2023)
 - License: MIT
 
 Files extracted from upstream repository:
@@ -509,10 +509,8 @@ Files extracted from upstream repository:
 - All files in `src/`
 - `LICENSE.md`
 
-An [experimental upstream feature](https://github.com/zeux/meshoptimizer/tree/simplify-attr),
-has been backported. On top of that, it was modified to report only distance
-error metrics instead of a combination of distance and attribute errors. Patches
-for both changes can be found in the `patches` directory.
+A patch is included to modify the simplifier to report only distance error
+metrics instead of a combination of distance and attribute errors.
 
 
 ## minimp3
diff --git a/thirdparty/meshoptimizer/LICENSE.md b/thirdparty/meshoptimizer/LICENSE.md
index b673c248b2..962ed41ffb 100644
--- a/thirdparty/meshoptimizer/LICENSE.md
+++ b/thirdparty/meshoptimizer/LICENSE.md
@@ -1,6 +1,6 @@
 MIT License
 
-Copyright (c) 2016-2022 Arseny Kapoulkine
+Copyright (c) 2016-2023 Arseny Kapoulkine
 
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal

(The commit hash is from your v0.20 tag, feel free to adjust in case this is based off another commit.)

@zeux
Copy link
Contributor Author

zeux commented Dec 12, 2023

@akien-mga Thanks! Applied this patch just now. And yeah the base for the patches is v0.20, the hash is correct.

@akien-mga akien-mga merged commit 5352490 into godotengine:master Dec 12, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first direct Godot contribution 🎉
(Far from the first counting your work on both meshoptimizer and volk which we use :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

7 participants