Merge tied, nearly parallel plain edges in their exact order in no_fit_polygon - #73
Merged
Merged
Conversation
…t_polygon The convex no_fit_polygon merges the edges of both shapes in angular order. It compares directions with direction_strictly_lesser, whose tolerance is meant for the floating-point noise of arc tangents but, on unit vectors, also ties plain edges up to ~1e-6 rad apart. Tied plain edges were then always merged fixed first, which is the wrong order whenever the orbiting edge has the smaller direction: the vertex between them is replaced by one inside the actual Minkowski sum. For the self-NFP of a hexagon whose edges 0 and 3 are ~9.5e-7 rad from parallel (fontanf/packingsolver#598), this happened on one side only: the NFP was off by up to ~7e-5, and not centrally symmetric anymore. The periodic packing then unioned the (inflated) NFPs of a shape and of its half-turn, which should be identical but differed by a few micro-units, and compute_union failed ("face area is not positive", "outline area is not positive") or overflowed the stack. Order tied plain edges by their exact directions instead, and only merge the fixed one first if they are exactly parallel. New regression test: the self-NFP of that hexagon, against the exact Minkowski sum. The two expected_outputs variants of Issue558SawtoothSelfPairing are re-recorded (same structure, ~1.1e-4 more area).
This file contains hidden or 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
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.
Fixes the cause of the failures remaining in fontanf/packingsolver#598 with the latest
shape(after #71 and #72).Problem
The convex
no_fit_polygonmerges the edges of both shapes in angular order. It compares directions withdirection_strictly_lesser, whose tolerance is meant for the floating-point noise of arc tangents. But, on unit vectors, it also ties plain edges up to ~1e-6 rad apart. Tied plain edges were then always merged fixed first, which is the wrong order whenever the orbiting edge has the smaller direction: the vertex between them is replaced by one inside the actual Minkowski sum.In packingsolver#598, instance B's hexagon has two edges ~9.5e-7 rad from parallel (edges 0 and 3). In its self-NFP, this happened on one side only:
The periodic packing then unions the inflated NFPs of a shape and of its half-turn. These should be identical, but differed by a few micro-units, and
compute_unionfailed with "face area is not positive" or "outline area is not positive", or overflowed the stack.Fix
Order tied plain edges by their exact directions (
strictly_lesser_angle), and only merge the fixed one first if they are exactly parallel. Ties involving arcs are unchanged.Testing
NoFitPolygonConvexTestcase: the self-NFP of that hexagon, against the exact Minkowski sum (computed with rational arithmetic). It fails onmainand passes with the fix. Its grid-sampling oracle check takes ~4 s.Issue558SawtoothSelfPairing: bothexpected_outputsvariants are re-recorded, without and with fused multiply-adds. The result keeps the same structure (1 component, 883 elements, no self-intersection), with ~1.1e-4 more area, as expected since the old order put some vertices inside the actual Minkowski sums. Before re-recording them, I checked that a plain x86-64 build reproduces the old variant 0 exactly, and an x86-64 build with-mfmathe old variant 1 (the AArch64 one).-mfmabuild.main.