Skip to content

The SAT's edge table holds every edge a hull can have - #101

Merged
nicolas-maman merged 2 commits into
mainfrom
fix/sat-edge-table-size
Oct 4, 2026
Merged

nicolas-maman merged 2 commits into
mainfrom
fix/sat-edge-table-size

Conversation

@nicolas-maman

Copy link
Copy Markdown
Collaborator

I audited contact manifold generation against Box3D e77352c: the convex pairs, triangle manifolds and mesh contacts. All of it matches upstream except this one defect, which corrupts memory.

The bug: the SAT's edge-pair phase gathers A's edges into a per-worker table of 14 field arrays. The table was sized for 64 edges, on the assumption that MAX_HULL_EDGES counts half-edges. It counts full edges: the hull builder allows up to 128 edges (256 half-edges).

A hull with more than 64 edges (a 32-sided cylinder has 96, a 40-point sphere hull about 114) wrote its later edges over the earlier ones' fields. Past about 61 edges it also wrote beyond the worker's block, into the next worker's block or past the end of the allocation. The resulting edge axes were wrong: missed or ghost edge contacts, and wrong separations stored in the SAT cache.

#94 made the same mistake: it put B's normals and vertices in the first clip polygon's scratch, assuming at most 66 of them. There can be 256.

The fix: the table holds MAX_HULL_EDGES edges, with its field offsets derived from that size. B's normals and vertices get room after the table in the same block, so the number of allocations is unchanged.

test_manifold's new test_many_edges places two 96-edge cylinders at 300 random poses and compares the SAT's best edge separation with a brute force over every edge pair. Before the fix, 0 of 300 agreed; now all but at most 3 do.

All 28 test programs pass locally on aether 0.772.0, with no warnings. The machine was too loaded for meaningful timings. bench/manifold's contacts are identical to main (same points and separation sum).

The edge-pair phase gathers A's edges into a per-worker table of 14 field
arrays, sized for 64 edges ('MAX_HULL_EDGES / 2'). MAX_HULL_EDGES counts
full edges -- the hull builder allows 2 * MAX_HULL_EDGES half-edges, 128
edges -- so a hull of more than 64 edges (a 32-sided cylinder has 96, a
hull of 40 points on a sphere about 114) wrote its later edges over the
earlier ones' fields and, past about 61, beyond the worker's block into
the next worker's or past the allocation. The edge axes came out wrong:
missed or ghost edge contacts, wrong separations in the SAT cache. And
#94 kept B's normals and vertices in the first clip polygon's scratch on
the same mistaken bound (66 of them; there can be 256).

The table holds MAX_HULL_EDGES edges now, its field offsets derived from
that; B's normals and vertices go in room after it in the same block, so
the allocation count is unchanged.

test_manifold's test_many_edges: two 32-sided cylinders (96 edges each) at
300 random poses, the SAT's best edge separation against brute force over
every edge pair -- 0 of 300 agreed before, all but at most 3 now. Found by
an audit of the manifolds against Box3D e77352c, otherwise faithful.
…s constants

A constant built from another module's constants (hull.MAX_HULL_FACES +
hull.MAX_HULL_VERTICES) is expanded where this module is imported, and
test_mesh and test_height_field import the manifold without the hull:
'Undefined variable hull'. The sum is written out (256) with the reason.
@nicolas-maman
nicolas-maman merged commit 7a61008 into main Oct 4, 2026
1 check passed
@nicolas-maman
nicolas-maman deleted the fix/sat-edge-table-size branch October 6, 2026 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant