# 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 ```rust 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 ```rust 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 ```rust 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 `MoveNode`s 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`. | | `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`, 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.