6 commits

Author	SHA1	Message	Date
andodeki	4cbb155cb7	perf(cad): stroke per group, not per item -- Phase 4 of the render plan ... A 2,000-part plan view with everything on screen cost 2,270 tessellation calls a frame. It now costs four: two for the grid, two for the parts. `stroke()` tessellates the whole accumulated path and clears it -- `tessellate_path_stroke` ends in `path.clear()` -- so queueing many subpaths and stroking once is one tessellation instead of N. The idiom was already in this file: `queue_dashed_line` has done it for the axis grid since Phase 3.9, guarded by a test. Phase 4 applies it to the two loops that never adopted it. **Base grid: two passes, two strokes.** Minors queued and stroked at 0.55, majors at 1.6 -- the stroke width is the one thing that genuinely needs its own call. `GridRange::has_minor_lines`/`has_major_lines` decide whether a pass runs at all and `frame_budget` counts strokes with the same two predicates, because an empty `stroke()` still enters the tessellator and a budget that assumed two when the renderer made one would be wrong in the direction that hides work. Minors stroke first so majors land on top where they cross; same colour either way, so the only visible difference is that the thicker line wins a crossing, which is the right answer. **Parts grouped by colour.** New `batching::ColorKey` -- the bit pattern, because `f32` is not `Hash` and two colours whose bits differ are two colours -- feeding the same `group_in_first_appearance_order` that Phase 3 groups shapes with. The colour policy moved out of the two draw loops into `constants::part_outline_color`, so the renderer and `frame_budget` cannot disagree about how many groups a frame has; the 2D loop had `vec4(1.0, 0.82, 0.40, 1.0)` written out where `PART_SELECT_COLOR` already existed. **Selected and hovered parts stroke last**, in their own groups, so a highlight is never hidden under a neighbour's outline. They were interleaved in document order before and could be. `FrameBudget` gained `grid_lines` and `part_outlines` beside the call counts. Geometry volume and call count are different numbers now and both are worth reading -- `VectorSubmission { outlines, stroke_calls }` mirrors Phase 3's `MeshSubmission` for the same reason. Measured (bench_frame_submission_budget, 1920x1080, 200 m site): zoom 5 m, 2000 parts: 12 visible outlines -> 4 tessellations (was 2170) zoom 200 m, 2000 parts: 2000 visible outlines -> 4 tessellations (was 2270) The second row is the point, and it is the row Phase 1 could not move: everything is on screen, culling removes nothing, and the frame still costs four calls. WHAT THIS DOES NOT DO: vertex volume is unchanged. The same 2,000 rectangles are tessellated -- in two calls rather than 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 after this, the remaining cost is triangles, which is Phase 5 and should only happen if a measurement asks for it. One visible-behaviour caveat, stated rather than buried: parts of the same colour are now drawn together, so where two outlines of *different* colours overlap, which is on top can change. They are 1.8 px outlines and the highlight ordering got strictly better, but it is a change to what is drawn, not only to how. Two tests were wrong before the code was, which is becoming this plan's pattern. `constants.rs` fell to 81.82% and the coverage floor caught it -- `part_outline_color` had no tests, and it now has five. And the guard test's first draft looked for a closing brace at a fixed indentation, matched the wrong one, and failed on correct code; it matches braces properly now. Verified: tools/test-cad-coverage.sh green -- total 97.33%, batching.rs 100%, cull.rs 100%, render_budget.rs 99.68%, constants.rs 98.55%, all floors met; cargo check --locked -p nigig-build --lib clean; cargo test --lib 1114 passed (1100 + 14 new); --test cad_integration 154 passed; CAD_BENCH=1 harness green; cargo fmt --check and git diff --check clean.	2026-08-21 05:08:14 +00:00
andodeki	dc1defd7e8	perf(cad): one draw call per shape, not per part -- Phase 3 of the render plan ... A 2,000-part model of six repeated shapes now draws in six calls. It drew in 2,000 before, and in 733 after Phase 1 culling at a working camera distance -- but at 400 m, with the whole site on screen and nothing to cull, it still drew in 2,000. Culling decides which parts are submitted; instancing decides how many calls carry them. They are different axes and this is the one that does not care where the camera is. No shader change was needed and that was the surprise of the phase. `DrawCadMesh`'s `transform`, `color`, `depth_clip` and `display_mode` are `#[live]` fields after `#[deref] draw_vars` in a `#[repr(C)]` struct, which is precisely the per-instance row `DrawVars::as_slice` packs and sends. The loop was already sending instance rows -- one per call. Three methods (`begin_instances`, `push_instance`, `end_instances`), ported from `DrawPbr::begin_many_instances_for_mesh` in the pinned fork, keep the call open across a group instead. New `batching.rs` (pure, 100% covered, floored): group_in_first_appearance_order(items) -> Vec<(K, Vec<T>)> batch_count(keys) -> usize First-appearance order rather than iterating a `HashMap`, deliberately. `HashMap` order is randomised per process, so batch order -- and the order parts reach the GPU -- would differ between runs and after a rehash. For opaque depth-tested geometry that is invisible, which is exactly what makes it a bad thing to depend on: the day someone adds a translucent material it becomes a flicker that reproduces on one machine in five. `FrameBudget` now carries `MeshSubmission { instances, batches }`. Named fields rather than a second positional `usize` because the entire point of the phase is that the two numbers now differ, and a caller that swapped them would report the win backwards. Measured (`bench_frame_submission_budget`, counts): camera 20 m, 2000 parts: 733 visible -> 6 draw calls (was 2000) camera 80 m, 2000 parts: 1459 visible -> 6 draw calls (was 2000) camera 400 m, 2000 parts: 2000 visible -> 6 draw calls (was 2000) ...and with every part a different size, 733/1459/2000 -- the ceiling, which is in the table for the same reason Phase 2's is. WHAT IS NOT VERIFIED, plainly. The instanced submission has never run. There is no GPU, no window and no `Cx` here, and tests/ui.rs still fails at child-build exit 101. What is verified: it compiles against the real Makepad API; the grouping is right (seven tests, including "no item is lost or duplicated", which is the failure mode hardest to see in a screenshot); the budget arithmetic is right; and the batch cannot be left open on any path -- `every_instanced_batch_is_closed_before_the_loop_turns` is a source check in the same style as the redraw_all guard, because a batch left open drops its rows and the parts simply vanish with no error anywhere. Someone with a window needs to open a 3D model and confirm the picture is unchanged. Two things limit the damage if it is not: `begin_instances` returning false falls back to the old one-call-per-part loop (it returns false while the draw shader is still compiling, which happens on the first frames of every window), and batch order is deterministic, so a defect reproduces instead of flickering. Verified: tools/test-cad-coverage.sh green -- batching.rs 100%, cull.rs 100%, total 97.30%, all floors met; cargo check --locked -p nigig-build --lib clean at the 127-warning baseline; cargo test --lib 1100 passed (1091 + 9 new); --test cad_integration 154 passed; CAD_BENCH=1 harness green; cargo fmt --check and git diff --check clean.	2026-08-21 04:41:48 +00:00
andodeki	1e5d014198	perf(cad): one GPU buffer per shape, not per part -- Phase 2 of the render plan ... Two hundred identical columns held two hundred GPU vertex buffers. They build byte-identical triangles -- `CadSolid::build_mesh` is a pure function of the solid payload and returns local-space geometry, and the upload path (`part_mesh_buffers_from_mesh` -> `MeshSpace::Model`) carries no transform and no colour -- so the duplication bought nothing but memory and upload time. `part_geoms` is now `HashMap<ShapeHash, Geometry>`. **ShapeHash**, new 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` then `ShapeHash` -- rather than repeating the match arm by arm. That is deliberate: last commit corrected a plan written on the assumption that equal `ParamHash` meant "same shape", which the leading `node.id.hash()` quietly made false. With one shared body the two hashes cannot drift apart again, and `param_hash_moves_whenever_the_shape_hash_moves` pins the join. Three consequences worth naming: - **Staleness became structural.** The key IS the content hash, so the draw loop's `.filter(|(hash, _)| *hash == ParamHash::from_node(part))` is gone: an edited part looks up a key that does not exist yet and `ensure_part_geometry` uploads it on the same frame. There is no "entry uploaded from different parameters" state left to guard against, which is the class of bug `subdivide_selected` shipped. - **Eviction is by live shape, not live id.** This is the hazard the change introduces and it is not obvious: deleting one of two hundred identical columns must NOT drop the buffer the other 199 draw from. An eviction written as "remove the deleted node's entry" would blank most of the model. `geometry_is_retained_by_live_shape_not_by_live_id` pins it. - **Uploads deduplicate within the frame.** Without the `queued` set, the first frame of a 200-column scene would call `get_or_build` two hundred times before the map had anything in it. Measured (`bench_geometry_buffers_shared_by_shape`, counts not timings): 200 identical walls 200 parts -> 1 buffer (200x) 420-part repetitive model 420 parts -> 6 buffers (70x) 420 all-distinct parts 420 parts -> 420 buffers (1x) The last row is in the table on purpose. Sharing is a property of the model, not of the code; a scene where every part differs gets nothing from this phase. And draw calls are unchanged -- still one per visible part -- exactly as the plan predicted. Collapsing those is Phase 3, which needed these shared buffers to be possible at all. Estimated at a week, took under a day. Two things the estimate did not know: `MeshCache::get_or_build` was already a pure function of `node.solid`, and the upload path already produced transform-free, colour-free buffers. It assumed both would need untangling; they were built right the first time. Verified: tools/test-cad-coverage.sh green -- total 97.27%, cad_scene.rs 98.56%, cull.rs 100%, all floors met; cargo check --locked -p nigig-build --lib clean (and one warning fewer: the `ParamHash` re-export in mod.rs is no longer needed by the widget layer); cargo test --lib 1091 passed (1085 + 6 new); --test cad_integration 154 passed; CAD_BENCH=1 harness green; cargo fmt --check and git diff --check clean.	2026-08-21 04:18:33 +00:00
andodeki	4aaebafe4e	perf(cad): cull parts against the viewport -- Phase 1 of the render plan ... Before this, `grep -niE "cull|frustum|offscreen|in_view"` over the 2,438 line renderer returned nothing. Every part in the document was tessellated in 2D and issued its own draw call in 3D, on screen or not. At a 5 m drafting zoom over a 200 m site, a 2,000-part model spent 2,000 tessellations and 2,001 draw calls per frame to show twelve parts. New `cull.rs` -- pure, host-testable, 100% covered with a floor in tools/test-cad-coverage.sh: 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 It lives in its own module for the same reason nav_pad.rs and render_budget.rs do: viewport_render.rs is 2,438 lines at 0.00% coverage and needs a Cx, so a predicate written inline there could not be tested at all. Both draw loops AND CadViewport::frame_budget call these same functions, which is what stops the reported budget from drifting away from what is drawn -- the discipline render_budget.rs already applies to the grid loops. Measured (bench_frame_submission_budget, 1920x1080, parts on a 200 m site; full tables in BENCH_BASELINE.md): 2D, 5 m zoom, 2,000 parts: 12 visible. 2,170 -> 182 tessellations, 2,001 -> 13 draw calls. 3D, 20 m camera, 2,000: 733 draw calls, 63% culled. 3D, 80 m camera, 2,000: 1,459 draw calls, 27% culled. Whole site on screen: nothing culled, and nothing should be. That last row is the honest half and it is in the doc too: when the view holds the whole model, all 2,000 parts are genuinely visible and culling cannot help. Phases 2 and 3 (shape-shared geometry, then instancing) are what address that case. Three things came out different from the plan I wrote last commit, and the plan was wrong about each: - **AABB, not bounding sphere.** A 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, and in particular no bounding_sphere FIELD on CadNode: the AABB is already cached by PlacedHash in SceneCache, which invalidates itself on a move or a resize, where a stored field would have to be maintained at 77 construction sites. - **world_aabb_from_local_bounds is shared with pick_part**, replacing the eight-corner transform that was inline there. Two copies would be two chances to disagree about a part's bounds, and picking a part the renderer culled is precisely the bug that disagreement produces. - **The "drawn extent" hazard split into two concrete rules.** The 2D test takes part_to_plane_2d and part_size_on_plane -- the very values the draw uses -- so it tests the drawn rect, not the model extent. And no selected or hovered part is ever culled: the highlight and the tooltip are drawn at the cursor, arbitrarily far from the part. The frustum comes from Gribb-Hartmann row arithmetic on projection * view, deliberately not from mat4_inverse -- that function had a mistyped index in all sixteen cofactors until 2ea5a74, and the terms that were wrong are exactly the ones a perspective matrix makes non-zero. Makepad's Mat4f::perspective is the OpenGL convention (xr/src/scene/xr_root.rs builds the camera with it), so -w <= x,y,z <= w is right; a 0..w depth projection would only make the near plane more permissive, which is the safe direction. Both predicates are conservative by construction: NaN coordinates, NaN AABBs, negative extents and an identity camera 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. One test of mine was wrong before the code was: the first draft asserted a part 40 m off axis was invisible from a 50 m camera at 60 deg on 16:9. tan(30 deg) * 16/9 is 1.03, so the horizontal half-angle reaches past 45 deg and the part is on screen. The frustum was right; the test is now narrow-fov and says so in its docstring. Verified: tools/test-cad-coverage.sh green -- cull.rs 100.00%, total 97.26% (was 97.20%), all floors met; cargo check --locked -p nigig-build --lib clean; cargo test --locked -p nigig-build --lib 1085 passed (1062 + 23 new); --test cad_integration 154 passed; cargo fmt --check clean; git diff --check clean. Not done here: the two hard-coded 1.2 margins in viewport_render.rs (397-400, 1374-1377) still do not read render_budget::VIEW_MARGIN. cull.rs does, so cull and grid agree by the constant rather than by the code. Folded into Phase 4, which rewrites those loops anyway.	2026-08-20 21:45:45 +00:00
andodeki	42b1e7152a	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
andodeki	f27ace8b7f	docs(cad): render optimisation plan — and a correction to the analysis it rests on ... Writing the plan meant checking the assumption the analysis rested on, and the assumption was wrong. I had called `draw_vector.stroke()` a "tessellation/flush point" and then reasoned about the 2D path as though each cost a draw call — "~670 flush points per frame where four would do". Reading draw/src/shader/draw_vector.rs instead of inferring from a comment: begin() clears CPU accumulation buffers stroke() calls tessellate_path_stroke + append_geometry — no draw call end() the ONLY place cx.new_draw_call appears, twice, both inside it The whole 2D vector scene 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 wording — "thousands of tessellations per frame" — was accurate and literal, and I read "draw call" into it. What survives: no culling anywhere, which was and remains the main finding; and 3D issuing one draw call per part, which is where draw-call multiplication is actually real. What changes: batching the 2D loops is a CPU per-call-overhead win, not a draw-call win, so it drops from second place to third in the plan and the document says plainly that it is the small one. The plan itself, ordered on the corrected facts: 0. Make it measurable. profile_benchmarks.rs has sixteen benchmarks and none measures frame submission. Check whether Cx already counts draw calls; if not, add a counting seam. This doubles as the first test surface viewport_render.rs has ever had — it is 2,083 lines at 0.00% coverage. 1. Cull against the viewport, reusing the cached world AABB that is already benchmarked at 4.72x and already wired to the mouse-move path but not the per-frame one. 2. Key part_geoms by ParamHash instead of part id so identical parts share geometry — the real draw-call win, and the one that matters for drawings full of repeated columns. 3. Reduce 2D tessellation calls with the queue-then-stroke idiom the axis grid already uses. 4. LOD, only if the Phase 0 numbers justify it. Explicitly not doing: a BVH (a linear pass over cached AABBs is microseconds at this scale) or a render-path rewrite (0% coverage). One implementation hazard recorded in Phase 1: cull on the drawn extent, not the model extent. Selection outlines and hover highlights exceed a part's AABB, and culling on the AABB alone makes them vanish at the viewport edge.	2026-08-20 20:44:18 +00:00