From 20536cf8c836601e5f1da37dca70041f29821730 Mon Sep 17 00:00:00 2001 From: Arseny Kapoulkine Date: Mon, 19 Aug 2024 12:32:21 -0700 Subject: [PATCH 1/5] demo: Add a test for simplification along attribute seam This test is intended mostly to track future improvements to the error behavior along the seam boundary. Right now, attribute error is not correctly computed along the seam boundary which prevents some collapses and can result in artificially inflated resulting error. We also seem to have some issues with positional errors for collapses along the seam which the test also demonstrates. --- demo/tests.cpp | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/demo/tests.cpp b/demo/tests.cpp index 995b032bb7..ddca880dfa 100644 --- a/demo/tests.cpp +++ b/demo/tests.cpp @@ -1338,6 +1338,78 @@ static void simplifyErrorAbsolute() assert(fabsf(error - 0.85f) < 0.01f); } +static void simplifySeam() +{ + // xyz+attr + float vb[] = { + 0, 0, 0, 0, + 0, 1, 0, 0, + 0, 1, 0, 1, + 0, 2, 0, 1, + 1, 0, 0, 0, + 1, 1, 1, 0, + 1, 1, 1, 1, + 1, 2, 0, 1, + 2, 0, 0, 0, + 2, 1, 0, 0, + 2, 1, 0, 1, + 2, 2, 0, 1, + 3, 0, 0, 0, + 3, 1, 0, 0, + 3, 1, 0, 1, + 3, 2, 0, 1, // clang-format :-/ + }; + + // 0 1-2 3 + // 4 5-6 7 + // 8 9-10 11 + // 12 13-14 15 + + unsigned int ib[] = { + 0, 1, 4, + 4, 1, 5, + 2, 3, 6, + 6, 3, 7, + 4, 5, 8, + 8, 5, 9, + 6, 7, 10, + 10, 7, 11, + 8, 9, 12, + 12, 9, 13, + 10, 11, 14, + 14, 11, 15, // clang-format :-/ + }; + + // note: vertices 1-2 and 13-14 are classified as locked, because they are on a seam & a border + // since seam->locked collapses are restriced, we only get to 3 triangles on each side as the seam is simplified to 3 vertices + + // so we get this structure initially, and then one of the internal seam vertices is collapsed to the other one: + // 0 1-2 3 + // 5-6 + // 9-10 + // 12 13-14 15 + unsigned int expected[] = { + 0, 1, 5, + 2, 3, 6, + 0, 5, 12, + 12, 5, 13, + 6, 3, 14, + 14, 3, 15, // clang-format :-/ + }; + + unsigned int res[36]; + float error = 0.f; + + assert(meshopt_simplify(res, ib, 36, vb, 16, 16, 18, 1.f, 0, &error) == 18); + assert(memcmp(res, expected, sizeof(expected)) == 0); + assert(fabsf(error - 0.22f) < 0.01f); // TODO: this is higher than normal due to border errors? + + float aw = 1; + assert(meshopt_simplifyWithAttributes(res, ib, 36, vb, 16, 16, vb + 3, 16, &aw, 1, NULL, 18, 2.f, 0, &error) == 18); + assert(memcmp(res, expected, sizeof(expected)) == 0); + assert(fabsf(error - 0.49f) < 0.01f); // TODO: this is higher than normal due to merged attributes? +} + static void adjacency() { // 0 1/4 @@ -1556,6 +1628,7 @@ void runTests() simplifyLockFlags(); simplifySparse(); simplifyErrorAbsolute(); + simplifySeam(); adjacency(); tessellation(); From e1e41147ca205077e1963f331b47bf6e01c8cba6 Mon Sep 17 00:00:00 2001 From: Arseny Kapoulkine Date: Mon, 19 Aug 2024 14:25:17 -0700 Subject: [PATCH 2/5] simplify: Add a more verbose tracing mode for edge collapses This is too noisy for any reasonably sized model but it helps debug small isolated test cases and unit tests. Ideally we would probably remove the 'edge eval' (as in a given pass we may evaluate way more edges than we end up collapsing) but for now since we are not storing position/attribute errors separately that's the only place where we can output both errors as well as evaluate bidirectional flips. --- src/simplifier.cpp | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/src/simplifier.cpp b/src/simplifier.cpp index a7264ff7cf..30b26a973f 100644 --- a/src/simplifier.cpp +++ b/src/simplifier.cpp @@ -914,7 +914,13 @@ static bool hasTriangleFlips(const EdgeAdjacency& adjacency, const Vector3* vert // early-out when at least one triangle flips due to a collapse if (hasTriangleFlip(vertex_positions[a], vertex_positions[b], v0, v1)) + { +#if TRACE >= 2 + printf("edge block %d -> %d: flip welded %d %d %d\n", i0, i1, a, i0, b); +#endif + return true; + } } return false; @@ -1017,6 +1023,10 @@ static void rankEdgeCollapses(Collapse* collapses, size_t collapse_count, const float ei = quadricError(vertex_quadrics[remap[i0]], vertex_positions[i1]); float ej = quadricError(vertex_quadrics[remap[j0]], vertex_positions[j1]); +#if TRACE >= 2 + float di = ei, dj = ej; +#endif + if (attribute_count) { ei += quadricError(attribute_quadrics[remap[i0]], &attribute_gradients[remap[i0] * attribute_count], attribute_count, vertex_positions[i1], &vertex_attributes[i1 * attribute_count]); @@ -1027,6 +1037,16 @@ static void rankEdgeCollapses(Collapse* collapses, size_t collapse_count, const c.v0 = ei <= ej ? i0 : j0; c.v1 = ei <= ej ? i1 : j1; c.error = ei <= ej ? ei : ej; + +#if TRACE >= 2 + if (i0 == j0) // c.bidi has been overwritten + printf("edge eval %d -> %d: error %f (pos %f, attr %f)\n", c.v0, c.v1, + sqrtf(c.error), sqrtf(ei <= ej ? di : dj), sqrtf(ei <= ej ? ei - di : ej - dj)); + else + printf("edge eval %d -> %d: error %f (pos %f, attr %f); reverse %f (pos %f, attr %f)\n", c.v0, c.v1, + sqrtf(ei <= ej ? ei : ej), sqrtf(ei <= ej ? di : dj), sqrtf(ei <= ej ? ei - di : ej - dj), + sqrtf(ei <= ej ? ej : ei), sqrtf(ei <= ej ? dj : di), sqrtf(ei <= ej ? ej - dj : ei - di)); +#endif } } @@ -1126,6 +1146,10 @@ static size_t performEdgeCollapses(unsigned int* collapse_remap, unsigned char* continue; } +#if TRACE >= 2 + printf("edge commit %d -> %d: kind %d->%d, error %f\n", i0, i1, vertex_kind[i0], vertex_kind[i1], sqrtf(c.error)); +#endif + assert(collapse_remap[r0] == r0); assert(collapse_remap[r1] == r1); @@ -1673,6 +1697,10 @@ size_t meshopt_simplifyEdge(unsigned int* destination, const unsigned int* indic if (edge_collapse_count == 0) break; +#if TRACE + printf("pass %d:%c", int(pass_count++), TRACE >= 2 ? '\n' : ' '); +#endif + rankEdgeCollapses(edge_collapses, edge_collapse_count, vertex_positions, vertex_attributes, vertex_quadrics, attribute_quadrics, attribute_gradients, attribute_count, remap); sortEdgeCollapses(collapse_order, edge_collapses, edge_collapse_count); @@ -1684,10 +1712,6 @@ size_t meshopt_simplifyEdge(unsigned int* destination, const unsigned int* indic memset(collapse_locked, 0, vertex_count); -#if TRACE - printf("pass %d: ", int(pass_count++)); -#endif - size_t collapses = performEdgeCollapses(collapse_remap, collapse_locked, vertex_quadrics, attribute_quadrics, attribute_gradients, attribute_count, edge_collapses, edge_collapse_count, collapse_order, remap, wedge, vertex_kind, vertex_positions, adjacency, triangle_collapse_goal, error_limit, result_error); // no edges can be collapsed any more due to hitting the error limit or triangle collapse limit From 52295951d1fed06e10ad2eaa5f26a68ad9e4e404 Mon Sep 17 00:00:00 2001 From: Arseny Kapoulkine Date: Mon, 19 Aug 2024 17:13:58 -0700 Subject: [PATCH 3/5] simplify: Separate attribute quadrics on attribute discontinuities Initial implementation of attribute metric used the same indexing that we use for positional quadrics, where multiple vertices with the same position are collapsed into a single element. This is correct and optimal for positions, but results in problems for attributes when the vertex is on a seam: two associated vertices carry very different attributes, and evaluating any attribute against a vertex from an opposite side of the seam results in a large error. This results in a suboptimal collapse sequence and an artificially inflated resulting error. This was originally difficult to correct since attributes and positions shared the same quadrics; because we need to separate these anyhow as the treatment wrt normalization and scaling is different, we can now fix this which mostly involves carefully indexing the attributes with the correct, non-remapped index. When performing a collapse, it's important to correctly aggregate quadrics for all edges that are being collapsed to maintain correct error for future collapses. This change does this for seam vertices (which is the main use case); for now it's unclear what the correct strategy is for complex vertices and they are not currently used in the classification so we can leave this as is. Ideally during ranking we should also take the maximum error from both sides of the seam; doing this is more computationally intensive and it's not obvious that it's necessary (as usually the seam construction means that both sides behave more or less identically). This can be changed in the future but for now this should suffice. --- demo/tests.cpp | 2 +- src/simplifier.cpp | 38 ++++++++++++++++++++++---------------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/demo/tests.cpp b/demo/tests.cpp index ddca880dfa..e41754147d 100644 --- a/demo/tests.cpp +++ b/demo/tests.cpp @@ -1407,7 +1407,7 @@ static void simplifySeam() float aw = 1; assert(meshopt_simplifyWithAttributes(res, ib, 36, vb, 16, 16, vb + 3, 16, &aw, 1, NULL, 18, 2.f, 0, &error) == 18); assert(memcmp(res, expected, sizeof(expected)) == 0); - assert(fabsf(error - 0.49f) < 0.01f); // TODO: this is higher than normal due to merged attributes? + assert(fabsf(error - 0.22f) < 0.01f); // note: this is the same error as above because the attribute is constant on either side of the seam } static void adjacency() diff --git a/src/simplifier.cpp b/src/simplifier.cpp index 30b26a973f..a2e3cb72c2 100644 --- a/src/simplifier.cpp +++ b/src/simplifier.cpp @@ -850,7 +850,7 @@ static void fillEdgeQuadrics(Quadric* vertex_quadrics, const unsigned int* indic } } -static void fillAttributeQuadrics(Quadric* attribute_quadrics, QuadricGrad* attribute_gradients, const unsigned int* indices, size_t index_count, const Vector3* vertex_positions, const float* vertex_attributes, size_t attribute_count, const unsigned int* remap) +static void fillAttributeQuadrics(Quadric* attribute_quadrics, QuadricGrad* attribute_gradients, const unsigned int* indices, size_t index_count, const Vector3* vertex_positions, const float* vertex_attributes, size_t attribute_count) { for (size_t i = 0; i < index_count; i += 3) { @@ -862,14 +862,13 @@ static void fillAttributeQuadrics(Quadric* attribute_quadrics, QuadricGrad* attr QuadricGrad G[kMaxAttributes]; quadricFromAttributes(QA, G, vertex_positions[i0], vertex_positions[i1], vertex_positions[i2], &vertex_attributes[i0 * attribute_count], &vertex_attributes[i1 * attribute_count], &vertex_attributes[i2 * attribute_count], attribute_count); - // TODO: This blends together attribute weights across attribute discontinuities, which is probably not a great idea - quadricAdd(attribute_quadrics[remap[i0]], QA); - quadricAdd(attribute_quadrics[remap[i1]], QA); - quadricAdd(attribute_quadrics[remap[i2]], QA); + quadricAdd(attribute_quadrics[i0], QA); + quadricAdd(attribute_quadrics[i1], QA); + quadricAdd(attribute_quadrics[i2], QA); - quadricAdd(&attribute_gradients[remap[i0] * attribute_count], G, attribute_count); - quadricAdd(&attribute_gradients[remap[i1] * attribute_count], G, attribute_count); - quadricAdd(&attribute_gradients[remap[i2] * attribute_count], G, attribute_count); + quadricAdd(&attribute_gradients[i0 * attribute_count], G, attribute_count); + quadricAdd(&attribute_gradients[i1 * attribute_count], G, attribute_count); + quadricAdd(&attribute_gradients[i2 * attribute_count], G, attribute_count); } } @@ -1029,8 +1028,9 @@ static void rankEdgeCollapses(Collapse* collapses, size_t collapse_count, const if (attribute_count) { - ei += quadricError(attribute_quadrics[remap[i0]], &attribute_gradients[remap[i0] * attribute_count], attribute_count, vertex_positions[i1], &vertex_attributes[i1 * attribute_count]); - ej += quadricError(attribute_quadrics[remap[j0]], &attribute_gradients[remap[j0] * attribute_count], attribute_count, vertex_positions[j1], &vertex_attributes[j1 * attribute_count]); + // note: ideally we would evaluate max/avg of attribute errors for seam edges, but it's not clear if it's worth the extra cost + ei += quadricError(attribute_quadrics[i0], &attribute_gradients[i0 * attribute_count], attribute_count, vertex_positions[i1], &vertex_attributes[i1 * attribute_count]); + ej += quadricError(attribute_quadrics[j0], &attribute_gradients[j0 * attribute_count], attribute_count, vertex_positions[j1], &vertex_attributes[j1 * attribute_count]); } // pick edge direction with minimal error @@ -1157,8 +1157,16 @@ static size_t performEdgeCollapses(unsigned int* collapse_remap, unsigned char* if (attribute_count) { - quadricAdd(attribute_quadrics[r1], attribute_quadrics[r0]); - quadricAdd(&attribute_gradients[r1 * attribute_count], &attribute_gradients[r0 * attribute_count], attribute_count); + quadricAdd(attribute_quadrics[i1], attribute_quadrics[i0]); + quadricAdd(&attribute_gradients[i1 * attribute_count], &attribute_gradients[i0 * attribute_count], attribute_count); + + if (vertex_kind[i0] == Kind_Seam) + { + unsigned int s0 = wedge[i0], s1 = wedge[i1]; + + quadricAdd(attribute_quadrics[s1], attribute_quadrics[s0]); + quadricAdd(&attribute_gradients[s1 * attribute_count], &attribute_gradients[s0 * attribute_count], attribute_count); + } } if (vertex_kind[i0] == Kind_Complex) @@ -1174,9 +1182,7 @@ static size_t performEdgeCollapses(unsigned int* collapse_remap, unsigned char* else if (vertex_kind[i0] == Kind_Seam) { // remap v0 to v1 and seam pair of v0 to seam pair of v1 - unsigned int s0 = wedge[i0]; - unsigned int s1 = wedge[i1]; - + unsigned int s0 = wedge[i0], s1 = wedge[i1]; assert(s0 != i0 && s1 != i1); assert(wedge[s0] == i0 && wedge[s1] == i1); @@ -1665,7 +1671,7 @@ size_t meshopt_simplifyEdge(unsigned int* destination, const unsigned int* indic fillEdgeQuadrics(vertex_quadrics, result, index_count, vertex_positions, remap, vertex_kind, loop, loopback); if (attribute_count) - fillAttributeQuadrics(attribute_quadrics, attribute_gradients, result, index_count, vertex_positions, vertex_attributes, attribute_count, remap); + fillAttributeQuadrics(attribute_quadrics, attribute_gradients, result, index_count, vertex_positions, vertex_attributes, attribute_count); #if TRACE size_t pass_count = 0; From e42624de74a49b2e0a33751d2b7e7e5b8b21ec41 Mon Sep 17 00:00:00 2001 From: Arseny Kapoulkine Date: Mon, 19 Aug 2024 17:54:54 -0700 Subject: [PATCH 4/5] simplify: Minor refactoring of performEdgeCollapses To avoid mistakes, extract kind into a variable (we're only concerned with source vertex kind for classification since that's what determines the wedges we need to remap), and also add a few comments. Notably, we probably don't need to worry about attribute quadrics for complex vertices long term; the intention for complex classification has been to treat vertices with attributes that are similar enough as one, so this probably can function similarly to ranking where we're okay with ignoring the topology somewhat. Also, while we're here, adjust complex collapses to collapse to i1, not r1. This code is not used right now but collapsing to r1 feels slightly wrong since in some cases we'd be collapsing to a vertex that was never connected to any wedge in the complex. --- src/simplifier.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/simplifier.cpp b/src/simplifier.cpp index a2e3cb72c2..26e46dac7f 100644 --- a/src/simplifier.cpp +++ b/src/simplifier.cpp @@ -1128,6 +1128,8 @@ static size_t performEdgeCollapses(unsigned int* collapse_remap, unsigned char* unsigned int r0 = remap[i0]; unsigned int r1 = remap[i1]; + unsigned char kind = vertex_kind[i0]; + // we don't collapse vertices that had source or target vertex involved in a collapse // it's important to not move the vertices twice since it complicates the tracking/remapping logic // it's important to not move other vertices towards a moved vertex to preserve error since we don't re-rank collapses mid-pass @@ -1160,8 +1162,10 @@ static size_t performEdgeCollapses(unsigned int* collapse_remap, unsigned char* quadricAdd(attribute_quadrics[i1], attribute_quadrics[i0]); quadricAdd(&attribute_gradients[i1 * attribute_count], &attribute_gradients[i0 * attribute_count], attribute_count); - if (vertex_kind[i0] == Kind_Seam) + // note: this is intentionally missing handling for Kind_Complex; we assume that complex vertices have similar attribute values so just using the primary vertex is fine + if (kind == Kind_Seam) { + // seam collapses involve two edges so we need to update attribute quadrics for both target vertices; position quadrics are shared unsigned int s0 = wedge[i0], s1 = wedge[i1]; quadricAdd(attribute_quadrics[s1], attribute_quadrics[s0]); @@ -1169,17 +1173,18 @@ static size_t performEdgeCollapses(unsigned int* collapse_remap, unsigned char* } } - if (vertex_kind[i0] == Kind_Complex) + if (kind == Kind_Complex) { + // remap all vertices in the complex to the target vertex unsigned int v = i0; do { - collapse_remap[v] = r1; + collapse_remap[v] = i1; v = wedge[v]; } while (v != i0); } - else if (vertex_kind[i0] == Kind_Seam) + else if (kind == Kind_Seam) { // remap v0 to v1 and seam pair of v0 to seam pair of v1 unsigned int s0 = wedge[i0], s1 = wedge[i1]; @@ -1200,7 +1205,7 @@ static size_t performEdgeCollapses(unsigned int* collapse_remap, unsigned char* collapse_locked[r1] = 1; // border edges collapse 1 triangle, other edges collapse 2 or more - triangle_collapses += (vertex_kind[i0] == Kind_Border) ? 1 : 2; + triangle_collapses += (kind == Kind_Border) ? 1 : 2; edge_collapses++; result_error = result_error < c.error ? c.error : result_error; From d2367d877b236a5e19c943f5803f0486fe907822 Mon Sep 17 00:00:00 2001 From: Arseny Kapoulkine Date: Mon, 19 Aug 2024 18:23:20 -0700 Subject: [PATCH 5/5] demo: Adjust simplifySeam test a bit to correct border misconceptions The seam vertices here have Z coordinate that is higher than zero; because of this, positional errors are not exactly zero. This is a good thing since it allows us to get a more stable result across fp environments and algorithm changes. We adjust the data a little bit to make the bump less pronounced and fix the comments accordingly. --- demo/tests.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/demo/tests.cpp b/demo/tests.cpp index e41754147d..02873cf6e4 100644 --- a/demo/tests.cpp +++ b/demo/tests.cpp @@ -1347,12 +1347,12 @@ static void simplifySeam() 0, 1, 0, 1, 0, 2, 0, 1, 1, 0, 0, 0, - 1, 1, 1, 0, - 1, 1, 1, 1, + 1, 1, 0.3f, 0, + 1, 1, 0.3f, 1, 1, 2, 0, 1, 2, 0, 0, 0, - 2, 1, 0, 0, - 2, 1, 0, 1, + 2, 1, 0.1f, 0, + 2, 1, 0.1f, 1, 2, 2, 0, 1, 3, 0, 0, 0, 3, 1, 0, 0, @@ -1402,12 +1402,12 @@ static void simplifySeam() assert(meshopt_simplify(res, ib, 36, vb, 16, 16, 18, 1.f, 0, &error) == 18); assert(memcmp(res, expected, sizeof(expected)) == 0); - assert(fabsf(error - 0.22f) < 0.01f); // TODO: this is higher than normal due to border errors? + assert(fabsf(error - 0.04f) < 0.01f); // note: the error is not zero because there is a small difference in height between the seam vertices float aw = 1; assert(meshopt_simplifyWithAttributes(res, ib, 36, vb, 16, 16, vb + 3, 16, &aw, 1, NULL, 18, 2.f, 0, &error) == 18); assert(memcmp(res, expected, sizeof(expected)) == 0); - assert(fabsf(error - 0.22f) < 0.01f); // note: this is the same error as above because the attribute is constant on either side of the seam + assert(fabsf(error - 0.04f) < 0.01f); // note: this is the same error as above because the attribute is constant on either side of the seam } static void adjacency()