Went looking for remaining defects before starting the viewport split and
found silent data loss in the two Save buttons.
cad_store::save_cad_script(&project.id, &source).ok();
self.view.label(..).set_text(cx, "Saved");
save_cad_state(&source, &solid).ok();
self.view.label(..).set_text(cx, "Saved with mesh");
Both discard a Result carrying a real io::Error and then report success
unconditionally. The user is told their work is safe when nothing was
written.
This is reachable, not theoretical. `cad_store::cad_projects_dir()` only
*logs* a `create_dir_all` failure and returns the path anyway, so the
following `fs::write` fails with a genuine error -- which was thrown
away. An unwritable data directory, a full disk or a permissions problem
all render as "Saved".
Save-As had a third silent failure on the same line of reasoning: if
`bake_parts_solid()` returned None the save never even ran, and it still
said "Saved with mesh".
Fixed via save_status_message(what, result), so the label is derived from
the outcome instead of assumed. Three distinct messages now: the caller's
success wording, "Save failed: <cause>" with the underlying error text
preserved, and a specific message for the no-mesh and no-active-project
cases, which were previously indistinguishable from success.
Also replaced the hand-rolled widget borrow in Save-As with
with_viewport, the helper added in 5.2.
Tests assert the two properties that matter: a failed save must not read
as success and must carry its cause through to the user, and a
successful save must use the caller's wording. Negative-tested.
CI gate added and verified in both directions: greps for a discarded
Result on a save/write/persist/store/export call. A dropped Result is
merely untidy in most places; on a save path it is data loss.
These were the only two instances -- the CAD module now has zero
discarded write Results.
685 lib + 154 integration, 0 failed. All seven gates pass.
Updated all makepad dependencies from rev 2c5cd97 to 817d881 which includes:
- Terrain hillshade landcover draping (drape.rs)
- Route overlays, markers, and position puck (overlay.rs)
- Map icon management system (icons.rs + 50 SVG icons)
- 3D road elevation and seamless joins
- Building shadow geometry and terrain shadows
- Night themes and emissive roads
- Water, grass, and shrub rendering
- Optimized road geometry with 2D/3D mode transitions
- i_overlay library for polygon boolean operations
This brings nigig-map in sync with the latest makepad dev branch improvements.
export_floor_plan_pdf, export_3d_viewer, export_stl and export_svg each
opened with a byte-identical 23-line block: borrow the main viewport,
bail if it is missing, bail if it has no parts, then take the cached
scene, the shared mesh cache and the part count. Verified identical with
a regex over all four -- the only difference was the label prefix
("PDF", "3D", "STL", "SVG"), and even that was consistent within each
copy.
Now one method, export_scene_source(cx, kind), returning Option of the
triple and setting the status label itself. Four call sites, each two
lines.
This is the same duplication shape that hid the last three real bugs in
this module: nine copies concealed the properties-panel no-op, one
uninspected copy concealed the dead Extend tool, and nine keymap arms
concealed the KeyCode bindings. Nobody re-reads the fourth copy of a
23-line block. There is no bug hiding in these four -- I checked -- but
the next edit to one of them would only have landed in one.
The user-visible strings are the behaviour here, so they are pinned
rather than assumed: ExportBlocked + export_blocked_message hold the two
messages, and export_prologue_tests asserts all eight exact strings the
copies produced. A second test asserts the two reasons stay
distinguishable -- "no viewport" is a wiring fault, "no parts" is the
user having drawn nothing, and reporting one as the other sends someone
to debug the wrong thing.
Negative-tested: making NoViewport render the NoParts text fails both.
bake_parts deliberately keeps its own prologue. It needs a different
tuple (solid + script, not scene + cache + count), so folding it in
would mean a helper with an unused half.
683 lib + 154 integration, 0 failed. All six CI gates pass.
ProjectionLayoutTree::hit_test's nearest-glyph fallback resolved any
point to a glyph, so taps landing inside tables or advanced nodes never
reached the widget's table_hit_test/node_hit_test branches — production
cell taps silently moved the text caret instead of parking in cells.
The fallback now returns None when the point is inside a table or node
rect (those layers own their taps), while empty-space snapping beside
text still places the caret.
Close the "widget input, selection, mobile gestures" test gap with a
draw-free Area::Rect stub harness (stub_hit_area): tests seed the input
area a GPU frame normally fills and mirror the two platform
pre-dispatch steps (first_mouse_button, committed key focus), so raw
MouseDown/MouseMove and IME TextInput dispatch through event.hits
against a real Cx — exactly like a running app. Covered end-to-end:
hit-dispatch sanity, caret placement from a tap, shift-press selection
extension, drag selection over glyphs, TextInput inserting at the caret
and replacing an active selection, and a cell tap + TextInput whole-cell
round trip. The harness also exposed the test fixture actor drifting
from production's single "local" actor (cross-actor mid-run anchoring
is deterministic but not chronological — now a documented doc-engine
projection invariant); the runtime fixture now uses "local" for
production fidelity.
READMEs: doc runtime-tests section documents the stub harness and the
two bugs it unearthed; roadmap item updated (only the renderer draw
pass remains GPU/Studio-bound); doc-engine projection invariants gain
the per-actor counter boundary bullet.
6 new tests, 675 -> 681.
Makes table merge reachable on touch devices, closing the last gap of
the CRDT-native merge/split milestone (Shift+Arrow has no touch
equivalent):
- Long-press inside a table cell falls through the text glyph hit test
into table_hit_test and starts a TableCellSelection with anchor =
focus = the pressed cell (start_cell_range), clearing any text
selection; long-press on text keeps selecting word atoms unchanged.
- While the router idles in Selecting, the continued drag moves the
focus cell (extend_cell_range_to) through the tap hit geometry; drags
outside the anchor's table keep the focus and the caret tracks it.
Text word selections share the Selecting state but carry no cell
range, so their behavior is untouched.
- Lifting the finger keeps the range for the workspace Merge button or
Ctrl/Cmd+M (merge_selected_cells); Split already worked from a cell
tap. No new draw code — the existing cell-range highlight renders the
span.
Tests: 3 new runtime suites on a real Cx (long-press entry, drag
extension with caret tracking and full-grid normalization then merge
consumption on the wire, out-of-table drag keeping focus, and the text
word-selection regression guard) for 675 total in nigig-build.
Closes the last documented CrdtDocWorkspace toolbar parity gap by
porting the legacy DocEditor's mobile interaction policy to the
CRDT-native editor:
- The widget carries the shared InteractionMode (Edit default, View)
and latches mobile_mode_initialized on the first real touch, which
drops the session to View; desktop sessions stay editable until
actually touched.
- Mobile View mode reduces the surface to the gesture router: passive
caret taps (text and table cells), long-press word selection and
handles keep working, KeyDown handling is fully gated, and the
area-hit match is skipped so drags fall through to a parent
ScrollYView.
- The workspace toolbar gains the mobile-only Edit/Done AdaptiveView
control; toggle_interaction_mode flips the mode, mirrors the button
label, takes key focus entering Edit, hides the IME and resets
in-flight gestures entering View.
- Edit-mode touch taps request the IME at the tap point and draw_walk
reasserts show_text_ime every frame while Edit holds key focus,
anchored bottom-left of the live text or cell caret in clipped-area
coordinates — the frame-driven reassert mobile backends require.
Tests: 3 new runtime suites on a real Cx with real TouchUpdate/KeyDown
events (first-touch drop + key gating incl. Ctrl+B, in-cell Backspace
gating, Edit-mode tap placement with continued editing) for 672 total
in nigig-build. Physical IME open/geometry remains device verification.
Closes the roadmap's table merge command item on the CRDT editor
surface with a rectangular cell-range selection model driven from the
keyboard and the workspace toolbar:
- ProjectionSession::cell_selection holds stable anchor/focus row and
column ids; Shift+Arrow at a cell boundary starts the range and steps
the focus cell on an active one, with table-edge clamping, caret
follow, plain-action collapse, and undo-stale self-clearing.
- table_cell_range normalizes the selection against the projected
table; cell_selection_rects renders the highlight over visible cells
only, so merged spans contribute one expanded anchor rect.
- Ctrl/Cmd+M (merge_selected_cells, toolbar Merge) routes through the
engine's MergeTableCells op; undo splits through the symmetric
compensation. cell_range_mergeable rejects ranges overlapping an
existing merge UI-side, where ambiguous nested spans belong.
- Ctrl/Cmd+Shift+M (split_cell_at_cursor, toolbar Split) resolves the
containing merge via merge_at_cell even from a covered cell and
reveals the preserved hidden text; undo re-merges through
RestoreTableMerge.
- Workspace toolbar gains Merge/Split buttons with status-line
guidance (the only split path on touch devices).
Tests: 9 new (4 pure layout: range normalization/staleness, mergeable
rules, merge_at_cell, covered-skip highlight rects; 5 runtime on a real
Cx: shift-span + merge + undo, edge clamp, split + re-merge, plain
collapse, overlap rejection) for 669 total in nigig-build. The
doc-engine README gains the merge/split materialization invariant.
Taps park a TableCellCursor (stable row/column ids + char offset) inside
cells; typing and Backspace/Delete edit cells through whole-cell
SetTableCell replacements with symmetric undo. Arrows walk cell text in
reading order, hop between cells across rows, and exit into the nearest
text block in unified order at the table edges. Return inserts a row
immediately below the cursor's row and follows with the caret. Block
boundaries upgraded: backspace at a text start after a table enters its
trailing cell, forward-delete at a text end before a table enters its
first cell instead of merging structure. Style toggles stay text-only.
The cell caret draws between rendered characters using the renderer's
shared 6px inset / 7px-per-char advance, and stale cursors clear on
structural undos. Also fixed while wiring the paths: pointer hit tests
and layout-space decorations (selection, handles, carets) now account
for the widget origin, so taps and visuals agree at any dock position.
Runtime integration tests cover in-cell editing with undo restore,
arrow traversal/exits/clamping, and return-inserts-row-below; layout
unit tests cover the char-safe edit helpers, cursor resolution and
clamping, caret geometry, neighbor wrapping, and neighbor_text_block.
Drive the production widget end-to-end in lib tests: instances are built
through the registry's ScriptNew::script_new factory (bare ScriptVm with
unit host/std), the engine via set_engine, and real KeyDown events with
real KeyEvent/KeyModifiers payloads through Widget::handle_event.
Covered: caret anchoring from an uncursored editor, arrow stepping,
shift selection extension, Ctrl+B bolding across the selection, Enter
split at the caret with caret following the trailing half, Ctrl+Z merge,
and Backspace merging an empty split tail with the previous block.
The #[makepad_test] Studio harness was evaluated and rejected for this
milestone: it builds and launches the full app through the StudioHub
buildbox, and the repo's only examples (map tests/ui.rs and
makepad_visual_tests.rs) target aspirational APIs that do not compile.
Consolidate navigation onto a single CRDT docs destination now that the
DSL switch made the runtime-test tab an exact duplicate: drop the
'Docs CRDT' dock tab, its crdt_docs_content view, the sidebar's
'Documents CRDT Test' button and its select_tab handler. The desktop
'Docs' tab and 'Documents' sidebar button land on CrdtDocWorkspace, and
mobile's workspace drawer resolves 'Documents' to the CrdtDocWorkspace
doc_page through the new pure workspace_page_id function, pinned by unit
tests (destination, label table, page distinctness, CAD fallback).
Three orphan closers left over from the CategoryDropdown/AddRoomModal
restructure broke the script_mod block and with it the nigig-build build
at HEAD; AddRoomModal now follows CategoryDropdown at DSL top level.
Both active workspace DSL sites (desktop dock docs_workspace and mobile
doc_page m_doc_content) now instantiate CrdtDocWorkspace instead of the
legacy DocWorkspace; the classic widget stays registered as fallback.
CrdtDocWorkspace graduates from the runtime-test shell to the full
workspace surface: Open/Save/SaveAs round-trip the shared #MP_CRDT_V1
wire via projection_session::{crdt_save_wire, crdt_engine_from_saved},
Undo/Redo and B/I/U share the editor's keyboard-driven engine paths,
alignment buttons route the legacy DocAlign wire strings, and
+Table/+Img/+Div insert through new CrdtDocEditor helpers with legacy
anchored-after-caret semantics. Stats count words/chars from the
projection (new projected_stats helper). The mobile Edit/Done IME
toggle stays legacy-only until the native widget owns an IME mode.
CrdtDocEditor now handles the desktop keyboard surface natively through
doc-engine operations: Ctrl/Cmd+Z/Shift+Z undo-redo via CrdtHistory,
Ctrl/Cmd+B/I/U selection style toggles, arrow-key caret moves with
shift-extend across block boundaries via the step_glyph stream,
selection-aware Backspace/Delete with block-boundary merges, Return
splitting paragraphs or appending table rows, and TextInput that
replaces the active selection at the session caret block.
Engine additions: Operation::SplitBlock/MergeBlocks with symmetric
Compensation pairs; materialization keeps a split parent's trailing
runs as a synthetic child spliced after the parent so peers converge
without a new InsertBlock op; merge_runs coalesces equal style runs.
- word_atom_range mirrors the legacy word_bounds whitespace-pivot
semantics and returns a word's first/last atoms for SelectWord
- selection_handles normalizes anchor/focus into document order and
emits 12px handle rects with a 6px touch-slop hit test; collapsed
selections (caret) produce no handles
- CrdtDocEditor drives the shared MobileGestureRouter: TouchUpdate Start
grabs visible handles or arms long-press on the frame clock, SelectWord
applies the atom word range, handle drags move selection endpoints,
drags past threshold stay unclaimed for ScrollYView, and short taps
end as passive cursor moves without the IME
- synthesized finger events are ignored once a real touch sequence is
seen; desktop mouse/IME behavior unchanged
- draw_selection_handle live field (default #x1f73e6) + 5 geometry tests
- layout_projection walks the unified DocumentProjection::order so
advanced nodes interleave with paragraphs and tables at their anchor
positions; block_origins are assigned by index for both paths, with a
blocks-only fallback for projections assembled without the op log
- projected_node_metrics mirrors legacy AdvancedLayout heights, labels
and interactivity per kind (unknown kinds follow the bridge's
EmbeddedWidget mapping: 'Widget: <kind>')
- ProjectionRenderer::draw_node_projection reuses the legacy
draw_advanced_blocks colors; dividers collapse to a centered 1px line;
new draw_node_fill/draw_node_border live fields on CrdtDocEditor
- fix(doc-engine): the after-chain was block-only, so any block anchored
after an advanced node was unreachable during materialization and
vanished from the projection; nodes now participate as chain connectors
- tests: unified-order interleaving, node metrics, node hit test, mixed
table/node stacking, order-less fallback (nigig-build); node-anchored
block materialization and mixed sibling ordering (doc-engine)
- projection_layout builds ProjectedTableLayout geometry (fixed 160x28
cells matching the legacy bridge width, merge spans with covered-cell
flags) and per-block origins so blocks after a table clear it instead
of using the fixed line step
- ProjectionRenderer draws the table grid through a new draw_table_border
live field and consumes layout block origins for text placement
- table_hit_test maps points to merge-anchor cells for future cell editing
- doc-engine: split the cramped style-patch line clippy flagged as
possible_missing_else; the crate is clippy-clean again
- README: tick verified CRDT bridge boxes (dependency, wiring, selected
replacement/formatting/table/toolbar migration, font size/color
migration) and document the table milestone
- CI: add doc-engine workflow (engine tests + clippy -D warnings +
nigig-build consumer check/test) and trigger nigig-build CI on doc
changes
Went looking for a clippy ratchet and found that clippy had never run on
this crate at all: two dependencies fail deny-by-default lints, so
`cargo clippy -p nigig-build` aborted before linting nigig-build. Fixing
those two unblocked the crate and immediately exposed a real defect
class.
THE BUG. A bare identifier in a match pattern that is not a known variant
is parsed by Rust as a NEW BINDING that matches everything. Nine such
names were used as KeyCode patterns:
KeyEnter, KeyBackspace, BracketLeft, BracketRight (cad/viewport.rs)
Digit0..Digit9, Equal, LeftBracket, RightBracket,
Apostrophe (doc, invoice, pm)
Real names are ReturnKey, Backspace, LBracket, RBracket, Key0..Key9,
Equals, Quote.
Consequences, in severity order:
- cad/viewport.rs: `BracketLeft => { .. }` is UNGUARDED and sits above
the numeric arms, so it swallowed every remaining key. All
direct-distance-entry input -- digits, '.', '-', ',' -- was
unreachable. The entire DDE feature was dead.
- The guarded ones fired for any key satisfying the guard: pressing Q
while drawing with a non-empty buffer committed the coordinate.
- doc/invoice/project_management: `Digit0 => '0'` swallowed the rest of
the keymap, so every digit and symbol typed produced '0'.
This compiles cleanly and no test catches it. rustc's only signal is
`unreachable_pattern` plus `unused variable: \`Capitalised\`` -- both
buried in the 200+ warnings nobody could see, because clippy never ran.
85 unreachable-pattern warnings before, 1 after (a benign catch-all).
UNBLOCKING CLIPPY. Two deny-level errors in dependencies:
- nigig-uikit user_project_pill.rs: a `for` loop returning on its first
iteration (never_loop). Rewritten as `.next()`.
- spreadsheet-engine formula2.rs: CellRef had an inherent to_string
shadowing Display (inherent_to_string_shadow_display). The naive fix is
a trap: Display::fmt was `f.write_str(&self.to_string())`, which
resolved to the inherent method -- delete it and the same call resolves
to ToString::to_string, which calls Display::fmt, recursing until the
stack overflows. Verified with a standalone repro before fixing. The
body moved into Display; Range got the Display impl it never had.
Tests: keycode_variant_tests names the four correct variants, so it stops
compiling if any is renamed -- the point being that the old code compiled
precisely because the names were wrong. Two formula2 tests pin that
`x.to_string()` and `format!("{x}")` agree, which is what the shadowing
lint exists to protect.
CI gate added and negative-tested: greps for a capitalised
`unused variable`, which is the signature of this bug. Restoring
BracketLeft makes it fire.
625 lib + 154 integration + 225 spreadsheet-engine + 44 doc-engine, 0
failed.
ARCHITECTURE.md invariant 2 has said since it was written that the
scattered clear_mesh_cache() calls are "unnecessary and expensive",
because MeshCache keys on (NodeId, ParamHash) and an edited node
invalidates itself. Phase 5.2 removed the first one. This removes the
rest: 8 of the 11 live call sites, leaving only invalidate_all (whose
contract is exactly that) and the definition.
Investigating them turned up something the invariant did not say, and
which makes the cleanup a bug fix rather than tidying:
MeshCache has NO eviction for deleted nodes.
Entries are keyed by NodeId and nothing ever removes one when its node
goes away. The wholesale clears were, accidentally, the cache's only
garbage collector. Deleting them naively would have converted a
performance problem into an unbounded memory leak over a session.
So the fix is a targeted replacement, not a deletion: MeshCache::retain,
SceneCache::retain_meshes and CadViewport::evict_dead_meshes drop exactly
the entries whose node is gone and leave every live one warm. Each call
site was then classified rather than pattern-matched:
- deletes (delete_selected, delete_part, CommandContext::delete_node)
-> evict dead entries
- pure additions (create_node, insert_node_at, paste, and the wall /
circle / rect / area / column / beam / polygon / polyline tool
completions) -> nothing at all. A new node has no cache entry to
invalidate and cannot affect any other node's mesh. These were
discarding the entire scene's meshes to make room for nothing.
- subdivide_selected -> invalidate only the selected ids. It builds a
fresh Arc<Solid> per part, so ParamHash already differs; the unselected
parts were being thrown away for no reason.
Measured: deleting 1 of 100 parts costs 62.1 us with clear+rebuild versus
21.9 us with evict+reuse, a 3x saving on every delete.
Two tests, both negative-tested by stubbing out the retain body:
retain_nodes_evicts_dead_entries_and_keeps_live_ones checks both
directions (dead gone, live still an Arc::ptr_eq hit), and
cache_does_not_grow_past_the_live_node_set simulates 50 add/delete rounds
and asserts the cache holds 1 entry, not 50. With retain stubbed it holds
exactly 50, which is the leak.
Also documented a sharp edge found while reading ParamHash: it hashes a
Csg node's Arc POINTER, not its geometry. Re-wrapping identical geometry
in a new Arc is therefore a safe false-positive, but mutating a Solid
behind a shared Arc would go unnoticed. Nothing does that today and
ARCHITECTURE.md now says it must stay that way.
624 lib + 154 integration, 0 failed.
Every SVG floor plan containing a rotated element was wrong. arch_svg read
only transform.translation and transform.scale, at all six projection
sites, so a wall yawed 90 degrees exported axis-aligned. A 2 x 6 footprint
came out 2 x 6 instead of 6 x 2.
This is the worst failure mode of the rotation bugs found in this project:
the export succeeds, the file opens cleanly in any viewer, and it shows a
different building. Nothing announces it.
I deferred this during Phase 1 as "a missing feature, not a wrong
calculation". That was the wrong call and PHASE1_STATUS.md now says so.
Silently emitting a different building is not a missing feature.
Fixed by routing all six sites -- box, cylinder/sphere/circle, polygon,
extruded polygon, CSG mesh and arc -- through one project_local() helper
that uses part_model_matrix, the renderer's own transform. Deliberately
not re-derived here: every hand-rolled rotation in an exporter has been
wrong in its own way (arch_stl destructured sin_cos backwards, arch_gltf
produced norm-0.125 quaternions, arch_pdf fed degrees to cos). This
exporter does not get another private copy.
The convention is subtler than it looks and I got it wrong twice while
writing the test. Both plan exporters map to world XZ then negate Z, so
local +X projects to (cos yaw, -sin yaw) and local +Z to
(sin yaw, -cos yaw). That is a reflection, not a rotation: the two model
axes are NOT perpendicular in plan space -- at yaw 30 they are 30 degrees
apart. My first two attempts asserted a rotation and then
perpendicularity, and both failed against real output. Verified against
rot_y_mat arithmetic before believing it. Written down in ARCHITECTURE.md
invariant 6, which previously recorded the bug.
Five tests, chosen so no single symmetry can hide a wrong fix:
- 0 degrees: baseline dimensions.
- 90 degrees: footprint must swap, 2x6 -> 6x2.
- 180 degrees: extents unchanged. Passes even when broken, and is here
precisely to stop a fix that rotates by the wrong factor from looking
correct on the 90 degree case alone.
- 45 degrees: no symmetry to hide behind; asserts the diagonal extent
8/sqrt(2). Tolerance is 0.011 because coordinates are emitted with two
decimals.
- The projection convention itself, cross-checked against arch_pdf.
Negative-tested: restoring the translation-and-scale projection fails
three of the five.
Also replaced an unchecked mesh.vertices[idx] index in the CSG arm with a
get() -- a malformed mesh would have panicked the exporter.
622 lib + 154 integration, 0 failed.
Phase 2.4, which I deferred during Phase 2 on a wrong assumption.
An unterminated CAD script hung the evaluator permanently. vm.eval is a
single blocking call, so `while true { s = s.translate(..) }` never
returns: the rebuild worker blocks forever, every later edit queues behind
it, and the UI sits on "Computing 3D model..." until the app is killed.
The source is user- and AI-authored, and an LLM will emit a loop like that
without much provocation.
Reproduced before fixing. A probe test was still running at 60 seconds,
and again at 90 with the budget removed.
I deferred this originally saying it needed "a cooperative check inside
the Makepad script VM or a watchdog that can kill and respawn the worker
thread", and called it a VM design decision. That was wrong, and I should
have read the VM before concluding it. makepad_script already provides
ScriptRunBudget::from_durations(soft, hard, sample_interval), checked in
run_core against a sampled Instant. The CAD module simply never set it.
The fix is six lines in eval_cad_script_in_vm. PHASE2_STATUS.md now
records the bad call rather than quietly marking the item done.
Only the hard deadline is used. A soft hit sets TimeBudgetYield, which
expects a host that drives the VM back to completion; there is no such
driver here, so a soft deadline would present as a script that silently
produced nothing at all. Soft is set equal to hard so the budget can only
ever produce a real, reported error. The previous budget is saved and
restored, so one evaluation cannot starve the next.
5 seconds, sampled every 4096 instructions. Generous deliberately: a real
document is a few hundred CSG ops and finishes in milliseconds, so
anything still running is a runaway rather than a slow model.
Result: the same script now returns "script time budget exceeded" in 5.2s.
Three tests. The runaway case is #[ignore]d because it burns the whole
budget by design; the other two guard the ways a timeout typically breaks
things -- that a normal script is unaffected across repeated evaluations
(catching a budget that is not reset), and that a syntax error still
reports itself rather than being misattributed to the timeout.
CI gate added and negative-tested: losing the budget line reintroduces a
hang that no test failure announces, only a frozen app.
617 lib + 154 integration, 0 failed.
The module had two independent paths from a node to triangles.
ensure_part_geometry called part.build_solid() directly; the exporters and
pick_part went through MeshCache::get_or_build -> CadSolid::build_mesh().
Neither saw the other's work, so a part that had just been exported or
hovered was still re-meshed from scratch for the GPU upload.
Proved they agree before merging them. pipeline_equivalence_tests builds
all 12 CadSolid variants through both paths and compares vertex positions,
triangle indices and winding exactly. They match. A second test asserts
the variant list has 12 entries, so adding a CadSolid variant without
covering it fails the build rather than silently narrowing the guarantee.
Both negative-tested: swapping width/height in build_solid's Rect2D arm
fails with "Rect2D: vertex 0 differs: (-1.5, -2, 0) vs (-2, -1.5, 0)";
dropping a variant from the list fails the coverage test.
The change itself: build_mesh_buffers now takes &TriMesh rather than
&Solid -- it only ever read solid.mesh() -- and ensure_part_geometry pulls
from the shared MeshCache. Added cad_mesh_data_from_mesh and
part_mesh_buffers_from_mesh as the mesh-taking entry points; the
Solid-taking ones remain as thin wrappers for callers that hold a Solid.
Measured, and the number is smaller than the section title suggests:
2x on a warm cache (boxes 0.27 -> 0.178 us/part, extruded 24-gons 1.11 ->
0.516 us/part), and *negative* on a cold one -- 0.79 vs 0.27 us/part,
because a first build now pays a ParamHash and a map insert. That is
recorded in BENCH_BASELINE.md with a note not to "optimise" it back by
special-casing cheap primitives.
The reason to do it anyway is correctness, not speed. Two match arms over
the same enum drift, and this drift would have been invisible: no crash,
no failing test, just a preview that quietly disagreed with the exported
file. Now there is one pipeline and a test that fails if it forks again.
build_mesh and build_solid both still exist because a Csg node needs a
real Solid to compose with. ARCHITECTURE.md's "Two meshing pipelines"
section, which told readers to apply every meshing change twice, is now
"One meshing pipeline".
615 lib + 154 integration, 0 failed.
Measured first. bench_viewport_snapshot_sync_per_frame puts the cost of
one sync frame at 98.5 us for 100 parts: 21.2 us of Vec<CadNode> deep
copies and 77.2 us of forced scene rebuilds. A generation comparison over
the same data is 0.051 us. Dragging a part sets script_dirty on every
MouseMove, so that was the steady-state per-frame cost of a drag.
Three separate wastes, each removed:
1. The source viewport was written back to. sync took a snapshot from the
dirty viewport and then handed it to all three, including the one it
came from -- a deep copy plus a full cache reset to install state that
viewport already had. It now pushes to the other two only.
2. Destinations were re-pushed unconditionally. They now compare
parts_generation() against the source's and skip if equal. During a
drag the same list was being reinstalled every frame; a frame where
nothing changed now costs three integer comparisons.
3. replace_parts_snapshot cleared part_geoms and the entire mesh cache.
The part_geoms.clear() was the expensive one and it is not in the
benchmark: it dropped every uploaded GPU buffer, so the next draw had
to re-mesh and re-upload the whole scene through ensure_part_geometry.
Now retains geometry for ids that survive the replacement.
The clear_mesh_cache() was redundant. MeshCache keys on
(NodeId, ParamHash) and ParamHash covers the solid discriminant, all
its parameters and the full transform, so a node whose geometry
changed misses on its own. ARCHITECTURE.md invariant 2 has said this
was unnecessary since it was written; this removes the first of them.
Three tests, each negative-tested by reintroducing the defect:
- mesh_cache_self_invalidates_on_parameter_and_transform_edits proves the
claim that justifies dropping the wipe. Deleting the transform hashing
from ParamHash makes it fail.
- replace_all_bumps_the_generation_so_destinations_can_compare pins the
precondition for the skip. Removing the bump makes it fail, which is
the failure mode that matters: a stale generation would make the sync
silently drop a real edit.
- geometry_is_retained_for_surviving_ids_only covers the retain
predicate. Geometry needs a live Cx, so this tests the id set logic --
keeping a buffer for a deleted id, or dropping one for a survivor.
Also collapsed six copies of the widget-borrow chain into with_viewport,
taking a callback rather than returning a guard: the WidgetRef is a
temporary, so returning a borrow of it does not compile (E0515, the same
shape as the let-else bug fixed in cf29ba7).
Not done: the three viewports still hold three copies of the parts list.
Sharing one Rc<RefCell<CadDocument>> and deleting the snapshot pair is
the rest of 5.2. This bounds the cost of the copy in the meantime, and
invariant 4 now says so.
BENCH_BASELINE.md records that this benchmark measures the primitives,
not the widget method, so its numbers do not move when the sync path is
fixed. It sizes the prize; the behaviour tests verify the change.
613 lib + 154 integration, 0 failed.
Collapsing the nine duplicated properties-panel blocks exposed that all
nine were silent no-ops, and so was the Extend tool.
CadNode::pos/rot/size return Vec3f BY VALUE. `p.pos().x = val` therefore
assigns to a temporary and discards it. It compiles, warns about nothing,
and produces a control that does nothing: type a position into the panel
and the part does not move. Confirmed with a standalone repro before
touching anything. Eleven call sites, all wrong the same way:
- the nine position/size/rotation inputs in workspace.rs
- viewport.rs Extend, which carefully computes new_w/new_h and throws
both away, so the tool has never extended a part
Fixed by reading into a local, mutating it and calling set_pos/set_rot/
set_size, which have existed in cad_scene.rs the whole time.
The duplication is what hid this. Nine copies of a 25-line block, each
differing in one token, is not a thing anyone reads closely. Phase 4.3
was scheduled as tidying; it turned out to be the only reason the bug
was found. The blocks now call one helper,
apply_field_to_selected_part(cx, value, set), which also fixes the other
hazard of the copy-paste: each block had to remember four separate
invalidation steps (part_geoms.remove, invalidate_node, mark_scene_dirty,
script_dirty), and missing the first one fails silently -- the part keeps
rendering its old shape until something else evicts it. part_geoms.remove
call sites in workspace.rs: 13 -> 5.
Three defences, each negative-tested by reintroducing the defect:
- cad_scene::size_tests::setters_write_through_but_getters_are_copies
asserts both halves: that writing through the getter does NOT reach the
node, and that the set_* methods do. It documents the trap rather than
just guarding against it.
- workspace::properties_panel_setter_tests applies all nine closures
exactly as the call sites do and asserts each reaches its own axis and
leaves the other two groups alone -- so a wrong-axis copy-paste fails
too. Verified: restoring one broken closure fails with
"size.y: setter did not reach the node (got 1)".
- A CI grep for `.pos()/.rot()/.size().[xyz] =` outside comments.
Verified it fires on the restored Extend defect.
610 lib + 154 integration, 0 failed.
src/.../cad/tests.rs -> tests/cad_integration.rs. It was 2,840 lines of
integration tests living inside the library, compiled into every `--lib`
build and able to reach anything in the crate.
The move is worth more than the line count suggests, because a test
target compiles as a separate crate and so sees only the public API. That
turned an invisible question into a compile error: six items would have
needed `pub` for the file to build in its new home —
cad_mesh_data_from_solid, part_mesh_buffers, CommandBorrows,
CadRenderMode, DrawingState, SnapSettings, plus the private fields of the
last two.
Visibility was not widened. Promoting editor internals to a public
contract so a file can sit in a different directory is the wrong trade,
and `pub` is far harder to take back than to grant. Instead the 16 tests
that reach those items moved to where the items live:
- viewport.rs gains `mod viewport_helper_tests` (10 tests: the mesh
producers and the plain-data helpers).
- mod.rs gains `mod editor_state_tests` (6 tests: SnapSettings polar
defaults, DrawingState beam sections, CadEditorActivePane).
Two of the relocated tests are worthless and are now visible as such:
cad_render_mode_variants and command_borrows_fields_accessible assert
that a type exists and derives Debug, which the compiler already
guarantees. Left in place rather than deleted in the same commit as a
move; they are for the Phase 6 sweep.
The rule and its rationale are now written down in ARCHITECTURE.md under
"Where a test goes", alongside corrected LOC figures for the six files
that changed size since the table was written.
CI gained a step. The existing job ran only `--lib`, which does not build
a test target, so all 154 relocated tests would have run in no pipeline.
The step names cad_integration explicitly instead of testing the whole
crate, because tests/cost_estimator.rs and tests/cost_estimator_ui.rs do
not compile — pre-existing upstream breakage, verified against a clean
checkout, documented in a comment with instructions to fold them in once
fixed.
Test names diffed before and after: 0 lost, 12 gained (the Phase 4.4
characterization tests). 608 lib + 154 integration = 762.
Removed 73 "v1".."v19" references. The rule applied: keep the what,
delete the what-it-used-to-be. A comment saying a cache "now keys on
(NodeId, ParamHash) instead of just NodeId" tells a reader about a
decision made in a version they cannot see; the same comment saying it
keys on (NodeId, ParamHash) so a parameter edit produces a fresh mesh
tells them why the code in front of them is shaped that way.
Three headers were not merely useless but actively wrong, because they
described mechanisms deleted in earlier phases:
- mod.rs: 40 lines of "Rewrite status (v4)" whose last bullet documented
the SceneCache length heuristic ("rebuilds if parts.len() differs from
the cached scene's node count") as a live safety net. Phase 5.1
replaced that with generation tracking. Replaced with a short note on
why the file exists at all (Script derive must sit next to script_mod!)
and a pointer to ARCHITECTURE.md, which has the tests to keep it honest.
- cad_scene.rs: 44 lines enumerating the 24 compile errors fixed during a
past integration, including item 7 describing the DofConstraint bridge
deleted in Phase 4.5. Replaced with what a CadScene actually is.
- commands.rs: a "Migration strategy" section explaining how the module
coexists with "the existing CadCommand enum in mod.rs (line ~3538)" and
how CadCommandWrapper adapts it. That enum was deleted in Phase 4.1;
the wrapper never existed. Replaced with the current design.
Also deleted the orphaned "Cross-impl: legacy DofConstraint" banner in
cad_scene.rs, left behind when the From impls under it were removed.
The one surviving vN match is math.rs "Triangle: v0, v1, v2", which is
vertex naming.
762 tests pass, unchanged. Comment-only except for the three headers.
Three pairs of near-identical functions collapsed into one each, behind a
parameter naming the difference. No behaviour change: 12 characterization
tests were written first to pin the current output of every pair, and each
merge was negative-tested by reintroducing the defect and confirming the
tests fail.
- cad_mesh_data_from_solid / part_mesh_buffers were two copies of the same
120-line triangle walk. They differ in exactly two ways, now named:
MeshSpace (ViewNormalised recentres and fits to 1.75 units for the
single-solid preview; Model keeps model coordinates for scene parts,
where rescaling would break the layout) and Winding (the scene path
emits p0,p2,p1). The winding difference is preserved rather than
"fixed" — the shader disables backface culling so both render the
same, and nothing in the code proves which the depth paths expect.
Naming it makes it reviewable instead of invisible.
- extract_cad_script / extract_streaming_cad_script differed only in how
they treat an incomplete response, now a ResponseState parameter. The
streaming path keeps trailing whitespace, takes the body of an unclosed
fence, and seeks the first script token past any preamble; the complete
path trims both ends and treats an unclosed fence as unfenced. The
script-token list is now a named const next to the enum documenting
that it tracks system_prompt.md.
- toggle_view_mode carried its own copy of set_view_mode's gesture-state
reset (part/view/pan dragging, touches, pinch, orbit anchor, 2D pan).
It now computes the target and delegates. The copies had already
diverged: toggle called cx.redraw_all() without self.area.redraw(cx).
The characterization tests are worth more than the merge. They document
that the two mesh producers disagree on winding and that the two
extractors disagree on trailing whitespace — facts that were previously
only discoverable by diffing 120 lines by eye.
`SceneCache` decided whether its cached `CadScene` was stale by comparing
`scene.node_count()` against `parts.len()`. That is a guess, and its own
doc comment called it "a safety net for missing `mark_dirty()` calls" —
the real contract was "call `mark_dirty()` after every mutation", enforced
only by discipline across ~15 write sites and already violated by the
drawing tools, which push nodes directly.
The guess has a blind spot: delete one node and add another in the same
frame and the length is unchanged, so the cache reports clean and hands
back a scene describing the previous contents. Exports and picking then
run on stale geometry.
`CadViewport.parts` is now a `PartsStore` — the `Vec<CadNode>` plus a
monotonic `generation` that every mutating method bumps. You cannot get
`&mut` to the nodes without going through a method that bumps it, so
`SceneCache::scene_for(&store)` can compare generations exactly. Both the
length heuristic and the manual-`mark_dirty` requirement are gone as a
class, not fixed case by case.
`scene(&[CadNode])` is kept for callers that hold no store (tests, the
export path's detached copies) and now always rebuilds: without a
generation there is nothing to compare, and guessing is what caused the
bug.
Migration is incremental by design. `as_mut_vec()` is an escape hatch that
bumps unconditionally; three call sites still use it rather than turning
this into one 118-site commit. Sharing one document across the three
viewports (5.2) and retiring the remaining `mark_dirty` calls are separate
changes.
Also converted 10 duplicated `vp.parts.iter_mut().find(..)` blocks in
workspace.rs to the checked accessor.
Tests: 741 -> 750 passing, 0 failing. 9 new, including the delete-plus-add
case the old heuristic missed and an in-place edit with no `mark_dirty()`;
both were verified to fail against the restored length check.
Phase 4 of CAD_ASSESSMENT_AND_PLAN.md: remove the second copy of things
the module carried alongside their replacements. No behaviour change.
Removed, after confirming each has no live caller:
- The entire LegacyCommand system: the trait, LegacyCommandCtx,
LegacyUndoRedoStack and the six LegacyMovePart/Resize/Rotate/Add/Delete/
ModifyPart structs, plus their tests. The editor has used the Command /
UndoRedoStack pair since v19; the legacy set was still exported and
still maintained, so grepping for a command type returned two answers.
- CadViewport::legacy_ctx(), which nothing called.
- CommandError::Legacy, which was never constructed.
- The duplicate DofConstraint in mod.rs and its two bridge From impls.
Production reads cad_scene::DofConstraint exclusively; the legacy struct
existed only so a conversion test could round-trip it.
- Three dead functions in exporters.rs. The dead write_3d_viewer also
carried a bug the live path does not: it wrote an absolute filesystem
path into the exported viewer's <model-viewer src>, which breaks the
moment the folder is copied anywhere.
Renamed LegacyCommandCtxHolder -> CommandBorrows. Despite the name it
holds the *new* UndoRedoStack and is the split-borrow helper for the live
command path.
Also refreshed the v10/v13/v18b archaeology comments that referenced the
deleted types, and ported bench_command_execute_overhead to the live
Command/CadCommandCtx path so the measurement survives.
1,588 lines removed: commands.rs 1863 -> 1318, tests.rs 3754 -> 2840,
exporters.rs 186 -> 93.
Tests: 728 -> 714 passing, 0 failing. The 29 removed all exercised the
deleted system; verified by diffing the full test-name list before and
after, which also caught 16 live-system tests that merely *mentioned* a
Legacy type in passing and had to be kept.
Rebasing onto origin/main also required repairing that branch, which did
not compile (4 errors) and so had never run its own tests:
- lookup.rs: an over-eager .clone() removal left *bt.category, deref'ing a
String field to an unsized str and breaking the key type.
- estimate_store.rs: two different tests shared one name; renamed both to
say what they assert.
- Once building, 3 tests failed. EstimateStore::dispatch took an undo
snapshot and emitted UndoRedoChanged even when the command was a no-op,
so a rejected out-of-range edit still consumed an undo slot and cleared
the redo stack. Command::Initialize also never cleared the room list, so
re-initialising kept the previous run's rooms.
- spreadsheet-ui/workspace.rs: missing `use crate::model::WorkspaceModel`.
Phase 3 of CAD_ASSESSMENT_AND_PLAN.md. Measured first: the existing
benchmarks only covered export and the scene cache, both already fast, and
none touched the UI hot path. Added three that do, and recorded numbers in
BENCH_BASELINE.md.
pick_part broad phase: 2.1x less work per frame
The per-part AABB was built from CadNode::size(). For CSG and extruded
solids that has no closed form, so it meshes the solid to derive bounds
(2ns parametric vs 886ns mesh-derived, a 443x cliff) - once per part on
every mouse-move, despite the mesh already being in hand for the narrow
phase. Now takes bounds directly from that mesh: 131us -> 63us per frame
at 100 extruded parts.
This also fixes a latent correctness bug. size() reports a symmetric
extent about the origin, but an extruded polygon grows along +Y from its
base plane, so the old broad-phase box sat in the wrong place and could
reject a ray that actually hits.
Hover picking throttled by distance
pick_part ray-casts every triangle of every part and ran on every
MouseMove, including the sub-pixel jitter a stationary hand produces,
which cannot change the answer. Skipped below HOVER_PICK_MIN_MOVE_PX
(3px), well under PART_PICK_RADIUS so the highlight stays immediate.
Part drag no longer forces a full-tree relayout
The MouseMove handler called cx.redraw_all() on top of area.redraw() for
every motion event. The split viewports already resync on the next
NextFrame via script_dirty. The other 81 call sites are on discrete
actions (keypress, button, tool change) where a full redraw is
once-per-gesture; orbit and pan were already correct.
Tests: 720 -> 728 passing, 0 failing. 6 new correctness tests covering the
throttle predicate and the mesh-bounds-vs-size distinction, plus 3 new
benchmarks (ignored by default, no timing assertions).
Phases 0-2 of CAD_ASSESSMENT_AND_PLAN.md. The crate did not compile and no
test had ever run; it now builds clean with a green suite.
Build and CI (Phase 0)
- Pin all 33 git dependency manifests to an explicit rev. A branch
dependency re-resolves on every build and is a code-execution path into
CI if force-pushed.
- Commit Cargo.lock (540 packages). Producing it required fixing three
resolution failures the workspace had always had: a non-existent
makepad-widgets feature, two rusqlite versions both linking sqlite3, and
four missed CellId call sites in spreadsheet-ui.
- Add .forgejo/workflows/nigig-build.yml.
- Replace five stale CAD docs that contradicted the code with one
ARCHITECTURE.md; add PHASE0/1/2_STATUS.md and TEST_BASELINE.md.
Correctness (Phase 1)
- Rotation units: transform_point bound sin_cos() backwards, transposed X
and Z, and applied axes in reverse order, so every exported STL was wrong
even at zero rotation. It now shares the renderer's matrix helpers.
- GLB quaternions had norm 0.125 (half-angle applied to cos/sin, degrees
read as radians) - invalid per the glTF spec.
- PDF wall/door/window yaw fed degrees to cos/sin.
- Fix a TOCTOU unwrap in touch picking; viewport.rs now has no unwrap().
- CommandContext gains update_node/insert_node_at/node_index: resize and
modify were delete+create, silently moving nodes to the end of the scene.
- Wire MAX_UNDO_LEVELS (defined, exported, never read) and switch the undo
stack to VecDeque; this also made the existing drag-merge logic reachable.
- CadNode::size() returned a fake 1x1x1 for CSG and extruded solids, making
them unpickable outside a 1x1x1 box at their origin.
- Reject non-finite script input; makepad_csg clamps NaN rather than
propagating it, so bad input produced silently wrong geometry.
Test baseline: 0 -> 722 passing, 0 failing
- 17 pre-existing failures fixed: 10 real defects (dependency-cycle
detection, over-allocation of unassigned tasks, quote/backslash
corruption on save, default rooms lost for all but the first region,
RGA text ordering) and 7 tests that were themselves wrong, each checked
against its production caller first.
Security (Phase 2)
- env!("CARGO_MANIFEST_DIR") was used as a runtime path in three places,
including as the AI agent's working directory. All runtime data now goes
under app_data_dir().
- Remove the hardcoded LAN LLM endpoint. It is now opt-in via
NIGIG_CAD_LOCAL_OPENAI_URL/_MODEL and refuses plaintext HTTP to anything
but loopback.
- Bound and content-sniff AI image attachments (8 MB cap, magic bytes);
the MIME type came from the filename extension.
- Escape SVG/HTML output, and add SRI to the exported viewer's script tag.
The pinned model-viewer@3.5.1 does not exist, so every exported viewer
was silently broken; now 4.0.0 with a verified hash.
- Stop embedding $USER in exported PDFs and logging document content in
release builds.
- CI now rejects reintroducing the runtime-path and hardcoded-endpoint
classes; both gates were verified to fail on a reintroduced defect.
Add system_prompt.md and embed it with include_str!. The file was missing
from the repository, so the agent silently used a one-line fallback.
