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 `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.