nigig-org/REVIEWS/CAD_RENDER_OPTIMISATION_PLAN.md

CAD render optimisation: a phased plan

Companion to REVIEWS/CAD_DRAWCALL_STRATEGY_ANALYSIS.md, which contains the evidence and one significant correction. Read its section 0 first: the 2D vector scene is one draw call, not hundreds, and the plan below is ordered on the corrected facts rather than the original ones.

Revision 2 merges a second, independently written optimisation plan (frustum culling → geometry merging → GPU instancing → octree/LOD) into this one. Section "Merging a second review" below records what that plan got right, what it got wrong, and — the part that matters most — the place where it and this document were both wrong about the same thing.

The three findings this plan acts on

1. No viewport culling anywhere. grep -niE "cull|frustum|offscreen|in_view" over the 2,461-line renderer returns nothing. Every part is submitted every frame, on screen or not — in 2D that is wasted tessellation, in 3D a wasted draw call.
2. 3D issues one draw call per part, each with its own geometry buffer, transform and colour. part_geoms is keyed by part id, so 200 identical columns are 200 uploads and 200 draws.
3. The base grid and the parts loop tessellate per item, where the file's own queue_dashed_line idiom — queue segments, stroke once, guarded by a test — shows how not to.

---

Merging a second review

Every claim below was checked against the tree at 14aa0d5 before it was accepted or rejected. Line numbers in the incoming plan were a few off (draw_scene is at viewport_render.rs:325, its parts loop at 345; DrawCadMesh is at mod.rs:152), which is drift, not error.

Incoming proposal	Verdict	Evidence
Frustum-plane culling in 3D	Adopted	SceneState3D { view, projection } is captured every frame into self.last_view / self.last_proj (viewport.rs:3781-3782) immediately before draw_scene. Planes come out of projection * view by adding and subtracting rows — no matrix inverse, which matters here (see below).
Store bounding_sphere on CadNode	Rejected as stored, adopted as derived	See "The field that must not exist".
Cull only in draw_scene	Widened	2D plan view is where drafting happens and where per-part cost is tessellation. Both loops get the predicate.
"Log skipped-part count per frame"	Replaced	Phase 0 already built the counting seam (render_budget::FrameBudget, bench_frame_submission_budget). A log line nobody reads is a step that cannot fail.
"Expect 30-60% of parts skipped"	Not adopted as a target	Unfalsifiable as written — it depends entirely on zoom. The exit criterion is a number in BENCH_BASELINE.md at a stated zoom, not a guess.
Merge geometry per PartKind with transforms baked into vertices	Rejected	See "Why merging bakes in a regression".
GPU instancing, one call per group	Adopted, and cheaper than the incoming plan thought	The machinery is already upstream. See "Instancing is not a shader rewrite".
Octree + LOD above 500 parts	Still rejected at that threshold	The cached world-AABB pass costs 20 µs at 500 parts (bench_pick_broadphase_world_aabb_recompute_vs_cache) — 0.12% of a 16.7 ms frame. A tree that replaces 20 µs cannot pay for itself. Revisit with a measurement, not a part count.
Dependency order 1 → 2 → 3, 4 optional	Agreed	Instancing batches per geometry, so shared geometry genuinely gates it.

The correction that applies to both plans

ParamHash::from_node hashes node.id first (cad_scene.rs:1871), before it touches a single geometric parameter.

This document previously said, in Phase 2:

Parts with equal ParamHash are geometrically identical by definition — that is what the hash means.

True, and useless: equal ParamHash also means the same node. Two identical columns at different coordinates have different hashes, so re-keying part_geoms by ParamHash would share nothing at all. The incoming plan inherited the same assumption from the same type name.

The id is harmless in the hash's current uses — MeshCache and part_geoms are keyed by NodeId and only ask "is this entry still valid for this node", where the id is a constant — so this is not a bug to fix, it is a second hash to add. ShapeHash: the same body with the id omitted. Pinned by param_hash_is_not_a_shape_key_it_includes_the_node_id in cad_scene.rs, added with this revision so the claim cannot be made a third time by reading the type name.

The field that must not exist

The incoming plan opens with "add bounding_sphere: (DVec3, f64) to CadNode, computed on add_part / update". Three reasons not to:

1. There are 77 CadNode { ... } construction sites across the crate and its tests. Every one becomes a place to get the sphere wrong, and the compiler only catches the ones that forget the field, not the ones that fill it in stale.
2. This codebase has already paid for exactly this mistake. The part_geoms staleness bug (viewport.rs:4629) was a map that had to be maintained by hand at every edit site; one site was missed and the viewport drew the pre-edit shape. The fix was to make the entry carry a hash that invalidates itself. ParamHash and PlacedHash exist because of that lesson.
3. The derived version already exists and is already cached. SceneCache::world_aabb_for is keyed by PlacedHash — so it is invalidated by a move and by a resize — and is benchmarked at 4.72× faster than recomputing (94 µs → 20 µs at 500 parts). A sphere is two lines from an AABB: centre = (min + max) / 2, radius = |max - min| / 2.

So: bounding_sphere(aabb) -> (DVec3, f64), pure, derived, in the cull module. No new field, no new invalidation surface.

Why merging bakes in a regression

Incoming Phase 2 proposes replacing part_geoms with one merged buffer per PartKind, "with per-part transform baked into vertex positions".

MeshCache stores local-space meshes on purpose. Its own docstring is explicit: "The transform and the material are NOT hashed. The cached mesh is local-space, so neither can change it." Baking transforms into vertices inverts that invariant, and the cost lands on the interaction users perform most:

• Today, dragging a part re-uploads that part's buffer, or nothing at all if only the transform changed — the transform never enters a vertex buffer, it is a per-draw shader value the vertex stage applies (mod.rs:377), and ParamHash deliberately excludes it for that reason.
• With transforms baked in, dragging one column invalidates the merged buffer for every column in the model, and re-concatenates and re-uploads all of it, on every frame of the drag.

The incoming plan's own draw loop shows the tension: it merges the buffers but still writes "offset handled via transform uniform set before each draw" — which is still one draw call per part, so the merge buys nothing it set out to buy. The two halves of that phase contradict each other.

Per-part colour is the second casualty. Selection and hover recolour a single part (viewport_render.rs:358-370). One merged buffer with one colour cannot express that without per-vertex colour and a full re-upload on every hover.

Keep local-space geometry; share it between parts of the same shape; vary transform and colour per instance. That is Phase 2 + Phase 3 below, and it is what the hardware wants anyway.

Instancing is not a shader rewrite

The incoming plan rates instancing "high effort, high risk, ~7 days, requires shader work" and hedges with "use cx.add_instances(...) if supported". It is supported, and the shader work is already done.

In the pinned Makepad fork (ecf5a572):

• Cx::begin_many_instances / end_many_instances (draw/src/draw_list_2d.rs:315,337) accumulate instance rows into a single draw item.
• DrawPbr already wraps that for exactly this case: begin_many_instances_for_mesh(cx, mesh), push_many_instance_with_transform(transform), end_many_instances(cx) (draw/src/shader/draw_pbr.rs:2696-2745) — one geometry, N transforms, one draw call. About thirty lines to mirror onto DrawCadMesh.
• Per-instance data is simply the #[live] fields declared after #[deref] draw_vars in a #[repr(C)] shader struct (DrawVars::as_slice documents that layout). DrawCadMesh already has that shape: color, transform, depth_clip, display_mode (mod.rs:152-171), and the vertex shader already reads self.transform (mod.rs:377).

One risk the incoming plan did not raise and one it did not need to:

• Batching reorders drawing. Safe here: DrawCadMesh is declared alpha_blend: false (mod.rs:346), so the mesh path is opaque and depth-tested. Had it been blended, regrouping would have changed the picture.
• A batch is per geometry. So Phase 2 really does gate Phase 3.

---

The phases

Phase 0 — Make it measurable — done (14aa0d5)

render_budget.rs (100% covered, floored in tools/test-cad-coverage.sh) owns the grid-loop decision and counts it, so the count and the drawing cannot disagree. grid_range() drives draw_2d_vector_scene's loops. bench_frame_submission_budget records tessellation and draw-call counts at 100/500/2,000 parts and two zoom levels in BENCH_BASELINE.md.

Baseline to beat, 1920×1080, 2,000 parts: 2,170 tessellations, 2,001 draw calls zoomed in; 2,270 / 2,001 zoomed out.

Phase 1 — Cull against the viewport — done (this commit)

New module cull.rs (100% covered, floored in tools/test-cad-coverage.sh), in the style of nav_pad.rs and render_budget.rs — pure, host-testable, and called by both draw loops and CadViewport::frame_budget, so the reported budget and the drawing cannot disagree:

draw_part_2d(centre, half_w, half_h, view, decorated) -> bool
Frustum::from_view_projection(view, projection)       -> Frustum
Frustum::draw_part_3d(aabb, decorated)                -> bool
world_aabb_from_local_bounds(min, max, model)         -> WorldAabb

Three things came out different from the plan above, and the plan was wrong about each:

1. AABB, not sphere. A bounding sphere around an AABB is looser at the same cost — a 6 m wall gets a 3 m radius ball. The p-vertex AABB-versus-plane test is strictly tighter, so there is no bounding_sphere helper at all, and nothing to go stale.
2. world_aabb_from_local_bounds is shared with pick_part, whose inline eight-corner transform it replaces. Two copies would be two chances to disagree about a part's bounds, and picking a part the renderer culled is exactly what that disagreement produces.
3. The "drawn extent" hazard resolved into two concrete rules, not one margin: the 2D test uses part_to_plane_2d and part_size_on_plane — the very values the draw uses, so it tests the drawn rect — and any selected or hovered part is never culled, because a selection highlight and a tooltip drawn at the cursor extend arbitrarily far from the part.

Both tests are conservative by construction: NaN geometry, negative extents and an identity camera matrix all fall through to "draw it". A part kept but invisible costs one submission; a part culled but visible is a bug the user sees.

Measured (bench_frame_submission_budget, 1920×1080, parts on a 200 m site — full tables in BENCH_BASELINE.md): drafting at a 5 m zoom a 2,000-part scene submits 12 parts instead of 2,000 (2,170 → 182 tessellations, 2,001 → 13 draw calls). In 3D at a 20 m working distance 63% of a 2,000-part model is off camera. The honest half: at a zoom or camera distance that fits the whole site on screen, culling removes nothing — all 2,000 parts are genuinely visible, and that case is what Phases 2 and 3 are for.

Not done here: the two hard-coded 1.2 margins in viewport_render.rs (397-400 and 1374-1377) still do not read render_budget::VIEW_MARGIN. cull.rs does, so the cull and the grid agree today by construction of the constant, not by construction of the code. Worth folding into Phase 4, which touches those loops anyway.

Phase 2 — Share geometry between parts of the same shape — done (this commit)

The prerequisite for instancing, and on its own it collapses GPU memory for the models this app is for: architectural drawings are repeated columns, windows and doors.

1. ShapeHash in cad_scene.rs: the solid payload and nothing else — no node id, no transform, no material. ParamHash is now defined in terms of it (id + ShapeHash), so the two cannot drift apart the way the plan's premise and the code did.
2. part_geoms is keyed by ShapeHash, value Geometry. The (hash, Geometry) pair and the "is this entry still valid" filter in the draw loop are both gone: the key is the content hash, so an edited part looks up a key that does not exist and gets a fresh upload. Staleness became structural rather than checked.
3. Eviction is by live shape, not live id. This is the one hazard the change introduces and it is not obvious: deleting one of two hundred identical columns must not drop the buffer the other 199 are drawing from. geometry_is_retained_by_live_shape_not_by_live_id pins it.
4. Geometry stays local-space; transform and colour stay per-draw values, exactly as before. Nothing is baked into a vertex buffer.

Measured (bench_geometry_buffers_shared_by_shape): 200 identical walls → 1 buffer. A 420-part repetitive model (columns, three wall lengths, two opening types) → 6 buffers, 70×. 420 all-distinct parts → 420, 1× — sharing is a property of the model, and that row is in the table so the ceiling is visible.

Draw calls are unchanged, as predicted: still one per visible part. That is Phase 3's job, and it is now unblocked.

Effort: under a day, against the week estimated. Two things made it cheap that the estimate did not know: MeshCache::get_or_build is already a pure function of node.solid, and the upload path (part_mesh_buffers_from_mesh → MeshSpace::Model) already produced transform-free, colour-free buffers. The estimate assumed those would need untangling; they were built right.

Phase 3 — Instance the 3D draw loop — done (this commit)

1. Three methods on DrawCadMesh — begin_instances, push_instance, end_instances — ported from DrawPbr::begin_many_instances_for_mesh / push_many_instance_with_transform / end_many_instances in the pinned fork. No shader change, as predicted: transform and color are #[live] fields after #[deref] draw_vars, which is exactly the per-instance row DrawVars::as_slice sends. The new #[rust] many_instances field sits before draw_vars for the same layout reason.
2. draw_scene collects the visible parts as (ShapeHash, (transform, colour)), groups them with batching::group_in_first_appearance_order, and issues one call per group.
3. frame_budget reports MeshSubmission { instances, batches } — named fields, because the whole point of the phase is that the two now differ and a caller that swapped two usizes would report the win backwards.

Measured (bench_frame_submission_budget): a 2,000-part model of six shapes draws in 6 calls instead of 2,000 — at every camera distance, including 400 m where the whole site is on screen and culling removes nothing. With every part a different size the count falls back to the visible-part count, which is the honest ceiling and is in the table.

What is not verified, plainly. The submission itself has never run: there is no GPU, no window and no Cx in this environment, and tests/ui.rs still fails at child-build exit 101 (Phase 0 of REVIEWS/CAD_COVERAGE_100_PLAN.md). What is verified is that it compiles against the real Makepad API, that the grouping is right (batching.rs, 100%, seven tests including "no item is lost or duplicated"), that the budget arithmetic is right, and that the batch cannot be left open on any path (every_instanced_batch_is_closed_before_the_loop_turns, a source check in the house style). Someone with a window needs to open a 3D model and confirm the picture is unchanged. Two things reduce the blast radius if it is not: begin_instances returning false falls back to the old one-call-per-part loop, and the batch order is deterministic, so a defect will reproduce rather than flicker.

Effort: a day. Risk: the highest of the four phases, for the reason above — not because the change is large.

Phase 4 — Reduce tessellation calls in 2D — done (this commit)

The honest small one, and it turned out to be the one that fixed the case Phase 1 could not: a 2,000-part model with everything on screen.

stroke() tessellates the whole accumulated path and then clears it (tessellate_path_stroke ends in path.clear()), so queueing many subpaths and stroking once costs one tessellation instead of N. That idiom was already in the file, in queue_dashed_line for the axis grid. Phase 4 applies it to the two loops that had not adopted it.

1. Base grid: two passes, two strokes. Minor lines queued and stroked at 0.55, majors at 1.6 — the width is what genuinely needs a separate call. GridRange::has_minor_lines / has_major_lines decide whether a pass runs, and frame_budget counts strokes with the same two predicates, so an empty group is not charged for. Minors are stroked first so majors land on top where they cross.
2. Parts grouped by colour. batching::ColorKey (bit-pattern key, exact round trip back to the colour) plus the same group_in_first_appearance_order Phase 3 groups shapes with. The colour policy moved to constants::part_outline_color so the renderer and frame_budget cannot disagree about how many groups a frame has.
3. Selected and hovered parts stroke last, in their own groups, so a highlight is never hidden under a neighbour's outline. Before, they were interleaved in document order and could be.
4. FrameBudget gained grid_lines and part_outlines alongside the call counts: geometry volume and call count are now different numbers and both are worth reading.

Measured (bench_frame_submission_budget, 1920×1080, 200 m site):

Zoom	Parts	Visible outlines	Tessellations	Before
5 m	2,000	12	4	2,170
200 m	2,000	2,000	4	2,270

The second row is the point. Everything is on screen, culling removes nothing, and the frame still costs four tessellation calls.

What this does not do: vertex volume is unchanged. The same 2,000 rectangles are tessellated; they are tessellated in two calls instead of 2,000. What is saved is per-call overhead — tessellator setup, two std::mem::takes and an append_geometry each time. If a 2,000-part plan view is still slow, the remaining cost is triangles, and that is Phase 5.

Guard: the_base_grid_and_the_parts_loop_stroke_per_group in viewport.rs matches braces to check that neither stroke() sits inside an item loop. Its first draft looked for a closing brace at a fixed indent, matched the wrong one, and failed on correct code — the test was wrong before the code was, for the second time in this plan.

Effort: a day. Risk: low, with one visible-behaviour caveat worth stating: parts of the same colour are now drawn together, so where two outlines of different colours overlap, which one is on top can change. Outlines are 1.8 px and the highlight ordering got strictly better, but it is a change to what is drawn, not only to how.

Phase 5 — Level of detail

A datagrid cell is never sub-pixel; a zoomed-out CAD part often is. Below a few pixels a filled box is indistinguishable from the mesh.

Gate on the numbers from Phases 1–4. If a 2,000-part scene at full zoom-out is comfortable by then, skip it: LOD adds a visual-fidelity axis to every future change and should not be paid for speculatively.

Effort: 1 week, if the numbers justify it.

Effort and risk, merged

Phase	Effort	Risk	Benefit
0 Measurement	done	—	Every claim below is now falsifiable
1 Culling (2D rect + 3D frustum)	done	Low	Submissions scale with visible parts
2 Shape-shared geometry	done	Medium	200 walls → 1 buffer; 70× on a mixed model
3 Instancing	done	Medium	2,000 parts, 6 shapes → 6 draw calls
4 2D tessellation batching	done	Low	2,000 parts on screen: 2,270 → 4 tessellations
5 LOD	~1 w	Low	Only if measured

What this plan deliberately does not do

• No BVH or octree. At 500–5,000 parts a linear pass over cached AABBs is 20–200 µs. A tree earns its complexity somewhere past ~50k parts and nothing suggests that is the target. If a benchmark ever shows the linear pass in the frame budget, revisit — with the number.
• No bounding_sphere field on CadNode. Derived from the cached AABB instead. See above.
• No merged vertex buffers with baked transforms. They convert an O(1) move into an O(parts-of-that-kind) re-upload per drag frame, and they cannot express per-part selection colour. See above.
• No render-path rewrite. Each phase is a local change behind a tested predicate or a key change. viewport_render.rs is 2,083 lines at 0% coverage; a rewrite there without the Phase 0 seam would be unverifiable.

Order and why

Phase 0 first because everything after it is otherwise unfalsifiable, and because it doubles as the first test coverage the render path has ever had. Phase 1 next: cheapest, largest, and it shrinks the input to every later phase. Phase 2 exists to make Phase 3 possible and is the invasive one, so it goes after the cheap wins are banked. Phase 3 is the real draw-call win and is now a port rather than a design. Phase 4 is small and honest about being small. Phase 5 only if measured.