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

```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<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.