diff --git a/.forgejo/workflows/nigig-build.yml b/.forgejo/workflows/nigig-build.yml index baae83a..a4c2cec 100644 --- a/.forgejo/workflows/nigig-build.yml +++ b/.forgejo/workflows/nigig-build.yml @@ -108,32 +108,6 @@ 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 @@ -180,23 +154,9 @@ jobs: - name: Check run: cargo check --locked -p nigig-build --lib - - name: Test (lib) + - name: Test 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 5886a2a..24bf445 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,11 +59,12 @@ of its cache-coherence complexity. | File | LOC | Responsibility | |---|--:|---| -| `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`. | +| `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/`. | | `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`. | @@ -73,7 +74,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` | 754 | `PartsStore` (the parts list + its generation counter) and `SceneCache` — the cached `Arc` + shared `Arc`. | +| `scene_holder.rs` | 390 | `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`. | @@ -215,29 +216,10 @@ enforces that `Cargo.lock` is committed and current — see The crate compiles and its tests run: ```bash -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 +cargo test --locked -p nigig-build --lib +# 741 passed; 0 failed; 10 ignored ``` -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. +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`. 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 d1b39bc..630c19f 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. /// - /// 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. + /// 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). pub fn build_glb( &self, scene: &CadScene, @@ -337,9 +337,10 @@ impl GltfExporter { }); } - // 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. + // 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). // // Parallel wins when each mesh build is expensive (complex // CSG with differences/unions, high-segment cylinders, etc.). @@ -478,8 +479,9 @@ impl Exporter for GltfExporter { self.export_with_cache(scene, &cache, writer) } - /// Overrides the `Exporter` default so the cache is actually - /// consulted; the default provided method ignores it. + /// 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. 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 4929d6f..44a9981 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 //! ``` //! -//! ## Classification +//! ## v2 design (preserved from previous version) //! //! 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, so the classification is explicit rather -// than inferred. +// rather than from geometry — this preserves the v2 "explicit type" +// design while letting us use the unified CadScene. // // 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), fall back to -// geometry-based classification. +// If a node has no layer (or an unknown layer), we fall back to +// geometry-based classification, which is what the v1 code did. struct PdfArchProjector<'a> { scene: &'a CadScene, @@ -655,8 +655,10 @@ impl Exporter for PdfExporter { self.export_with_cache(scene, &cache, writer) } - /// The PDF exporter accepts the mesh cache for API symmetry with - /// `GltfExporter` but does not use it + /// 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 /// (PDF works in 2D plan view, no triangle meshes needed). fn export_with_cache( &self, @@ -776,7 +778,7 @@ fn compute_viewport(elements: &[ArchElement], o: &PdfExportOptions) -> Option PdfColor { @@ -915,7 +917,7 @@ fn draw_text( } // =========================================================================== -// Grid + element renderers +// Grid + element renderers — unchanged drawing logic from v2 // =========================================================================== fn draw_grid(layer: &PdfLayerReference, vp: &Viewport, o: &PdfExportOptions) { @@ -1079,7 +1081,7 @@ fn draw_element(layer: &PdfLayerReference, e: &ArchElement, vp: &Viewport, font: } // =========================================================================== -// Decorations: title block, scale bar, north arrow +// Decorations: title block, scale bar, north arrow — unchanged from v2 // =========================================================================== 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 f28d3a4..6d9cebd 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,24 +1,47 @@ -//! # cad_scene — the immutable CAD scene graph and the export foundation. +//! # cad_scene — immutable CAD scene graph + shared export foundation. //! -//! 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)`. +//! ## v2 — compile fixes after first integration attempt //! -//! 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`. +//! This version fixes the 24 compile errors from the first integration: //! -//! `SceneBuilder` is the only supported way to construct a scene; it -//! owns the `IdAllocator` so ids cannot collide. +//! 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. //! -//! 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. - +//! 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. use std::collections::HashMap; use std::sync::{Arc, RwLock}; @@ -1470,10 +1493,10 @@ impl<'a> NodeBuilder<'a> { // Mesh cache (#3) — with parameter-hash invalidation // =========================================================================== // -// 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. +// 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. /// Stable hash of a node's geometry + transform + material parameters. /// Computed via `DefaultHasher` (cheap, deterministic within a process). @@ -1649,12 +1672,13 @@ impl Default for MeshCache { } // =========================================================================== -// Exporter trait +// Exporter trait (#1) — consolidated in v2 // =========================================================================== // -// `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 +// 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 // to use the cache override the method directly. pub trait Exporter { @@ -1846,6 +1870,16 @@ 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 // =========================================================================== @@ -1872,47 +1906,6 @@ 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 5935330..2d870b7 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,21 +1,34 @@ -//! # commands — the undo/redo command stack. +//! # commands — Command pattern for undo/redo (#7) //! //! ## Design //! -//! 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). +//! The existing `CadCommand` enum in `mod.rs` (line ~3538) works for +//! the current command set, but it has two limitations: //! -//! 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. +//! 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. //! -//! `MAX_UNDO_LEVELS` bounds the stack; it is a `VecDeque` so the oldest -//! entry is dropped in O(1). +//! 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. //! //! ## 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 0b466ef..fdf2f8a 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,13 +1,44 @@ // File: src/construction_frame/pages/workspace/cad/mod.rs // -// 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. +// ## Rewrite status (v4 — cached CadScene + shared MeshCache) // -// 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. +// 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. pub use ::makepad_ai; pub use ::makepad_code_editor; @@ -41,7 +72,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. +// arch_gltf: GLB 2.0 export for interactive 3D viewing (added by v3 patch) pub mod arch_gltf; // arch_stl: Binary STL export for 3D printing and CAD interchange. pub mod arch_stl; @@ -52,10 +83,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: 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. +// scene_holder: cached Arc + shared Arc for CadViewport. +// New in v4 — lets the GLB exporter reuse the preview renderer's cached meshes. pub mod scene_holder; +// v6: wired-in extracted modules: pub mod constants; pub mod persistence; pub mod math; @@ -70,6 +101,8 @@ 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; @@ -90,7 +123,7 @@ pub use cad_scene::{ SheetId, walk_scene, }; pub use scene_holder::{PartsStore, SceneCache}; -// Re-exports so call sites need not name the sub-module. +// v6: re-export wired-in module types so existing call sites keep working. 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}; @@ -101,8 +134,8 @@ pub(crate) use commands::{ MoveNode, DeleteNode, CreateNode, YawNode, ResizeNode, RotateNode, ModifyNode, CadCommandCtx, }; -// Types defined in viewport.rs but named as field types in the -// CadViewport/CadWorkspace struct definitions below. +// v16: re-export types moved to viewport.rs that are still used +// as field types in CadViewport/CadWorkspace struct definitions. pub(crate) use viewport::{ CadStats, CadMeshData, CadRenderMode, CadViewportViewSnapshot, CadRebuildWorker, CadRebuildRequest, CadRebuildPayload, CadRebuildResult, @@ -1575,7 +1608,7 @@ script_mod! { // [moved to viewport.rs: struct CadViewportViewSnapshot] -/// Holder for simultaneous mutable borrows of `parts`, +/// v11: 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 @@ -1643,9 +1676,10 @@ pub struct CadViewport { part_geoms: HashMap, /// Cached `Arc` + shared `Arc`. /// - /// 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. + /// 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. /// /// After *any* mutation to `self.parts`, call /// `self.scene_cache.mark_dirty()`. The `scene()` safety net @@ -1693,8 +1727,8 @@ pub struct CadViewport { script_dirty: bool, #[rust(false)] view_dirty: bool, - /// Trait-based command stack (`Command` + `UndoRedoStack`) with - /// automatic cache invalidation. + /// v19: trait-based command stack with automatic cache + /// invalidation. Uses the new `Command` trait + `UndoRedoStack`. #[rust] command_stack: UndoRedoStack, /// Screen position of the last hover pick. @@ -1754,7 +1788,7 @@ pub struct CadViewport { #[rust] last_middle_click_abs: DVec2, - // ---- CAD tool system ---- + // ---- v3 CAD tool system ---- #[rust] tool: CadTool, #[rust] @@ -1766,7 +1800,7 @@ pub struct CadViewport { frame_timer_avg_ms: f64, #[rust(0u32)] frame_timer_count: u32, - // ---- multi-select, clipboard, drag-select ---- + // ---- v4: multi-select, clipboard, drag-select ---- #[rust(false)] shift_pressed: bool, #[rust] @@ -2022,66 +2056,3 @@ 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 40aca4f..2f6947c 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 { } } - /// Compare parallel vs sequential GLB export. + /// v17: 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/tests/cad_integration.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/tests.rs similarity index 88% rename from crates/apps/nigig-build/tests/cad_integration.rs rename to crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/tests.rs index 2050fa0..15bd63c 100644 --- a/crates/apps/nigig-build/tests/cad_integration.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/tests.rs @@ -1,14 +1,13 @@ -//! # cad_integration — integration tests for the CAD module. +//! # tests — comprehensive integration tests for the CAD rewrite //! -//! 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. +//! 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. //! -//! Run with: `cargo test --package nigig-build --test cad_integration` +//! Run with: `cargo test --package nigig-build cad::tests` -use nigig_build::construction_frame::pages::workspace::cad::cad_scene::{ +use crate::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, @@ -100,7 +99,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 = nigig_build::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&original); + let parts = crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&original); assert_eq!(parts.len(), 11); // Convert back via the legacy adapter. @@ -147,7 +146,7 @@ mod scene_conversion { PartKind::Arc, ] { // The From impls in mod.rs handle this. - let cad_kind: nigig_build::construction_frame::pages::workspace::cad::cad_scene::PartKind = kind.into(); + let cad_kind: crate::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); } @@ -157,7 +156,7 @@ mod scene_conversion { #[test] fn empty_scene_converts_cleanly() { let empty = CadScene::default(); - let parts = nigig_build::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&empty); + let parts = crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&empty); assert!(parts.is_empty()); } } @@ -191,7 +190,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: &nigig_build::makepad_csg::Solid) { self.csgs += 1; } + fn visit_csg(&mut self, _node: &CadNode, _solid: &crate::makepad_csg::Solid) { self.csgs += 1; } fn visit_group(&mut self, _node: &CadNode) { self.groups += 1; } } @@ -240,7 +239,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)) @@ -249,7 +248,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)) @@ -355,7 +354,7 @@ mod mesh_cache { mod glb_validity { use super::*; - use nigig_build::construction_frame::pages::workspace::cad::arch_gltf::{GltfExporter, GltfExportOptions}; + use crate::construction_frame::pages::workspace::cad::arch_gltf::{GltfExporter, GltfExportOptions}; /// GLB 2.0 binary format: /// bytes 0..4 = "glTF" magic @@ -491,7 +490,7 @@ mod glb_validity { mod pdf_dimensions { use super::*; - use nigig_build::construction_frame::pages::workspace::cad::arch_pdf::{ + use crate::construction_frame::pages::workspace::cad::arch_pdf::{ Orientation, PaperSize, PdfExporter, PdfExportOptions, }; @@ -554,7 +553,7 @@ mod pdf_dimensions { assert_eq!(pdf.format_name(), "PDF 1.7"); assert_eq!(pdf.file_extension(), "pdf"); - let glb = nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter::default(); + let glb = crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter::default(); assert_eq!(glb.format_name(), "GLB 2.0"); assert_eq!(glb.file_extension(), "glb"); } @@ -566,8 +565,8 @@ mod pdf_dimensions { mod exporter_trait { use super::*; - use nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; - use nigig_build::construction_frame::pages::workspace::cad::arch_pdf::PdfExporter; + use crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; + use crate::construction_frame::pages::workspace::cad::arch_pdf::PdfExporter; #[test] fn both_exporters_can_be_used_through_trait_object() { @@ -596,7 +595,7 @@ mod exporter_trait { mod round_trip { use super::*; - use nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; + use crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; #[test] fn build_export_reparse_preserves_node_count() { @@ -648,16 +647,16 @@ mod round_trip { } // =========================================================================== -// SceneCache integration tests +// SceneCache integration tests (v4) // =========================================================================== mod scene_cache_integration { use super::*; - use nigig_build::construction_frame::pages::workspace::cad::scene_holder::SceneCache; + use crate::construction_frame::pages::workspace::cad::scene_holder::SceneCache; use std::sync::Arc; /// Build a Vec with N walls of different colors. - use nigig_build::construction_frame::pages::workspace::cad::scene_holder::PartsStore; + use crate::construction_frame::pages::workspace::cad::scene_holder::PartsStore; fn make_colored_store(n: usize) -> PartsStore { let mut store = PartsStore::new(); @@ -678,7 +677,7 @@ mod scene_cache_integration { .finish(); } let scene = builder.build(); - nigig_build::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene) + crate::construction_frame::pages::workspace::cad::cad_scene::nodes_from_scene(&scene) } #[test] @@ -803,7 +802,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 nigig_build::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; + use crate::construction_frame::pages::workspace::cad::arch_gltf::GltfExporter; let cache = SceneCache::new(); let parts = make_colored_parts(5); @@ -884,6 +883,164 @@ 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 @@ -895,10 +1052,10 @@ mod scene_cache_integration { mod editing_tools_tests { use super::*; - use nigig_build::construction_frame::pages::workspace::cad::commands::{ + use crate::construction_frame::pages::workspace::cad::commands::{ }; - use nigig_build::construction_frame::pages::workspace::cad::scene_holder::SceneCache; - use nigig_build::construction_frame::pages::workspace::cad::math::point_in_polygon; + use crate::construction_frame::pages::workspace::cad::scene_holder::SceneCache; + use crate::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 { @@ -1366,7 +1523,7 @@ mod editing_tools_tests { #[cfg(test)] mod dde_integration_tests { - use nigig_build::construction_frame::pages::workspace::cad::construction_geometry::{ + use crate::construction_frame::pages::workspace::cad::construction_geometry::{ CoordInput, parse_coord_input, }; use makepad_widgets::DVec2; @@ -1584,7 +1741,7 @@ mod dde_integration_tests { #[cfg(test)] mod window_crossing_tests { - use nigig_build::construction_frame::pages::workspace::cad::SelectionMode; + use crate::construction_frame::pages::workspace::cad::SelectionMode; use makepad_widgets::DVec2; /// A minimal 2D part representation for testing marquee selection logic. @@ -1896,7 +2053,7 @@ mod window_crossing_tests { #[cfg(test)] mod polar_tracking_tests { - use nigig_build::construction_frame::pages::workspace::cad::construction_geometry::{ + use crate::construction_frame::pages::workspace::cad::construction_geometry::{ snap_to_polar_angle, next_polar_increment, }; use std::f64::consts::PI; @@ -1981,6 +2138,21 @@ 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); + } } // =========================================================================== @@ -1989,8 +2161,8 @@ mod polar_tracking_tests { #[cfg(test)] mod section_shape_tests { - use nigig_build::construction_frame::pages::workspace::cad::cad_scene::{CadNode, CadSolid, PartKind}; - use nigig_build::construction_frame::pages::workspace::cad::section_shape::{ + use crate::construction_frame::pages::workspace::cad::cad_scene::{CadNode, CadSolid, PartKind}; + use crate::construction_frame::pages::workspace::cad::section_shape::{ SectionShape, IBeamParams, HSSParams, section_vertices, section_bounding_box, section_area, rect_vertices, ibeam_vertices, hss_vertices, @@ -2163,6 +2335,25 @@ 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); + } } // =========================================================================== @@ -2322,8 +2513,8 @@ mod selection_action_tests { #[cfg(test)] mod export_tests { use super::*; - use nigig_build::construction_frame::pages::workspace::cad::arch_stl::{StlExporter, StlExportOptions}; - use nigig_build::construction_frame::pages::workspace::cad::arch_svg::{SvgExporter, SvgExportOptions}; + use crate::construction_frame::pages::workspace::cad::arch_stl::{StlExporter, StlExportOptions}; + use crate::construction_frame::pages::workspace::cad::arch_svg::{SvgExporter, SvgExportOptions}; fn make_box_node(id: u64, pos: Vec3f) -> CadNode { CadNode { @@ -2530,6 +2721,21 @@ 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); + } } @@ -2539,7 +2745,7 @@ mod export_tests { #[cfg(test)] mod hover_throttle_tests { - use nigig_build::construction_frame::pages::workspace::cad::constants::HOVER_PICK_MIN_MOVE_PX; + use crate::construction_frame::pages::workspace::cad::constants::HOVER_PICK_MIN_MOVE_PX; use makepad_widgets::DVec2; /// Mirrors the predicate in `CadViewport::handle_event`. @@ -2577,7 +2783,7 @@ mod hover_throttle_tests { /// would visibly lag the cursor. #[test] fn threshold_is_smaller_than_pick_radius() { - use nigig_build::construction_frame::pages::workspace::cad::constants::PART_PICK_RADIUS; + use crate::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}" @@ -2587,7 +2793,7 @@ mod hover_throttle_tests { #[cfg(test)] mod pick_bounds_tests { - use nigig_build::construction_frame::pages::workspace::cad::cad_scene::{ + use crate::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/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs index 250dff5..08a62e5 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,17 +87,23 @@ 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 { - let next = match self.view_mode { + self.view_mode = match self.view_mode { ViewMode::ThreeD => ViewMode::TwoD, ViewMode::TwoD => ViewMode::ThreeD, }; - self.set_view_mode(cx, next); + 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.view_mode } @@ -150,8 +156,9 @@ impl CadViewport { self.selection = selection; self.next_part_id = next_part_id; self.part_geoms.clear(); - // Wholesale parts replacement: the scene snapshot is stale and - // node ids may have shifted, so clear the mesh cache too. + // v4: invalidate caches. Wholesale parts replacement = 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; @@ -212,10 +219,10 @@ impl CadViewport { self.area.redraw(cx); } - // ----- command-based undo/redo ----- + // ----- v19: command-based undo/redo system ----- // - // `Command` + `UndoRedoStack`, with `CadCommandCtx` providing the - // `CommandContext` implementation. + // Uses the new `Command` trait + `UndoRedoStack` with + // `CadCommandCtx` providing the `CommandContext` implementation. // Cache invalidation happens automatically inside each command's // `execute()` / `undo()` via the context. @@ -328,10 +335,10 @@ impl CadViewport { self.command_stack.clear(); } - // ----- command constructors ----- + // ----- v19: convenience methods using new Command types ----- // - // These build a command and push it onto the UndoRedoStack via - // execute_command(). Cache invalidation is + // These construct the new command structs and push them onto the + // UndoRedoStack via execute_command(). Cache invalidation is // automatic inside each command. /// Move a part and push a `MoveNode` onto the command stack. @@ -382,8 +389,8 @@ impl CadViewport { /// Record an add-part operation on the command stack. /// - /// Records a `CreateNode` on the command stack. The caller must - /// push the part onto `self.parts` BEFORE calling this. + /// v19: now uses `CreateNode` on the new command stack. The caller + /// must push the part onto `self.parts` BEFORE calling this method. 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) { @@ -427,7 +434,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)), - // Architectural defaults — realistic dimensions in metres. + // v2: architectural defaults — realistic dimensions in meters. 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)), @@ -1216,8 +1223,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; - // Structural edit (added N parts, removed 1): invalidate both - // caches. + // v4: structural edit (added N parts, removed 1). Invalidate + // both caches. self.mark_scene_dirty(); self.clear_mesh_cache(); self.script_dirty = true; @@ -1263,8 +1270,8 @@ impl CadViewport { } p }; - // modify_part_command gives automatic cache invalidation and - // undo recording on the command stack. + // v12: use modify_part_command for automatic cache invalidation + // + 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; @@ -1329,9 +1336,10 @@ impl CadViewport { p.parent = Some(NodeId(gid)); } } - // Parameter edit: group_id feeds NodeMetadata.parent, so the - // cached snapshot must be rebuilt. No mesh invalidation — the - // group id does not affect geometry. + // 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. self.mark_scene_dirty(); } @@ -1509,9 +1517,10 @@ impl CadViewport { /// Get the current scene as a cached `Arc`. /// - /// 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. + /// 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). /// /// The returned `Arc` can be held independently of the viewport's /// lifetime — the scene stays alive even if the viewport mutates @@ -1525,8 +1534,9 @@ impl CadViewport { /// can be held independently of the viewport's lifetime — /// the cache stays alive even if the viewport is dropped. /// - /// Returns an `Arc` so the GLB exporter can reuse the preview - /// renderer's cached meshes; both hold the same underlying cache. + /// 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. pub fn mesh_cache(&self) -> std::sync::Arc { self.scene_cache.mesh_cache() } @@ -1535,8 +1545,9 @@ impl CadViewport { /// the node's parameters change (e.g. wall length edited in the /// properties panel). /// - /// Rarely necessary: parameter edits are detected automatically - /// via ParamHash. Use this only to force a rebuild. + /// 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. pub fn invalidate_node(&self, id: u64) { self.scene_cache.invalidate_node(id); } @@ -1551,10 +1562,9 @@ impl CadViewport { /// *any* mutation to `self.parts`. The next `scene()` call will /// rebuild. /// - /// 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()`. + /// v4: new method. The `scene()` safety net catches length + /// mismatches, but parameter edits (same length, different + /// content) need explicit `mark_dirty()`. pub fn mark_scene_dirty(&self) { self.scene_cache.mark_dirty(); } @@ -1642,7 +1652,7 @@ impl CadViewport { format!("cube(0.01,0.01,0.01,true)") } } - // Arch kinds emit cube() or cylinder() in the CAD script. + // v2: arch kinds emit cube() or cylinder() in the CAD script. PartKind::Wall | PartKind::Slab | PartKind::Door @@ -2586,7 +2596,7 @@ impl CadViewport { self.draw_vector.line_to(o.x as f32, o.y as f32 + 6.0); self.draw_vector.stroke(1.0); - // Rubber band preview. + // v3: rubber band preview self.draw_rubber_band(cx); self.draw_polar_guidelines(cx); self.draw_section_indicators(cx); @@ -2792,7 +2802,7 @@ impl CadViewport { } // ======================================================================= - // CAD tool system — snapping, drawing, rubber band, coordinate readout + // v3: CAD Tool System — snapping, drawing, rubber band, coordinate readout // ======================================================================= pub(crate) fn set_tool(&mut self, cx: &mut Cx, tool: CadTool) { @@ -3353,15 +3363,8 @@ 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) { - // `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); + p.size().x = new_w; + p.size().z = new_h; } } } @@ -3479,8 +3482,9 @@ impl CadViewport { } } self.selection = vec![id]; - // Structural edit: a tool finished and added one or more parts - // (wall/circle/rect/area/column/beam). Invalidate caches. + // v4: structural edit (tool finished, one or more parts added). + // Invalidate caches. Catches wall/circle/rect/area/column/beam + // tool completions. self.mark_scene_dirty(); self.clear_mesh_cache(); self.script_dirty = true; @@ -3516,7 +3520,7 @@ impl CadViewport { kind_hint: Some(PartKind::Polygon2D), }); self.add_part_command(id); - // Structural edit (new polygon part): invalidate caches. + // v4: structural edit (new polygon part). Invalidate caches. self.mark_scene_dirty(); self.clear_mesh_cache(); } @@ -3636,7 +3640,7 @@ impl CadViewport { prev = next; } self.selection = ids; - // Structural edit (added N walls from path): invalidate caches. + // v4: structural edit (added N walls from path). Invalidate caches. self.mark_scene_dirty(); self.clear_mesh_cache(); self.script_dirty = true; @@ -6751,7 +6755,7 @@ impl Widget for CadViewport { // =========================================================================== -// Helpers that need no #[derive(Script)], so they live outside mod.rs +// v16: Helpers moved from mod.rs (no #[derive(Script)] needed) // =========================================================================== impl DrawCadMesh { @@ -6802,55 +6806,7 @@ pub(crate) struct CadMeshData { pub(crate) stats: CadStats, } -/// 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) { +pub(crate) fn cad_mesh_data_from_solid(solid: &Solid) -> CadMeshData { let mesh = solid.mesh(); let mut stats = CadStats { vertices: solid.vertex_count(), @@ -6859,27 +6815,24 @@ fn build_mesh_buffers( }; if mesh.vertices.is_empty() || mesh.triangles.is_empty() { - return (Vec::new(), Vec::new(), stats); + return CadMeshData { + stats, + ..Default::default() + }; } - // 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; + 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); #[cfg(target_os = "android")] let y_flip = -1.0; @@ -6890,26 +6843,32 @@ fn build_mesh_buffers( 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 { + 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 { 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, ey, ez) = (bx - ax, by - ay, bz - az); - let (fx, fy, fz) = (cx_ - ax, cy - ay, cz - az); + 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 mut nx = ey * fz - ez * fy; let mut ny = ez * fx - ex * fz; let mut nz = ex * fy - ey * fx; @@ -6924,64 +6883,73 @@ fn build_mesh_buffers( 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 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 dot = nx * centroid_x + ny * centroid_y + nz * centroid_z; + let offset = 0.0001; - 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)) + 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 { - (place(ax, ay, az), place(bx, by, bz), place(cx_, cy, cz)) + ( + [ + (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 normal = [fnx as f32, (fny * y_flip) as f32, fnz as f32]; - let ordered = match winding { - Winding::AsComputed => [p0, p1, p2], - Winding::Reversed => [p0, p2, p1], - }; - for p in ordered { + for p in [p0, p1, p2] { 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 } - } -} - -/// 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)) + CadMeshData { + indices, + vertices, + stats, + } } } @@ -7123,6 +7091,76 @@ 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(|| { @@ -7198,331 +7236,3 @@ 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 1657f01..5f23923 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,86 +63,6 @@ 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 { @@ -339,38 +259,6 @@ 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, @@ -870,7 +758,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) { - // Cached Arc + shared Arc + // v4: get the 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) = { @@ -922,7 +810,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) { - // Cached Arc + shared Arc. + // v4: get the 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. @@ -961,7 +849,7 @@ impl CadWorkspace { Ok(()) => { let glb_path = dir.join("model.glb"); let html_path = dir.join("viewer.html"); - // Use the cached scene + shared mesh cache from the + // v4: 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); @@ -1436,11 +1324,51 @@ impl CadWorkspace { } fn extract_cad_script(text: &str) -> String { - extract_script(text, ResponseState::Complete) + 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() } fn extract_streaming_cad_script(text: &str) -> String { - extract_script(text, ResponseState::Streaming) + 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() + } } fn stream_ai_response_to_editor(&mut self, cx: &mut Cx) { @@ -2351,7 +2279,7 @@ impl CadWorkspace { } } - // Tool selection buttons. + // v3 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) { @@ -2482,7 +2410,7 @@ impl CadWorkspace { vp.redo(cx); }); } - // Update undo/redo button labels with command descriptions. + // v14: 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. { @@ -2983,10 +2911,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.pos(); - t.x = v; - p.set_pos(t); + 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; }); } let val = self @@ -3001,10 +2936,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.pos(); - t.y = v; - p.set_pos(t); + 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; }); } let val = self @@ -3019,10 +2961,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.pos(); - t.z = v; - p.set_pos(t); + 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; }); } @@ -3039,10 +2988,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.size(); - t.x = v; - p.set_size(t); + 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; }); } let val = self @@ -3057,10 +3013,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.size(); - t.y = v; - p.set_size(t); + 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; }); } let val = self @@ -3075,10 +3038,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.size(); - t.z = v; - p.set_size(t); + 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; }); } @@ -3095,10 +3065,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.rot(); - t.x = v; - p.set_rot(t); + 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; }); } let val = self @@ -3113,10 +3090,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.rot(); - t.y = v; - p.set_rot(t); + 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; }); } let val = self @@ -3131,10 +3115,17 @@ impl CadWorkspace { .and_then(|(t, _)| t.parse::().ok()) }); if let Some(val) = val { - self.apply_field_to_selected_part(cx, val, |p, v| { - let mut t = p.rot(); - t.z = v; - p.set_rot(t); + 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; }); } @@ -3508,190 +3499,6 @@ 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/spreadsheet/spreadsheet-ui/src/workspace.rs b/crates/apps/spreadsheet/spreadsheet-ui/src/workspace.rs index ba4b340..fc35b50 100644 --- a/crates/apps/spreadsheet/spreadsheet-ui/src/workspace.rs +++ b/crates/apps/spreadsheet/spreadsheet-ui/src/workspace.rs @@ -181,13 +181,7 @@ impl SpreadsheetWorkspace { } fn exchange_grid_with_model(&mut self, cx: &mut Cx, index: usize) -> bool { - // 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 { + let Some(mut grid) = self.view.widget(cx, ids!(grid)).borrow_mut::() else { return false; }; let mut model = WorkspaceModel::from_parts(