Commit bd56861
Record normals on Manifold; auto-substitute on GetMeshGL; round-trip via MeshGL (#1718)
* Record normals on Manifold; auto-substitute on GetMeshGL; roundtrip via MeshGLP
Following Emmett's #1712 direction: make "where are my normals?" a
property of the Manifold rather than something the caller threads
through every call. After CalculateNormals(), GetMeshGL() returns
solid-frame normals without needing the idx repeated; an ofMesh ->
getMesh roundtrip preserves the recording via a new optional
MeshGLP::hasNormals flag.
The standard slot is the first three extra-property channels (MeshGL
channels 3, 4, 5). CalculateNormals / SmoothByNormals / UpdateNormals
gain default normalIdx values (0 / 0 / -1 respectively) so the no-arg
call is the new preferred form; non-zero values are kept for
compatibility but flagged in the docs. Refine clears the recording
(interpolated normals are wrong).
Bindings updated in lock-step: Python (nb::arg defaults, has_normals
field on MeshGL/MeshGL64), WASM (JS wrapper for smoothByNormals,
helpers.cpp roundtrips hasNormals, .d.ts type updates), C (
manifold_meshgl{,64}_has_normals accessors). Single-signature shape
means no overload disambiguation needed in any binding.
Open follow-ups: [[deprecated]] attribute + internal caller migration,
SetProperties detection (flagged with TODO).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Record normals on Manifold; auto-apply transforms via runFlags bit 1
Fixes #1712, fixes #1719.
Make "where are my normals?" a property of the Manifold rather than
something the caller threads through every call. After CalculateNormals,
GetMeshGL returns world-frame normals without an explicit normalIdx, and
a MeshGL <-> Manifold round-trip preserves the recording per-run even
across mixed-input Booleans (e.g., a cube without normals composed with
a sphere that has them).
The #1712 repro from
(sphere - sphere.Translate(10)).CalculateNormals().GetMeshGL()
now returns the expected outward-from-cavity normals. The underlying
cross-mesh seam-vert sign issue (#1719) is fixed in SetNormals.
Mechanism:
- runFlags becomes a bitmask: bit 0 = backside (existing), bit 1 =
hasNormals (new). New MeshGLP::HasNormals(run) accessor mirrors
Backside(run). Backside fixed from `runFlags[run] == 1` to `& 1` so
it survives the addition of a second bit.
- CalculateNormals(0) marks the per-meshID Relation.hasNormals flag,
which Impl::HasNormals() ANDs across to give the impl-wide answer.
GetMeshGL(-1) auto-substitutes slot 0 on output when HasNormals() is
true.
- Normals at the standard slot are stored world-frame on the Impl.
Impl::Transform and csg_tree.cpp::Compose eager-transform
properties_[0..2] per-meshID so the slot stays in sync with vertPos_
and faceNormal_ across any sequence of transforms, including mixed-
input Boolean/Compose outputs where some meshIDs carry normals and
others don't. EagerTransformPropNormals is a shared helper.
- SetNormals multi-normal walk sign-flips cross(next, here) when it
disagrees with faceNormal_[next.face]. This handles Boolean
subtractee triangles (winding unchanged, faceNormal_ negated by the
Boolean post-pass) without losing the existing "flair" curving for
smooth surfaces. This is what fixes #1719.
- CreateProperties negates Q's slot 0..2 per source-meshID for
subtractee cavity inversion.
- GetNormal returns properties_[0..2] directly when the meshID's
Relation.hasNormals is true (world-frame already); applies the per-
meshID inverse-normal-transform when false, preserving the master
contract for hand-built MeshGL inputs that don't set the bit.
- Non-zero normalIdx to CalculateNormals uses the legacy deferred-
transform path: stores per-mesh-frame, recovered to world-frame by
GetMeshGL's per-run transform on export. Docstring notes this path
will be removed in a future release.
API additions:
- MeshGLP::HasNormals(run) C++ accessor plus C and Python bindings.
- MeshGLP::Backside(run) C and Python bindings (previously C++-only).
Tests:
- Properties.CalculateNormals: previously-FIXME'd dot(pos, norm) > 0
asserts enabled (#1719 regression).
- Manifold.CalculateNormalsRoundTripsThroughGetMeshGL: ofMesh+getMesh
preserving the per-run flag, Rotate before and after CalculateNormals,
no-arg invocation, non-zero idx.
- Manifold.GetNormalLegacyContract: per-mesh-frame fallback in
GetNormal for MeshGLs without bit 1.
- Manifold.TransformMixedNormalsPerMeshID,
Manifold.ComposeMixedNormalsPerMeshID,
Manifold.BooleanSubtractMixedQPerMeshIDNegation: per-meshID iteration
in Impl::Transform / Compose / CreateProperties for mixed-input
Boolean outputs.
- Manifold.CalculateNormalsNonZeroIdxSurvivesTransform: legacy non-zero
normalIdx path across post-calc transforms.
- CBIND.run_flag_accessors: exercises the new C accessors.
Known limitations:
- Hand-built MeshGL inputs that share propVerts across runs with
differing hasNormals settings produce one-interpretation-wins
behavior at the shared propVerts (the stored value can only have one
semantic). Standard CalculateNormals / Boolean / Compose outputs
never produce this shape. Documented on MeshGLP::HasNormals(run).
- Warp / WarpBatch / SetProperties keep the recording set; if the user
replaces slot 0..2 or warps non-rigidly, the stored normals are stale
until they call CalculateNormals again. Documented on each method.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Address PR #1718 review feedback
Substantive:
- Split CalculateNormalsRoundTripsThroughGetMeshGL into 8 named tests
(NormalsCavity, NormalsRotateBefore/AfterCalc, NormalsAutoSubstitute,
NormalsRoundTrip, NormalsRefinePreserved, NormalsSmoothByNormalsNoArg,
NormalsNonStandardSlotNotRecorded). Pithier names per Emmett's request.
- Fix Backside(run) docstring: framework already orients stored normals
on the standard flow, the bit is informational only.
- SafeNormalize during the eager-transform in
Manifold::Impl::EagerTransformPropNormals so non-orthogonal transforms
and barycentric interpolation don't leave non-unit values that compound
downstream (e.g., at Boolean edges feeding SmoothByNormals).
- Add `backside(run)` and `hasNormals(run)` methods on the WASM Mesh
class, plus runFlags field documentation in the .d.ts.
- Add a deliberately-failing test (NormalsSharedPropVertMixedFlags) that
demonstrates the data-model conflict in the shared-propVert + mixed-
hasNormals case. Hand-built MeshGL inputs with shared propVerts across
runs of differing hasNormals can only store one slot 0..2 value;
Impl::Transform picks one interpretation, corrupting the other. Test
is intentionally red as a discussion-driver for PR #1718 review
thread (whether to fix by splitting propVerts in the Manifold(MeshGL)
ctor, or to document the constraint).
Trivial:
- Rename wasHasNormals -> hadNormals in Impl::InitializeOriginal.
- Drop verbose eager-transform comment block in Impl::Transform; the
helper call site is self-documenting.
- Note AND-not-OR semantics on Impl::HasNormals(); rename hint covered
by the new doc.
- TODOs at the legacy non-zero-normalIdx codepaths (the getTransform
lambda in SetNormals, the legacy fallback in GetMeshGLImpl per-vert
normalization) marking them for removal alongside the deprecated API.
- Deprecation notice on GetMeshGL/GetMeshGL64's explicit normalIdx
parameter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Split shared propVerts across runs with mixed hasNormals
Addresses comment 3254084222 on PR #1718. When a single input propVert
is referenced by triangles from runs with differing hasNormals flags,
the eager-transform contract can't track both interpretations through
one slot 0..2: a Transform would rotate the hasNormals=true run's
normal and corrupt the hasNormals=false run's color (or vice versa).
The MeshGL ctor now duplicates such propVerts so each camp gets its
own slot. The first-seen camp keeps the original index; the opposite
camp gets an appended alt copy holding a snapshot of the original
property data. Subsequent triangles from each camp resolve through
their respective indices. DedupePropVerts already skips cross-meshID
pairs, so the duplicate survives ctor cleanup, and
EagerTransformPropNormals is gated on TriHasNormals per triangle, so
only the hasNormals camp's copy gets rotated.
Behaviour preserved for the existing numProp == 0 cases (the
single-array legacy push pattern is retained when there's no distinct
prop space). For numProp > 0 we always populate both triProp and
triVert now - prior code skipped triVert when prop2vert was empty, but
the split can introduce triProp values >= numVert that need triVert
for valid v0/v1 in CreateHalfedges.
Docs: re-add a paragraph to MeshGLP::HasNormals (and the WASM
hasNormals method) describing the auto-split, replacing the
"undefined for hand-built mixed-flag inputs" caveat that ce0e18d
removed when it staged the failing test.
The new test NormalsSharedPropVertMixedFlags (added in ce0e18d as a
discussion-driver red test) now passes; its docstring is updated to
describe the fix rather than the failure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Fix clang-format on myHasN assignment
clang-format v20 (CI) wants the && wrapped onto a continuation rather
than aligned after `=`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Revert "Split shared propVerts across runs with mixed hasNormals"
Per discussion on PR #1718 (review comment 3254084222): the shared-
propVert + mixed-hasNormals input shape is genuinely weird (no standard
output produces it) and adding the auto-split to every MeshGL ingest
slows the import path - already a sore spot relative to Boolean - and
adds maintenance surface for a case users shouldn't be hitting.
Reverting cf8d898 (the split implementation + doc changes) and ce6e394
(the clang-format follow-up that touched the same block). Test wording
and updated doc explaining the constraint follow in a separate commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Document undefined behavior for cross-run shared propVerts
Add a caveat paragraph on `MeshGLP::HasNormals` (C++ header) and the
WASM `hasNormals(run)` method noting:
- hasNormals is per-run, so different runs may set it differently
(leading with this since the original removed-then-not-restored caveat
was misread on review as forbidding mixed-flag runs).
- Behavior is undefined when a single propVert is shared by triangles
from runs that disagree - a Transform rotates the slot on behalf of
the hasNormals=true camp and clobbers any hasNormals=false sharer.
- Standard CalculateNormals / Boolean / Compose outputs never produce
that shape - hand-built MeshGL inputs are the only way to hit it.
Repurpose the `NormalsSharedPropVertMixedFlagsUndefined` test (renamed
from `NormalsSharedPropVertMixedFlags`) to pin the documented
behaviour: assert the hasNormals camp's slot rotates as expected
(`run0Bad == 0`) and the no-normals camp's value is clobbered
(`run1Bad > 0`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>1 parent 22571ee commit bd56861
19 files changed
Lines changed: 711 additions & 161 deletions
File tree
- bindings
- c
- include/manifold
- python
- wasm
- include/manifold
- src
- test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
448 | 448 | | |
449 | 449 | | |
450 | 450 | | |
451 | | - | |
| 451 | + | |
| 452 | + | |
452 | 453 | | |
453 | 454 | | |
454 | 455 | | |
| |||
474 | 475 | | |
475 | 476 | | |
476 | 477 | | |
477 | | - | |
| 478 | + | |
| 479 | + | |
478 | 480 | | |
479 | 481 | | |
480 | 482 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
694 | 694 | | |
695 | 695 | | |
696 | 696 | | |
697 | | - | |
698 | | - | |
| 697 | + | |
| 698 | + | |
| 699 | + | |
| 700 | + | |
| 701 | + | |
| 702 | + | |
699 | 703 | | |
700 | 704 | | |
701 | 705 | | |
| |||
775 | 779 | | |
776 | 780 | | |
777 | 781 | | |
778 | | - | |
779 | | - | |
| 782 | + | |
| 783 | + | |
| 784 | + | |
| 785 | + | |
| 786 | + | |
| 787 | + | |
780 | 788 | | |
781 | 789 | | |
782 | 790 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
324 | 324 | | |
325 | 325 | | |
326 | 326 | | |
327 | | - | |
| 327 | + | |
328 | 328 | | |
329 | 329 | | |
330 | | - | |
| 330 | + | |
331 | 331 | | |
332 | 332 | | |
333 | 333 | | |
| |||
610 | 610 | | |
611 | 611 | | |
612 | 612 | | |
| 613 | + | |
| 614 | + | |
613 | 615 | | |
614 | 616 | | |
615 | 617 | | |
| |||
723 | 725 | | |
724 | 726 | | |
725 | 727 | | |
| 728 | + | |
| 729 | + | |
726 | 730 | | |
727 | 731 | | |
728 | 732 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
180 | 180 | | |
181 | 181 | | |
182 | 182 | | |
183 | | - | |
| 183 | + | |
184 | 184 | | |
185 | 185 | | |
186 | 186 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
252 | 252 | | |
253 | 253 | | |
254 | 254 | | |
255 | | - | |
| 255 | + | |
256 | 256 | | |
257 | 257 | | |
258 | 258 | | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
259 | 263 | | |
260 | 264 | | |
261 | 265 | | |
| |||
447 | 451 | | |
448 | 452 | | |
449 | 453 | | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
450 | 464 | | |
451 | 465 | | |
452 | 466 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
739 | 739 | | |
740 | 740 | | |
741 | 741 | | |
742 | | - | |
| 742 | + | |
| 743 | + | |
| 744 | + | |
743 | 745 | | |
744 | 746 | | |
745 | | - | |
| 747 | + | |
746 | 748 | | |
747 | 749 | | |
748 | 750 | | |
| |||
846 | 848 | | |
847 | 849 | | |
848 | 850 | | |
849 | | - | |
850 | | - | |
851 | | - | |
852 | | - | |
| 851 | + | |
| 852 | + | |
| 853 | + | |
| 854 | + | |
| 855 | + | |
| 856 | + | |
| 857 | + | |
853 | 858 | | |
854 | 859 | | |
855 | 860 | | |
| |||
859 | 864 | | |
860 | 865 | | |
861 | 866 | | |
862 | | - | |
| 867 | + | |
863 | 868 | | |
864 | 869 | | |
865 | 870 | | |
| |||
1336 | 1341 | | |
1337 | 1342 | | |
1338 | 1343 | | |
| 1344 | + | |
| 1345 | + | |
| 1346 | + | |
| 1347 | + | |
| 1348 | + | |
| 1349 | + | |
| 1350 | + | |
| 1351 | + | |
1339 | 1352 | | |
1340 | 1353 | | |
1341 | 1354 | | |
| |||
1429 | 1442 | | |
1430 | 1443 | | |
1431 | 1444 | | |
| 1445 | + | |
| 1446 | + | |
| 1447 | + | |
| 1448 | + | |
| 1449 | + | |
| 1450 | + | |
| 1451 | + | |
| 1452 | + | |
| 1453 | + | |
| 1454 | + | |
| 1455 | + | |
| 1456 | + | |
| 1457 | + | |
| 1458 | + | |
| 1459 | + | |
| 1460 | + | |
| 1461 | + | |
| 1462 | + | |
| 1463 | + | |
| 1464 | + | |
| 1465 | + | |
| 1466 | + | |
| 1467 | + | |
| 1468 | + | |
| 1469 | + | |
| 1470 | + | |
| 1471 | + | |
1432 | 1472 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
245 | 245 | | |
246 | 246 | | |
247 | 247 | | |
248 | | - | |
| 248 | + | |
249 | 249 | | |
250 | 250 | | |
251 | 251 | | |
| |||
256 | 256 | | |
257 | 257 | | |
258 | 258 | | |
259 | | - | |
| 259 | + | |
260 | 260 | | |
261 | 261 | | |
262 | 262 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
157 | 157 | | |
158 | 158 | | |
159 | 159 | | |
160 | | - | |
161 | | - | |
162 | | - | |
163 | | - | |
164 | | - | |
165 | | - | |
166 | | - | |
167 | | - | |
168 | | - | |
169 | | - | |
170 | | - | |
171 | 160 | | |
172 | 161 | | |
173 | 162 | | |
| |||
235 | 224 | | |
236 | 225 | | |
237 | 226 | | |
238 | | - | |
239 | | - | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
240 | 230 | | |
241 | 231 | | |
242 | 232 | | |
243 | 233 | | |
244 | | - | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
245 | 255 | | |
246 | 256 | | |
247 | 257 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
569 | 569 | | |
570 | 570 | | |
571 | 571 | | |
572 | | - | |
| 572 | + | |
| 573 | + | |
573 | 574 | | |
574 | 575 | | |
575 | 576 | | |
| |||
608 | 609 | | |
609 | 610 | | |
610 | 611 | | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
| 615 | + | |
| 616 | + | |
| 617 | + | |
| 618 | + | |
| 619 | + | |
| 620 | + | |
611 | 621 | | |
612 | 622 | | |
613 | 623 | | |
| |||
665 | 675 | | |
666 | 676 | | |
667 | 677 | | |
668 | | - | |
| 678 | + | |
| 679 | + | |
| 680 | + | |
669 | 681 | | |
670 | 682 | | |
671 | 683 | | |
| |||
936 | 948 | | |
937 | 949 | | |
938 | 950 | | |
939 | | - | |
| 951 | + | |
940 | 952 | | |
941 | 953 | | |
942 | 954 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
314 | 314 | | |
315 | 315 | | |
316 | 316 | | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
317 | 329 | | |
318 | 330 | | |
319 | 331 | | |
| |||
0 commit comments