simplifier: Evaluate attribute error for both edges for seam collapses #801
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When collapsing seam->seam or seam->locked edges, we were assuming that the attribute error on both the primary edge and the seam pair is about the same. There are cases when this is not true; that leads to the collapse with an ostensibly low error significantly changing attribute appearance. To evaluate the error, we need to use the same logic as we use during perform() to discover the seam pair, and use the maximum error of the two for the attribute component.
To get consistent results for seam collapses, we need to accumulate the error instead of using max/avg. The intuition is that if the seam did not exist, we would automatically get the aggregation of errors because we aggregate attribute quadrics; this would result in the error being scaled with the associated triangle area. When the vertices are split along the seam edge, we don't get this aggregation for free and must perform it manually.
Since we have to unfortunately do seam pair discovery in two places again, copy the comments and a memory safety guard so that these two copies align better. performEdgeCollapses still does more validation in debug here. As in performEdgeCollapses, the memory safety guard is not actually hit in practice, but it's not explicitly guarded against anywhere and instead isn't hit through a complex reasoning chain due to how seams and loops are structured. Not ideal, but probably still worth keeping for now.
zeux
force-pushed
the
simplify-seamerr
branch
from
6bbcaf8 to
2b8fe97
Compare
Even if the attributes are the same on either side, the aggregation is necessary to compute properly weighted error; without the previous fix, the reported error here was markedly lower (0.20 vs 0.35).
zeux
force-pushed
the
simplify-seamerr
branch
from
2b8fe97 to
d111658
Compare
This helps fuzz code paths that are only active when we have at least one attribute.
While in many cases most of the edges are bidirectional, some models have a high number of unidirectional edges, and that number only grew with unlocked seam/border->locked collapses. We've been using a branchless error evaluation strategy, where a unidirectional edge is evaluated twice so that the control flow is similar between different edges. However, this does not seem to materially improve runtime for cases when most edges are indeed bidirectional; for models with a high number of unidirectional edges this has a visible negative impact on performance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When collapsing seam->seam or seam->locked edges, we were assuming that the attribute error on both the primary edge and the seam pair is about the same. There are cases when this is not true; that leads to the collapse with an ostensibly low error significantly changing attribute appearance.
To evaluate the error, we need to use the same logic as we use during perform() to discover the seam pair, and use the accumulated error of the two for the attribute component. The earlier notes in the comments suggested max/avg would be appropriate, but accumulation is actually correct: each error is weighted in proportion to the adjacent triangle areas, and if the seam didn't exist at all, these errors would be naturally accumulated through quadrics accumulation. So even on seams where the edges align in their respective attribute deviation, this change improves the "fairness" of seam collapses as it comes to attribute errors.
This fixes issues like this with normal shading on Bistro (LOD selection exaggerated to a ~16px threshold):
Since we evaluate slightly more edges now, this makes simplification of complex models a little slower; on models with few seams, like Buddha, this is neutral on performance; on Bistro it's ~2% slower. This performance delta is mitigated by skipping reverse evaluations for unidirectional edges, as it doesn't look like this is actually helpful - the reduction in branch misprediction is not worth the extra ALU cost.
This contribution is sponsored by Valve.