3 commits

Author	SHA1	Message	Date
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
andodeki	e6fbcc12da	docs(cad): analysis — the renderer has no culling and no draw batching ... Asked whether the CAD viewport has the kind of minimal-drawcall strategy a datagrid needs, and whether virtual-viewport techniques transfer. Answer: the codebase already knows the technique, applies it in exactly one function, and does not apply it in the two loops that run every frame. `draw_vector.stroke()` is the tessellation/flush point. `queue_dashed_line` queues segments and issues ONE stroke for the whole axis grid, and a test in viewport.rs enforces it — "the grid lines should share exactly one stroke", with a comment warning that stroking per dash is "thousands of tessellations per frame". The discipline is understood and guarded. Two loops away from it: - the base grid strokes once per line (~167 on a 1080p viewport, where two would do: one for minors, one for majors); - the parts loop strokes once per part; - the 3D path issues one draw_mesh.draw() per part, each with its own geometry buffer and uniforms. No instancing, no state sorting. And there is no culling at all: grep for cull/frustum/offscreen/in_view across the 2,461-line renderer returns nothing. Every part is submitted every frame whether on screen or not. The sharp part is that the broad-phase already exists. SceneCache::world_aabb_for is cached by PlacedHash and BENCH_BASELINE.md records it at 4.72x faster than recomputing. Its only caller is pick_part — the mouse-move path, which is additionally throttled by HOVER_PICK_MIN_MOVE_PX. The cheap visibility test is wired to the occasional path and not to the per-frame one. The grid, to be fair, IS virtualised properly: visible world bounds plus 20%, with a 1/2/5 nice-number step that adapts to zoom. That is the datagrid technique done right. It just stops at the grid. Also recorded: what does not transfer. Widget recycling has no CAD analogue, and index-range virtualisation does not either — CAD is continuous space, so it needs a spatial test rather than a row range. At 500 parts a linear pass over cached AABBs is microseconds; a BVH only earns its complexity somewhere past ~50k parts and nothing suggests that is the target. Caveat stated in the document: no profiling was run, and there is no frame-submission benchmark in profile_benchmarks.rs — its sixteen benchmarks all measure CPU work. The structural claims are read off the code and are solid; the consequence in dropped frames is not measured, and measuring it needs a live Cx.	2026-08-20 20:37:00 +00:00