nigig-org/PHASE1_STATUS.md

Phase 1 — Correctness — status

Phase 1 of CAD_ASSESSMENT_AND_PLAN.md. Every change here is covered by a test that fails before the fix and passes after.

Phase 1 is complete. Test suite: 634 → 679 passing. Failures unchanged at 17 (the pre-existing set in TEST_BASELINE.md, verified identical by name, not just by count). No regressions.

---

1.1 Rotation units — DONE

The plan called this "degrees vs radians confusion". The reality was worse: four distinct bugs, three of which corrupted geometry even when the part's rotation was zero.

1.1a — STL exporter: sin_cos() destructured backwards

let (crx, srx) = rx.x.to_radians().sin_cos();   // returns (sin, cos)!

f32::sin_cos() returns (sin, cos). Binding it as (cos, sin) swaps the two. Verified directly: at 0°, crx = 0 and srx = 1, so the "identity" rotation mapped (1, 2, 3) to (3, -2, 1).

Every exported STL was wrong, whether or not anything was rotated.

1.1b — STL exporter: X and Z matrices transposed

After fixing the binding, 3 of 8 tests still failed: 90° about X sent +Y to -Z instead of +Z. The hand-rolled X and Z matrices were transposed (i.e. rotating backwards); Y happened to be correct.

1.1c — STL exporter: axes applied in reverse order

The exporter composed Z→Y→X; math::part_model_matrix composes Rz·Ry·Rx (X applied first). Even with correct matrices, combined rotations diverged from the preview.

Fix for all three: transform_point no longer re-derives Euler math. It now calls the renderer's own rot_*_mat / mat4_mul / translate_mat helpers, so exporter and preview cannot drift apart. A test (agrees_with_renderer_model_matrix) pins them together at a non-trivial rotation (30°, 45°, 60°).

1.1d — GLB exporter: non-unit quaternions, radians assumed

let (cx, sx) = (rx.cos() * 0.5, rx.sin() * 0.5);

Two errors: the input was treated as radians (it is degrees), and the half-angle * 0.5 was applied to the cos/sin results rather than to the angle. Every quaternion came out with norm 0.125 instead of 1.0 — invalid per the glTF 2.0 spec — and identity rotation serialised as [0, 0, 0, 0.125].

1.1e — PDF exporter: degrees fed to cos/sin

make_wall, make_door and make_window all did yaw.cos() on the degrees field. A wall at 90° was drawn at 90 radians (≈ 5157°).

1.1f — API contract corrected

CadTransform::rotate_x/y/z, SceneBuilder::rotate_*, and yaw/pitch/roll all named their parameter radians while storing into a degrees field. Renamed to degrees and documented on the field itself. CadSolid::Arc genuinely is radians and was left alone (verified: it feeds trig directly and add_part seeds it with PI).

Tests added: 17 — arch_stl::transform_point_tests (8), arch_gltf::quat_tests (6), arch_pdf::yaw_unit_tests (3). The PDF tests were confirmed to genuinely catch the bug by reverting the fix and watching 2 of 3 fail.

Still open

arch_svg ignores rotation entirely. Fixed later — see the arch_svg rotation commit.

I called this "a missing feature, not a wrong calculation" and deferred it. That framing was wrong. Every other rotation defect in this phase was loud once you looked; this one silently exported a different building and the file opened cleanly, which makes it worse than the ones I fixed first, not lesser. Rotation was ignored at all six projection sites.

---

1.2 TOCTOU unwrap — DONE

if self.pick_part(touch.abs).is_some() {
    let id = self.pick_part(touch.abs).unwrap();   // second call

Two independent ray casts against a mesh cache that takes a write lock and can rebuild meshes between them. Replaced with a single if let Some. Also halves the cost of the touch path's most expensive operation.

Swept the rest of the file while there:

• polyline_points.pop().unwrap() → let else (the match guard proves non-emptiness today, but guard and pop are 4 lines apart).
• Two × arc_mid_world = Some(x); … arc_mid_world.unwrap() → bind x to a local first.

viewport.rs (7,177 lines) now contains zero unwrap()/expect().

---

1.4 Undo cap — DONE

MAX_UNDO_LEVELS = 100 was defined, re-exported, and never read. CadViewport::command_stack is a #[rust] field, so it is built by Default, which set max_history: None — unbounded. Every command retains a full CadNode snapshot (ModifyNode keeps two), so a long session grew without limit. On mobile that is an OOM path.

• UndoRedoStack::default() is now bounded by MAX_UNDO_LEVELS. Added unbounded() for callers that want to opt out explicitly.
• Backing store changed Vec → VecDeque: eviction was Vec::remove(0), which is O(n) per eviction.

---

1.5 Drag-move merging — DONE (as a side effect)

The assessment recorded merging as dead code. It was subtler than that: MoveNode::can_merge / merge were already correctly implemented, but UndoRedoStack::execute reached them through a pop / unwrap() / push dance whose result was discarded. Rewriting that path to use VecDeque::back_mut() — done for 1.4 — made the existing merge logic reachable.

Confirmed by test: 50 consecutive MoveNodes on one node now collapse to one undo entry, and undoing it restores the original position. The editor's drag path (viewport::move_part_command) constructs exactly these commands, so a drag is now one undo step instead of one per frame.

This also removed the unwrap() that the assessment flagged as a latent panic in the undo path.

---

1.3 CommandContext::update_node — DONE

The bug

ResizeNode, RotateNode and ModifyNode implemented "modify" as delete_node() + create_node(). create_node appends, so editing a node silently moved it to the end of the parts vector. Confirmed by reverting the fix: resizing node 2 of [1, 2, 3] produced [1, 3, 2].

Consequences: 2D painter order changes (a resized slab jumps in front of walls), any index-based assumption breaks, and each edit fired a global clear_mesh_cache() — discarding every cached mesh in the scene to change one node.

YawNode was worse: it wrote the new rotation into a local copy of the transform, discarded it, called move_node with the existing translation, and returned Ok(()). A no-op that reported success.

DeleteNode::undo appended too, so undoing a delete moved the node to the end — even though viewport::delete_part_command already received the correct index and explicitly threw it away (// deleted_index kept for API compat).

The fix

Three additions to CommandContext:

Method	Purpose
update_node(id, &mut dyn FnMut(&mut CadNode))	In-place edit preserving list position. &mut dyn FnMut (not a generic) keeps the trait object-safe, since commands are Box<dyn Command>.
insert_node_at(index, node)	Restore a deleted node to its original slot.
node_index(id)	Query current ordering.

Yaw/Resize/Rotate/ModifyNode now use update_node and invalidate one node instead of the whole cache. DeleteNode carries index: Option<usize>, and delete_part_command passes the index it was already being given.

Also removed CommandContext::redraw, which was a documented no-op ("callers handle redraw") called from ~20 sites for nothing.

YawNode::from_radians/to_radians renamed to from_degrees/to_degrees to match the Phase 1.1 contract.

Test-mock defect found along the way

MockCommandContext stored nodes in a HashMap and its rebuild_scene sorted by id. Node ordering was therefore unobservable, which is why no existing test caught any of this — a test would have passed vacuously. The mock now uses a Vec in insertion order, mirroring the real CadCommandCtx, and exposes node_order().

Tests added: 7 — order preservation for resize/rotate/modify, delete-undo slot restoration, YawNode actually rotating, and single-node (not global) cache invalidation. Verified by reverting ResizeNode and watching resize_preserves_node_order fail with [1, 3, 2].

---

1.6 Swallowed command errors — DONE

Every command invocation was let _ = self.execute_command(...) — six call sites. A NodeNotFound produced no log, no status message, and left the undo stack diverged from the scene, so the next undo would apply against a state it was never recorded from.

• Added CadViewport::execute_command_reporting(cmd, what), which logs via error! and records the message in a new last_command_error field.
• All six call sites routed through it.
• CadWorkspace::drain_viewport_command_errors polls the three viewports each NextFrame and surfaces the message in the status label.

Two tests: a command against a missing node returns NodeNotFound, and a failed command is not pushed onto the undo stack (otherwise a later undo would try to reverse something that never happened).

---

1.7 CadNode::size() returning a fake 1×1×1 — DONE

size() had a catch-all _ => Vec3f { 1.0, 1.0, 1.0 } covering Csg, Polygon2D, ExtrudedPolygon, ExtrudedIBeam, ExtrudedHSS and Empty.

pick_part builds its broad-phase AABB from size(), so CSG and extruded parts were unpickable outside a 1×1×1 box at their origin — a 6-metre extruded beam could only be clicked near its centre. resize_selected also multiplied the fictional size.

Now: Empty/None report zero extent, and the mesh-derived variants compute their real extent from the TriMesh bounding box. The cheap parametric arms (Box, Cylinder, Sphere, Rect2D, Circle2D, Arc) are untouched and still avoid meshing.

Tests: 7, including one asserting the parametric variants are unchanged.

---

1.8 Non-finite geometry — DONE

Zero is_nan/is_finite checks existed in 27k lines.

The interesting discovery: makepad_csg clamps a non-finite dimension rather than propagating it. cube(0.0/0.0, 1, 1) does not produce NaN vertices — it produces a 0.001-thick sliver. So the geometry was silently wrong rather than detectably wrong, and a check on the output mesh alone would not have caught it.

Two layers, because they catch different things:

1. Argument boundary. arg_f64/arg_u32 are the single chokepoint for all 32 numeric script arguments. Non-finite input now sets a thread-local flag (the bindings have no way to raise a script error from inside a method body), which eval_cad_script_in_vm converts into a real error. NaN as u32 also saturates to 0, which would have become a degenerate segment count.
2. Output validation. first_non_finite_vertex rejects NaN/Inf that entered by any other route.

Also hardened arch_gltf::compute_min_max: it now skips non-finite components and falls back to zeros. glTF requires finite accessor min/max; NaN there yields a file some viewers reject and others render with an infinite bounding box.

Tests: 9 (5 script-level, 4 exporter-level).

---

1.9 add_part placement grid — DONE

let count = self.parts.len(); let grid_x = -1.5 + (count % 5.0) * 0.8;

Deleting a part rewinds the counter, so the next part is placed exactly on top of an existing one. Now derived from the monotonic id.

---

Deferred

Nothing from Phase 1 remains.

---

Verification

cargo test --locked -p nigig-build --lib
# before Phase 1:        634 passed; 17 failed; 7 ignored
# after 1.1/1.2/1.4/1.5: 655 passed; 17 failed; 7 ignored
# after 1.3:             661 passed; 17 failed; 7 ignored
# after 1.6-1.9:         679 passed; 17 failed; 7 ignored

Phase 1 added 45 passing regression tests. The 17 failures were verified identical to the baseline by test name, not merely by count.

The 17 failures are the pre-existing set in TEST_BASELINE.md and are untouched by this work. CI gates on that baseline, so any new failure or any drop below the recorded pass count fails the build.