nigig-org/REVIEWS/CAD_DRAWCALL_STRATEGY_ANALYSIS.md
andodeki 42b1e7152a
Some checks failed
nigig-build (CAD) / supply-chain (push) Has been cancelled
nigig-build (CAD) / cad-module (push) Has been cancelled
nigig-build (CAD) / full-crate-check (push) Has been cancelled
nigig-build (CAD) / cad-engine-coverage (push) Has been cancelled
nigig-build (CAD) / doc-workspace-coverage (push) Has been cancelled
nigig-build (CAD) / cad-widget-coverage (push) Has been cancelled
repo hygiene / hygiene (push) Has been cancelled
docs(cad): merge a second render review into the plan, and correct the hash both reviews got wrong
A second optimisation plan arrived (frustum culling, geometry merging,
GPU instancing, octree/LOD). Rather than run two plans, every claim in
it was checked against the tree at 14aa0d5 and merged into
REVIEWS/CAD_RENDER_OPTIMISATION_PLAN.md with a verdict and the evidence
behind it. Three of its ideas change what the phases will do; two would
have made things worse; one of them found nothing, but reading it found
something in mine.

The correction that matters, and it lands on my own plan:

  ParamHash::from_node hashes node.id FIRST (cad_scene.rs:1871), before
  a single geometric parameter. My Phase 2 said "parts with equal
  ParamHash are geometrically identical by definition -- that is what
  the hash means". True and vacuous: equal ParamHash also means the same
  node. Two identical columns hash differently, so re-keying part_geoms
  by ParamHash -- the central move of that phase -- would have shared
  exactly nothing. The incoming plan inherited the same assumption from
  the same type name, which is the kind of coincidence that argues for a
  test rather than a paragraph, so there is now one:
  param_hash_is_not_a_shape_key_it_includes_the_node_id.

  The id is not a bug. MeshCache and part_geoms are both keyed by NodeId
  and only ask "is this entry still valid for this node", where the id
  is a constant. Sharing geometry needs a second hash, ShapeHash: the
  same body without the id. Phase 2 now carries that instead.

Adopted from the incoming plan:

- Frustum culling in 3D. SceneState3D { view, projection } is already
  captured into self.last_view/last_proj every frame (viewport.rs:3781)
  immediately before draw_scene, so the planes are a row add/subtract
  away -- and deliberately not via mat4_inverse, which had a mistyped
  index in all sixteen cofactors until 2ea5a74. My Phase 1 had only an
  AABB-versus-rect test, which is right for 2D and wrong for perspective.
- Instancing, upgraded from "confirm the machinery exists" to "here it
  is". DrawPbr::begin_many_instances_for_mesh /
  push_many_instance_with_transform / end_many_instances
  (draw/src/shader/draw_pbr.rs:2696-2745 in the pinned fork) do exactly
  this for a shared mesh geometry, and per-instance data is just the
  #[live] fields after #[deref] draw_vars -- the layout DrawCadMesh
  already has (color, transform, depth_clip, display_mode). No shader
  rewrite; ~30 lines ported. The incoming plan rated it 7 days and high
  risk with "if supported"; it is neither. DrawCadMesh is also declared
  alpha_blend: false, so regrouping draw order is safe here.
- Its dependency graph: instancing batches per geometry, so shared
  geometry genuinely gates it.

Rejected, with reasons in the doc:

- bounding_sphere as a stored field on CadNode. 77 construction sites,
  and this codebase has already paid for hand-maintained derived state
  once -- the part_geoms staleness bug (viewport.rs:4629) drew the
  pre-edit shape because one edit site forgot to invalidate. The sphere
  is two lines from SceneCache::world_aabb_for, which is keyed by
  PlacedHash so it invalidates itself on a move or a resize, and is
  benchmarked at 4.72x.
- Merged vertex buffers per PartKind with transforms baked in. MeshCache
  stores local-space meshes on purpose ("the transform and the material
  are NOT hashed"); baking transforms inverts that and turns dragging
  one column into a full re-concatenate and re-upload of every column,
  every frame of the drag. It also cannot express per-part selection
  colour. That phase's own draw loop still sets a transform per part,
  which is still N draw calls -- the two halves contradict each other.
- Octree above 500 parts. The cached world-AABB pass is 20 us at 500
  parts, 0.12% of a 16.7 ms frame. A tree that replaces 20 us cannot pay
  for itself. Revisit on a measurement, not a part count.
- "Log skipped-part count per frame" as verification. Phase 0 already
  built the counting seam; a log line nobody reads is a step that cannot
  fail.

Also corrected REVIEWS/CAD_DRAWCALL_STRATEGY_ANALYSIS.md, which is where
the ParamHash claim originated: section 0 is now two corrections rather
than one.

Verified: tools/test-cad-coverage.sh green, total 97.20%, all floors met,
cad_scene.rs 98.53%; rustfmt clean; git diff --check clean.
2026-08-20 21:17:07 +00:00

12 KiB
Raw Permalink Blame History

Does the CAD viewport have a minimal-drawcall strategy?

Short answer: no, but the codebase already knows how, and applies the technique correctly in exactly one place. The two hottest loops do not use it, and there is no viewport culling anywhere in the renderer — despite the data structure needed for it already existing, being benchmarked, and being used on a colder path.

This was prompted by a datagrid brief asking for "virtual viewport on both axes" and "an optimal minimal drawcall strategy". Those are two distinct techniques. CAD has a partial version of the first and essentially none of the second.

Everything below is from reading viewport_render.rs (2,461 lines) and viewport.rs. No profiling was run — see "What is not measured".

0. Corrections to earlier versions of this note

0.1 stroke() is not a draw call

The first version of this document called stroke() a "tessellation/flush point" and then reasoned about the 2D path as if each one cost a draw call — "~670 flush points per frame where four would do". That is wrong, and it was wrong because I inferred Makepad's batching model from a comment instead of reading it.

Read now, in draw/src/shader/draw_vector.rs:

  • begin() clears CPU-side accumulation buffers (acc_verts, acc_indices).
  • stroke() calls tessellate_path_stroke(...) and append_geometry(...). It tessellates the current path into those buffers. It issues no draw call.
  • end(cx) is the only place cx.new_draw_call(&self.draw_vars) appears — twice in the whole file, both inside end(), for the gradient and plain paths.

So the entire 2D vector scene — grid, parts, overlays — is one draw call. Makepad's DrawVector is already a batching design and the CAD code uses it correctly in that respect. The guard test's own wording, "thousands of tessellations per frame", was accurate and literal; I read "draw call" into it.

0.2 ParamHash is not a shape key

Section 3 and the suggested order both said identical parts could share a GPU buffer by re-keying part_geoms on ParamHash. They cannot: ParamHash::from_node hashes node.id first (cad_scene.rs:1871), before any geometric parameter. Two identical columns at different coordinates therefore have different ParamHashes, and re-keying on it would share nothing whatsoever.

I took the type name and its docstring ("keyed on the node's mesh parameters") for the whole story and did not read the body. The same assumption appeared independently in a second optimisation review, which is the sort of coincidence that argues for a test rather than a note: param_hash_is_not_a_shape_key_it_includes_the_node_id in cad_scene.rs now pins it.

The id is not a bug in ParamHash — its two users, MeshCache and part_geoms, are keyed by NodeId and only ask "is this cached entry still valid for this node", where the id is a constant. Sharing geometry needs a second hash, ShapeHash: the same body without the id. REVIEWS/CAD_RENDER_OPTIMISATION_PLAN.md Phase 2 carries it.

What survives the correction, and what changes:

Claim Status
No viewport culling anywhere Stands. Still the main finding.
3D issues one draw call per part Stands. That is where draw-call multiplication is real.
2D costs ~670 draw calls a frame Wrong. One draw call; the per-item cost is CPU tessellation.
Batching the 2D loops is worthwhile Weaker, still true. It cuts tessellation-call overhead, not draw calls.

The rest of this document is the corrected version.

1. Batching: the technique is present, used once, guarded by a test

draw_vector is an immediate-mode path builder. stroke() tessellates the current path into an accumulation buffer; end() turns the whole accumulation into one draw call. So the cost of calling stroke() per item is CPU tessellation and per-call setup, not GPU submission. The codebase demonstrably knows this. From the axis grid:

fn queue_dashed_line(&mut self, x1: f32, y1: f32, x2: f32, y2: f32) {
    while pos < dist {
        self.draw_vector.move_to(...);
        self.draw_vector.line_to(...);      // queue only
        pos = seg_end + gap;
    }
}                                            // no stroke() here

…and a test in viewport.rs enforces it:

"the grid lines should share exactly one stroke"

"draw_dashed_line strokes per dash, which is thousands of tessellations per frame"

That is precisely the minimal-drawcall discipline the datagrid brief asks for. It is applied to draw_axis_grid_2d and nowhere else.

Where it is not applied

draw_2d_vector_scene has 13 stroke() sites, three of them inside loops:

for i in start_x..=end_x {              // base grid, vertical lines
    self.draw_vector.move_to(p1);
    self.draw_vector.line_to(p2);
    self.draw_vector.stroke(if is_major { 1.6 } else { 0.55 });   // per line
}
// …same again for horizontals…

for part in read_parts(&doc).iter() {   // every part, every frame
    self.draw_vector.set_color(col);
    self.draw_vector.rect(...);
    self.draw_vector.stroke(1.8);                                  // per part
}

The grid targets ~18 px spacing, so a 1920×1080 viewport is roughly 107 vertical + 60 horizontal ≈ 167 tessellation calls for the grid alone, plus one per part. At 500 parts that is ~670 calls into tessellate_path_stroke per frame where the file's own proven idiom would give roughly four.

To be clear about the magnitude after the correction above: this is CPU work — tessellator setup, two std::mem::takes and an append_geometry per call — not 670 draw calls. The same total segment count still has to be tessellated either way. The win is per-call overhead, and it is worth having, but it is a smaller prize than culling or 3D instancing.

The base grid is the easier of the two: it only alternates between two stroke widths, so it is two batches, not 167. The parts loop alternates colour per part (selected / hovered / own colour), so it needs grouping by colour or a per-instance colour attribute.

3D is one draw call per part

for part in read_parts(&doc).iter() {
    if let Some(geom) = self.part_geoms.get(&part.id.raw()) {
        self.draw_mesh.transform = part_model_matrix_cadnode(part);
        self.draw_mesh.color = ;
        self.draw_mesh.draw(cx, geom.geometry_id());   // one call, per part
    }
}

Each part carries its own geometry buffer and its own transform/colour uniforms, so N parts is N draw calls. No instancing, no batching, no sorting by state.

2. Virtual viewport: present for the grid, absent for the content

The grid is virtualised, and well:

let world_left  = self.pan_2d.x - half_w * 1.2;   // visible bounds + 20%
let start_x = ((world_left - gx) / step).floor() as i32;
let end_x   = ((world_right - gx) / step).ceil()  as i32;
for i in start_x..=end_x {  }

Only visible grid lines are emitted, and the step adapts to zoom with a 1/2/5 nice-number progression. That is the datagrid's "only materialise what is on screen", done properly.

The parts are not culled at all. grep -niE "cull|frustum|offscreen|in_view" over the whole 2,461-line renderer returns nothing. Every part is projected and submitted every frame whether or not it is on screen, in both the 2D and 3D paths.

The part that stings

The broad-phase structure needed for culling already exists:

pub fn world_aabb_for(&self, node: &CadNode, build: impl FnOnce(&CadNode) -> WorldAabb) -> WorldAabb

It is cached by PlacedHash, and BENCH_BASELINE.md records it at 4.72× faster than recomputing (94 µs → 20 µs at 500 parts). Its only caller is CadViewport::pick_part — the picking path, which runs on mouse-move and is additionally throttled by HOVER_PICK_MIN_MOVE_PX.

So the cheap visibility test exists, is cached, is benchmarked, and is wired to the path that runs occasionally rather than the path that runs every frame.

3. Which datagrid techniques transfer

Technique Transfers? How it maps to CAD
Virtual viewport Yes, directly Test each part's cached world AABB against the view rect (2D) or frustum (3D) before submitting. The AABB cache is already built.
Minimal draw calls Partly — 2D is already one call DrawVector batches to a single draw call at end(). The transferable part is reducing tessellation calls via the queue_dashed_line idiom. The real draw-call win is in 3D, below.
Instancing Yes, and it is the big one 200 identical columns are one mesh. part_geoms is keyed by part id, so today that is 200 uploads and 200 draws. Keyed by shape it becomes one geometry plus 200 instance transforms. Keyed by ParamHash — see correction 0.2 above.
Proper clipping Partly The datagrid needs nested clip rects per cell; CAD has one viewport. Parts are currently emitted in screen coordinates and left to the GPU to clip, which is correct but pays the submission cost first — culling fixes that more cheaply than clipping.
Level of detail Yes, and CAD needs it more A datagrid cell is never sub-pixel; a zoomed-out CAD part often is. Below a few pixels, a box outline or a point is indistinguishable from the mesh.

What does not transfer

  • Widget recycling. Datagrid cells host child widgets and need a pool. CAD parts are geometry, not widgets; there is nothing to recycle.
  • Index-range virtualisation. A grid virtualises by row/column index, which is O(1) to compute. CAD is continuous space, so the equivalent needs a spatial test. At the scale this app targets a linear pass over cached AABBs is fine — it is a few microseconds for 500 parts. A BVH or grid hash only earns its complexity somewhere north of ~50k parts, and nothing here suggests that is the target.

4. What is not measured

No profiling was run for this note, and there is no draw-call benchmark in profile_benchmarks.rs — the sixteen benchmarks there measure CPU work (mesh building, caching, hashing, export, snapshot sync) and none measures frame submission. So:

  • The structural claims above are read off the code and are solid: the loops, the absence of culling, the per-part draw call.
  • The consequence — whether this actually drops frames, and at what part count — is unmeasured. It would need a frame-time benchmark with a live Cx, which is the same harness problem described in REVIEWS/CAD_COVERAGE_100_PLAN.md.

Worth stating plainly because the CPU-side work here is genuinely good: the mesh cache is 488× on a warm hit, the scene cache is 512×, the AABB cache 4.72×. Someone did the caching work carefully. The gap is specifically in per-frame submission, which no benchmark covers.

5. Suggested order, cheapest first

  1. Cull parts against the view rect using the existing cached AABB. Perhaps twenty lines in each of the two loops. Biggest win per line changed, and it makes every later optimisation cheaper by shrinking the working set.
  2. Key part_geoms by shape rather than part id and draw identical parts instanced. After culling this is the largest remaining win, because 3D is where draw calls actually multiply. The key has to be a new ShapeHash, not ParamHash — correction 0.2 above.
  3. Add a frame-submission benchmark so the rest stops being unmeasured. See the phased plan.
  4. Batch the base grid into two tessellation calls (minor, major) instead of ~167, and group the parts loop by colour. CPU-side win; the idiom and its guard test already exist one function away.
  5. Level of detail for sub-pixel parts.

Phased in REVIEWS/CAD_RENDER_OPTIMISATION_PLAN.md.