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. **Before Phase 1 there was no viewport culling.** The original
   `grep -niE "cull|frustum|offscreen|in_view"` over the 2,461-line
   renderer returned nothing. Every part was submitted every frame, on
   screen or not — in 2D that was wasted tessellation, in 3D a wasted
   draw call. Phase 1 now owns the shared visibility predicates.
2. **Before Phases 2–3, 3D issued one draw call per part**, each with
   its own geometry buffer, transform and colour. `part_geoms` was keyed
   by part id, so 200 identical columns were 200 uploads and 200 draws.
   Phases 2–3 now share shape geometry and instance repeated parts.
3. **Before Phase 4, the base grid and parts loop tessellated per item**,
   where the file's own `queue_dashed_line` idiom — queue segments,
   stroke once, guarded by a test — showed how not to. Phase 4 now
   groups those paths; Phase 5 reduces the geometry for sub-pixel parts.

---

# 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` (99.75% line-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
culling, batching and LOD counts at 100/500/2,000 parts across the two
original zoom levels plus a far-zoom LOD case 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.

**Follow-up completed in Phase 5:** the hard-coded `1.2` margins in
`viewport_render.rs` now read `render_budget::VIEW_MARGIN`, including the
auxiliary axis, story-level and reference-plane paths. The cull, grid and
overlays therefore share both the value and the source of truth.


## 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 at the Phase 4 boundary:** 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. The Phase 5 gate below measures whether that
remaining geometry volume is actually unresolvable at the current zoom.

**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 — **done** (this commit)

The Phase 4 budget separated geometry volume from tessellation-call
count, which made the gate answerable. In 2-D, a 1 m part is only about
2.7 logical pixels across at the 200 m site view and about 1.1 pixels at
500 m. In 3-D, the 400 m camera case leaves all 2,000 parts visible, but
1,620 of them have a projected AABB below three pixels. Both are measured
cases where a full representation carries shape detail the display cannot
resolve.

Phase 5 now covers both rendering paths:

1. `lod.rs` projects 2-D plane extents and 3-D world-AABB corners into
   logical pixels. The shared threshold is strictly below 3 pixels.
   Invalid view, extent, clip-space, or proxy-bound data falls back to
   the full representation, never to hidden geometry.
2. In 2-D, ordinary sub-pixel parts become one bounded 2×2 logical-pixel
   filled marker. Full outlines remain colour-batched. Selected and
   hovered parts always stay full detail, so the interaction affordance
   is not replaced by a generic marker.
3. In 3-D, ordinary sub-pixel parts become instances of one shared unit
   cube whose transform is built from the part's world AABB. This keeps
   the approximate footprint and colour while replacing expensive
   cylinder, sphere, CSG, section, or otherwise high-resolution meshes.
   The proxy is stored separately from `part_geoms`, so document shape
   cache eviction cannot remove it. The original local-space geometry is
   not rewritten or re-uploaded.
4. The filled 3-D path and the wireframe/hidden-line edge overlay use the
   same 3-D LOD policy. A proxy marker that cannot be projected falls
   back to the full edge path rather than disappearing.
5. `FrameBudget` reports full 3-D shape instances and simplified proxy
   instances separately, and counts the shared proxy batch. The budget
   cannot mistake a reduction in full-shape batches for a reduction in
   visible parts.
6. The remaining hard-coded `1.2` view margins in the auxiliary 2-D
   drawing paths now use `render_budget::VIEW_MARGIN`, so cull, grid and
   overlays share both the value and its source of truth.

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

| Camera/zoom | Parts | Visible | Full detail | LOD representation | Calls |
|---|---:|---:|---:|---:|---:|
| 2-D, 5 m | 2,000 | 12 | 12 outlines | 0 markers | 4 tessellations |
| 2-D, 200 m | 2,000 | 2,000 | 1 outline | 1,999 markers | 4 tessellations |
| 3-D, 80 m | 2,000 | 1,459 | 1,459 meshes | 0 proxy instances | 6 shape calls |
| 3-D, 400 m | 2,000 | 2,000 | 380 meshes | 1,620 proxy instances | 7 calls |

At 400 m with six catalogue shapes, the full-detail groups still use six
calls and the simplified parts use one shared proxy call: **7 calls
instead of 2,000**. With every part distinct, the same row is 381 calls
(380 full shapes plus one proxy batch), which is the honest remaining
ceiling. The 2-D rows retain one vector draw call; LOD reduces geometry
volume rather than claiming another draw-call change there.

The implementation is intentionally a proxy LOD, not a second set of
parametric meshes. It avoids a per-shape resolution cache and keeps
transitions deterministic. A live GPU/window visual check is still
required for release confidence; this environment can compile and execute
the policy and submission arithmetic, but cannot execute Makepad's draw
submission.

**Effort:** under two days. **Risk:** low-to-medium, with an intentional
visual-fidelity boundary below three pixels, conservative malformed-state
fallbacks, and the existing first-frame instancing fallback preserved.

## 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 2-D/3-D LOD | done | Medium | 1,999 outlines and 1,620 meshes → bounded markers/proxies |

## 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,749 lines
  and remains outside the host-only coverage harness; 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 is intentionally gated on a
measured sub-pixel case and now covers both 2-D markers and 3-D
bounding-box proxies. The remaining
release check is visual GPU verification, not another speculative geometry
cache.