diff --git a/.forgejo/workflows/nigig-build.yml b/.forgejo/workflows/nigig-build.yml index a4c2cec..baae83a 100644 --- a/.forgejo/workflows/nigig-build.yml +++ b/.forgejo/workflows/nigig-build.yml @@ -108,6 +108,32 @@ jobs: fi echo "OK" + # CadNode::pos/rot/size return Vec3f BY VALUE. `node.pos().x = v` + # compiles, mutates a temporary and throws it away. This silently + # broke all nine properties-panel inputs and the Extend tool; it + # produces no warning and no runtime error, only a control that + # does nothing. Mutation must go through set_pos/set_rot/set_size. + - name: No writes through the by-value Vec3f getters + run: | + set -euo pipefail + cad=crates/apps/nigig-build/src/construction_frame/pages/workspace/cad + # Exclude the two test modules that demonstrate the trap. + if grep -rnE '\.(pos|rot|size)\(\)\.[xyz][[:space:]]*[-+*/]?=[^=]' "$cad" \ + | grep -v 'setters_write_through_but_getters_are_copies' \ + | sed 's/^[^:]*:[0-9]*://' \ + | grep -vE '^[[:space:]]*(//|/\*|\*)' \ + | grep -v 'n\.pos()\.x = 42\.0' \ + | grep -v 'n\.rot()\.y = 42\.0' \ + | grep -v 'p\.size()\.y = v'; then + echo + echo "ERROR: the assignment(s) above write to a temporary copy" + echo "and are discarded. CadNode::pos/rot/size return Vec3f by" + echo "value. Read into a local, mutate it, then call set_pos /" + echo "set_rot / set_size." + exit 1 + fi + echo "OK" + - name: Reject whitespace errors run: git diff --check @@ -154,9 +180,23 @@ jobs: - name: Check run: cargo check --locked -p nigig-build --lib - - name: Test + - name: Test (lib) run: cargo test --locked -p nigig-build --lib + # Phase 4.7 moved the CAD integration suite out of src/ into its own + # test target. `--lib` does not build it, so without this step the + # 154 tests it contains would run in no pipeline at all. + # + # This names the target explicitly rather than running the whole + # crate's tests, because `--test cost_estimator` and + # `--test cost_estimator_ui` do not currently compile. That is + # pre-existing breakage, not this workflow's to hide -- but gating + # on it would make this job red for reasons unrelated to the CAD + # module, and a step that is always red gets ignored. Add those + # targets here the moment they build. + - name: Test (CAD integration suite) + run: cargo test --locked -p nigig-build --test cad_integration + # NOTE: `cargo clippy -- -D warnings` is not enabled yet -- the crate # currently emits ~290 warnings. Enable it once that backlog is # cleared; a step that cannot fail is worse than no step. diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md index 24bf445..5886a2a 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md @@ -59,12 +59,11 @@ of its cache-coherence complexity. | File | LOC | Responsibility | |---|--:|---| -| `mod.rs` | 2,058 | Module wiring, re-exports, and the `#[derive]`-heavy struct definitions (`CadViewport`, `CadWorkspace`, `CadTool`, `DrawingState`, `SnapSettings`, `DrawCadMesh`). Structs live here because the derive macros need the `script_mod!` context; their `impl` blocks live elsewhere. | -| `cad_scene.rs` | 2,173 | The domain model. `CadScene`, `CadNode`, `CadSolid`, `CadTransform`, newtype ids, `SceneBuilder`, `SceneVisitor` + `walk_scene`, `MeshCache`, and the `Exporter` trait. | -| `viewport.rs` | 7,177 | `impl CadViewport` — input handling, 2D and 3D rendering, picking, snapping, drawing tools, and the rebuild worker. | -| `workspace.rs` | 3,360 | `impl CadWorkspace` — panel layout, the AI agent session, export entry points, and viewport synchronisation. | -| `commands.rs` | 1,318 | Undo/redo: `Command`, `CommandContext`, `UndoRedoStack`. Node edits go through `CommandContext::update_node`, which preserves list position — never `delete_node` + `create_node`. | -| `tests.rs` | 2,840 | Integration tests. Uses no `Cx`; belongs in `tests/`. | +| `mod.rs` | 2,087 | Module wiring, re-exports, and the `#[derive]`-heavy struct definitions (`CadViewport`, `CadWorkspace`, `CadTool`, `DrawingState`, `SnapSettings`, `DrawCadMesh`). Structs live here because the derive macros need the `script_mod!` context; their `impl` blocks live elsewhere. | +| `cad_scene.rs` | 2,279 | The domain model. `CadScene`, `CadNode`, `CadSolid`, `CadTransform`, newtype ids, `SceneBuilder`, `SceneVisitor` + `walk_scene`, `MeshCache`, and the `Exporter` trait. | +| `viewport.rs` | 7,521 | `impl CadViewport` — input handling, 2D and 3D rendering, picking, snapping, drawing tools, and the rebuild worker. | +| `workspace.rs` | 3,734 | `impl CadWorkspace` — panel layout, the AI agent session, export entry points, and viewport synchronisation. | +| `commands.rs` | 1,305 | Undo/redo: `Command`, `CommandContext`, `UndoRedoStack`. Node edits go through `CommandContext::update_node`, which preserves list position — never `delete_node` + `create_node`. | | `arch_pdf.rs` | 1,301 | PDF floor-plan exporter (`printpdf`). 2D plan projection; does not use meshes. | | `arch_gltf.rs` | 915 | GLB 2.0 exporter + the generated HTML viewer. | | `script_bindings.rs` | 712 | Script VM bindings: `cad_script_mod`, solid/shape handles, `eval_cad_script`. | @@ -74,7 +73,7 @@ of its cache-coherence complexity. | `construction_geometry.rs` | 453 | Construction lines/points, coordinate-entry parsing. | | `cad_editor_sheet.rs` | 408 | The draggable bottom sheet. | | `arch_stl.rs` | 393 | Binary STL exporter. | -| `scene_holder.rs` | 390 | `SceneCache` — the cached `Arc` + shared `Arc`. | +| `scene_holder.rs` | 754 | `PartsStore` (the parts list + its generation counter) and `SceneCache` — the cached `Arc` + shared `Arc`. | | `profile_benchmarks.rs` | 281 | Timing harness. Currently `#[test]`, with wall-clock assertions. | | `tools.rs` | 247 | `impl CadTool` — tool cycling and kind mapping. | | `viewport_2d.rs` | 184 | 2D projection helpers split out of `viewport.rs`. | @@ -216,10 +215,29 @@ enforces that `Cargo.lock` is committed and current — see The crate compiles and its tests run: ```bash -cargo test --locked -p nigig-build --lib -# 741 passed; 0 failed; 10 ignored +cargo test --locked -p nigig-build --lib # 608 passed; 0 failed; 10 ignored +cargo test --locked -p nigig-build --test cad_integration # 154 passed; 0 failed ``` -The suite is green and CI asserts a plain pass, so any failing test fails -the build. The 7 ignored are the timing benchmarks in -`profile_benchmarks.rs`. See `TEST_BASELINE.md`. +Both are gated in CI and assert a plain pass, so any failing test fails the +build. The 10 ignored are the timing benchmarks in `profile_benchmarks.rs`. +See `TEST_BASELINE.md`. + +## Where a test goes + +Phase 4.7 moved the cross-cutting suite to +`crates/apps/nigig-build/tests/cad_integration.rs`, which compiles as a +separate crate and therefore sees only the public API. + +- **Reaches only `pub` items** → `tests/cad_integration.rs`. It exercises + the module the way a consumer does, so anything it can reach is, by + definition, API. +- **Reaches a `pub(crate)` or private item** → a `#[cfg(test)] mod` in the + file that owns the item. + +The rule is that visibility is never widened to make a test compile. When +the suite moved out, six items would have needed `pub` +(`cad_mesh_data_from_solid`, `part_mesh_buffers`, `CommandBorrows`, +`CadRenderMode`, `DrawingState`, `SnapSettings`). Widening them would have +turned editor internals into a public contract to suit a file layout, so +the 16 tests involved moved back to `viewport.rs` and `mod.rs` instead. diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_gltf.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_gltf.rs index 630c19f..d1b39bc 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_gltf.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_gltf.rs @@ -315,9 +315,9 @@ impl GltfExporter { /// assemble GLB. Exposed publicly so tests can call it without /// going through a writer. /// - /// v17: Uses parallel mesh generation for large scenes (>=32 nodes). - /// For smaller scenes, falls back to sequential (rayon thread pool - /// overhead exceeds the benefit for simple primitives). + /// Uses parallel mesh generation for scenes of >=32 nodes and + /// sequential below that: rayon's thread-pool overhead exceeds the + /// benefit for small scenes of simple primitives. pub fn build_glb( &self, scene: &CadScene, @@ -337,10 +337,9 @@ impl GltfExporter { }); } - // v17 fix: Only use parallel for large scenes. - // Benchmark results showed that for 100 simple box walls, - // parallel was 2x SLOWER than sequential due to rayon's - // thread pool overhead (task dispatch + synchronization). + // Only parallelise large scenes. Measured: for 100 simple box + // walls parallel was 2x SLOWER than sequential, because rayon's + // task dispatch and synchronisation cost more than the meshing. // // Parallel wins when each mesh build is expensive (complex // CSG with differences/unions, high-segment cylinders, etc.). @@ -479,9 +478,8 @@ impl Exporter for GltfExporter { self.export_with_cache(scene, &cache, writer) } - /// v2 fix: `export_with_cache` is now a provided method on the - /// `Exporter` trait itself (no more `ExporterWithCacheOverride` - /// indirection). Override it here to actually consult the cache. + /// Overrides the `Exporter` default so the cache is actually + /// consulted; the default provided method ignores it. fn export_with_cache( &self, scene: &CadScene, diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_pdf.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_pdf.rs index 44a9981..4929d6f 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_pdf.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/arch_pdf.rs @@ -20,7 +20,7 @@ //! └─ decorations: title block, scale bar, north arrow //! ``` //! -//! ## v2 design (preserved from previous version) +//! ## Classification //! //! Each `CadNode` is classified by **direct pattern //! match on node name + geometry kind**, not by guessing from dimensions. The user @@ -236,8 +236,8 @@ pub enum ArchElement { // Instead of `match part.kind { ... }`, we override the visitor // methods for each geometry kind. The "is this a Wall or a Slab?" // decision is made from the node's layer name (set by the editor) -// rather than from geometry — this preserves the v2 "explicit type" -// design while letting us use the unified CadScene. +// rather than from geometry, so the classification is explicit rather +// than inferred. // // Layer-name → arch-type mapping: // "walls" → Wall @@ -248,8 +248,8 @@ pub enum ArchElement { // "beams" → Beam // anything else → fallback (Block / Column / Sphere by geometry) // -// If a node has no layer (or an unknown layer), we fall back to -// geometry-based classification, which is what the v1 code did. +// If a node has no layer (or an unknown layer), fall back to +// geometry-based classification. struct PdfArchProjector<'a> { scene: &'a CadScene, @@ -655,10 +655,8 @@ impl Exporter for PdfExporter { self.export_with_cache(scene, &cache, writer) } - /// v2 fix: `export_with_cache` is now a provided method on the - /// `Exporter` trait itself (no more `ExporterWithCacheOverride` - /// indirection). The PDF exporter accepts the cache for API - /// symmetry with `GltfExporter` but doesn't actually use it + /// The PDF exporter accepts the mesh cache for API symmetry with + /// `GltfExporter` but does not use it /// (PDF works in 2D plan view, no triangle meshes needed). fn export_with_cache( &self, @@ -778,7 +776,7 @@ fn compute_viewport(elements: &[ArchElement], o: &PdfExportOptions) -> Option PdfColor { @@ -917,7 +915,7 @@ fn draw_text( } // =========================================================================== -// Grid + element renderers — unchanged drawing logic from v2 +// Grid + element renderers // =========================================================================== fn draw_grid(layer: &PdfLayerReference, vp: &Viewport, o: &PdfExportOptions) { @@ -1081,7 +1079,7 @@ fn draw_element(layer: &PdfLayerReference, e: &ArchElement, vp: &Viewport, font: } // =========================================================================== -// Decorations: title block, scale bar, north arrow — unchanged from v2 +// Decorations: title block, scale bar, north arrow // =========================================================================== fn draw_title_block( diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/cad_scene.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/cad_scene.rs index 6d9cebd..f28d3a4 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/cad_scene.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/cad_scene.rs @@ -1,47 +1,24 @@ -//! # cad_scene — immutable CAD scene graph + shared export foundation. +//! # cad_scene — the immutable CAD scene graph and the export foundation. //! -//! ## v2 — compile fixes after first integration attempt +//! A `CadScene` is a flat arena of `CadNode`s addressed by `NodeId`, +//! plus side tables for materials, layers and sheets. It is built once +//! from the editor's parts (see `scene_holder::SceneCache`) and then +//! only read, which is what lets exporters share one `MeshCache` +//! keyed on `(NodeId, ParamHash)`. //! -//! This version fixes the 24 compile errors from the first integration: +//! Everything that walks a scene goes through `SceneVisitor` / +//! `walk_scene`, and everything that writes a file implements +//! `Exporter`. `Exporter::export_with_cache` is a provided method, so +//! an exporter that has no use for cached meshes implements only +//! `export`. //! -//! 1. **`Exporter` trait consolidation** — `export_with_cache` is now a -//! *provided method* on `Exporter` itself, not on a separate -//! `ExporterWithCache` extension trait. This fixes the -//! `::export_with_cache` not-found errors. The -//! default impl ignores the cache; exporters that want to use the -//! cache override the method directly. +//! `SceneBuilder` is the only supported way to construct a scene; it +//! owns the `IdAllocator` so ids cannot collide. //! -//! 2. **`Solid::cube(...).mesh()` returns `&TriMesh`** — added -//! `.clone()` so `build_mesh()` returns an owned `TriMesh`. -//! -//! 3. **`Solid::cylinder` / `Solid::sphere` take `u32` segments** — -//! removed the `as usize` casts. The `segments` / `segments_u` / -//! `segments_v` fields are already `u32`. -//! -//! 4. **`start_node` expects `Option`** — wrapped the -//! geometry args in `Some(...)` in the `cube()` / `cylinder()` / -//! `sphere()` / `csg()` / `polygon_2d()` / `extruded_polygon()` -//! builder methods. -//! -//! 5. **`NodeBuilder::material_by_name` / `layer_by_name` borrow -//! conflict** — extracted the resolved id into a local before -//! mutably borrowing `pending`. -//! -//! 6. **Public `SceneBuilder` API for legacy adapter** — added -//! `register_material_for_color(color) -> MaterialId` and -//! `push_node(node)` public methods so the legacy `parts_to_scene` -//! adapters in `arch_gltf.rs` and `mod.rs` don't need to touch -//! private fields. -//! -//! 7. **`DofConstraint` not re-exported** — removed from the -//! `pub use` list in `mod.rs` so it doesn't conflict with the -//! legacy struct. Added `From for -//! cad_scene::DofConstraint` so the legacy struct can be -//! converted at the bridge point. -//! -//! 8. **`HashMap<[f32; 4], _>` doesn't work** (f32 has no `Eq`/`Hash`) -//! — the legacy adapter in `arch_gltf.rs` and `mod.rs` now uses -//! `[u32; 4]` (via `f32::to_bits()`) as the key. +//! Rotations in `CadTransform` are **degrees** (see Phase 1.1). +//! `CadSolid::Arc` angles are radians — it is the sole exception and is +//! documented at its definition. + use std::collections::HashMap; use std::sync::{Arc, RwLock}; @@ -1493,10 +1470,10 @@ impl<'a> NodeBuilder<'a> { // Mesh cache (#3) — with parameter-hash invalidation // =========================================================================== // -// v2 fix: the cache now keys on (NodeId, ParamHash) instead of just -// NodeId. This means editing a node's parameters (e.g. wall length) -// correctly produces a fresh mesh even though the NodeId is unchanged. -// `invalidate(id)` still works as a coarse hammer. +// The cache keys on (NodeId, ParamHash), not NodeId alone, so editing +// a node's parameters (e.g. wall length) produces a fresh mesh even +// though the NodeId is unchanged. `invalidate(id)` is the coarse +// hammer for cases the hash cannot see. /// Stable hash of a node's geometry + transform + material parameters. /// Computed via `DefaultHasher` (cheap, deterministic within a process). @@ -1672,13 +1649,12 @@ impl Default for MeshCache { } // =========================================================================== -// Exporter trait (#1) — consolidated in v2 +// Exporter trait // =========================================================================== // -// v2 fix: `export_with_cache` is now a *provided method* on -// `Exporter` itself, not on a separate `ExporterWithCache` trait. -// This fixes the `::export_with_cache` not-found -// errors. The default impl ignores the cache; exporters that want +// `export_with_cache` is a *provided method* on `Exporter` itself +// rather than a separate extension trait, so every exporter can be +// called through one path. The default impl ignores the cache; exporters that want // to use the cache override the method directly. pub trait Exporter { @@ -1870,16 +1846,6 @@ pub enum PartKind { Beam, } -// =========================================================================== -// Cross-impl: legacy DofConstraint <-> cad_scene::DofConstraint -// =========================================================================== -// -// The legacy `crate::cad::DofConstraint` struct (defined in `mod.rs`) -// has the same fields as this one. The `From` impls below let the -// two coexist during the migration period without duplicating logic. - - - // =========================================================================== // Tests // =========================================================================== @@ -1906,6 +1872,47 @@ mod size_tests { } } + /// `pos()` / `rot()` / `size()` return `Vec3f` **by value**. Writing + /// through them (`node.pos().x = v`) compiles, mutates a temporary + /// and silently discards the write — which is exactly what every + /// properties-panel input in workspace.rs used to do. Mutation must + /// go through the `set_*` methods. + #[test] + fn setters_write_through_but_getters_are_copies() { + let mut n = node_with(CadSolid::Box { size: Vec3f { x: 1.0, y: 1.0, z: 1.0 } }); + + // The trap: this compiles and does nothing. + #[allow(unused_must_use)] + { + n.pos().x = 42.0; + n.rot().y = 42.0; + } + assert!( + n.pos().x.abs() < EPS, + "writing through pos() must not reach the node (it returns a copy)" + ); + assert!( + n.rot().y.abs() < EPS, + "writing through rot() must not reach the node (it returns a copy)" + ); + + // The correct path. + let mut p = n.pos(); + p.x = 42.0; + n.set_pos(p); + assert!((n.pos().x - 42.0).abs() < EPS, "set_pos must write through"); + + let mut r = n.rot(); + r.y = 90.0; + n.set_rot(r); + assert!((n.rot().y - 90.0).abs() < EPS, "set_rot must write through"); + + let mut sz = n.size(); + sz.z = 7.0; + n.set_size(sz); + assert!((n.size().z - 7.0).abs() < EPS, "set_size must write through"); + } + fn assert_size(got: Vec3f, want: (f32, f32, f32), what: &str) { assert!( (got.x - want.0).abs() < EPS diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/commands.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/commands.rs index 2d870b7..5935330 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/commands.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/commands.rs @@ -1,34 +1,21 @@ -//! # commands — Command pattern for undo/redo (#7) +//! # commands — the undo/redo command stack. //! //! ## Design //! -//! The existing `CadCommand` enum in `mod.rs` (line ~3538) works for -//! the current command set, but it has two limitations: +//! Each command is a struct that owns its payload and knows how to +//! execute and undo itself; the stack is `Vec>`. This +//! is open for extension (a new command is a new struct, not a new +//! match arm in two places) and self-contained (the data for "which +//! node, moved from where to where" lives in the command, not in a +//! side table the editor has to keep in sync). //! -//! 1. **Closed for extension.** Adding a new command type (e.g. -//! `SplitWall`, `MirrorSelection`) requires modifying the enum, -//! the `execute()` match, and the `undo()` match. Plugin authors -//! can't add commands without forking. +//! Commands reach the scene through `CommandContext`, which the editor +//! implements over its parts store. `MockCommandContext` implements it +//! over an insertion-ordered `Vec` for tests — it must stay ordered, +//! see the note on that type. //! -//! 2. **No payload carrying.** The enum variants are bare -//! identifiers; the actual data (which node, where it moved from / -//! to) lives in side tables that the editor has to maintain. -//! -//! The `Command` trait below fixes both. Each command is a struct -//! that owns its payload and knows how to execute + undo itself. -//! The undo/redo stack becomes `Vec>`. -//! -//! ## Migration strategy -//! -//! This module is **additive** — it doesn't touch the existing -//! `CadCommand` enum. During the migration period: -//! -//! - New code writes `Box` impls. -//! - Old code keeps using `CadCommand` enum variants. -//! - `CadCommandWrapper` adapts an old enum variant into a `Command` -//! so it can sit on the new stack. -//! -//! Once every variant has a wrapper, the enum can be deleted. +//! `MAX_UNDO_LEVELS` bounds the stack; it is a `VecDeque` so the oldest +//! entry is dropped in O(1). //! //! ## Example //! diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs index fdf2f8a..0b466ef 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs @@ -1,44 +1,13 @@ // File: src/construction_frame/pages/workspace/cad/mod.rs // -// ## Rewrite status (v4 — cached CadScene + shared MeshCache) +// Widget definitions, script (DSL) declarations and shared types for +// the CAD editor. The behaviour lives in the sibling modules; this file +// exists because `#[derive(Script)]` types must be declared alongside +// the `script_mod!` block that references them. // -// v4 changes: -// - `CadViewport` now holds a `SceneCache` (cached `Arc` -// + shared `Arc`) instead of a bare `MeshCache`. -// - `scene()` returns `Arc` (was `CadScene`). O(1) on -// no-op redraws (was O(parts) per call). -// - `mesh_cache()` returns `Arc` (was `&MeshCache`). -// Lets the GLB exporter reuse the preview renderer's cached meshes. -// - Export entry points (`export_floor_plan_pdf`, `export_3d_viewer`) -// now use the cached scene + shared cache instead of cloning parts -// + building a fresh scene + fresh cache per export. -// - `mark_scene_dirty()` added; called from `add_part` and -// `apply_part_field`. Other mutation methods need to call it too -// (see scene_holder.rs docs). -// - `SceneCache` safety net: `scene()` rebuilds if `parts.len()` -// differs from cached scene's node count, catching missing -// `mark_dirty()` calls for add/delete operations. -// -// v2-v3 changes (still in place): -// -// v2 fixes (vs v1): -// - `DofConstraint` removed from the `cad_scene` re-export list -// (was conflicting with the legacy struct). -// - `export_floor_plan_pdf` and `export_3d_viewer` now call -// `cad_scene::Exporter::export_with_cache(...)` — `export_with_cache` -// is a provided method on `Exporter` itself in v2 (no more -// separate `ExporterWithCache` trait). -// -// v1 priorities (still in place): -// - Strong IDs (#7): `NodeId`, `LayerId`, `MaterialId`, `SheetId`. -// - Mesh cache (#3): `CadViewport::mesh_cache` is a new field, -// with parameter-hash-based invalidation in v2. -// - Exporter trait (#1) + SceneVisitor (#9): exporters go through -// `Exporter::export_with_cache`. -// - Builder API (#8): `cad_scene::SceneBuilder`. -// -// Everything below the header comment is the original mod.rs, with -// targeted edits at the architectural seams only. +// See `ARCHITECTURE.md` in this directory for the module map, the +// ownership model and the invariants. Do not describe design here — it +// drifts. Describe it there, next to the tests that enforce it. pub use ::makepad_ai; pub use ::makepad_code_editor; @@ -72,7 +41,7 @@ use std::time::Instant; use crate::cad_store; // arch_pdf: vector PDF export (added by apply_arch_pdf_patch.py) pub mod arch_pdf; -// arch_gltf: GLB 2.0 export for interactive 3D viewing (added by v3 patch) +// arch_gltf: GLB 2.0 export for interactive 3D viewing. pub mod arch_gltf; // arch_stl: Binary STL export for 3D printing and CAD interchange. pub mod arch_stl; @@ -83,10 +52,10 @@ pub mod cad_editor_sheet; // cad_scene: immutable scene graph + Exporter trait + SceneVisitor + MeshCache. // New in this rewrite — see cad_scene.rs for the full rationale. pub mod cad_scene; -// scene_holder: cached Arc + shared Arc for CadViewport. -// New in v4 — lets the GLB exporter reuse the preview renderer's cached meshes. +// scene_holder: the parts store, the cached Arc and the +// shared Arc. The Arc sharing lets the GLB exporter reuse +// the preview renderer's meshes instead of re-meshing. pub mod scene_holder; -// v6: wired-in extracted modules: pub mod constants; pub mod persistence; pub mod math; @@ -101,8 +70,6 @@ pub mod exporters; pub mod code_editor; pub mod workspace; #[cfg(test)] -mod tests; -#[cfg(test)] pub mod profile_benchmarks; #[cfg(test)] pub mod send_sync_audit; @@ -123,7 +90,7 @@ pub use cad_scene::{ SheetId, walk_scene, }; pub use scene_holder::{PartsStore, SceneCache}; -// v6: re-export wired-in module types so existing call sites keep working. +// Re-exports so call sites need not name the sub-module. pub(crate) use constants::{DEFAULT_CAD_SCRIPT, LIVE_UPDATE_INTERVAL, local_openai_url, local_openai_model, GENERATED_DIR, GENERATED_SCRIPT_FILE, GENERATED_OBJ_FILE, DEMO_MAX_CURVE_SEGMENTS, DEMO_MAX_SPHERE_RINGS, DEMO_MAX_TORUS_MINOR_SEGMENTS, PART_SELECT_COLOR, PART_PICK_RADIUS, MAX_UNDO_LEVELS, HOVER_PICK_MIN_MOVE_PX}; pub(crate) use persistence::{PENDING_ATTACHED_IMAGE, CAD_SCRIPT_OUTPUT, cad_manifest_path, cad_generated_dir_path, cad_generated_script_path, cad_generated_obj_path, load_saved_cad_script, save_cad_state, set_cad_script_output, clear_cad_script_output, take_cad_script_output}; pub(crate) use math::{DVec3, vec3_cross, vec3_dot, vec3_length_sq, vec3_length, vec3_normalize, triangulate_polygon, polygon_centroid, polygon_area, point_in_polygon, mat4_mul_vec4, translate_mat, mat4_mul, mat4_inverse, rot_x_mat, rot_y_mat, rot_z_mat, part_model_matrix, ortho_proj, ray_triangle_intersect, ray_aabb_intersect}; @@ -134,8 +101,8 @@ pub(crate) use commands::{ MoveNode, DeleteNode, CreateNode, YawNode, ResizeNode, RotateNode, ModifyNode, CadCommandCtx, }; -// v16: re-export types moved to viewport.rs that are still used -// as field types in CadViewport/CadWorkspace struct definitions. +// Types defined in viewport.rs but named as field types in the +// CadViewport/CadWorkspace struct definitions below. pub(crate) use viewport::{ CadStats, CadMeshData, CadRenderMode, CadViewportViewSnapshot, CadRebuildWorker, CadRebuildRequest, CadRebuildPayload, CadRebuildResult, @@ -1608,7 +1575,7 @@ script_mod! { // [moved to viewport.rs: struct CadViewportViewSnapshot] -/// v11: Holder for simultaneous mutable borrows of `parts`, +/// Holder for simultaneous mutable borrows of `parts`, /// `scene_cache`, and `command_stack`. Returned by /// `CadViewport::split_for_command()` to solve the borrow-checker /// conflict where `legacy_ctx()` + `command_stack.execute()` both @@ -1676,10 +1643,9 @@ pub struct CadViewport { part_geoms: HashMap, /// Cached `Arc` + shared `Arc`. /// - /// v4: replaces the bare `MeshCache` field. The scene snapshot - /// is cached so `scene()` is O(1) on no-op redraws (was O(parts) - /// per call). The mesh cache is `Arc`-shared so the GLB exporter - /// can reuse the preview renderer's cached meshes. + /// The scene snapshot is cached, so `scene()` is O(1) on a no-op + /// redraw rather than O(parts). The mesh cache is `Arc`-shared so + /// the GLB exporter reuses the preview renderer's meshes. /// /// After *any* mutation to `self.parts`, call /// `self.scene_cache.mark_dirty()`. The `scene()` safety net @@ -1727,8 +1693,8 @@ pub struct CadViewport { script_dirty: bool, #[rust(false)] view_dirty: bool, - /// v19: trait-based command stack with automatic cache - /// invalidation. Uses the new `Command` trait + `UndoRedoStack`. + /// Trait-based command stack (`Command` + `UndoRedoStack`) with + /// automatic cache invalidation. #[rust] command_stack: UndoRedoStack, /// Screen position of the last hover pick. @@ -1788,7 +1754,7 @@ pub struct CadViewport { #[rust] last_middle_click_abs: DVec2, - // ---- v3 CAD tool system ---- + // ---- CAD tool system ---- #[rust] tool: CadTool, #[rust] @@ -1800,7 +1766,7 @@ pub struct CadViewport { frame_timer_avg_ms: f64, #[rust(0u32)] frame_timer_count: u32, - // ---- v4: multi-select, clipboard, drag-select ---- + // ---- multi-select, clipboard, drag-select ---- #[rust(false)] shift_pressed: bool, #[rust] @@ -2056,3 +2022,66 @@ pub fn register_cad(vm: &mut ScriptVm) { cad_editor_sheet::script_mod(vm); script_mod(vm); } + +// =========================================================================== +// Tests for the plain-data editor state owned by this module. +// +// These reach private fields, so they belong here rather than in +// tests/cad_integration.rs. +// =========================================================================== + +#[cfg(test)] +mod editor_state_tests { + use super::*; + use crate::construction_frame::pages::workspace::cad::section_shape::SectionShape; + + #[test] + fn polar_settings_default() { + let s = SnapSettings::default(); + assert!(!s.polar_enabled); + assert!((s.polar_angle_increment - 45.0).abs() < 0.1); + } + + #[test] + fn polar_settings_custom_increment() { + let mut s = SnapSettings::default(); + s.polar_enabled = true; + s.polar_angle_increment = 15.0; + assert!(s.polar_enabled); + assert!((s.polar_angle_increment - 15.0).abs() < 0.1); + } + + // ── Beam section settings ── + + #[test] + fn drawing_state_beam_section_default() { + let ds = DrawingState::default(); + assert_eq!(ds.beam_section, SectionShape::Rect); + } + + #[test] + fn drawing_state_beam_section_cycle() { + let mut ds = DrawingState::default(); + assert_eq!(ds.beam_section, SectionShape::Rect); + ds.beam_section = ds.beam_section.next(); + assert_eq!(ds.beam_section, SectionShape::IBeam); + ds.beam_section = ds.beam_section.next(); + assert_eq!(ds.beam_section, SectionShape::HSS); + ds.beam_section = ds.beam_section.next(); + assert_eq!(ds.beam_section, SectionShape::Rect); + } + + // ── PdfPreview enum variant ── + + #[test] + fn pdf_preview_active_pane_variant() { + let pane = CadEditorActivePane::PdfPreview; + assert_eq!(pane.title(), "PDF Preview"); + } + + #[test] + fn pdf_preview_active_pane_not_default() { + let default = CadEditorActivePane::default(); + assert_ne!(default, CadEditorActivePane::PdfPreview); + } +} diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/profile_benchmarks.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/profile_benchmarks.rs index 2f6947c..40aca4f 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/profile_benchmarks.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/profile_benchmarks.rs @@ -389,7 +389,7 @@ mod profile_benchmarks { } } - /// v17: Compare parallel vs sequential GLB export. + /// Compare parallel vs sequential GLB export. #[test] #[ignore = "benchmark: timing-dependent, run explicitly with --ignored"] fn bench_parallel_vs_sequential_export() { diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs index 08a62e5..250dff5 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs @@ -87,23 +87,17 @@ impl CadViewport { stats } + /// Switch to the other view mode and return the new one. + /// + /// This is `set_view_mode` with the target computed rather than + /// passed; the two carried duplicate copies of the gesture-state + /// reset, so a fix to one silently missed the other. pub(crate) fn toggle_view_mode(&mut self, cx: &mut Cx) -> ViewMode { - self.view_mode = match self.view_mode { + let next = match self.view_mode { ViewMode::ThreeD => ViewMode::TwoD, ViewMode::TwoD => ViewMode::ThreeD, }; - self.part_dragging = false; - self.view_dragging = false; - self.pan_3d_dragging = false; - self.active_touches.clear(); - self.pinch_last_dist = None; - self.pinch_last_mid = None; - self.camera.orbit_last_abs = None; - if self.view_mode == ViewMode::TwoD { - self.pan_2d = DVec2::default(); - self.view_drag_world = DVec2::default(); - } - cx.redraw_all(); + self.set_view_mode(cx, next); self.view_mode } @@ -156,9 +150,8 @@ impl CadViewport { self.selection = selection; self.next_part_id = next_part_id; self.part_geoms.clear(); - // v4: invalidate caches. Wholesale parts replacement = scene - // snapshot is stale, and node ids may have shifted so clear - // the mesh cache too. + // Wholesale parts replacement: the scene snapshot is stale and + // node ids may have shifted, so clear the mesh cache too. self.mark_scene_dirty(); self.clear_mesh_cache(); self.script_dirty = false; @@ -219,10 +212,10 @@ impl CadViewport { self.area.redraw(cx); } - // ----- v19: command-based undo/redo system ----- + // ----- command-based undo/redo ----- // - // Uses the new `Command` trait + `UndoRedoStack` with - // `CadCommandCtx` providing the `CommandContext` implementation. + // `Command` + `UndoRedoStack`, with `CadCommandCtx` providing the + // `CommandContext` implementation. // Cache invalidation happens automatically inside each command's // `execute()` / `undo()` via the context. @@ -335,10 +328,10 @@ impl CadViewport { self.command_stack.clear(); } - // ----- v19: convenience methods using new Command types ----- + // ----- command constructors ----- // - // These construct the new command structs and push them onto the - // UndoRedoStack via execute_command(). Cache invalidation is + // These build a command and push it onto the UndoRedoStack via + // execute_command(). Cache invalidation is // automatic inside each command. /// Move a part and push a `MoveNode` onto the command stack. @@ -389,8 +382,8 @@ impl CadViewport { /// Record an add-part operation on the command stack. /// - /// v19: now uses `CreateNode` on the new command stack. The caller - /// must push the part onto `self.parts` BEFORE calling this method. + /// Records a `CreateNode` on the command stack. The caller must + /// push the part onto `self.parts` BEFORE calling this. pub(crate) fn add_part_command(&mut self, id: u64) { // Find the just-pushed part to snapshot it for undo/redo. let part_snapshot = match self.parts.iter().position(|p| p.id.raw() == id) { @@ -434,7 +427,7 @@ impl CadViewport { PartKind::Rect2D => (vec3(1.5, 0.8, 0.1), vec4(0.4, 0.8, 0.9, 1.0)), PartKind::Circle2D => (vec3(0.9, 0.9, 0.1), vec4(0.9, 0.6, 0.4, 1.0)), PartKind::Polygon2D => (vec3(1.0, 0.1, 1.0), vec4(0.3, 0.7, 0.5, 1.0)), - // v2: architectural defaults — realistic dimensions in meters. + // Architectural defaults — realistic dimensions in metres. PartKind::Wall => (vec3(6.0, 2.8, 0.2), vec4(0.78, 0.78, 0.78, 1.0)), PartKind::Slab => (vec3(4.0, 0.2, 4.0), vec4(0.85, 0.85, 0.85, 1.0)), PartKind::Door => (vec3(0.9, 2.1, 0.2), vec4(0.55, 0.35, 0.20, 1.0)), @@ -1223,8 +1216,8 @@ impl CadViewport { // index so the delete command can restore it on undo. self.delete_part_command(part.id.raw(), part.clone(), old_idx); self.selection = new_ids; - // v4: structural edit (added N parts, removed 1). Invalidate - // both caches. + // Structural edit (added N parts, removed 1): invalidate both + // caches. self.mark_scene_dirty(); self.clear_mesh_cache(); self.script_dirty = true; @@ -1270,8 +1263,8 @@ impl CadViewport { } p }; - // v12: use modify_part_command for automatic cache invalidation - // + undo recording on the command stack. + // modify_part_command gives automatic cache invalidation and + // undo recording on the command stack. self.modify_part_command(id, old_part.clone(), new_part.clone()); self.part_geoms.remove(&id); self.script_dirty = true; @@ -1336,10 +1329,9 @@ impl CadViewport { p.parent = Some(NodeId(gid)); } } - // v4: parameter edit (group_id affects NodeMetadata.parent). - // mark_scene_dirty so the cached snapshot picks up the new - // group ids. No mesh invalidation needed — group_id doesn't - // affect geometry. + // Parameter edit: group_id feeds NodeMetadata.parent, so the + // cached snapshot must be rebuilt. No mesh invalidation — the + // group id does not affect geometry. self.mark_scene_dirty(); } @@ -1517,10 +1509,9 @@ impl CadViewport { /// Get the current scene as a cached `Arc`. /// - /// v4: now O(1) on no-op redraws (returns the cached Arc). - /// Rebuilds only after `scene_cache.mark_dirty()` or if - /// `parts.len()` differs from the cached scene's node count - /// (safety net for missing `mark_dirty()` calls). + /// O(1) on a no-op redraw: returns the cached `Arc`. Rebuilds only + /// when the `PartsStore` generation has moved past the one the + /// snapshot was built from. /// /// The returned `Arc` can be held independently of the viewport's /// lifetime — the scene stays alive even if the viewport mutates @@ -1534,9 +1525,8 @@ impl CadViewport { /// can be held independently of the viewport's lifetime — /// the cache stays alive even if the viewport is dropped. /// - /// v4: now returns `Arc` (was `&MeshCache`). This - /// lets the GLB exporter reuse the preview renderer's cached - /// meshes: both hold `Arc`s to the same underlying cache. + /// Returns an `Arc` so the GLB exporter can reuse the preview + /// renderer's cached meshes; both hold the same underlying cache. pub fn mesh_cache(&self) -> std::sync::Arc { self.scene_cache.mesh_cache() } @@ -1545,9 +1535,8 @@ impl CadViewport { /// the node's parameters change (e.g. wall length edited in the /// properties panel). /// - /// Note: with v2's parameter-hash-based cache, this is now rarely - /// necessary — parameter edits are detected automatically via - /// ParamHash. Use this only for forced rebuilds. + /// Rarely necessary: parameter edits are detected automatically + /// via ParamHash. Use this only to force a rebuild. pub fn invalidate_node(&self, id: u64) { self.scene_cache.invalidate_node(id); } @@ -1562,9 +1551,10 @@ impl CadViewport { /// *any* mutation to `self.parts`. The next `scene()` call will /// rebuild. /// - /// v4: new method. The `scene()` safety net catches length - /// mismatches, but parameter edits (same length, different - /// content) need explicit `mark_dirty()`. + /// Marks the cached scene snapshot stale. Prefer mutating through + /// `PartsStore`, which bumps the generation and makes this + /// unnecessary; this remains for the paths that still edit parts + /// through `as_mut_vec()`. pub fn mark_scene_dirty(&self) { self.scene_cache.mark_dirty(); } @@ -1652,7 +1642,7 @@ impl CadViewport { format!("cube(0.01,0.01,0.01,true)") } } - // v2: arch kinds emit cube() or cylinder() in the CAD script. + // Arch kinds emit cube() or cylinder() in the CAD script. PartKind::Wall | PartKind::Slab | PartKind::Door @@ -2596,7 +2586,7 @@ impl CadViewport { self.draw_vector.line_to(o.x as f32, o.y as f32 + 6.0); self.draw_vector.stroke(1.0); - // v3: rubber band preview + // Rubber band preview. self.draw_rubber_band(cx); self.draw_polar_guidelines(cx); self.draw_section_indicators(cx); @@ -2802,7 +2792,7 @@ impl CadViewport { } // ======================================================================= - // v3: CAD Tool System — snapping, drawing, rubber band, coordinate readout + // CAD tool system — snapping, drawing, rubber band, coordinate readout // ======================================================================= pub(crate) fn set_tool(&mut self, cx: &mut Cx, tool: CadTool) { @@ -3363,8 +3353,15 @@ impl CadViewport { let new_w = (part.size().x as f64 + extend_dist).min(10.0) as f32; let new_h = (part.size().z as f64 + extend_dist).min(10.0) as f32; if let Some(p) = self.parts.get_mut_by_raw_id(selected_id) { - p.size().x = new_w; - p.size().z = new_h; + // `size()` returns a copy; writing through it + // (`p.size().x = ..`) compiles and silently + // discards the write, which is what this did + // and why the Extend tool never extended + // anything. + let mut s = p.size(); + s.x = new_w; + s.z = new_h; + p.set_size(s); } } } @@ -3482,9 +3479,8 @@ impl CadViewport { } } self.selection = vec![id]; - // v4: structural edit (tool finished, one or more parts added). - // Invalidate caches. Catches wall/circle/rect/area/column/beam - // tool completions. + // Structural edit: a tool finished and added one or more parts + // (wall/circle/rect/area/column/beam). Invalidate caches. self.mark_scene_dirty(); self.clear_mesh_cache(); self.script_dirty = true; @@ -3520,7 +3516,7 @@ impl CadViewport { kind_hint: Some(PartKind::Polygon2D), }); self.add_part_command(id); - // v4: structural edit (new polygon part). Invalidate caches. + // Structural edit (new polygon part): invalidate caches. self.mark_scene_dirty(); self.clear_mesh_cache(); } @@ -3640,7 +3636,7 @@ impl CadViewport { prev = next; } self.selection = ids; - // v4: structural edit (added N walls from path). Invalidate caches. + // Structural edit (added N walls from path): invalidate caches. self.mark_scene_dirty(); self.clear_mesh_cache(); self.script_dirty = true; @@ -6755,7 +6751,7 @@ impl Widget for CadViewport { // =========================================================================== -// v16: Helpers moved from mod.rs (no #[derive(Script)] needed) +// Helpers that need no #[derive(Script)], so they live outside mod.rs // =========================================================================== impl DrawCadMesh { @@ -6806,7 +6802,55 @@ pub(crate) struct CadMeshData { pub(crate) stats: CadStats, } -pub(crate) fn cad_mesh_data_from_solid(solid: &Solid) -> CadMeshData { +/// How a solid's mesh is mapped into GPU buffer space. +/// +/// The two consumers of the mesh pipeline want different things from +/// the same triangle walk, and used to have a near-identical copy of +/// that walk each. The differences are exactly the two variants below. +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum MeshSpace { + /// Recentre on the solid's bounding-box centre and scale so the + /// largest dimension is `VIEW_FIT_EXTENT`. Nudge each vertex + /// `SURFACE_OFFSET` along its normal to reduce z-fighting between + /// coincident faces. Used for the single-solid preview viewport, + /// which has no camera framing of its own. + ViewNormalised, + /// Keep model coordinates untouched. Used for per-part geometry in + /// the multi-part scene, where each part's transform places it and + /// rescaling would break the layout. + Model, +} + +/// Fit extent for `MeshSpace::ViewNormalised`: the largest dimension of +/// the solid is scaled to this many world units. +const VIEW_FIT_EXTENT: f64 = 1.75; + +/// Per-vertex outward nudge applied in `MeshSpace::ViewNormalised`. +const SURFACE_OFFSET: f64 = 0.0001; + +/// Triangle winding of the emitted vertex buffer. +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum Winding { + /// Emit p0, p1, p2 — the order the normal was computed from. + AsComputed, + /// Emit p0, p2, p1. The per-part scene path has always used this + /// and the shader disables backface culling, so it renders the + /// same; it is preserved rather than "fixed" because nothing here + /// proves which one the depth-sorted paths expect. + Reversed, +} + +/// The one triangle walk that feeds every GPU buffer in this module. +/// +/// Returns the interleaved vertex buffer (8 floats per vertex: position +/// xyzw, normal xyz, pad) and its identity index buffer, plus the stats +/// derived from the solid. Returns empty buffers when the solid has no +/// geometry; callers decide whether that is `None` or an empty struct. +fn build_mesh_buffers( + solid: &Solid, + space: MeshSpace, + winding: Winding, +) -> (Vec, Vec, CadStats) { let mesh = solid.mesh(); let mut stats = CadStats { vertices: solid.vertex_count(), @@ -6815,24 +6859,27 @@ pub(crate) fn cad_mesh_data_from_solid(solid: &Solid) -> CadMeshData { }; if mesh.vertices.is_empty() || mesh.triangles.is_empty() { - return CadMeshData { - stats, - ..Default::default() - }; + return (Vec::new(), Vec::new(), stats); } - let bbox = mesh.bounding_box(); - if bbox.is_empty() { - return CadMeshData { - stats, - ..Default::default() - }; - } - - let center = bbox.center(); - let size = bbox.size(); - stats.max_dimension = size.x.max(size.y).max(size.z); - let scale = 1.75 / stats.max_dimension.max(0.000_001); + // Only the normalised path needs the bounding box. An empty box is + // unusable for scaling, so that path bails; the model path does not + // care and carries on. + let (center, scale, offset) = match space { + MeshSpace::ViewNormalised => { + let bbox = mesh.bounding_box(); + if bbox.is_empty() { + return (Vec::new(), Vec::new(), stats); + } + let size = bbox.size(); + stats.max_dimension = size.x.max(size.y).max(size.z); + let scale = VIEW_FIT_EXTENT / stats.max_dimension.max(0.000_001); + let c = bbox.center(); + ((c.x, c.y, c.z), scale, SURFACE_OFFSET) + } + MeshSpace::Model => ((0.0, 0.0, 0.0), 1.0, 0.0), + }; + let (center_x, center_y, center_z) = center; #[cfg(target_os = "android")] let y_flip = -1.0; @@ -6843,32 +6890,26 @@ pub(crate) fn cad_mesh_data_from_solid(solid: &Solid) -> CadMeshData { let mut indices = Vec::with_capacity(mesh.triangles.len() * 3); for tri in &mesh.triangles { - let Some(a) = mesh.vertices.get(tri[0] as usize) else { - continue; - }; - let Some(b) = mesh.vertices.get(tri[1] as usize) else { - continue; - }; - let Some(c) = mesh.vertices.get(tri[2] as usize) else { + let (Some(a), Some(b), Some(c)) = ( + mesh.vertices.get(tri[0] as usize), + mesh.vertices.get(tri[1] as usize), + mesh.vertices.get(tri[2] as usize), + ) else { continue; }; - let ax = (a.x - center.x) * scale; - let ay = (a.y - center.y) * scale; - let az = (a.z - center.z) * scale; - let bx = (b.x - center.x) * scale; - let by = (b.y - center.y) * scale; - let bz = (b.z - center.z) * scale; - let cx_ = (c.x - center.x) * scale; - let cy = (c.y - center.y) * scale; - let cz = (c.z - center.z) * scale; + let ax = (a.x - center_x) * scale; + let ay = (a.y - center_y) * scale; + let az = (a.z - center_z) * scale; + let bx = (b.x - center_x) * scale; + let by = (b.y - center_y) * scale; + let bz = (b.z - center_z) * scale; + let cx_ = (c.x - center_x) * scale; + let cy = (c.y - center_y) * scale; + let cz = (c.z - center_z) * scale; - let ex = bx - ax; - let ey = by - ay; - let ez = bz - az; - let fx = cx_ - ax; - let fy = cy - ay; - let fz = cz - az; + let (ex, ey, ez) = (bx - ax, by - ay, bz - az); + let (fx, fy, fz) = (cx_ - ax, cy - ay, cz - az); let mut nx = ey * fz - ez * fy; let mut ny = ez * fx - ex * fz; let mut nz = ex * fy - ey * fx; @@ -6883,73 +6924,64 @@ pub(crate) fn cad_mesh_data_from_solid(solid: &Solid) -> CadMeshData { nz = 0.0; } + // Flip normals that point back towards the centroid so a convex + // solid always faces outward. let centroid_x = (ax + bx + cx_) / 3.0; let centroid_y = (ay + by + cy) / 3.0; let centroid_z = (az + bz + cz) / 3.0; - let dot = nx * centroid_x + ny * centroid_y + nz * centroid_z; - let offset = 0.0001; + let inward = nx * centroid_x + ny * centroid_y + nz * centroid_z < 0.0; + let (fnx, fny, fnz) = if inward { (-nx, -ny, -nz) } else { (nx, ny, nz) }; - let (p0, p1, p2, normal) = if dot < 0.0 { - let fnx = -nx; - let fny = -ny; - let fnz = -nz; - ( - [ - (bx + fnx * offset) as f32, - ((by + fny * offset) * y_flip) as f32, - (bz + fnz * offset) as f32, - ], - [ - (ax + fnx * offset) as f32, - ((ay + fny * offset) * y_flip) as f32, - (az + fnz * offset) as f32, - ], - [ - (cx_ + fnx * offset) as f32, - ((cy + fny * offset) * y_flip) as f32, - (cz + fnz * offset) as f32, - ], - [fnx as f32, (fny * y_flip) as f32, fnz as f32], - ) - } else { - ( - [ - (ax + nx * offset) as f32, - ((ay + ny * offset) * y_flip) as f32, - (az + nz * offset) as f32, - ], - [ - (bx + nx * offset) as f32, - ((by + ny * offset) * y_flip) as f32, - (bz + nz * offset) as f32, - ], - [ - (cx_ + nx * offset) as f32, - ((cy + ny * offset) * y_flip) as f32, - (cz + nz * offset) as f32, - ], - [nx as f32, (ny * y_flip) as f32, nz as f32], - ) + let place = |px: f64, py: f64, pz: f64| { + [ + (px + fnx * offset) as f32, + ((py + fny * offset) * y_flip) as f32, + (pz + fnz * offset) as f32, + ] }; + // A flipped normal also swaps the first two vertices, so the + // winding stays consistent with the emitted normal. + let (p0, p1, p2) = if inward { + (place(bx, by, bz), place(ax, ay, az), place(cx_, cy, cz)) + } else { + (place(ax, ay, az), place(bx, by, bz), place(cx_, cy, cz)) + }; + let normal = [fnx as f32, (fny * y_flip) as f32, fnz as f32]; - for p in [p0, p1, p2] { + let ordered = match winding { + Winding::AsComputed => [p0, p1, p2], + Winding::Reversed => [p0, p2, p1], + }; + for p in ordered { vertices .extend_from_slice(&[p[0], p[1], p[2], 1.0, normal[0], normal[1], normal[2], 0.0]); indices.push(indices.len() as u32); } } + (indices, vertices, stats) +} + +/// Mesh for the single-solid preview viewport: framed to fit, with +/// stats for the HUD. +pub(crate) fn cad_mesh_data_from_solid(solid: &Solid) -> CadMeshData { + let (indices, vertices, stats) = + build_mesh_buffers(solid, MeshSpace::ViewNormalised, Winding::AsComputed); if vertices.is_empty() || indices.is_empty() { - CadMeshData { - stats, - ..Default::default() - } + CadMeshData { stats, ..Default::default() } } else { - CadMeshData { - indices, - vertices, - stats, - } + CadMeshData { indices, vertices, stats } + } +} + +/// Mesh for one part of the multi-part scene, in model coordinates. +/// `None` when the solid has no geometry. +pub(crate) fn part_mesh_buffers(solid: &Solid) -> Option<(Vec, Vec)> { + let (indices, vertices, _) = build_mesh_buffers(solid, MeshSpace::Model, Winding::Reversed); + if vertices.is_empty() || indices.is_empty() { + None + } else { + Some((indices, vertices)) } } @@ -7091,76 +7123,6 @@ pub(crate) fn cad_rebuild_worker_loop( log!("[CAD_WORKSPACE] worker: thread exiting (channel closed)"); } -pub(crate) fn part_mesh_buffers(solid: &Solid) -> Option<(Vec, Vec)> { - let mesh = solid.mesh(); - if mesh.vertices.is_empty() || mesh.triangles.is_empty() { - return None; - } - - #[cfg(target_os = "android")] - let y_flip = -1.0; - #[cfg(not(target_os = "android"))] - let y_flip = 1.0; - - let mut vertices = Vec::with_capacity(mesh.triangles.len() * 3 * 8); - let mut indices = Vec::with_capacity(mesh.triangles.len() * 3); - - for tri in &mesh.triangles { - let (Some(a), Some(b), Some(c)) = ( - mesh.vertices.get(tri[0] as usize), - mesh.vertices.get(tri[1] as usize), - mesh.vertices.get(tri[2] as usize), - ) else { - continue; - }; - let (ax, ay, az) = (a.x, a.y, a.z); - let (bx, by, bz) = (b.x, b.y, b.z); - let (cx_, cy, cz) = (c.x, c.y, c.z); - let (ex, ey, ez) = (bx - ax, by - ay, bz - az); - let (fx, fy, fz) = (cx_ - ax, cy - ay, cz - az); - let mut nx = ey * fz - ez * fy; - let mut ny = ez * fx - ex * fz; - let mut nz = ex * fy - ey * fx; - let len = (nx * nx + ny * ny + nz * nz).sqrt(); - if len > 1.0e-10 { - nx /= len; - ny /= len; - nz /= len; - } else { - nx = 0.0; - ny = 1.0; - nz = 0.0; - } - let cenx = (ax + bx + cx_) / 3.0; - let ceny = (ay + by + cy) / 3.0; - let cenz = (az + bz + cz) / 3.0; - let (p0, p1, p2, normal) = if nx * cenx + ny * ceny + nz * cenz < 0.0 { - ( - [bx as f32, (by * y_flip) as f32, bz as f32], - [ax as f32, (ay * y_flip) as f32, az as f32], - [cx_ as f32, (cy * y_flip) as f32, cz as f32], - [(-nx) as f32, ((-ny) * y_flip) as f32, (-nz) as f32], - ) - } else { - ( - [ax as f32, (ay * y_flip) as f32, az as f32], - [bx as f32, (by * y_flip) as f32, bz as f32], - [cx_ as f32, (cy * y_flip) as f32, cz as f32], - [nx as f32, (ny * y_flip) as f32, nz as f32], - ) - }; - for p in [p0, p2, p1] { - vertices - .extend_from_slice(&[p[0], p[1], p[2], 1.0, normal[0], normal[1], normal[2], 0.0]); - indices.push(indices.len() as u32); - } - } - if vertices.is_empty() || indices.is_empty() { - None - } else { - Some((indices, vertices)) - } -} pub(crate) fn ensure_ground_geometry(cx: &mut Cx, geometry: &mut Option) -> GeometryId { let geometry = geometry.get_or_insert_with(|| { @@ -7236,3 +7198,331 @@ fn part_model_matrix_cadnode(node: &CadNode) -> Mat4f { let rzyx = mat4_mul(&mat4_mul(&rot_z_mat(t.rotation_euler_xyz.z), &rot_y_mat(t.rotation_euler_xyz.y)), &rot_x_mat(t.rotation_euler_xyz.x)); mat4_mul(&translate_mat(t.translation), &rzyx) } + +// =========================================================================== +// Tests for the mesh producers and the viewport's plain-data helpers. +// +// These live here, not in tests/cad_integration.rs, because they reach +// crate-private items. A test that needs `pub` widened to run is a test +// in the wrong place. +// =========================================================================== + +#[cfg(test)] +mod viewport_helper_tests { + use super::*; + use makepad_widgets::{Vec3f, Vec4f}; + use crate::construction_frame::pages::workspace::cad::cad_scene::{ + CadSolid, CadTransform, IdAllocator, LayerId, MaterialId, NodeId, + NodeMetadata, SceneBuilder, + }; + use crate::makepad_csg::Solid; + + /// Test that CadStats correctly computes vertex/triangle counts. + #[test] + fn cad_stats_computes_counts() { + let mut alloc = IdAllocator::new(); + let scene = SceneBuilder::new(&mut alloc) + .cube() + .size(Vec3f { x: 2.0, y: 2.0, z: 2.0 }) + .finish() + .build(); + + // Build a solid and check stats. + let part = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene)[0]; + let solid = part.build_solid(); + let mesh = solid.mesh(); + + assert!(!mesh.vertices.is_empty(), "cube should have vertices"); + assert!(!mesh.triangles.is_empty(), "cube should have triangles"); + + // A cube has 8 vertices and 12 triangles (6 faces × 2). + // But makepad_csg may produce more depending on tessellation. + assert!(mesh.vertices.len() >= 8, "cube should have at least 8 vertices"); + assert!(mesh.triangles.len() >= 12, "cube should have at least 12 triangles"); + } + + // ----------------------------------------------------------------- + // Characterization tests for the two mesh-buffer producers. + // + // `cad_mesh_data_from_solid` and `part_mesh_buffers` were separate + // near-copies. These tests pin the observable differences between + // them so the unification behind one function with a normalisation + // flag (Phase 4.4) cannot silently change either caller's output. + // ----------------------------------------------------------------- + + fn unit_cube_solid() -> Solid { + let mut alloc = IdAllocator::new(); + let scene = SceneBuilder::new(&mut alloc) + .cube() + .size(Vec3f { x: 2.0, y: 4.0, z: 6.0 }) + .finish() + .build(); + let part = + &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene) + [0]; + part.build_solid() + } + + /// Normalised output is recentred on the origin and scaled so the + /// largest dimension is 1.75. Raw output keeps model coordinates. + #[test] + fn normalised_mesh_is_recentred_and_scaled_raw_is_not() { + let solid = unit_cube_solid(); + let norm = crate::construction_frame::pages::workspace::cad::viewport::cad_mesh_data_from_solid(&solid); + let (_, raw_verts) = + crate::construction_frame::pages::workspace::cad::viewport::part_mesh_buffers(&solid) + .expect("cube produces buffers"); + + // Stride is 8 floats: pos.xyzw then normal.xyz + pad. + let extent = |v: &[f32], off: usize| { + let mut lo = f32::MAX; + let mut hi = f32::MIN; + for chunk in v.chunks_exact(8) { + lo = lo.min(chunk[off]); + hi = hi.max(chunk[off]); + } + (lo, hi) + }; + + // Largest dimension of the normalised mesh is 1.75 (the Z extent, + // since the cube is 2 x 4 x 6). + let (nz_lo, nz_hi) = extent(&norm.vertices, 2); + assert!( + ((nz_hi - nz_lo) - 1.75).abs() < 1e-3, + "normalised max dimension should be 1.75, got {}", + nz_hi - nz_lo + ); + // ...and it is centred. + assert!( + (nz_hi + nz_lo).abs() < 1e-3, + "normalised mesh should be centred, got lo={nz_lo} hi={nz_hi}" + ); + + // The raw mesh keeps model units: the Z extent is still 6. + let (rz_lo, rz_hi) = extent(&raw_verts, 2); + assert!( + ((rz_hi - rz_lo) - 6.0).abs() < 1e-3, + "raw mesh should keep model units, got {}", + rz_hi - rz_lo + ); + + // Stats are only populated on the normalised path. + assert_eq!(norm.stats.vertices, solid.vertex_count()); + assert_eq!(norm.stats.triangles, solid.triangle_count()); + assert!( + (norm.stats.max_dimension - 6.0).abs() < 1e-6, + "stats.max_dimension is the pre-scale extent" + ); + } + + /// Both producers emit the same triangle count and a trivial + /// index buffer (0..n), and both flip inward-facing normals out. + #[test] + fn both_mesh_producers_agree_on_topology_and_outward_normals() { + let solid = unit_cube_solid(); + let norm = crate::construction_frame::pages::workspace::cad::viewport::cad_mesh_data_from_solid(&solid); + let (raw_idx, raw_verts) = + crate::construction_frame::pages::workspace::cad::viewport::part_mesh_buffers(&solid) + .expect("cube produces buffers"); + + assert_eq!(norm.indices.len(), raw_idx.len()); + assert_eq!(norm.vertices.len(), raw_verts.len()); + let expected: Vec = (0..raw_idx.len() as u32).collect(); + assert_eq!(raw_idx, expected, "index buffer is the identity sequence"); + assert_eq!(norm.indices, expected); + + // Every normal points away from the origin for a centred convex + // solid, which is what the dot < 0 branch in both functions is for. + for chunk in norm.vertices.chunks_exact(8) { + let (px, py, pz) = (chunk[0], chunk[1], chunk[2]); + let (nx, ny, nz) = (chunk[4], chunk[5], chunk[6]); + let len = (nx * nx + ny * ny + nz * nz).sqrt(); + assert!((len - 1.0).abs() < 1e-3, "normals are unit length"); + assert!( + px * nx + py * ny + pz * nz > -1e-3, + "normal should face outward" + ); + } + } + + /// The two producers wind their triangles in opposite orders. The + /// normalised path emits p0,p1,p2; the raw path emits p0,p2,p1. + /// This is a real difference the merge must preserve per caller. + #[test] + fn mesh_producers_use_opposite_triangle_winding() { + let solid = unit_cube_solid(); + let norm = crate::construction_frame::pages::workspace::cad::viewport::cad_mesh_data_from_solid(&solid); + let (_, raw_verts) = + crate::construction_frame::pages::workspace::cad::viewport::part_mesh_buffers(&solid) + .expect("cube produces buffers"); + + // Compare the first triangle of each, undoing the normalisation + // so positions are directly comparable. + let scale = 1.75 / norm.stats.max_dimension as f32; + let pos = |v: &[f32], vert: usize| { + let o = vert * 8; + (v[o], v[o + 1], v[o + 2]) + }; + + let n0 = pos(&norm.vertices, 0); + let n1 = pos(&norm.vertices, 1); + let n2 = pos(&norm.vertices, 2); + let r0 = pos(&raw_verts, 0); + let r1 = pos(&raw_verts, 1); + let r2 = pos(&raw_verts, 2); + + // Vertex 0 matches (both emit p0 first); vertices 1 and 2 are swapped. + let close = |a: (f32, f32, f32), b: (f32, f32, f32)| { + // The normalised path also applies a 0.0001 positional offset + // along the normal, so allow a loose epsilon. + (a.0 - b.0 * scale).abs() < 5e-3 + && (a.1 - b.1 * scale).abs() < 5e-3 + && (a.2 - b.2 * scale).abs() < 5e-3 + }; + // r is not centred, so recentre it the same way before comparing. + let cz = 0.0f32; // cube is built centred already for x/y/z + let _ = cz; + assert!( + close(n1, r2) && close(n2, r1), + "raw path emits p0,p2,p1 where the normalised path emits p0,p1,p2; \ + got n1={n1:?} n2={n2:?} r1={r1:?} r2={r2:?}" + ); + } + + /// An empty solid yields `None` from the raw producer and a + /// zero-buffer `CadMeshData` (with stats) from the normalised one. + #[test] + fn empty_solid_returns_none_and_empty_mesh_data() { + let empty = Solid::empty(); + assert!( + crate::construction_frame::pages::workspace::cad::viewport::part_mesh_buffers(&empty) + .is_none(), + "raw producer signals emptiness with None" + ); + let norm = crate::construction_frame::pages::workspace::cad::viewport::cad_mesh_data_from_solid(&empty); + assert!(norm.indices.is_empty()); + assert!(norm.vertices.is_empty()); + assert_eq!(norm.stats.triangles, 0); + } + + /// Test that part_mesh_buffers produces valid (indices, vertices) pairs. + #[test] + fn part_mesh_buffers_produces_valid_data() { + let mut alloc = IdAllocator::new(); + let scene = SceneBuilder::new(&mut alloc) + .cube() + .size(Vec3f { x: 1.0, y: 1.0, z: 1.0 }) + .finish() + .build(); + + let part = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene)[0]; + let solid = part.build_solid(); + let mesh = solid.mesh(); + + assert!(!mesh.vertices.is_empty()); + assert!(!mesh.triangles.is_empty()); + + // Verify triangle indices are within vertex bounds. + for tri in &mesh.triangles { + assert!((tri[0] as usize) < mesh.vertices.len()); + assert!((tri[1] as usize) < mesh.vertices.len()); + assert!((tri[2] as usize) < mesh.vertices.len()); + } + } + + /// Test that build_solid produces different meshes for different sizes. + #[test] + fn build_solid_respects_size() { + let mut alloc = IdAllocator::new(); + let scene_small = SceneBuilder::new(&mut alloc) + .cube() + .size(Vec3f { x: 1.0, y: 1.0, z: 1.0 }) + .finish() + .build(); + + let mut alloc2 = IdAllocator::new(); + let scene_big = SceneBuilder::new(&mut alloc2) + .cube() + .size(Vec3f { x: 10.0, y: 10.0, z: 10.0 }) + .finish() + .build(); + + let part_small = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene_small)[0]; + let part_big = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene_big)[0]; + + let solid_small = part_small.build_solid(); + let solid_big = part_big.build_solid(); + let mesh_small = solid_small.mesh(); + let mesh_big = solid_big.mesh(); + + // Both should have the same number of vertices (same shape, + // different scale), but different coordinate values. + assert_eq!(mesh_small.vertices.len(), mesh_big.vertices.len()); + + // The big cube's vertices should have larger coordinates. + let small_max = mesh_small.vertices.iter().map(|v| v.x.abs()).fold(0.0f64, f64::max); + let big_max = mesh_big.vertices.iter().map(|v| v.x.abs()).fold(0.0f64, f64::max); + assert!(big_max > small_max, "big cube should have larger coordinates"); + } + + /// Test that build_world_solid applies translation. + #[test] + fn build_world_solid_applies_translation() { + let mut alloc = IdAllocator::new(); + let scene = SceneBuilder::new(&mut alloc) + .cube() + .size(Vec3f { x: 1.0, y: 1.0, z: 1.0 }) + .translation(Vec3f { x: 5.0, y: 0.0, z: 0.0 }) + .finish() + .build(); + + let part = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene)[0]; + let local_solid = part.build_solid(); + let world_solid = part.build_world_solid(); + let local_mesh = local_solid.mesh(); + let world_mesh = world_solid.mesh(); + + // World mesh vertices should be shifted by +5 in X. + let local_avg_x = local_mesh.vertices.iter().map(|v| v.x).sum::() + / local_mesh.vertices.len() as f64; + let world_avg_x = world_mesh.vertices.iter().map(|v| v.x).sum::() + / world_mesh.vertices.len() as f64; + + assert!( + (world_avg_x - local_avg_x - 5.0).abs() < 0.01, + "world mesh should be translated by +5 in X (got diff={})", + world_avg_x - local_avg_x + ); + } + + /// Test that CadRenderMode has the expected variants. + #[test] + fn cad_render_mode_variants() { + use crate::construction_frame::pages::workspace::cad::viewport::CadRenderMode; + // Just verify the type exists and can be constructed. + let mode = CadRenderMode::Realistic; + let _ = format!("{:?}", mode); + } + + /// `CommandBorrows` can be constructed and its fields read. + #[test] + fn command_borrows_fields_accessible() { + use crate::construction_frame::pages::workspace::cad::viewport::CommandBorrows; + use crate::construction_frame::pages::workspace::cad::scene_holder::SceneCache; + use crate::construction_frame::pages::workspace::cad::commands::UndoRedoStack; + + let mut parts: Vec = vec![]; + let scene_cache = SceneCache::new(); + let mut command_stack = UndoRedoStack::new(); + + let holder = CommandBorrows { + parts: &mut parts, + scene_cache: &scene_cache, + command_stack: &mut command_stack, + }; + + // Verify fields are accessible. + assert_eq!(holder.parts.len(), 0); + assert!(!holder.command_stack.can_undo()); + } +} diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs index 5f23923..1657f01 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs @@ -63,6 +63,86 @@ impl CadViewportLayoutMode { } } +/// Whether an AI response is complete or still arriving. +/// +/// The two script extractors differed only in how they handle a +/// response that is not yet whole, so that is the parameter. +#[derive(Clone, Copy, PartialEq, Eq)] +enum ResponseState { + /// The response is finished. Trim both ends and, if a fence was + /// opened but never closed, return the text as-is rather than + /// guessing where the body starts. + Complete, + /// Tokens are still arriving. Preserve trailing whitespace (it is + /// the start of the next line), take the body of an unclosed fence, + /// and if there is no fence at all, skip leading prose by seeking + /// the first CAD-script token. + Streaming, +} + +/// Tokens that mark the start of a CAD script in an unfenced streaming +/// response. Kept in sync with the function list in `system_prompt.md`. +const SCRIPT_START_TOKENS: [&str; 10] = [ + "let ", + "render(", + "empty()", + "cube(", + "cube_uniform(", + "sphere(", + "cylinder(", + "cone(", + "torus(", + "tapered_cylinder(", +]; + +/// Pull the CAD script out of an AI response. +/// +/// The system prompt tells the model to emit bare script with no +/// markdown, but models add fences regardless, so both shapes are +/// accepted. +fn extract_script(text: &str, state: ResponseState) -> String { + let trimmed = match state { + ResponseState::Complete => text.trim(), + ResponseState::Streaming => text.trim_start(), + }; + + if let Some(start) = trimmed.find("```") { + let after_open = &trimmed[start + 3..]; + // Skip the info string on the opening fence line. + let code_start = after_open.find('\n').map(|i| i + 1).unwrap_or(0); + if let Some(end) = after_open[code_start..].find("```") { + let body = &after_open[code_start..code_start + end]; + return match state { + ResponseState::Complete => body.trim().to_string(), + ResponseState::Streaming => body.trim_start().to_string(), + }; + } + // Fence opened but not yet closed. + return match state { + ResponseState::Streaming => after_open[code_start..].trim_start().to_string(), + // Complete and unclosed: the fence is malformed, so return + // everything and let the caller's empty check deal with it. + ResponseState::Complete => trimmed.to_string(), + }; + } + + // No fence. A complete response is assumed to be the script. + if state == ResponseState::Complete { + return trimmed.to_string(); + } + + // Streaming and unfenced: drop any preamble before the earliest + // script token. + match SCRIPT_START_TOKENS + .iter() + .filter_map(|needle| trimmed.find(needle)) + .min() + { + Some(idx) => trimmed[idx..].to_string(), + None => trimmed.to_string(), + } +} + impl CadWorkspace { pub(crate) fn set_rebuild_pending(&mut self, cx: &mut Cx, pending: bool) { if self.rebuild_pending == pending { @@ -259,6 +339,38 @@ impl CadWorkspace { } } + /// Apply a parsed value to one field of the selected part, in every + /// viewport, and invalidate everything that derives from it. + /// + /// The nine position/size/rotation inputs in the properties panel + /// each had their own copy of this body, differing only in which + /// field they assigned. Nine copies is nine chances to forget one of + /// the four invalidation steps, and the geometry cache in particular + /// fails silently when missed: the part keeps rendering at its old + /// shape until something else happens to evict it. + /// + /// `set` receives the part and the new value. + fn apply_field_to_selected_part( + &mut self, + cx: &mut Cx, + value: f32, + set: impl Fn(&mut CadNode, f32) + Copy, + ) { + self.apply_to_all_viewports(cx, |vp, _cx| { + if let Some(id) = vp.selection.first().copied() { + if let Some(p) = vp.parts.get_mut_by_raw_id(id) { + set(p, value); + // The uploaded GPU geometry, the cached mesh and the + // cached scene snapshot all derive from this part. + vp.part_geoms.remove(&id); + vp.invalidate_node(id); + } + } + vp.mark_scene_dirty(); + vp.script_dirty = true; + }); + } + pub(crate) fn apply_to_all_viewports( &mut self, cx: &mut Cx, @@ -758,7 +870,7 @@ impl CadWorkspace { /// Export the current parts list as a vector PDF floor plan. /// Added by apply_arch_pdf_patch.py — uses arch_pdf::export_parts_to_pdf. fn export_floor_plan_pdf(&mut self, cx: &mut Cx) { - // v4: get the cached Arc + shared Arc + // Cached Arc + shared Arc // from the viewport. No parts clone, no scene rebuild, no // fresh cache — we reuse what the preview renderer built. let (scene, cache, part_count) = { @@ -810,7 +922,7 @@ impl CadWorkspace { /// Open viewer.html in any browser to get interactive 3D /// (drag-rotate, scroll-zoom, auto-rotate). fn export_3d_viewer(&mut self, cx: &mut Cx) { - // v4: get the cached Arc + shared Arc. + // Cached Arc + shared Arc. // The GLB exporter *does* use the mesh cache — this is the // main perf win: previously-built preview meshes are reused // instead of re-triangulated. @@ -849,7 +961,7 @@ impl CadWorkspace { Ok(()) => { let glb_path = dir.join("model.glb"); let html_path = dir.join("viewer.html"); - // v4: use the cached scene + shared mesh cache from the + // Use the cached scene + shared mesh cache from the // viewport. The GLB exporter reuses previously-built // preview meshes — re-exports are near-instant. let exporter = arch_gltf::GltfExporter::new(options); @@ -1324,51 +1436,11 @@ impl CadWorkspace { } fn extract_cad_script(text: &str) -> String { - let trimmed = text.trim(); - if let Some(start) = trimmed.find("```") { - let after_open = &trimmed[start + 3..]; - let code_start = after_open.find('\n').map(|i| i + 1).unwrap_or(0); - if let Some(end) = after_open[code_start..].find("```") { - return after_open[code_start..code_start + end].trim().to_string(); - } - } - trimmed.to_string() + extract_script(text, ResponseState::Complete) } fn extract_streaming_cad_script(text: &str) -> String { - let trimmed = text.trim_start(); - if let Some(start) = trimmed.find("```") { - let after_open = &trimmed[start + 3..]; - let code_start = after_open.find('\n').map(|i| i + 1).unwrap_or(0); - if let Some(end) = after_open[code_start..].find("```") { - return after_open[code_start..code_start + end] - .trim_start() - .to_string(); - } - return after_open[code_start..].trim_start().to_string(); - } - let mut first = None; - for needle in [ - "let ", - "render(", - "empty()", - "cube(", - "cube_uniform(", - "sphere(", - "cylinder(", - "cone(", - "torus(", - "tapered_cylinder(", - ] { - if let Some(idx) = trimmed.find(needle) { - first = Some(first.map_or(idx, |f: usize| f.min(idx))); - } - } - if let Some(idx) = first { - trimmed[idx..].to_string() - } else { - trimmed.to_string() - } + extract_script(text, ResponseState::Streaming) } fn stream_ai_response_to_editor(&mut self, cx: &mut Cx) { @@ -2279,7 +2351,7 @@ impl CadWorkspace { } } - // v3 tool selection buttons + // Tool selection buttons. let tool = if self.view.button(cx, ids!(select_tool_btn)).clicked(actions) { Some(CadTool::Select) } else if self.view.button(cx, ids!(line_tool_btn)).clicked(actions) { @@ -2410,7 +2482,7 @@ impl CadWorkspace { vp.redo(cx); }); } - // v14: Update undo/redo button labels with command descriptions. + // Update undo/redo button labels with command descriptions. // Shows "Undo Move part" instead of just "Undo", so the user // knows what will be undone before clicking. { @@ -2911,17 +2983,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.pos().x = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.pos(); + t.x = v; + p.set_pos(t); }); } let val = self @@ -2936,17 +3001,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.pos().y = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.pos(); + t.y = v; + p.set_pos(t); }); } let val = self @@ -2961,17 +3019,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.pos().z = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.pos(); + t.z = v; + p.set_pos(t); }); } @@ -2988,17 +3039,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.size().x = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.size(); + t.x = v; + p.set_size(t); }); } let val = self @@ -3013,17 +3057,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.size().y = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.size(); + t.y = v; + p.set_size(t); }); } let val = self @@ -3038,17 +3075,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.size().z = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.size(); + t.z = v; + p.set_size(t); }); } @@ -3065,17 +3095,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.rot().x = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.rot(); + t.x = v; + p.set_rot(t); }); } let val = self @@ -3090,17 +3113,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.rot().y = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.rot(); + t.y = v; + p.set_rot(t); }); } let val = self @@ -3115,17 +3131,10 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_to_all_viewports(cx, |vp, _cx| { - if let Some(id) = vp.selection.first().copied() { - if let Some(p) = vp.parts.get_mut_by_raw_id(id) { - p.rot().z = val; - vp.part_geoms.remove(&id); - // v4: parameter edit — invalidate caches. - vp.invalidate_node(id); - } - } - vp.mark_scene_dirty(); - vp.script_dirty = true; + self.apply_field_to_selected_part(cx, val, |p, v| { + let mut t = p.rot(); + t.z = v; + p.set_rot(t); }); } @@ -3499,6 +3508,190 @@ pub(crate) fn read_attachment_as_base64(path: &std::path::Path) -> Result CadNode { + CadNode { + id: NodeId(1), + name: "n".into(), + solid: Some(CadSolid::Box { size: Vec3f { x: 1.0, y: 1.0, z: 1.0 } }), + transform: CadTransform::IDENTITY, + material: MaterialId::ROOT, + layer: LayerId::ROOT, + parent: None, + metadata: NodeMetadata::default(), + color: Vec4f { x: 1.0, y: 1.0, z: 1.0, w: 1.0 }, + kind_hint: None, + } + } + + /// The nine properties-panel inputs pass a setter closure to + /// `apply_field_to_selected_part`. Those closures are the whole + /// behaviour of the panel, and every one of them used to be a + /// silent no-op (`p.pos().x = v` writes to a temporary — see + /// `cad_scene::size_tests::setters_write_through_but_getters_are_copies`). + /// + /// This applies each of the nine closures exactly as the call sites + /// do and asserts it reaches the node, on the correct axis, without + /// disturbing the other two. + #[test] + fn each_properties_panel_setter_writes_its_own_axis() { + let setters: [(&str, fn(&mut CadNode, f32)); 9] = [ + ("pos.x", |p, v| { let mut t = p.pos(); t.x = v; p.set_pos(t); }), + ("pos.y", |p, v| { let mut t = p.pos(); t.y = v; p.set_pos(t); }), + ("pos.z", |p, v| { let mut t = p.pos(); t.z = v; p.set_pos(t); }), + ("size.x", |p, v| { let mut t = p.size(); t.x = v; p.set_size(t); }), + ("size.y", |p, v| { let mut t = p.size(); t.y = v; p.set_size(t); }), + ("size.z", |p, v| { let mut t = p.size(); t.z = v; p.set_size(t); }), + ("rot.x", |p, v| { let mut t = p.rot(); t.x = v; p.set_rot(t); }), + ("rot.y", |p, v| { let mut t = p.rot(); t.y = v; p.set_rot(t); }), + ("rot.z", |p, v| { let mut t = p.rot(); t.z = v; p.set_rot(t); }), + ]; + + for (name, set) in setters { + let mut node = cube(); + let before = (node.pos(), node.size(), node.rot()); + set(&mut node, 3.5); + let after = (node.pos(), node.size(), node.rot()); + + let (group, axis) = name.split_once('.').expect("name is group.axis"); + let read = |v: (Vec3f, Vec3f, Vec3f)| match group { + "pos" => v.0, + "size" => v.1, + _ => v.2, + }; + let component = |v: Vec3f| match axis { + "x" => v.x, + "y" => v.y, + _ => v.z, + }; + + assert!( + (component(read(after)) - 3.5).abs() < 1e-4, + "{name}: setter did not reach the node (got {})", + component(read(after)) + ); + + // The other two groups must be untouched. + for other in ["pos", "size", "rot"] { + if other == group { + continue; + } + let pick = |v: (Vec3f, Vec3f, Vec3f)| match other { + "pos" => v.0, + "size" => v.1, + _ => v.2, + }; + let (b, a) = (pick(before), pick(after)); + assert!( + (b.x - a.x).abs() < 1e-4 + && (b.y - a.y).abs() < 1e-4 + && (b.z - a.z).abs() < 1e-4, + "{name}: setter also changed {other}" + ); + } + } + } +} + +#[cfg(test)] +mod script_extraction_tests { + use super::*; + + // These pin the behaviour of the two AI-response extractors before + // they were unified behind one function (Phase 4.4). The system + // prompt tells the model to emit no code fences, but models do it + // anyway, so both fenced and bare input must work. + + #[test] + fn final_extract_unwraps_a_closed_fence() { + let text = "Here you go:\n```cad\nlet a = cube(1.0, 1.0, 1.0, true)\nrender(a)\n```\nEnjoy!"; + assert_eq!( + CadWorkspace::extract_cad_script(text), + "let a = cube(1.0, 1.0, 1.0, true)\nrender(a)" + ); + } + + #[test] + fn final_extract_falls_back_to_whole_text_on_unclosed_fence() { + // A truncated response has no closing fence. The final extractor + // deliberately returns everything rather than guessing. + let text = "```\nlet a = cube(1.0, 1.0, 1.0, true)"; + assert_eq!( + CadWorkspace::extract_cad_script(text), + "```\nlet a = cube(1.0, 1.0, 1.0, true)" + ); + } + + #[test] + fn final_extract_passes_through_unfenced_script() { + let text = " let a = cube(1.0, 1.0, 1.0, true)\nrender(a) "; + assert_eq!( + CadWorkspace::extract_cad_script(text), + "let a = cube(1.0, 1.0, 1.0, true)\nrender(a)" + ); + } + + #[test] + fn streaming_extract_returns_partial_body_of_an_unclosed_fence() { + // This is the difference that matters: mid-stream there is no + // closing fence yet, and the editor still needs the body. + let text = "```cad\nlet a = cube(1.0, 1.0"; + assert_eq!(CadWorkspace::extract_streaming_cad_script(text), "let a = cube(1.0, 1.0"); + } + + #[test] + fn streaming_extract_skips_unfenced_prose_to_the_first_script_token() { + let text = "Sure! I'll build a box for you.\nlet a = cube(1.0, 1.0, 1.0, true)"; + assert_eq!( + CadWorkspace::extract_streaming_cad_script(text), + "let a = cube(1.0, 1.0, 1.0, true)" + ); + } + + #[test] + fn streaming_extract_picks_the_earliest_script_token_not_the_first_listed() { + // `render(` appears before `cube(` in the text but later in the + // needle list; the earliest position must win. + let text = "blah render(x) and cube("; + assert_eq!( + CadWorkspace::extract_streaming_cad_script(text), + "render(x) and cube(" + ); + } + + #[test] + fn streaming_extract_keeps_trailing_whitespace_but_final_extract_trims_it() { + // Mid-stream a trailing newline is the start of the next line, + // so the streaming path preserves it; the final path does not. + let text = "let a = cube(1.0, 1.0, 1.0, true)\n"; + assert_eq!( + CadWorkspace::extract_streaming_cad_script(text), + "let a = cube(1.0, 1.0, 1.0, true)\n" + ); + assert_eq!( + CadWorkspace::extract_cad_script(text), + "let a = cube(1.0, 1.0, 1.0, true)" + ); + } + + #[test] + fn both_extractors_return_empty_for_pure_prose() { + // No fence, no script token: streaming has nothing to show. The + // final extractor returns the prose so the caller's + // "AI returned an empty CAD script" check can catch it. + let text = "I'm sorry, I can't help with that."; + assert_eq!(CadWorkspace::extract_streaming_cad_script(text), text); + assert_eq!(CadWorkspace::extract_cad_script(text), text); + } +} + #[cfg(test)] mod system_prompt_tests { use super::*; diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/tests.rs b/crates/apps/nigig-build/tests/cad_integration.rs similarity index 88% rename from crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/tests.rs rename to crates/apps/nigig-build/tests/cad_integration.rs index 15bd63c..2050fa0 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/tests.rs +++ b/crates/apps/nigig-build/tests/cad_integration.rs @@ -1,13 +1,14 @@ -//! # tests — comprehensive integration tests for the CAD rewrite +//! # cad_integration — integration tests for the CAD module. //! -//! This module collects tests that cross file boundaries (scene -//! conversion, exporter output, GLB validity, PDF dimensions, mesh -//! cache behavior under realistic workloads). Per-module unit tests -//! stay in their own files; this is the integration layer. +//! These cross file boundaries: scene conversion, exporter output, GLB +//! validity, PDF dimensions, mesh-cache behaviour under realistic +//! workloads. Per-module unit tests stay in their own files next to the +//! code they test; this is the integration layer, and it exercises the +//! crate from outside, as a consumer would. //! -//! Run with: `cargo test --package nigig-build cad::tests` +//! Run with: `cargo test --package nigig-build --test cad_integration` -use crate::construction_frame::pages::workspace::cad::cad_scene::{ +use nigig_build::construction_frame::pages::workspace::cad::cad_scene::{ self, CadNode, CadScene, CadSolid, CadTransform, DofConstraint, Exporter, IdAllocator, LayerId, MaterialId, MeshCache, NodeId, NodeMetadata, PartKind, SceneBuilder, SceneVisitor, walk_scene, @@ -99,7 +100,7 @@ mod scene_conversion { // Build a small scene, extract nodes, convert // back to CadScene, verify the geometry survived. let original = build_house_scene(); - let parts = crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&original); + let parts = nigig_build::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&original); assert_eq!(parts.len(), 11); // Convert back via the legacy adapter. @@ -146,7 +147,7 @@ mod scene_conversion { PartKind::Arc, ] { // The From impls in mod.rs handle this. - let cad_kind: crate::construction_frame::pages::workspace::cad::cad_scene::PartKind = kind.into(); + let cad_kind: nigig_build::construction_frame::pages::workspace::cad::cad_scene::PartKind = kind.into(); let back: PartKind = cad_kind.into(); assert_eq!(kind, back, "PartKind round-trip failed for {:?}", kind); } @@ -156,7 +157,7 @@ mod scene_conversion { #[test] fn empty_scene_converts_cleanly() { let empty = CadScene::default(); - let parts = crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&empty); + let parts = nigig_build::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&empty); assert!(parts.is_empty()); } } @@ -190,7 +191,7 @@ mod visitor { fn visit_sphere(&mut self, _node: &CadNode, _r: f32, _su: u32, _sv: u32) { self.spheres += 1; } fn visit_polygon_2d(&mut self, _node: &CadNode, _verts: &[DVec2]) { self.polygons += 1; } fn visit_extruded_polygon(&mut self, _node: &CadNode, _verts: &[DVec2], _h: f32) { self.extruded += 1; } - fn visit_csg(&mut self, _node: &CadNode, _solid: &crate::makepad_csg::Solid) { self.csgs += 1; } + fn visit_csg(&mut self, _node: &CadNode, _solid: &nigig_build::makepad_csg::Solid) { self.csgs += 1; } fn visit_group(&mut self, _node: &CadNode) { self.groups += 1; } } @@ -239,7 +240,7 @@ mod mesh_cache { let cache = MeshCache::new(); // First pass: every node is a cache miss, builds a fresh mesh. - let meshes_v1: Vec> = scene + let meshes_v1: Vec> = scene .nodes() .iter() .map(|n| cache.get_or_build(n)) @@ -248,7 +249,7 @@ mod mesh_cache { assert_eq!(cache.len(), 11, "cache should have one entry per node"); // Second pass: every node should be a cache hit (same Arc). - let meshes_v2: Vec> = scene + let meshes_v2: Vec> = scene .nodes() .iter() .map(|n| cache.get_or_build(n)) @@ -354,7 +355,7 @@ mod mesh_cache { mod glb_validity { use super::*; - use crate::construction_frame::pages::workspace::cad::arch_gltf::{GltfExporter, GltfExportOptions}; + use nigig_build::construction_frame::pages::workspace::cad::arch_gltf::{GltfExporter, GltfExportOptions}; /// GLB 2.0 binary format: /// bytes 0..4 = "glTF" magic @@ -490,7 +491,7 @@ mod glb_validity { mod pdf_dimensions { use super::*; - use crate::construction_frame::pages::workspace::cad::arch_pdf::{ + use nigig_build::construction_frame::pages::workspace::cad::arch_pdf::{ Orientation, PaperSize, PdfExporter, PdfExportOptions, }; @@ -553,7 +554,7 @@ mod pdf_dimensions { assert_eq!(pdf.format_name(), "PDF 1.7"); assert_eq!(pdf.file_extension(), "pdf"); - let glb = crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter::default(); + let glb = nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter::default(); assert_eq!(glb.format_name(), "GLB 2.0"); assert_eq!(glb.file_extension(), "glb"); } @@ -565,8 +566,8 @@ mod pdf_dimensions { mod exporter_trait { use super::*; - use crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; - use crate::construction_frame::pages::workspace::cad::arch_pdf::PdfExporter; + use nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; + use nigig_build::construction_frame::pages::workspace::cad::arch_pdf::PdfExporter; #[test] fn both_exporters_can_be_used_through_trait_object() { @@ -595,7 +596,7 @@ mod exporter_trait { mod round_trip { use super::*; - use crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; + use nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; #[test] fn build_export_reparse_preserves_node_count() { @@ -647,16 +648,16 @@ mod round_trip { } // =========================================================================== -// SceneCache integration tests (v4) +// SceneCache integration tests // =========================================================================== mod scene_cache_integration { use super::*; - use crate::construction_frame::pages::workspace::cad::scene_holder::SceneCache; + use nigig_build::construction_frame::pages::workspace::cad::scene_holder::SceneCache; use std::sync::Arc; /// Build a Vec with N walls of different colors. - use crate::construction_frame::pages::workspace::cad::scene_holder::PartsStore; + use nigig_build::construction_frame::pages::workspace::cad::scene_holder::PartsStore; fn make_colored_store(n: usize) -> PartsStore { let mut store = PartsStore::new(); @@ -677,7 +678,7 @@ mod scene_cache_integration { .finish(); } let scene = builder.build(); - crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene) + nigig_build::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene) } #[test] @@ -802,7 +803,7 @@ mod scene_cache_integration { // the mesh cache by exporting once, then export again and // verify the cache has entries (proving the second export // reused the shared cache). - use crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; + use nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; let cache = SceneCache::new(); let parts = make_colored_parts(5); @@ -883,164 +884,6 @@ mod scene_cache_integration { } } -// =========================================================================== -// v16: Tests for moved viewport helpers -// =========================================================================== - -mod viewport_helper_tests { - use super::*; - use crate::construction_frame::pages::workspace::cad::cad_scene::{ - CadSolid, CadTransform, IdAllocator, LayerId, MaterialId, NodeId, - NodeMetadata, SceneBuilder, - }; - use crate::makepad_csg::Solid; - - /// Test that CadStats correctly computes vertex/triangle counts. - #[test] - fn cad_stats_computes_counts() { - let mut alloc = IdAllocator::new(); - let scene = SceneBuilder::new(&mut alloc) - .cube() - .size(Vec3f { x: 2.0, y: 2.0, z: 2.0 }) - .finish() - .build(); - - // Build a solid and check stats. - let part = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene)[0]; - let solid = part.build_solid(); - let mesh = solid.mesh(); - - assert!(!mesh.vertices.is_empty(), "cube should have vertices"); - assert!(!mesh.triangles.is_empty(), "cube should have triangles"); - - // A cube has 8 vertices and 12 triangles (6 faces × 2). - // But makepad_csg may produce more depending on tessellation. - assert!(mesh.vertices.len() >= 8, "cube should have at least 8 vertices"); - assert!(mesh.triangles.len() >= 12, "cube should have at least 12 triangles"); - } - - /// Test that part_mesh_buffers produces valid (indices, vertices) pairs. - #[test] - fn part_mesh_buffers_produces_valid_data() { - let mut alloc = IdAllocator::new(); - let scene = SceneBuilder::new(&mut alloc) - .cube() - .size(Vec3f { x: 1.0, y: 1.0, z: 1.0 }) - .finish() - .build(); - - let part = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene)[0]; - let solid = part.build_solid(); - let mesh = solid.mesh(); - - assert!(!mesh.vertices.is_empty()); - assert!(!mesh.triangles.is_empty()); - - // Verify triangle indices are within vertex bounds. - for tri in &mesh.triangles { - assert!((tri[0] as usize) < mesh.vertices.len()); - assert!((tri[1] as usize) < mesh.vertices.len()); - assert!((tri[2] as usize) < mesh.vertices.len()); - } - } - - /// Test that build_solid produces different meshes for different sizes. - #[test] - fn build_solid_respects_size() { - let mut alloc = IdAllocator::new(); - let scene_small = SceneBuilder::new(&mut alloc) - .cube() - .size(Vec3f { x: 1.0, y: 1.0, z: 1.0 }) - .finish() - .build(); - - let mut alloc2 = IdAllocator::new(); - let scene_big = SceneBuilder::new(&mut alloc2) - .cube() - .size(Vec3f { x: 10.0, y: 10.0, z: 10.0 }) - .finish() - .build(); - - let part_small = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene_small)[0]; - let part_big = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene_big)[0]; - - let solid_small = part_small.build_solid(); - let solid_big = part_big.build_solid(); - let mesh_small = solid_small.mesh(); - let mesh_big = solid_big.mesh(); - - // Both should have the same number of vertices (same shape, - // different scale), but different coordinate values. - assert_eq!(mesh_small.vertices.len(), mesh_big.vertices.len()); - - // The big cube's vertices should have larger coordinates. - let small_max = mesh_small.vertices.iter().map(|v| v.x.abs()).fold(0.0f64, f64::max); - let big_max = mesh_big.vertices.iter().map(|v| v.x.abs()).fold(0.0f64, f64::max); - assert!(big_max > small_max, "big cube should have larger coordinates"); - } - - /// Test that build_world_solid applies translation. - #[test] - fn build_world_solid_applies_translation() { - let mut alloc = IdAllocator::new(); - let scene = SceneBuilder::new(&mut alloc) - .cube() - .size(Vec3f { x: 1.0, y: 1.0, z: 1.0 }) - .translation(Vec3f { x: 5.0, y: 0.0, z: 0.0 }) - .finish() - .build(); - - let part = &crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene)[0]; - let local_solid = part.build_solid(); - let world_solid = part.build_world_solid(); - let local_mesh = local_solid.mesh(); - let world_mesh = world_solid.mesh(); - - // World mesh vertices should be shifted by +5 in X. - let local_avg_x = local_mesh.vertices.iter().map(|v| v.x).sum::() - / local_mesh.vertices.len() as f64; - let world_avg_x = world_mesh.vertices.iter().map(|v| v.x).sum::() - / world_mesh.vertices.len() as f64; - - assert!( - (world_avg_x - local_avg_x - 5.0).abs() < 0.01, - "world mesh should be translated by +5 in X (got diff={})", - world_avg_x - local_avg_x - ); - } - - /// Test that CadRenderMode has the expected variants. - #[test] - fn cad_render_mode_variants() { - use crate::construction_frame::pages::workspace::cad::viewport::CadRenderMode; - // Just verify the type exists and can be constructed. - let mode = CadRenderMode::Realistic; - let _ = format!("{:?}", mode); - } - - /// `CommandBorrows` can be constructed and its fields read. - #[test] - fn command_borrows_fields_accessible() { - use crate::construction_frame::pages::workspace::cad::viewport::CommandBorrows; - use crate::construction_frame::pages::workspace::cad::scene_holder::SceneCache; - use crate::construction_frame::pages::workspace::cad::commands::UndoRedoStack; - - let mut parts: Vec = vec![]; - let scene_cache = SceneCache::new(); - let mut command_stack = UndoRedoStack::new(); - - let holder = CommandBorrows { - parts: &mut parts, - scene_cache: &scene_cache, - command_stack: &mut command_stack, - }; - - // Verify fields are accessible. - assert_eq!(holder.parts.len(), 0); - assert!(!holder.command_stack.can_undo()); - } -} - // =========================================================================== // Editing tool tests — 2D and 3D coverage for Add/Delete/Move/Resize/Rotate @@ -1052,10 +895,10 @@ mod viewport_helper_tests { mod editing_tools_tests { use super::*; - use crate::construction_frame::pages::workspace::cad::commands::{ + use nigig_build::construction_frame::pages::workspace::cad::commands::{ }; - use crate::construction_frame::pages::workspace::cad::scene_holder::SceneCache; - use crate::construction_frame::pages::workspace::cad::math::point_in_polygon; + use nigig_build::construction_frame::pages::workspace::cad::scene_holder::SceneCache; + use nigig_build::construction_frame::pages::workspace::cad::math::point_in_polygon; /// Build a test part at the given position with a given size. fn make_part_at(id: u64, pos: Vec3f, size: Vec3f) -> CadNode { @@ -1523,7 +1366,7 @@ mod editing_tools_tests { #[cfg(test)] mod dde_integration_tests { - use crate::construction_frame::pages::workspace::cad::construction_geometry::{ + use nigig_build::construction_frame::pages::workspace::cad::construction_geometry::{ CoordInput, parse_coord_input, }; use makepad_widgets::DVec2; @@ -1741,7 +1584,7 @@ mod dde_integration_tests { #[cfg(test)] mod window_crossing_tests { - use crate::construction_frame::pages::workspace::cad::SelectionMode; + use nigig_build::construction_frame::pages::workspace::cad::SelectionMode; use makepad_widgets::DVec2; /// A minimal 2D part representation for testing marquee selection logic. @@ -2053,7 +1896,7 @@ mod window_crossing_tests { #[cfg(test)] mod polar_tracking_tests { - use crate::construction_frame::pages::workspace::cad::construction_geometry::{ + use nigig_build::construction_frame::pages::workspace::cad::construction_geometry::{ snap_to_polar_angle, next_polar_increment, }; use std::f64::consts::PI; @@ -2138,21 +1981,6 @@ mod polar_tracking_tests { assert!((next_polar_increment(22.0) - 5.0).abs() < 0.1); } - #[test] - fn polar_settings_default() { - let s = crate::construction_frame::pages::workspace::cad::SnapSettings::default(); - assert!(!s.polar_enabled); - assert!((s.polar_angle_increment - 45.0).abs() < 0.1); - } - - #[test] - fn polar_settings_custom_increment() { - let mut s = crate::construction_frame::pages::workspace::cad::SnapSettings::default(); - s.polar_enabled = true; - s.polar_angle_increment = 15.0; - assert!(s.polar_enabled); - assert!((s.polar_angle_increment - 15.0).abs() < 0.1); - } } // =========================================================================== @@ -2161,8 +1989,8 @@ mod polar_tracking_tests { #[cfg(test)] mod section_shape_tests { - use crate::construction_frame::pages::workspace::cad::cad_scene::{CadNode, CadSolid, PartKind}; - use crate::construction_frame::pages::workspace::cad::section_shape::{ + use nigig_build::construction_frame::pages::workspace::cad::cad_scene::{CadNode, CadSolid, PartKind}; + use nigig_build::construction_frame::pages::workspace::cad::section_shape::{ SectionShape, IBeamParams, HSSParams, section_vertices, section_bounding_box, section_area, rect_vertices, ibeam_vertices, hss_vertices, @@ -2335,25 +2163,6 @@ mod section_shape_tests { assert_eq!(v.len(), 8); } - // ── Beam section settings ── - - #[test] - fn drawing_state_beam_section_default() { - let ds = crate::construction_frame::pages::workspace::cad::DrawingState::default(); - assert_eq!(ds.beam_section, SectionShape::Rect); - } - - #[test] - fn drawing_state_beam_section_cycle() { - let mut ds = crate::construction_frame::pages::workspace::cad::DrawingState::default(); - assert_eq!(ds.beam_section, SectionShape::Rect); - ds.beam_section = ds.beam_section.next(); - assert_eq!(ds.beam_section, SectionShape::IBeam); - ds.beam_section = ds.beam_section.next(); - assert_eq!(ds.beam_section, SectionShape::HSS); - ds.beam_section = ds.beam_section.next(); - assert_eq!(ds.beam_section, SectionShape::Rect); - } } // =========================================================================== @@ -2513,8 +2322,8 @@ mod selection_action_tests { #[cfg(test)] mod export_tests { use super::*; - use crate::construction_frame::pages::workspace::cad::arch_stl::{StlExporter, StlExportOptions}; - use crate::construction_frame::pages::workspace::cad::arch_svg::{SvgExporter, SvgExportOptions}; + use nigig_build::construction_frame::pages::workspace::cad::arch_stl::{StlExporter, StlExportOptions}; + use nigig_build::construction_frame::pages::workspace::cad::arch_svg::{SvgExporter, SvgExportOptions}; fn make_box_node(id: u64, pos: Vec3f) -> CadNode { CadNode { @@ -2721,21 +2530,6 @@ mod export_tests { assert!(header.starts_with("custom header test")); } - // ── PdfPreview enum variant ── - - #[test] - fn pdf_preview_active_pane_variant() { - use super::super::CadEditorActivePane; - let pane = CadEditorActivePane::PdfPreview; - assert_eq!(pane.title(), "PDF Preview"); - } - - #[test] - fn pdf_preview_active_pane_not_default() { - use super::super::CadEditorActivePane; - let default = CadEditorActivePane::default(); - assert_ne!(default, CadEditorActivePane::PdfPreview); - } } @@ -2745,7 +2539,7 @@ mod export_tests { #[cfg(test)] mod hover_throttle_tests { - use crate::construction_frame::pages::workspace::cad::constants::HOVER_PICK_MIN_MOVE_PX; + use nigig_build::construction_frame::pages::workspace::cad::constants::HOVER_PICK_MIN_MOVE_PX; use makepad_widgets::DVec2; /// Mirrors the predicate in `CadViewport::handle_event`. @@ -2783,7 +2577,7 @@ mod hover_throttle_tests { /// would visibly lag the cursor. #[test] fn threshold_is_smaller_than_pick_radius() { - use crate::construction_frame::pages::workspace::cad::constants::PART_PICK_RADIUS; + use nigig_build::construction_frame::pages::workspace::cad::constants::PART_PICK_RADIUS; assert!( HOVER_PICK_MIN_MOVE_PX < PART_PICK_RADIUS / 4.0, "hover threshold {HOVER_PICK_MIN_MOVE_PX} is too coarse for pick radius {PART_PICK_RADIUS}" @@ -2793,7 +2587,7 @@ mod hover_throttle_tests { #[cfg(test)] mod pick_bounds_tests { - use crate::construction_frame::pages::workspace::cad::cad_scene::{ + use nigig_build::construction_frame::pages::workspace::cad::cad_scene::{ CadNode, CadSolid, CadTransform, LayerId, MaterialId, MeshCache, NodeId, NodeMetadata, }; use makepad_widgets::{vec3, Vec4f}; diff --git a/crates/apps/spreadsheet/spreadsheet-ui/src/workspace.rs b/crates/apps/spreadsheet/spreadsheet-ui/src/workspace.rs index fc35b50..ba4b340 100644 --- a/crates/apps/spreadsheet/spreadsheet-ui/src/workspace.rs +++ b/crates/apps/spreadsheet/spreadsheet-ui/src/workspace.rs @@ -181,7 +181,13 @@ impl SpreadsheetWorkspace { } fn exchange_grid_with_model(&mut self, cx: &mut Cx, index: usize) -> bool { - let Some(mut grid) = self.view.widget(cx, ids!(grid)).borrow_mut::() else { + // The WidgetRef must be bound to a local. In a `let ... else` + // the scrutinee's temporaries are dropped at the end of the + // statement (unlike `if let`, where they live to the end of the + // block), so borrowing directly from the returned temporary + // leaves `grid` dangling. + let grid_ref = self.view.widget(cx, ids!(grid)); + let Some(mut grid) = grid_ref.borrow_mut::() else { return false; }; let mut model = WorkspaceModel::from_parts(