Add boolean2 and CrossSection regression and corpus tests#1755
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1755 +/- ##
==========================================
+ Coverage 92.74% 93.83% +1.08%
==========================================
Files 44 44
Lines 9990 9985 -5
==========================================
+ Hits 9265 9369 +104
+ Misses 725 616 -109 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Also, sorry for the noise here: fuzz testing was stable for days, then of course the minute I put up the PR, it finds something. (ETA: but what it found was a good research project and I extended #1757 as well after that first comment.) |
elalish
left a comment
There was a problem hiding this comment.
Thanks, really helpful to see all this. I've scanned over these pretty quickly, so please highlight any particularly interesting ones that I haven't commented on.
|
|
||
| // Read the polygon corpus (name, expectedNumTri, epsilon, numPolys, then the | ||
| // polygons), keeping just the name and the polygon set. | ||
| std::vector<std::pair<std::string, Polygons>> ReadCorpus( |
There was a problem hiding this comment.
We must already have this function and the next somewhere, right? Can we reuse?
There was a problem hiding this comment.
Extracted a shared ReadPolygonCorpus into test/polygon_corpus.h; offset_corpus and polygon_test now use it (with a guard so a missing corpus still fails loudly rather than silently registering no tests). polygon_fuzz keeps its own copy (separate fuzz harness, doesn't pull the gtest test infra) - can adopt it in a follow-up.
| for (const auto& entry : corpus) { | ||
| const CrossSection cs(entry.second); | ||
| const vec2 size = cs.Bounds().Size(); | ||
| const double diag = std::sqrt(size.x * size.x + size.y * size.y); |
There was a problem hiding this comment.
Done - and while here I did a broader la:: pass across the boolean2 tests and source (length/cross/isfinite/maxelem, vector add/sub); an earlier review pass had missed a bunch of these.
| SCOPED_TRACE(entry.first + " frac=" + std::to_string(frac)); | ||
| const CrossSection off = cs.Offset(frac * diag, jt); | ||
| const Polygons result = off.ToPolygons(); | ||
| if (!result.empty()) ++nonEmpty; |
There was a problem hiding this comment.
Could we tighten this further to say an outset has more area than the input and an inset less? In fact, can we also say the join types are always ordered by area? Seems like Miter should be the largest area, followed by Round, then Square and Bevel, though not quite sure what order those are in - can never remember the distinction. Makes me wonder if we should have both in our API...
There was a problem hiding this comment.
Tightened - asserted outset >= input >= inset per join type, plus the outset join-type ordering, which the corpus confirms is Miter >= Square >= Round >= Bevel (not what I'd have guessed either). The Square-vs-Bevel API question is fair but out of scope here.
| } | ||
|
|
||
| double AreaTol(const CrossSection& a, const CrossSection& b) { | ||
| return 1e-6 * (1.0 + std::fabs(a.Area()) + std::fabs(b.Area())); |
There was a problem hiding this comment.
If these functions are only called once, let's just inline them.
There was a problem hiding this comment.
Right - ExpectBooleanInvariants and ExpectDistributesOverUnion were each single-use; inlined both. AreaTol stays (used across ~10 tests).
| const double scale = 1.0 + std::fabs(a.Area()) + std::fabs(b.Area()); | ||
| EXPECT_NEAR(aUb.Area(), a.Area() + b.Area() - aIb.Area(), 1e-3 * scale) | ||
| << "inclusion-exclusion violated at offset " << offset; | ||
| EXPECT_NEAR(soup.Area(), aUb.Area(), 1e-5 * scale) |
There was a problem hiding this comment.
Where do these various near tolerance values come from? If you're really just testing for the presence/absence of a feature, would a bounding box check be more precise?
There was a problem hiding this comment.
The tolerances are area-relative (scale = 1+|a|+|b|): 1e-3 for the inclusion-exclusion identity (generous - it's a sub-eps near-coincidence) and 1e-5 for parity between the binary a + b and building one CrossSection from both polygon sets at once (same arrangement, so tighter). A bbox check wouldn't discriminate here - the tiny polygon sits right at the big one's corner, so dropping it barely moves the union's bbox; the lost area is what catches it.
There was a problem hiding this comment.
Revising my earlier reply here - I've made the area check independent of the engine since.
The bbox answer still holds: the small polygon sits at the big one's corner, so dropping it barely moves the union's bbox; the lost area is what catches it. But I was measuring that area with inclusion-exclusion, every term from the same engine - self-referential, so a uniform collapse passes 0 == 0 + 0 - 0 vacuously. I added an independent floor: the union must retain at least the raw shoelace area of the larger input, computed from the input coordinates with no engine call (ExpectUnionRetainsArea). That catches both a dropped polygon and a full collapse.
|
|
||
| // Host-drop only at large offset (passes at the origin): the StarRing host plus | ||
| // an 8-vertex feature anchored 1e-9 from host[1]. | ||
| TEST(CrossSection, DISABLED_TinyFeatureNearCornerHostDropAtOffset1024) { |
There was a problem hiding this comment.
Same answers as above. This one's the offset-sweep variant: it passes at the origin and only drops the piece once everything's shifted out to a large coordinate, which is why it sweeps {0, 1024, 4096} - same winding instability, surfaced by absolute-coordinate magnitude rather than the merge details. Same loose bound and explicit intersect-empty EXPECT; stays DISABLED_ while I'm still digging into the winding issue.
| const vec2 d = b - a; | ||
| const double len2 = dot(d, d); | ||
| if (len2 == 0.0) return false; | ||
| return std::fabs(la::cross(d, p - a)) <= eps * std::sqrt(len2); |
There was a problem hiding this comment.
What about when a and b are less than eps apart and p is distant from them? That's why CCW is built how it is - I believe this function will sometimes say they are not collinear. Unless you restrict the kind of input you give this function?
There was a problem hiding this comment.
Good catch - you're right it'd call them non-collinear in that case. It's saved by the input: these oracle predicates only run on retained output, where the validator has already dropped any eps-zero edge, so |b - a| > eps. I added a note making that precondition explicit - it's not meant as a general collinearity test.
| return std::fabs(la::cross(d, p - a)) <= eps * std::sqrt(len2); | ||
| } | ||
|
|
||
| ::testing::AssertionResult CheckRetainedGraphValidity( |
There was a problem hiding this comment.
This looks pretty complicated - can we have a description of what it's doing? Also, I subscribe to the philosophy that simple testing is preferable, since otherwise there is uncertainty when a test fails as to whether the bug is in the algorithm or the test itself. However, you've been using these for awhile, so you probably know better than I what this has been useful in catching.
There was a problem hiding this comment.
Added a contract. The aim is an independent oracle - predicates not reused from the engine - that's caught missed splits, dropped/stacked verts and sign errors area checks miss. Happy to trim anything murky.
| /*debug=*/false, WindRule::Add); | ||
| } | ||
|
|
||
| void ExpectSameFingerprint(const OverlapResult& a, const OverlapResult& b, |
There was a problem hiding this comment.
Some description here would be nice too.
There was a problem hiding this comment.
Added one - a determinism/idempotence check (arrangements equal up to eps-quantization).
| } | ||
|
|
||
| // RemoveOverlaps2D produces an edge-balance imbalance for some | ||
| // vertices on these mixed-scale inputs - 1e-6 alongside 1024. |
There was a problem hiding this comment.
Signed sum of incident edge multiplicities at a vertex (out minus in); a valid arrangement conserves it. Defined in the comment, and fixed the stale wording - the test asserts the balance holds.
|
Thanks for the review. A few tests that didn't get their own comments but are worth a look:
They're all area-based algebraic invariants rather than fixed-value assertions, so they keep holding as the arrangement internals shift. |
elalish
left a comment
There was a problem hiding this comment.
This is looking pretty good, perhaps we should get this merged so we can take a more serious look at #1707 (comment)?
| // overlap geometrically. (A - B) + (A ∩ B) ends up at 39605 versus | ||
| // a.Area=90895, missing ~51290 area units. The tiny x translation puts B's | ||
| // vertices just inside the eps band of A's; the intersect path appears to | ||
| // mis-classify the overlap region as empty. |
There was a problem hiding this comment.
I'm confused - the comment says there's a problem, but the test appears to be passing. Did this get fixed? If so, perhaps update the comment?
There was a problem hiding this comment.
Fixed - passes on the boolean2 backend this is built on. I didn't bisect which commit closed it, so I left the attribution out of the comment and just rewrote it to past tense.
| }; | ||
| } // namespace | ||
|
|
||
| TEST(CrossSection, BooleanAssociativitySeeds) { |
There was a problem hiding this comment.
What is the distinction between cross_section_test and boolean2_test, particularly if this test is here?
There was a problem hiding this comment.
Good point - moved the one direct-engine test (the Boolean2D non-finite check) to boolean2_test in a separate commit, so this file is now just the CrossSection public API while boolean2_test keeps the engine internals. This seed's a public-API algebraic identity, so it stays here.
| // the TinyVsLargeStars needle case - this is two large spiky | ||
| // shapes, not a tiny shape vs a needle. May share the off-axis | ||
| // T-junction root cause diagnosed earlier in this table, but the spike- | ||
| // collision geometry could be its own failure mode. |
There was a problem hiding this comment.
Can we update all these comments to their resolution?
There was a problem hiding this comment.
Good call - trimmed each to the distinguishing geometry plus the past-tense outcome, dropped the speculative root-cause notes. All green now.
| std::cerr << "[seed] " << c.name << std::endl; | ||
| const CrossSection a = MakeShape(c.a), b = MakeShape(c.b); | ||
| if (c.kind == SubtractKind::InclusionExclusion) { | ||
| const auto aIb = a.Boolean(b, OpType::Intersect); |
There was a problem hiding this comment.
Done in the subtract-invariant tests (here and SubtractInvariantsEmptyIntersectionDrop). There are other Boolean(.., Intersect) sites in the file if you'd like the whole thing converted - happy to, just kept this focused.
There was a problem hiding this comment.
Yes, I generally prefer to update all related code at once, simply so it's not forgotten. Not a big deal though.
| const auto aUb = a + b; | ||
| const double tol = AreaTol(a, b); | ||
| EXPECT_NEAR(aUb.Area(), a.Area() + b.Area() - aIb.Area(), tol) | ||
| << "inclusion-exclusion violated"; |
There was a problem hiding this comment.
Why does this branch test fewer things? Do they fail?
There was a problem hiding this comment.
They hold - tested it (subtract residuals ~1e-14 against a ~3e-6 tol). Inclusion-exclusion was just the identity that originally broke for this seed, so its standalone test only checked that one. Unified the loop to run all three for every seed and dropped the SubtractKind split.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4f8039b to
116c530
Compare
Yup, I will work on cleaning this up. Ignore the force push just now, just reconciling branches- no new content yet. |
…ents Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Sorry for the churn - I hadn't run the full style pass on these test files before putting the PR up, which is why a couple of your comments turned into broader cleanup. Done now: one anonymous namespace instead of six, the fuzz-diagnosis prose trimmed out of the seed comments, |
elalish
left a comment
There was a problem hiding this comment.
LGTM - I'll probably run across other things in here eventually, but better to have it in and iterate. Thanks!
Test suite for the boolean2 cutover - #1707 staging item 5, following #1722/#1749/#1751/#1754. Adds 79 tests:
test/boolean2_test.cpp(new): 37 white-box tests of the engine in suiteBoolean2- graph order, vertex merge, edge-vertex lists, intersection insertion, the retained-graph validator, and winding filtering. The validator's geometric predicates are deliberately independent of the engine's (usingIntersectSegmentsto check its own output would be circular); a comment in the file records how their epsilon band differs fromCCW's.test/cross_section_test.cpp: extended from its existing 15 tests with 41 new public-API tests - Boolean/BatchBoolean invariants (commutativity, distributivity, area conservation), Offset join types and edge cases, Decompose containment, Simplify, and degenerate/non-finite inputs.test/cross_section_offset_corpus_test.cpp(new): offsets the polygon triangulation corpus across every join type and gates on clean triangulation; runs on WASM via the preloaded corpus.Most regression cases are seeds from a fuzz campaign against the backend, kept as literal geometry with a short note on what each shape triggers; the fuzz harness itself follows in a separate PR. Two of these - tiny-feature point contacts near a host vertex - are marked
DISABLED_; they expose an arrangement/winding-robustness issue tracked separately. White-box../srcincludes follow the existing pattern (boolean_test, manifold_test, and three others). Suite runs green on Linux/Mac/Windows/WASM and under ASan+UBSan.