ARCHITECTURE.md invariant 2 has said since it was written that the
scattered clear_mesh_cache() calls are "unnecessary and expensive",
because MeshCache keys on (NodeId, ParamHash) and an edited node
invalidates itself. Phase 5.2 removed the first one. This removes the
rest: 8 of the 11 live call sites, leaving only invalidate_all (whose
contract is exactly that) and the definition.
Investigating them turned up something the invariant did not say, and
which makes the cleanup a bug fix rather than tidying:
MeshCache has NO eviction for deleted nodes.
Entries are keyed by NodeId and nothing ever removes one when its node
goes away. The wholesale clears were, accidentally, the cache's only
garbage collector. Deleting them naively would have converted a
performance problem into an unbounded memory leak over a session.
So the fix is a targeted replacement, not a deletion: MeshCache::retain,
SceneCache::retain_meshes and CadViewport::evict_dead_meshes drop exactly
the entries whose node is gone and leave every live one warm. Each call
site was then classified rather than pattern-matched:
- deletes (delete_selected, delete_part, CommandContext::delete_node)
-> evict dead entries
- pure additions (create_node, insert_node_at, paste, and the wall /
circle / rect / area / column / beam / polygon / polyline tool
completions) -> nothing at all. A new node has no cache entry to
invalidate and cannot affect any other node's mesh. These were
discarding the entire scene's meshes to make room for nothing.
- subdivide_selected -> invalidate only the selected ids. It builds a
fresh Arc<Solid> per part, so ParamHash already differs; the unselected
parts were being thrown away for no reason.
Measured: deleting 1 of 100 parts costs 62.1 us with clear+rebuild versus
21.9 us with evict+reuse, a 3x saving on every delete.
Two tests, both negative-tested by stubbing out the retain body:
retain_nodes_evicts_dead_entries_and_keeps_live_ones checks both
directions (dead gone, live still an Arc::ptr_eq hit), and
cache_does_not_grow_past_the_live_node_set simulates 50 add/delete rounds
and asserts the cache holds 1 entry, not 50. With retain stubbed it holds
exactly 50, which is the leak.
Also documented a sharp edge found while reading ParamHash: it hashes a
Csg node's Arc POINTER, not its geometry. Re-wrapping identical geometry
in a new Arc is therefore a safe false-positive, but mutating a Solid
behind a shared Arc would go unnoticed. Nothing does that today and
ARCHITECTURE.md now says it must stay that way.
624 lib + 154 integration, 0 failed.
The module had two independent paths from a node to triangles.
ensure_part_geometry called part.build_solid() directly; the exporters and
pick_part went through MeshCache::get_or_build -> CadSolid::build_mesh().
Neither saw the other's work, so a part that had just been exported or
hovered was still re-meshed from scratch for the GPU upload.
Proved they agree before merging them. pipeline_equivalence_tests builds
all 12 CadSolid variants through both paths and compares vertex positions,
triangle indices and winding exactly. They match. A second test asserts
the variant list has 12 entries, so adding a CadSolid variant without
covering it fails the build rather than silently narrowing the guarantee.
Both negative-tested: swapping width/height in build_solid's Rect2D arm
fails with "Rect2D: vertex 0 differs: (-1.5, -2, 0) vs (-2, -1.5, 0)";
dropping a variant from the list fails the coverage test.
The change itself: build_mesh_buffers now takes &TriMesh rather than
&Solid -- it only ever read solid.mesh() -- and ensure_part_geometry pulls
from the shared MeshCache. Added cad_mesh_data_from_mesh and
part_mesh_buffers_from_mesh as the mesh-taking entry points; the
Solid-taking ones remain as thin wrappers for callers that hold a Solid.
Measured, and the number is smaller than the section title suggests:
2x on a warm cache (boxes 0.27 -> 0.178 us/part, extruded 24-gons 1.11 ->
0.516 us/part), and *negative* on a cold one -- 0.79 vs 0.27 us/part,
because a first build now pays a ParamHash and a map insert. That is
recorded in BENCH_BASELINE.md with a note not to "optimise" it back by
special-casing cheap primitives.
The reason to do it anyway is correctness, not speed. Two match arms over
the same enum drift, and this drift would have been invisible: no crash,
no failing test, just a preview that quietly disagreed with the exported
file. Now there is one pipeline and a test that fails if it forks again.
build_mesh and build_solid both still exist because a Csg node needs a
real Solid to compose with. ARCHITECTURE.md's "Two meshing pipelines"
section, which told readers to apply every meshing change twice, is now
"One meshing pipeline".
615 lib + 154 integration, 0 failed.
Measured first. bench_viewport_snapshot_sync_per_frame puts the cost of
one sync frame at 98.5 us for 100 parts: 21.2 us of Vec<CadNode> deep
copies and 77.2 us of forced scene rebuilds. A generation comparison over
the same data is 0.051 us. Dragging a part sets script_dirty on every
MouseMove, so that was the steady-state per-frame cost of a drag.
Three separate wastes, each removed:
1. The source viewport was written back to. sync took a snapshot from the
dirty viewport and then handed it to all three, including the one it
came from -- a deep copy plus a full cache reset to install state that
viewport already had. It now pushes to the other two only.
2. Destinations were re-pushed unconditionally. They now compare
parts_generation() against the source's and skip if equal. During a
drag the same list was being reinstalled every frame; a frame where
nothing changed now costs three integer comparisons.
3. replace_parts_snapshot cleared part_geoms and the entire mesh cache.
The part_geoms.clear() was the expensive one and it is not in the
benchmark: it dropped every uploaded GPU buffer, so the next draw had
to re-mesh and re-upload the whole scene through ensure_part_geometry.
Now retains geometry for ids that survive the replacement.
The clear_mesh_cache() was redundant. MeshCache keys on
(NodeId, ParamHash) and ParamHash covers the solid discriminant, all
its parameters and the full transform, so a node whose geometry
changed misses on its own. ARCHITECTURE.md invariant 2 has said this
was unnecessary since it was written; this removes the first of them.
Three tests, each negative-tested by reintroducing the defect:
- mesh_cache_self_invalidates_on_parameter_and_transform_edits proves the
claim that justifies dropping the wipe. Deleting the transform hashing
from ParamHash makes it fail.
- replace_all_bumps_the_generation_so_destinations_can_compare pins the
precondition for the skip. Removing the bump makes it fail, which is
the failure mode that matters: a stale generation would make the sync
silently drop a real edit.
- geometry_is_retained_for_surviving_ids_only covers the retain
predicate. Geometry needs a live Cx, so this tests the id set logic --
keeping a buffer for a deleted id, or dropping one for a survivor.
Also collapsed six copies of the widget-borrow chain into with_viewport,
taking a callback rather than returning a guard: the WidgetRef is a
temporary, so returning a borrow of it does not compile (E0515, the same
shape as the let-else bug fixed in cf29ba7).
Not done: the three viewports still hold three copies of the parts list.
Sharing one Rc<RefCell<CadDocument>> and deleting the snapshot pair is
the rest of 5.2. This bounds the cost of the copy in the meantime, and
invariant 4 now says so.
BENCH_BASELINE.md records that this benchmark measures the primitives,
not the widget method, so its numbers do not move when the sync path is
fixed. It sizes the prize; the behaviour tests verify the change.
613 lib + 154 integration, 0 failed.
Phase 3 of CAD_ASSESSMENT_AND_PLAN.md. Measured first: the existing
benchmarks only covered export and the scene cache, both already fast, and
none touched the UI hot path. Added three that do, and recorded numbers in
BENCH_BASELINE.md.
pick_part broad phase: 2.1x less work per frame
The per-part AABB was built from CadNode::size(). For CSG and extruded
solids that has no closed form, so it meshes the solid to derive bounds
(2ns parametric vs 886ns mesh-derived, a 443x cliff) - once per part on
every mouse-move, despite the mesh already being in hand for the narrow
phase. Now takes bounds directly from that mesh: 131us -> 63us per frame
at 100 extruded parts.
This also fixes a latent correctness bug. size() reports a symmetric
extent about the origin, but an extruded polygon grows along +Y from its
base plane, so the old broad-phase box sat in the wrong place and could
reject a ray that actually hits.
Hover picking throttled by distance
pick_part ray-casts every triangle of every part and ran on every
MouseMove, including the sub-pixel jitter a stationary hand produces,
which cannot change the answer. Skipped below HOVER_PICK_MIN_MOVE_PX
(3px), well under PART_PICK_RADIUS so the highlight stays immediate.
Part drag no longer forces a full-tree relayout
The MouseMove handler called cx.redraw_all() on top of area.redraw() for
every motion event. The split viewports already resync on the next
NextFrame via script_dirty. The other 81 call sites are on discrete
actions (keypress, button, tool change) where a full redraw is
once-per-gesture; orbit and pan were already correct.
Tests: 720 -> 728 passing, 0 failing. 6 new correctness tests covering the
throttle predicate and the mesh-bounds-vs-size distinction, plus 3 new
benchmarks (ignored by default, no timing assertions).
Arena Agent