# 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 `usize`s 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::take`s 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.