Do not weld coincident vertices when it would fuse touching shells - #105
Merged
Conversation
Reported as a magenta patch in the viewport -- the renderer's inverted-normal warning -- on an XOR chain routed through render() and polyhedron(). The solid also silently lost 500 units of volume. Welding coincident vertices is a REPAIR, for meshes whose seams and poles carry duplicates; BOSL2 VNFs routinely do, and without it they arrive NotManifold. Applied unconditionally it destroys a mesh that was already sound. A solid whose shells TOUCH -- the two halves of an XOR meeting along a shared surface, a rod's stubs meeting the block they pass through -- has genuinely distinct vertices at identical positions. Merging those fuses the shells into edges with four faces. Measured on the reported model: 248 vertices, 168 distinct positions. Welding produced a watertight but NON-manifold mesh with 76 non-manifold edges, and 3304.65 where the same geometry built inline gives 3804.65. So weld only when it provably does no harm: build the welded candidate, check it, and keep it only if it is still manifold. checkMesh (mesh_check.hpp) is a cheap combinatorial pass with no Manifold construction, and its own doc comment already names this exact hazard -- "two boxes fused along a face are watertight but have edges with four faces". Both sides had the same flaw and both are gated: - measure_geometry.cpp, exporting a Manifold body to VNF - resolvePolyhedron, importing VNF back Fixing only one would not have helped: a faithful 248-vertex export was re-fused to 168 by polyhedron's own 1e-6 weld. The weld is emphatically NOT abandoned. Manifold splits property-vertices, so a plain cube arrives as 24 vertices and a script reading obj.vertices should still see 8; a BOSL2 spheroid still welds to watertight with zero unwelded vertices. Verified all three cases: tidy where safe, intact where not. resolvePolyhedron now triangulates against raw indices and derives the welded triangles by remapping. Welding only renames vertices, never moves them, so the ear clipping is identical either way -- no second triangulation. Four regression tests, including the reduced XOR chain from the report and a rod-through-block case where welding measurably turns 104 manifold vertices into 72 with 32 non-manifold edges. 957 tests pass under both engines. User confirmed the viewport visually. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Patch, not minor: no new surface, and the previous behaviour was simply wrong -- 0.38.0 silently produced a non-manifold solid with the wrong volume for any mesh whose shells touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
revarbat
added a commit
to BelfrySCAD/BelfrySCAD
that referenced
this pull request
Aug 21, 2026
0.38.0 silently produced a non-manifold solid with the wrong volume for any mesh whose shells touch -- welding coincident vertices fused them. See BelfrySCAD/openscad_cpp_evaluator#105. The lock needed two attempts. The first, taken immediately after the wheels workflow reported success, captured only 3 of the 7 files: PyPI's index had not finished propagating, so Windows, manylinux x86_64 and the sdist were missing and the lock would have failed to install on those platforms. Third time this has bitten the project, and only the post-lock file-count check catches it -- neither uv nor the workflow reports anything wrong. So: wait until PyPI's simple index actually lists all 7, THEN lock, THEN verify the count before pushing. Note `uv cache clean` alone is not enough either -- it clears package artifacts but not the cached index response, which is why --refresh-package is required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
revarbat
added a commit
to BelfrySCAD/BelfrySCAD
that referenced
this pull request
Aug 21, 2026
* Require openscad_cpp_evaluator 0.38.0 for render() expressions
obj = render() difference() { cube(100); sphere(20); };
echo(obj.volume, obj.genus, obj.boundingbox);
polyhedron(obj);
A script can now measure its own geometry. The expression form of render()
builds its children, measures them, and DISCARDS the geometry -- nothing is
drawn -- so the same rule holds at top level, in a module body, in a
function body, in a list comprehension and in a ternary, and function
purity survives.
Documented in CLAUDE.md as a language EXTENSION, not part of upstream
OpenSCAD, with the two things that will actually bite someone: `render` is
now a reserved keyword, and `obj = render() cube(1);` does not parse
because a bare call's child_statement swallows the semicolon.
`render` deliberately stays in editor.py's _BUILTIN_MODULES rather than
moving to the syntax highlighter's `keywords` list: `echo` and `assert` are
grammar keywords too and live there, so moving only this one would be the
inconsistent choice. Completion already works from _BUILTIN_MODULES.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Lock openscad_cpp_evaluator 0.38.0
Verified the lock carries all 6 wheels plus the sdist before pushing -- a
partial wheel set has slipped through twice before.
Note `uv cache clean openscad_cpp_evaluator` was NOT sufficient here: it
clears the package artifacts but not the cached simple-index response, so
the first lock attempt still resolved against 0.37.0 and failed. `uv lock
--refresh-package openscad-cpp-evaluator` is what actually picked up the
new release.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Make the Find/Replace bar readable in dark mode
Reported symptom: the bar's background stayed off-white while its text
followed the dark palette to white, so the whole panel was near-invisible.
The cause was one hardcoded QColor("#F3F3F3") -- but the same mistake
appeared four times in FindBar, and fixing only the reported one would have
left the user to rediscover the rest immediately:
- bar background: hardcoded light grey
- "No results" field: pale pink with NO foreground set
- current match: white on #FF9900 -- only ~2.3:1, poor even in light mode
- other matches: pale amber with NO foreground set
The last three share the reported bug's root cause: a pale background with
no explicit text colour, so the text follows the palette and vanishes the
moment the app goes dark. Both match formats and the no-match field now set
their foreground explicitly, which makes them theme-independent rather than
merely dark-corrected.
Colours live in ui_colors.py beside gutter_colors()/header_colors(),
following that module's light/dark-pair convention. The bar also registers
on_appearance_change() so a live theme switch repaints it (and re-runs the
highlights, which may already be on screen) instead of only painting once
at construction.
The current match's foreground changed from white to black. That is a
light-mode change the report did not ask for, but white on #FF9900 was
below any readable threshold; black measures 9.8:1.
Verified with a throwaway contrast script rather than by eye, since this
sandbox cannot screenshot -- which is what caught the first attempt at the
dark no-match red at 3.7:1. Every pair now measures above 7:1 in both
themes. User confirmed the result visually in the running app before this
commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Require openscad_cpp_evaluator 0.38.1; bump to 0.75.1
0.38.0 silently produced a non-manifold solid with the wrong volume for any
mesh whose shells touch -- welding coincident vertices fused them. See
BelfrySCAD/openscad_cpp_evaluator#105.
The lock needed two attempts. The first, taken immediately after the wheels
workflow reported success, captured only 3 of the 7 files: PyPI's index had
not finished propagating, so Windows, manylinux x86_64 and the sdist were
missing and the lock would have failed to install on those platforms. Third
time this has bitten the project, and only the post-lock file-count check
catches it -- neither uv nor the workflow reports anything wrong.
So: wait until PyPI's simple index actually lists all 7, THEN lock, THEN
verify the count before pushing. Note `uv cache clean` alone is not enough
either -- it clears package artifacts but not the cached index response,
which is why --refresh-package is required.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Reported as a magenta patch in the viewport — the renderer's inverted-normal warning — on an XOR chain routed through
render()andpolyhedron(). The solid had also silently lost 500 units of volume.The mistake
Welding coincident vertices is a repair, for meshes whose seams and poles carry duplicates. BOSL2 VNFs routinely do, and without it they arrive
NotManifold. Applied unconditionally, it destroys a mesh that was already sound.A solid whose shells touch — the two halves of an XOR meeting along a shared surface, a rod's stubs meeting the block they pass through — has genuinely distinct vertices at identical positions. Merging those fuses the shells into edges with four faces.
Measured on the reported model:
3804.65 is what the same geometry built inline produces.
The fix
Weld only when it provably does no harm: build the welded candidate, check it, keep it only if it is still manifold.
checkMesh(mesh_check.hpp) is a cheap combinatorial pass with no Manifold construction — and its own doc comment already names this exact hazard: "two boxes fused along a face are watertight but have edges with four faces."Both sides had the same flaw, and both are gated. Fixing one would not have helped: a faithful 248-vertex export was re-fused to 168 by
polyhedron()'s own 1e-6 weld.measure_geometry.cpp— exporting a Manifold body to VNFresolvePolyhedron— importing VNF backThe weld is not abandoned
That was the regression risk, so all three cases are verified:
resolvePolyhedronnow triangulates against raw indices and derives the welded triangles by remapping. Welding only renames vertices, never moves them, so the ear clipping is identical either way — no second triangulation.Testing
Four regression tests, including the reduced XOR chain from the report and a rod-through-block case where welding measurably turns 104 manifold vertices into 72 with 32 non-manifold edges. 957 tests pass under both
OSCAD_BYTECODE_VM=0and=1. User confirmed the viewport visually.Version 0.38.1 — patch, not minor: no new surface, and the previous behaviour was simply wrong.
🤖 Generated with Claude Code