From 929f185ba57c30c95a0d329169801a3165666b4e Mon Sep 17 00:00:00 2001 From: yztink Date: Sat, 19 Sep 2026 16:40:41 -0700 Subject: [PATCH 01/18] feat(plugin): add layer creation and modification host API - Add LayerConfig struct and add_layer / modify_layer methods to HostApi trait - Implement IPC request/response serialization for AddLayer and ModifyLayer - Implement HostSession methods with CAD property defaults and selective modification - Refresh layer panel UI and invalidate scene layer dependencies upon layer changes - Add unit tests for add_layer with defaults and partial modify_layer --- crates/ocs_plugin_api/src/host.rs | 31 ++++ crates/ocs_plugin_api/src/ipc/client.rs | 28 +++ crates/ocs_plugin_api/src/ipc/protocol.rs | 6 + crates/ocs_plugin_api/src/ipc/server.rs | 2 + crates/ocs_plugin_api/src/ipc/v4/client.rs | 28 +++ src/app/plugin_host.rs | 203 +++++++++++++++++++++ 6 files changed, 298 insertions(+) diff --git a/crates/ocs_plugin_api/src/host.rs b/crates/ocs_plugin_api/src/host.rs index 19130c596..c64ff4e91 100644 --- a/crates/ocs_plugin_api/src/host.rs +++ b/crates/ocs_plugin_api/src/host.rs @@ -552,6 +552,37 @@ pub trait HostApi { let _ = tab_id; None } + + /// Add a layer to the active document with full initial properties. + /// If an optional property in `config` is `None`, standard CAD defaults are applied. + /// Returns `None` if the layer already exists or `config.name` is invalid. + fn add_layer(&mut self, config: LayerConfig) -> Option { + let _ = config; + None + } + + /// Modify specified properties of an existing layer in the active document. + /// Properties that are `None` in `config` are left untouched as-is. + /// Returns `false` if the layer does not exist or `config.name` is invalid. + fn modify_layer(&mut self, config: LayerConfig) -> bool { + let _ = config; + false + } +} + +/// Configuration properties for creating or modifying a layer. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Default)] +pub struct LayerConfig { + pub name: String, + pub color: Option, + pub linetype: Option, + pub lineweight: Option, + pub off: Option, + pub frozen: Option, + pub locked: Option, + pub plottable: Option, + pub transparency: Option, + pub description: Option, } /// Simplified, read-only entity kind exposed by [`DocumentReader`]. diff --git a/crates/ocs_plugin_api/src/ipc/client.rs b/crates/ocs_plugin_api/src/ipc/client.rs index 3a617fc02..0a7baa779 100644 --- a/crates/ocs_plugin_api/src/ipc/client.rs +++ b/crates/ocs_plugin_api/src/ipc/client.rs @@ -416,6 +416,34 @@ impl HostApi for PluginHostApi { } } } + + fn add_layer(&mut self, config: crate::host::LayerConfig) -> Option { + match self.client.request(PluginRequest::AddLayer(config)) { + Ok(PluginResponse::OptHandle(h)) => h, + Ok(other) => { + eprintln!("[plugin] unexpected AddLayer response: {other:?}"); + None + } + Err(e) => { + eprintln!("[plugin] AddLayer request failed: {e}"); + None + } + } + } + + fn modify_layer(&mut self, config: crate::host::LayerConfig) -> bool { + match self.client.request(PluginRequest::ModifyLayer(config)) { + Ok(PluginResponse::Bool(b)) => b, + Ok(other) => { + eprintln!("[plugin] unexpected ModifyLayer response: {other:?}"); + false + } + Err(e) => { + eprintln!("[plugin] ModifyLayer request failed: {e}"); + false + } + } + } } /// Sentinel reader used when the shared-memory view could not be initialized. diff --git a/crates/ocs_plugin_api/src/ipc/protocol.rs b/crates/ocs_plugin_api/src/ipc/protocol.rs index c20cad1b2..81a1ea3cf 100644 --- a/crates/ocs_plugin_api/src/ipc/protocol.rs +++ b/crates/ocs_plugin_api/src/ipc/protocol.rs @@ -138,6 +138,10 @@ pub enum PluginRequest { GetTabId, /// V5: ask the host for the filesystem path of the document in `tab_id`. DocumentPath { tab_id: u64 }, + /// Add a layer to the active document with full initial properties. + AddLayer(crate::host::LayerConfig), + /// Modify specified properties of an existing layer in the active document. + ModifyLayer(crate::host::LayerConfig), } /// Responses the host sends back for `PluginRequest`. @@ -164,6 +168,8 @@ pub enum PluginResponse { TabId(u64), /// V5: filesystem path of the document in the requested tab, if any. DocumentPath(Option), + /// Optional entity handle (e.g. from AddLayer). + OptHandle(Option), } /// Messages sent from the host to the plugin runner. diff --git a/crates/ocs_plugin_api/src/ipc/server.rs b/crates/ocs_plugin_api/src/ipc/server.rs index 055880ef0..05d03998f 100644 --- a/crates/ocs_plugin_api/src/ipc/server.rs +++ b/crates/ocs_plugin_api/src/ipc/server.rs @@ -77,5 +77,7 @@ pub fn handle_plugin_request( DocumentPath { tab_id } => PluginResponse::DocumentPath( host.document_path(tab_id).map(|path| path.into_os_string()), ), + AddLayer(config) => PluginResponse::OptHandle(host.add_layer(config)), + ModifyLayer(config) => PluginResponse::Bool(host.modify_layer(config)), } } diff --git a/crates/ocs_plugin_api/src/ipc/v4/client.rs b/crates/ocs_plugin_api/src/ipc/v4/client.rs index 03cbf20ae..1e5149c53 100644 --- a/crates/ocs_plugin_api/src/ipc/v4/client.rs +++ b/crates/ocs_plugin_api/src/ipc/v4/client.rs @@ -730,6 +730,34 @@ impl HostApi for V4PluginHostApi { } } } + + fn add_layer(&mut self, config: crate::host::LayerConfig) -> Option { + match self.request(PluginRequest::AddLayer(config)) { + Ok(PluginResponse::OptHandle(h)) => h, + Ok(other) => { + eprintln!("[plugin] unexpected AddLayer response: {other:?}"); + None + } + Err(e) => { + eprintln!("[plugin] AddLayer request failed: {e}"); + None + } + } + } + + fn modify_layer(&mut self, config: crate::host::LayerConfig) -> bool { + match self.request(PluginRequest::ModifyLayer(config)) { + Ok(PluginResponse::Bool(b)) => b, + Ok(other) => { + eprintln!("[plugin] unexpected ModifyLayer response: {other:?}"); + false + } + Err(e) => { + eprintln!("[plugin] ModifyLayer request failed: {e}"); + false + } + } + } } #[cfg(test)] diff --git a/src/app/plugin_host.rs b/src/app/plugin_host.rs index d60992822..5ac7204c2 100644 --- a/src/app/plugin_host.rs +++ b/src/app/plugin_host.rs @@ -257,6 +257,144 @@ impl<'a> HostSession<'a> { pub fn push_error(&mut self, msg: &str) { self.app.command_line.push_error(msg); } + + pub fn add_layer(&mut self, config: ocs_plugin_api::host::LayerConfig) -> Option { + let trimmed = config.name.trim(); + if trimmed.is_empty() { + return None; + } + + let doc = self.document_mut(); + if doc.layers.contains(trimmed) { + return None; + } + + let resolved_lt = if let Some(ref lt_name) = config.linetype { + let matched = doc + .line_types + .iter() + .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) + .map(|lt| lt.name.clone()); + if let Some(m) = matched { + m + } else { + crate::io::linetypes::populate_document(doc); + doc.line_types + .iter() + .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) + .map(|lt| lt.name.clone()) + .unwrap_or_else(|| lt_name.clone()) + } + } else { + "Continuous".to_string() + }; + + let mut layer = acadrust::tables::Layer::new(trimmed); + let handle = doc.allocate_handle(); + layer.handle = handle; + layer.color = config.color.unwrap_or(acadrust::types::Color::Index(7)); + layer.line_type = resolved_lt; + layer.line_weight = config.lineweight.unwrap_or(acadrust::types::LineWeight::ByLayer); + layer.flags.off = config.off.unwrap_or(false); + if let Some(frz) = config.frozen { + if frz { + layer.freeze(); + } else { + layer.thaw(); + } + } + layer.flags.locked = config.locked.unwrap_or(false); + layer.is_plottable = config.plottable.unwrap_or(true); + layer.transparency = config.transparency.unwrap_or(acadrust::types::Transparency::ByLayer); + layer.description = config.description.unwrap_or_default(); + + let _ = doc.layers.add(layer); + + self.app.tabs[self.tab].dirty = true; + self.app.tabs[self.tab] + .scene + .invalidate_layer_dependencies(&[trimmed.to_string()]); + self.app.refresh_layer_panel(); + self.publish_document_view(); + Some(handle) + } + + pub fn modify_layer(&mut self, config: ocs_plugin_api::host::LayerConfig) -> bool { + let trimmed = config.name.trim(); + if trimmed.is_empty() { + return false; + } + + let resolved_lt = if let Some(ref lt_name) = config.linetype { + let doc = self.document_mut(); + let matched = doc + .line_types + .iter() + .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) + .map(|lt| lt.name.clone()); + if let Some(m) = matched { + Some(m) + } else { + crate::io::linetypes::populate_document(doc); + Some( + doc.line_types + .iter() + .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) + .map(|lt| lt.name.clone()) + .unwrap_or_else(|| lt_name.clone()), + ) + } + } else { + None + }; + + let doc = self.document_mut(); + let Some(existing) = doc.layers.get_mut(trimmed) else { + return false; + }; + + if let Some(c) = config.color { + existing.color = c; + existing.color_name = None; + existing.book_name = None; + } + if let Some(lt) = resolved_lt { + existing.line_type = lt; + } + if let Some(lw) = config.lineweight { + existing.line_weight = lw; + } + if let Some(off) = config.off { + existing.flags.off = off; + } + if let Some(frz) = config.frozen { + if frz { + existing.freeze(); + } else { + existing.thaw(); + } + } + if let Some(lck) = config.locked { + existing.flags.locked = lck; + } + if let Some(plt) = config.plottable { + existing.is_plottable = plt; + } + if let Some(tr) = config.transparency { + existing.transparency = tr; + } + if let Some(desc) = config.description { + existing.description = desc; + } + + self.app.tabs[self.tab].dirty = true; + self.app.tabs[self.tab] + .scene + .invalidate_layer_dependencies(&[trimmed.to_string()]); + self.app.refresh_layer_panel(); + self.publish_document_view(); + true + } } /// The stable contract a plugin's `dispatch` sees. Each method forwards to the @@ -360,6 +498,12 @@ impl HostApi for HostSession<'_> { fn close_document_view_v4(&mut self, tab_id: u64) { self.close_document_view_v4(tab_id) } + fn add_layer(&mut self, config: ocs_plugin_api::host::LayerConfig) -> Option { + self.add_layer(config) + } + fn modify_layer(&mut self, config: ocs_plugin_api::host::LayerConfig) -> bool { + self.modify_layer(config) + } } /// Bridges a plugin's [`InteractiveCommand`](ocs_plugin_api::host::InteractiveCommand) @@ -860,4 +1004,63 @@ mod tests { assert_eq!(got.values.len(), 1); assert!(matches!(got.values[0], XDataValue::Integer32(123))); } + + #[test] + fn test_plugin_add_layer_with_defaults_and_modify() { + use ocs_plugin_api::host::LayerConfig; + + let mut app = OpenCADStudio::new_for_test(); + app.tabs[0].is_start = false; + let mut host = HostSession::new(&mut app, 0); + + // 1. Add layer with minimal config (only name specified) + let config = LayerConfig { + name: "ELECTRICAL".to_string(), + ..Default::default() + }; + let handle = host.add_layer(config).expect("should create layer"); + assert_ne!(handle, acadrust::Handle::NULL); + + // Verify defaults were applied + let layer = host.document().layers.get("ELECTRICAL").expect("layer should exist"); + assert_eq!(layer.color, acadrust::types::Color::Index(7)); + assert_eq!(layer.line_type, "Continuous"); + assert_eq!(layer.line_weight, acadrust::types::LineWeight::ByLayer); + assert!(!layer.flags.off); + assert!(!layer.flags.frozen); + assert!(!layer.flags.locked); + assert!(layer.is_plottable); + + // 2. Duplicate add_layer should be rejected (return None) + let dup_config = LayerConfig { + name: "ELECTRICAL".to_string(), + color: Some(acadrust::types::Color::Index(1)), + ..Default::default() + }; + assert!(host.add_layer(dup_config).is_none(), "duplicate layer should return None"); + + // 3. Modify only color and locked; other properties should remain untouched + let mod_config = LayerConfig { + name: "ELECTRICAL".to_string(), + color: Some(acadrust::types::Color::Index(1)), + locked: Some(true), + ..Default::default() + }; + assert!(host.modify_layer(mod_config)); + + let updated = host.document().layers.get("ELECTRICAL").expect("layer should exist"); + assert_eq!(updated.color, acadrust::types::Color::Index(1)); // Modified to red + assert!(updated.flags.locked); // Modified to locked + assert_eq!(updated.line_type, "Continuous"); // Kept as-is + assert_eq!(updated.line_weight, acadrust::types::LineWeight::ByLayer); // Kept as-is + assert!(!updated.flags.off); // Kept as-is + + // 4. Modify nonexistent layer returns false + let non_existent = LayerConfig { + name: "DOES_NOT_EXIST".to_string(), + color: Some(acadrust::types::Color::Index(2)), + ..Default::default() + }; + assert!(!host.modify_layer(non_existent)); + } } From 2e871c2c6cd7cbd7a828183b0ab3b5fc27936fa1 Mon Sep 17 00:00:00 2001 From: Helen Kwok <107752876+helenkwok@users.noreply.github.com> Date: Sun, 20 Sep 2026 12:50:39 +0930 Subject: [PATCH 02/18] docs(automation): document driving the web build Describe the two JavaScript functions the web build exposes (ocs_control_submit / ocs_control_take), how to poll them, the queue and reply limits, opening a drawing from bytes with data_base64, and closing the startup dialog before new. Everything is taken from src/app/control/mod.rs, src/app/startup.rs and docs/automation/web_save_smoke.cjs. --- docs/automation/README.md | 2 ++ docs/automation/web.md | 68 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 docs/automation/web.md diff --git a/docs/automation/README.md b/docs/automation/README.md index 1dcf4a90e..19e874121 100644 --- a/docs/automation/README.md +++ b/docs/automation/README.md @@ -156,3 +156,5 @@ The repeatable live-editor evaluation draws three isolated entities in one batch ```sh python3 docs/automation/mcp_eval.py target/debug/OpenCADStudio ``` + +The web build is driven through two JavaScript functions instead of a process; see [web.md](web.md). diff --git a/docs/automation/web.md b/docs/automation/web.md new file mode 100644 index 000000000..16a80c622 --- /dev/null +++ b/docs/automation/web.md @@ -0,0 +1,68 @@ +# Driving the web build + +The web build exposes the same control core as `--serve` and `--mcp`, through two +functions on the WebAssembly module. This page covers only what is specific to the +browser; the operations, request envelope and replies are the ones in the +[README](README.md). + +## Reaching the control channel + +The Trunk loader publishes the wasm-bindgen module as `window.wasmBindings`. The +control channel is ready once `window.wasmBindings?.ocs_control_submit` is defined +(the [save smoke test](web_save_smoke.cjs) waits for exactly that). + +| Function | Behaviour | +|---|---| +| `ocs_control_submit(request: string) → string` | Queues one request (a JSON string) and returns a ticket immediately. | +| `ocs_control_take(ticket: string) → string \| undefined` | Returns the reply (a JSON string) once it is ready, `undefined` before that. A reply is removed when taken. | + +Poll `ocs_control_take` until it returns a reply, as the smoke test does: + +```js +async function control(request) { + const api = window.wasmBindings; + const ticket = api.ocs_control_submit(JSON.stringify(request)); + for (let i = 0; i < 600; i++) { + const reply = api.ocs_control_take(ticket); + if (reply) return JSON.parse(reply); + await new Promise((resolve) => setTimeout(resolve, 50)); + } + throw new Error('control timed out'); +} +``` + +Limits: at most 64 requests wait in the queue (a further `submit` is answered with the +`busy` error), and only the newest 64 replies are kept, so take a reply soon after it is +ready. A request that is not valid JSON is answered with `op: "invalid"` and a +`parse_error`. + +## Opening a drawing from bytes + +The web build has no file system, so `open` takes the file's bytes instead of a path: + +```json +{"op":"open","name":"plan.dwg","data_base64":"…"} +``` + +`name` is reduced to its file name and labels the tab and the recent-files entry; +`data_base64` must be valid base64 and non-empty (`invalid_request` otherwise). While an +earlier drawing is still opening the request is answered with `busy`; retry it once +`state` shows the tab. A `path` is resolved against the browser's "recent files" store +rather than a disk, so use `data_base64` for a file the page has not seen. +`data_base64` is rejected on native builds; use `path` there. + +## Startup dialogs + +A dialog can be open when the page is ready. The donation prompt appears once per +application version in each browser profile, so a fresh or disposable profile always +sees it. While a modal is open `state.modal` is set, and `{"op":"new"}` returns `ok` +without creating a tab until the modal is closed. Close it first: + +```js +const state = await control({ op: 'state' }); +if (state.modal) await control({ op: 'action', name: 'close_modal' }); +await control({ op: 'new' }); +``` + +Native builds can show a second dialog at launch (the file-association prompt), so check +`state.modal` again after closing one. From cf004107b2fcd4fbe6574cfd3d8c23a83a1f8ce0 Mon Sep 17 00:00:00 2001 From: Helen Kwok <107752876+helenkwok@users.noreply.github.com> Date: Sun, 20 Sep 2026 12:56:22 +0930 Subject: [PATCH 03/18] Keep the task a dispatched command returns CmdResult::Dispatch and CmdResult::Relaunch end the active command and run the assembled line through dispatch_command, then discarded the Task it returned (`let _ =`). A command whose effect is a message carried by that Task was announced but never applied. VSCURRENT's keyword picker is one: choosing "Gouraud Shaded" printed "Visual style: Gouraud Shaded." and left the drawing in wireframe, because SetRenderMode travels in that Task. Hold the Task and batch it with the focus task that apply_cmd_result_inner returns. A regression test drives both results through VSCURRENT and checks the tab's render mode. --- src/app/command_driver.rs | 65 +++++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 12 deletions(-) diff --git a/src/app/command_driver.rs b/src/app/command_driver.rs index 87abf6df6..7a70a2ae5 100644 --- a/src/app/command_driver.rs +++ b/src/app/command_driver.rs @@ -1849,6 +1849,9 @@ impl OpenCADStudio { .active_cmd .as_ref() .is_some_and(|command| command.preserve_commit_layer()); + // Task produced by a command the arm dispatches; it must reach the + // runtime or messages such as a chosen render mode are dropped. + let mut dispatched = Task::none(); match result { CmdResult::OpenAutoConstrainSettings => { self.auto_constrain_saved = Some(self.auto_constrain_settings.clone()); @@ -4754,7 +4757,7 @@ impl OpenCADStudio { self.tabs[i].snap_result = None; self.tabs[i].scene.clear_preview_wire(); self.restore_pre_cmd_tangent(); - let _ = self.dispatch_command(&cmd); + dispatched = self.dispatch_command(&cmd); } CmdResult::Dispatch(cmd) => { // End this interactive front-end, then run the assembled command @@ -4763,7 +4766,7 @@ impl OpenCADStudio { self.tabs[i].snap_result = None; self.tabs[i].scene.clear_preview_wire(); self.restore_pre_cmd_tangent(); - let _ = self.dispatch_command(&cmd); + dispatched = self.dispatch_command(&cmd); } CmdResult::EditTableCell { handle, point } => { // TABLEDIT's pick: end the pick phase and hand (table, point) @@ -7494,16 +7497,15 @@ impl OpenCADStudio { // The rich text canvas owns keyboard editing itself. Leaving the // hidden command input focused would make it consume Left/Right before // the editor can handle them. - if self.mtext_editor.is_some() { - return self.unfocus_widgets(); - } - // The in-place TEXT editor needs keyboard focus on its own field. - if self.text_inline.is_some() { - return iced::widget::operation::focus(iced::widget::Id::new( - super::view::TEXT_INLINE_ID, - )); - } - self.focus_cmd_input() + let focus = if self.mtext_editor.is_some() { + self.unfocus_widgets() + } else if self.text_inline.is_some() { + // The in-place TEXT editor needs keyboard focus on its own field. + iced::widget::operation::focus(iced::widget::Id::new(super::view::TEXT_INLINE_ID)) + } else { + self.focus_cmd_input() + }; + Task::batch([dispatched, focus]) } /// Restore the tangent-snap / ortho state that was in effect before the command started. @@ -10101,3 +10103,42 @@ mod thicken_tests { assert!(app.tabs[i].scene.document.get_entity(source).is_some()); } } + +#[cfg(test)] +mod dispatched_command_task_tests { + use super::*; + use crate::command::CmdResult; + use acadrust::entities::ViewportRenderMode as Mode; + + /// `CmdResult::Dispatch` and `CmdResult::Relaunch` run the assembled line + /// through `dispatch_command`, and the Task it returns must reach the + /// runtime: the render mode a VSCURRENT keyword picker chooses is applied + /// by a message that Task carries. + #[test] + fn dispatch_and_relaunch_keep_the_task_the_command_returns() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + let i = app.active_tab; + + app.tabs[i].render_mode = Mode::Wireframe2D; + let task = app.apply_cmd_result(CmdResult::Dispatch("VSCURRENT GOURAUDSHADED".into())); + app.drive_headless_task(task).unwrap(); + assert_eq!( + app.tabs[i].render_mode, + Mode::GouraudShaded, + "Dispatch dropped the dispatched command's task" + ); + + app.tabs[i].render_mode = Mode::Wireframe2D; + let task = app.apply_cmd_result(CmdResult::Relaunch( + "VSCURRENT GOURAUDSHADED".into(), + Vec::new(), + )); + app.drive_headless_task(task).unwrap(); + assert_eq!( + app.tabs[i].render_mode, + Mode::GouraudShaded, + "Relaunch dropped the dispatched command's task" + ); + } +} From 899af80c2f5227732b4947a544170690b9f96fb2 Mon Sep 17 00:00:00 2001 From: buyun14 <112449589+buyun14@users.noreply.github.com> Date: Sun, 20 Sep 2026 05:54:43 +0800 Subject: [PATCH 04/18] fix(automation): consume and report the tail of a batch command line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run_command_line` feeds the tokens after the command verb to the interactive command and then terminates it as if Enter were pressed. Two gaps left a headless caller unable to tell what actually happened. In-place text editor takes over, remaining tokens were dropped ---------------------------------------------------------------- The `TEXT` content step hands over to the in-place editor, at which point the command has already ended (`active_cmd` is `None`). The feed loop stopped there, so the rest of the line — including the text itself — vanished: `TEXT 0,0 5 0 hello` reported `completed` with `added: 0` and created nothing. The tail is now typed into that editor and committed, and the commit is done synchronously so its result is observable: `text_inline_commit()` returning false leaves the tokens in `unconsumed` instead of reporting a successful line. Dispatch the commit as a queued message instead and a failed commit would look exactly like a good one — the text dropped, `added: 0`, `unconsumed: []`. Only an editor this line opens is taken over, and after a successful commit the line is closed with an escape: `TEXT` re-arms for the next line (`open_next_line`), which would otherwise let the *next* batch line be eaten as the next text position. Leftover tokens are dropped --------------------------- Any token no prompt ever asked for is recorded in `CommandLine::unconsumed` and surfaced by the automation `run` response, so over-typed or misplaced input is visible instead of silently ignored: `CIRCLE 0,0 5 9` reports `"unconsumed": ["9"]` while a fully consumed line reports an empty list. Pending surfaces are reported instead of `completed` ---------------------------------------------------- `run` now carries `blocked_by` — `"command"`, `"text_editor"`, `"mtext_editor"`, `"modal:"`, or `null` — and answers `waiting_input` when something is still open rather than claiming `completed`. `blocked_by` is attributed to the line that opened the surface: a condition and its snapshot are taken before the command runs, so an editor left open by an earlier line, another tab, or a startup dialog does not turn a finished command into `waiting_input`. Without that attribution a caller polling for `completed` would wait forever on someone else's editor. `docs/automation` documented `waiting_input` as "more input is required", which is exactly what a pending surface is; the two new fields are documented there as well. Tests ----- * `batch_text_line_creates_text_and_consumes_every_token` — the regression case creates exactly one `Text` entity and leaves nothing unconsumed. * `multi_word_text_keeps_the_spaces` — asserts the committed string, so an off-by-one in the consumed index or a wrong join cannot pass. * `run_reports_leftover_tokens_instead_of_dropping_them`. * `text_without_content_reports_the_open_editor`. * `a_stray_editor_is_not_hijacked_by_the_next_line` — the next line neither types into the stray editor nor inherits its blocker (`blocked_by: null`, `status: completed`). * `a_command_still_waiting_is_named_as_the_blocker`. Verified over the `--serve` protocol: `TEXT 0,0 5 0 hello world` → `completed`, `added: 1`, and the entity really holds `hello world`; `TEXT 40,0 5 0` → `waiting_input` + `blocked_by: "text_editor"`; the following `CIRCLE 0,0 5 9` → `completed`, `blocked_by: null`, `unconsumed: ["9"]`; `LINE` → `waiting_input` + `blocked_by: "command"`. Full lib suite: 1497 passed, 2 failed — both failures (`io::plot_style::…named_tables_and_missing_files_get_a_reason`, `scene::parametric_constraints::…arc_grips_drive_center_start_and_end_but_not_midpoint`) reproduce on the unmodified tree; run with `LC_ALL=C` since several xref tests otherwise fail on a non-English locale. Known remaining gap, unchanged here: `MTEXT` / `HATCH` still answer `completed` with `added: 0` in this path (they need interactive surfaces the feeder has no tokens to fill). Reported separately in #1390. --- docs/automation/README.md | 22 +++++- src/app/automation.rs | 152 +++++++++++++++++++++++++++++++++++++- src/app/command_driver.rs | 47 ++++++++++++ src/ui/command_line.rs | 8 ++ 4 files changed, 227 insertions(+), 2 deletions(-) diff --git a/docs/automation/README.md b/docs/automation/README.md index 1dcf4a90e..28fd67348 100644 --- a/docs/automation/README.md +++ b/docs/automation/README.md @@ -138,7 +138,27 @@ Execute responses use `response_detail: "compact"` by default and return only th For unfamiliar or conditional commands, use `start`, then inspect `state.command` in every response. Its `accepts` array gives the valid MCP input kinds, `options` gives the current tokens, and `input_example` gives the next request shape. Add the current state fields and a new `request_id` to each step. -Every `ocs_execute` request requires a caller-generated `request_id`. Reuse that ID only to retry the identical request after a timeout, together with the same session ID, document ID, expected revision, and selection. Commands report `waiting_input` while more input is required and asynchronous work remains `running` until its real callback finishes. Geometry stays in OpenCADStudio and its geometry kernel. +Every `ocs_execute` request requires a caller-generated `request_id`. Reuse that ID only to retry the identical request after a timeout, together with the same session ID, document ID, expected revision, and selection. Commands report `waiting_input` while more input is required and asynchronous work remains `running` until its real callback finishes. + +A `run` reply also carries two fields that describe what happened to the line: + +* `blocked_by` — `null` when the command finished, otherwise what *this line* + left open and waiting: `"command"` (an interactive command), `"text_editor"` + (the in-place editor opened by the `TEXT` content step), `"mtext_editor"`, or + `"modal:"` for a dialog. A surface that was already open when the line + started (an earlier line's editor, another tab, a startup dialog) is not + attributed to this line, so a caller polling for `completed` is never stuck + waiting on someone else's editor. `blocked_by` is set whenever `status` is + `waiting_input`. +* `unconsumed` — the tokens of the line that no prompt ever asked for, in order. + A fully consumed line reports `[]`; `CIRCLE 0,0 5 9` reports `["9"]` because + the radius step ends the command. Over-typed input is reported instead of + being dropped on the floor. + +When a line ends at the `TEXT` content step (`TEXT 0,0 5 0 hello`) the text is +typed into the in-place editor and committed, so the line behaves exactly as the +same input typed into the GUI command line would: one `Text` entity, nothing +unconsumed, and the command closed rather than left armed for a second line. Geometry stays in OpenCADStudio and its geometry kernel. `ocs_capture` defaults to the drawing viewport and a longest edge of 1600 pixels. Set `scope` to `window` for the full interface or change `max_dimension` between 256 and 4096. diff --git a/src/app/automation.rs b/src/app/automation.rs index ff0bc2776..d9121c64e 100644 --- a/src/app/automation.rs +++ b/src/app/automation.rs @@ -350,6 +350,14 @@ impl OpenCADStudio { let i = self.active_tab; let before = self.tabs[i].scene.document.entities().count(); let error_revision = self.command_line.error_revision; + // Surfaces that are already open belong to an earlier line (or + // another tab, or startup). Only what *this* line left open is + // reported as a blocker, otherwise a finished command would keep + // answering `waiting_input` because of someone else's editor and + // a caller polling for `completed` would never get there. + let editor_before = self.text_inline.is_some(); + let mtext_before = self.mtext_editor.is_some(); + let modal_before = self.active_modal.is_some(); if let Err(error) = self.run_headless(&cmd) { return err(error); } @@ -357,12 +365,32 @@ impl OpenCADStudio { return err(self.command_line.last_error.clone().unwrap_or_default()); } let after = self.tabs[i].scene.document.entities().count(); + // A command can finish with an interactive surface still open: + // the in-place text editor (the `TEXT` content step), the MTEXT + // editor, or a modal. Reporting `completed` there hides the fact + // that nothing was committed, so reuse the `waiting_input` + // vocabulary and name the blocker. + let blocked_by: Option = if self.tabs[i].active_cmd.is_some() { + Some("command".to_string()) + } else if let (false, Some(modal)) = (modal_before, self.active_modal.as_ref()) { + Some(format!("modal:{modal:?}")) + } else if !editor_before && self.text_inline.is_some() { + Some("text_editor".to_string()) + } else if !mtext_before && self.mtext_editor.is_some() { + Some("mtext_editor".to_string()) + } else { + None + }; json!({ "ok": true, "cmd": cmd, - "status": if self.tabs[i].active_cmd.is_some() { "waiting_input" } else { "completed" }, + "status": if blocked_by.is_some() { "waiting_input" } else { "completed" }, + "blocked_by": blocked_by, "entities": after, "added": after as i64 - before as i64, + // Tokens no prompt ever asked for: leftover input is + // reported rather than dropped on the floor. + "unconsumed": self.command_line.unconsumed.clone(), }) } "entities" => self.entity_summary(), @@ -772,6 +800,128 @@ mod tests { assert!(app.tabs[i].scene.last_tess_wires.get() > before); } + #[test] + fn batch_text_line_creates_text_and_consumes_every_token() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + + // Regression: the content token used to be dropped once the in-place + // editor took over, so this line created nothing while reporting ok. + let r = app.automation_op(r#"{"op":"run","cmd":"TEXT 0,0 5 0 hello"}"#); + assert_eq!(r["ok"], true); + assert_eq!(r["status"], "completed", "{r}"); + assert_eq!(r["added"], 1, "{r}"); + assert_eq!(r["unconsumed"].as_array().map(|v| v.len()), Some(0), "{r}"); + + let counts = app.automation_op(r#"{"op":"entities"}"#); + assert_eq!(counts["by_type"]["Text"], 1, "{counts}"); + assert_eq!(counts["total"], 1, "no phantom entity: {counts}"); + assert_eq!(assert_one_text(&app), "hello"); + } + + #[test] + fn multi_word_text_keeps_the_spaces() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + + let r = app.automation_op(r#"{"op":"run","cmd":"TEXT 0,0 5 0 hello world"}"#); + assert_eq!(r["status"], "completed", "{r}"); + assert_eq!(r["added"], 1, "{r}"); + assert_eq!(r["unconsumed"].as_array().map(|v| v.len()), Some(0), "{r}"); + let counts = app.automation_op(r#"{"op":"entities"}"#); + assert_eq!(counts["by_type"]["Text"], 1, "{counts}"); + + // The spaces between the words have to survive the token split, and the + // whole tail (not just its first token) has to land in the entity — an + // off-by-one in the consumed index would still pass the checks above. + assert_eq!(assert_one_text(&app), "hello world"); + } + + #[test] + fn a_stray_editor_is_not_hijacked_by_the_next_line() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + + // Line 1 stops at the content step and leaves the editor open. + let r = app.automation_op(r#"{"op":"run","cmd":"TEXT 30,0 5 0"}"#); + assert_eq!(r["blocked_by"], "text_editor", "{r}"); + + // Line 2 must not type its leftover token into that unrelated editor, + // and — since line 2 did not open anything itself — it must not inherit + // line 1's blocker either: a caller that polls for `completed` would + // otherwise never get there. + let r = app.automation_op(r#"{"op":"run","cmd":"CIRCLE 0,0 5 9"}"#); + assert_eq!(r["added"], 1, "{r}"); + assert_eq!(r["unconsumed"][0], "9", "{r}"); + assert_eq!(r["blocked_by"], serde_json::Value::Null, "{r}"); + assert_eq!(r["status"], "completed", "{r}"); + let counts = app.automation_op(r#"{"op":"entities"}"#); + assert_eq!(counts["by_type"]["Circle"], 1, "{counts}"); + assert!( + counts["by_type"].get("Text").is_none(), + "phantom text from stray editor: {counts}" + ); + } + + #[test] + fn run_reports_leftover_tokens_instead_of_dropping_them() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + + // The radius step ends the command, so the extra token is never asked for. + let r = app.automation_op(r#"{"op":"run","cmd":"CIRCLE 0,0 5 9"}"#); + assert_eq!(r["added"], 1, "{r}"); + assert_eq!(r["unconsumed"][0], "9", "{r}"); + + // A fully consumed line reports nothing left over. + let r = app.automation_op(r#"{"op":"run","cmd":"LINE 0,0 10,10"}"#); + assert_eq!(r["unconsumed"].as_array().map(|v| v.len()), Some(0), "{r}"); + } + + #[test] + fn text_without_content_reports_the_open_editor() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + + let r = app.automation_op(r#"{"op":"run","cmd":"TEXT 0,0 5 0"}"#); + assert_eq!(r["status"], "waiting_input", "{r}"); + assert_eq!(r["blocked_by"], "text_editor", "{r}"); + let counts = app.automation_op(r#"{"op":"entities"}"#); + assert_eq!(counts["total"], 0, "{counts}"); + + // The same editor can be finished through the control-surface messages. + let _ = app.update(crate::app::Message::TextInlineInput("hello".into())); + let _ = app.update(crate::app::Message::TextInlineOk); + let counts = app.automation_op(r#"{"op":"entities"}"#); + assert_eq!(counts["by_type"]["Text"], 1, "{counts}"); + } + + #[test] + fn a_command_still_waiting_is_named_as_the_blocker() { + let mut app = OpenCADStudio::new_for_test(); + let _ = app.automation_op(r#"{"op":"new"}"#); + + let r = app.automation_op(r#"{"op":"run","cmd":"LINE"}"#); + assert_eq!(r["status"], "waiting_input", "{r}"); + assert_eq!(r["blocked_by"], "command", "{r}"); + } + + /// The text of the single `Text` entity in the drawing (panics otherwise). + fn assert_one_text(app: &OpenCADStudio) -> String { + let i = app.active_tab; + let texts: Vec = app.tabs[i] + .scene + .document + .entities() + .filter_map(|e| match e { + acadrust::EntityType::Text(t) => Some(t.value.clone()), + _ => None, + }) + .collect(); + assert_eq!(texts.len(), 1, "expected exactly one Text entity, got {texts:?}"); + texts.into_iter().next().unwrap() + } + #[test] fn automation_ops_round_trip() { let mut app = OpenCADStudio::new_for_test(); diff --git a/src/app/command_driver.rs b/src/app/command_driver.rs index 87abf6df6..f02c731ea 100644 --- a/src/app/command_driver.rs +++ b/src/app/command_driver.rs @@ -566,6 +566,7 @@ impl OpenCADStudio { /// the headless automation feeder so both behave identically. pub(super) fn run_command_line(&mut self, cmd: &str) -> Task { let i = self.active_tab; + self.command_line.unconsumed.clear(); let tokens: Vec<&str> = cmd.split_whitespace().collect(); if tokens.len() <= 1 { return self.dispatch_command(cmd); @@ -597,18 +598,64 @@ impl OpenCADStudio { /// Feed `tokens[1..]` to the active interactive command as points / option /// keywords, then terminate it as if Enter were pressed. No-op when no /// command is active. + /// + /// Two things happen when the tokens run out mid-way, both needed so a + /// headless caller can tell what actually happened: + /// + /// * If the in-place text editor took over (the `TEXT` content step — the + /// command itself has already ended by then, `active_cmd` is `None`), the + /// remaining tokens are that text: they are typed into the editor and + /// committed, the same messages the control surface's `text_input` / + /// `text_commit` actions dispatch. Without this the tail of the line was + /// silently dropped and `TEXT 0,0 5 0 hi` created nothing at all. + /// * Anything still left over was never claimed by any prompt; it is + /// recorded in [`CommandLine::unconsumed`] instead of vanishing. pub(super) fn finish_active_command(&mut self, tokens: &[String]) -> Task { let i = self.active_tab; if self.tabs[i].active_cmd.is_none() { return Task::none(); } self.last_point = None; + // An editor that is *already* open belongs to someone else (an earlier + // line that stopped at the content step). Only the editor this line + // opens by feeding its own tokens may be handed the tail. + let editor_was_open = self.text_inline.is_some(); let mut tasks = Vec::new(); + // First token is the command verb itself; prompts start consuming after it. + let mut consumed = 1; for tok in &tokens[1..] { if self.tabs[i].active_cmd.is_none() { break; } tasks.push(self.feed_active_cmd(tok)); + consumed += 1; + } + if !editor_was_open && self.text_inline.is_some() && consumed < tokens.len() { + // Fill the editor the same way `Message::TextInlineInput` does, then + // commit *synchronously* so the outcome is observable: dispatching + // `TextInlineOk` as a task would hide whether the entity was really + // created, and the tail token would be counted as consumed either + // way — the caller would have no way to tell "text created" from + // "text silently dropped". + if let Some(editor) = self.text_inline.as_mut() { + editor.value = tokens[consumed..].join(" "); + } + let committed = self.text_inline_commit(); + tasks.push(self.post_editor_closed(committed)); + if committed { + consumed = tokens.len(); + // `TEXT` repeats by design: on a committed string it re-arms for + // the next line (`open_next_line`), which re-opens the editor. A + // batch line is a complete unit, and leaving that repeat armed + // would make the *next* line get eaten as the next text position, + // so end the command here, exactly as Esc does in the GUI. + tasks.push(Task::done(Message::CommandEscape)); + } + // On a failed commit the tokens stay unconsumed (assigned below), so + // the caller still sees the text it passed in. + } + if consumed < tokens.len() { + self.command_line.unconsumed = tokens[consumed..].to_vec(); } tasks.push(self.feed_command(StepInput::Enter)); Task::batch(tasks) diff --git a/src/ui/command_line.rs b/src/ui/command_line.rs index a703c3800..d4259f78c 100644 --- a/src/ui/command_line.rs +++ b/src/ui/command_line.rs @@ -129,6 +129,13 @@ pub struct CommandLine { pub history: Vec, pub error_revision: u64, pub last_error: Option, + /// Tokens from the last multi-token command line that no prompt ever + /// consumed — e.g. a trailing word after an in-place text step, or a typo'd + /// extra argument (`CIRCLE 0,0 5 9`). The headless automation feeder reports + /// them so a caller can tell "part of my line was silently dropped" from + /// "the command ran exactly as typed". Cleared at the start of every + /// `run_command_line`, not persisted. + pub unconsumed: Vec, /// Successfully dispatched commands used for ↑/↓ recall, newest last. /// Stored separately because recall also maintains its own cursor and draft. pub cmd_recall: Vec, @@ -186,6 +193,7 @@ impl Default for CommandLine { history: Vec::new(), error_revision: 0, last_error: None, + unconsumed: Vec::new(), cmd_recall: Vec::new(), recent_commands: Vec::new(), recent_inputs: Vec::new(), From 747245a75fb8befee3349953d373caa0c27acb9c Mon Sep 17 00:00:00 2001 From: buyun14 <112449589+buyun14@users.noreply.github.com> Date: Sun, 20 Sep 2026 19:21:49 +0800 Subject: [PATCH 05/18] docs(automation): document the native entry points and their limits Refs #1349. The automation README describes `--mcp` and never mentions `--serve`, even though every native build accepts it and it is the only channel that works without a display. It also leaves out the two startup dialogs, the `document_id` routing, and what `waiting_input` actually means. `docs/automation/native.md` covers, all measured on a Linux headless machine with `OpenCADStudio 2026.37+165`: - choosing between `--mcp` and `--serve`, and what each needs (display, no-display, `--export` for one-shot conversion); - the writable `HOME`/`XDG_CONFIG_HOME` requirement, without which `ocs_sessions` answers `No user configuration directory`; - the startup dialogs: `AssocPrompt` then `DonationPrompt`, `new` answering `ok` with no document until `close_modal` clears them, and that the donation prompt appears once per application version per profile; - targeting a document: reads and writes follow the active tab, so `document_id` has to be passed explicitly after `new`; - feeding a whole line: `waiting_input` means more input is required (not that nothing happened), `cancel` closes a command, and the `unconsumed` / `blocked_by` fields added in #1392; - the interactive `start`/`input` shapes, including that a typed value goes through `kind: "text"` while a keyword uses `kind: "token"` with its payload in a field named `text`, and that the `TEXT` content step belongs to the in-place editor (`text_input` / `text_commit`); - what only native builds have: `measure`, `ocs_capture` (inline base64, or a PNG path), and `set_properties` with `collection` and colours as `{"Index": n}` / `"ByLayer"`; - the remaining gap: `MTEXT`, `HATCH` and `POLYGON` report `completed` with `added: 0` on the `--serve` path (#1390). The README gains one line pointing to the page, placed after the tool list rather than at the end, so it does not overlap #1394. Docs only; no code changes. --- docs/automation/README.md | 4 ++ docs/automation/native.md | 107 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 docs/automation/native.md diff --git a/docs/automation/README.md b/docs/automation/README.md index 1dcf4a90e..8293aec55 100644 --- a/docs/automation/README.md +++ b/docs/automation/README.md @@ -17,6 +17,10 @@ The server provides four tools: - `ocs_execute` performs one operation, an atomic record update, or a sequential batch against the real editor. - `ocs_capture` returns a bounded PNG of the drawing viewport or complete window. +Native builds also accept a second, headless entry point (`OpenCADStudio --serve`, one +JSON request per line) for scripts and CI; the two native channels, their startup dialogs +and their limits are described in [native.md](native.md). + The normal flow is to call `ocs_sessions`, pass its returned `session_id` as `ocs_session_id` to the other tools, read the chosen session with `ocs_read`, then pass the returned document state into `ocs_execute`. For example, an undo request has this shape: ```json diff --git a/docs/automation/native.md b/docs/automation/native.md new file mode 100644 index 000000000..3d3c9f9a7 --- /dev/null +++ b/docs/automation/native.md @@ -0,0 +1,107 @@ +# Driving the native builds + +Every native build exposes the same control core through two entry points: the MCP +server described in the [README](README.md) (`OpenCADStudio --mcp`), and a headless +JSON-line channel meant for scripts and CI (`OpenCADStudio --serve`). The operations, +request envelope and replies are the same; this page covers what is specific to the +native side. The browser entry is described in [web.md](web.md). + +## Choosing a channel + +| | `--mcp` | `--serve` | +|---|---|---| +| Transport | MCP over stdio | one JSON request per line on stdin, one reply per line on stdout | +| Editor | starts the desktop editor and exposes the live document | no window; works on its own document | +| Needs a display | yes — on a headless machine `xvfb-run` works, on software rendering the viewport may show the GPU warning dialog | no | +| `measure`, `ocs_capture`, `set_properties`, interactive steps | available | `measure` and `capture` are not; the remaining operations are | +| Good for | an MCP client driving a real editor | batch jobs, CI, agents without a display | + +`OpenCADStudio --export IN OUT` converts a drawing in one shot, without any session. It +accepts `.dwg` and `.dxf` targets only. + +## Requirements + +The process needs a writable `HOME` (or `XDG_CONFIG_HOME`) — without one, `ocs_sessions` +answers `No user configuration directory` and nothing else works. + +## Startup dialogs + +A fresh profile opens two dialogs before any drawing exists, and both of them block +document creation: + +| Step | `ocs_sessions` reports | +|---|---| +| right after launch | `modal: "AssocPrompt"` | +| after `close_modal` | `modal: "DonationPrompt"` | +| after a second `close_modal` | `modal: null` | + +While a dialog is open, `{"op":"new"}` answers `ok` but no tab is created, so there is +no `document_id` to work with. Close the dialogs first: + +```python +while (info := sessions())["modal"]: + execute({"op": "action", "name": "close_modal", "document_id": info["document_id"]}) +``` + +The donation prompt appears once per application version per profile, so an automated +profile meets it once per version rather than on every run. See +[web.md](web.md) for the same pattern on the browser side. + +## Targeting a document + +Reads and mutations target the active tab. `new` creates a document but does not move +the active tab, so pass the `document_id` from the reply explicitly — without it, reads +answer empty and it looks like nothing was created. `activate` switches the active tab. +A client connection to `--mcp` starts a new editor instance, so session state does not +carry over between connections. + +## Feeding a whole command line + +`run` takes one complete command line, including the answers to every prompt, and then +terminates the command as if Enter were pressed. A prompt cannot be continued on a +following line — each line is parsed as a fresh command line, so a command left waiting +is replaced by the next one (send `{"op":"cancel"}` if you want to drop it explicitly). + +`waiting_input` means input is still required — not that nothing happened. `SPLINE` and +`ARC`, for example, keep accepting further input even after the entity exists, and +`zoom_extents` reports it while the view has already changed. Send `{"op":"cancel"}` to +close the command when the line is done with it. + +Two reply fields describe what happened to the line: + +| Field | Meaning | +|---|---| +| `unconsumed` | Tokens that no prompt ever asked for, in order. `CIRCLE 0,0 5 9` reports `["9"]`, because the radius step ends the command; a fully consumed line reports `[]`. | +| `blocked_by` | What *this line* left open: `"command"`, `"text_editor"`, `"mtext_editor"`, `"modal:"`, or `null`. Set whenever `status` is `waiting_input`. | + +`TEXT 0,0 5 0 hello world` therefore creates one `Text` entity that holds +`hello world`, reports `completed` with `added: 1`, and leaves nothing unconsumed. The +content step runs in the in-place editor, which the feeder commits and then closes so +that the command does not stay armed for a second line. + +## Interactive steps + +For commands whose answers cannot be written on one line, use `start` and then `input`. +`start` replies with `state.command`, whose `accepts`, `options`, `prompt` and +`input_example` say what the current step wants: + +| Step | Request | Note | +|---|---|---| +| a point | `{"op":"input","kind":"point","point":[x,y,0],"space":"wcs"}` | the point is an array; separate `x`/`y` fields are rejected | +| a typed value (height, rotation) | `{"op":"input","kind":"text","text":"5"}` | digits go through `text`, not `token`; `{"op":"input","kind":"enter"}` accepts the default | +| a keyword (`J`, `ST`, `C`) | `{"op":"input","kind":"token","text":"C"}` | the payload field is named `text` | +| the text of `TEXT`/`MTEXT` | `{"op":"action","name":"text_input","value":"hello"}` then `{"op":"action","name":"text_commit"}` | the command has already ended at that point and the editor owns the input | + +## Native-only operations + +| Operation | Device | +|---|---| +| `measure` | `ocs_read` with `op: "measure"` and `parameters.handles` returns kernel-computed length, area, bounds and mass properties. | +| `ocs_capture` | Returns the viewport or the whole window. Either an inline base64 image, or a PNG written under the temporary directory with the reply carrying its `path`. | +| `set_properties` | Mutates record properties. It requires `collection`, and colours are the serialised enum — `{"Index": 1}` or `"ByLayer"`; a colour name such as `"Red"` is rejected with `invalid_value`. | + +## Known gaps + +On the `--serve` path, `MTEXT`, `HATCH` and `POLYGON` report `completed` with +`added: 0`: they need interactive surfaces (a pattern choice, a boundary selection) that +a single command line has no tokens to fill. `TEXT` no longer belongs to that list. From 0d023d267bc5b7afeca3b54e98875b0efd4f3926 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 15:45:32 +0000 Subject: [PATCH 06/18] Release v2026.38 --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 64e87c24a..4da766abd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,7 +4,7 @@ version = 4 [[package]] name = "OpenCADStudio" -version = "2026.37.0" +version = "2026.38.0" dependencies = [ "acadrust", "ashpd", diff --git a/Cargo.toml b/Cargo.toml index 7b9768873..6e78631dc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "OpenCADStudio" -version = "2026.37.0" +version = "2026.38.0" edition = "2021" build = "build.rs" default-run = "OpenCADStudio" From 88450954320ed1545358785a26403d53b2cadf9a Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Sun, 20 Sep 2026 20:18:18 +0300 Subject: [PATCH 07/18] fix: DXF attribute rotation converted once; plugin layers refuse unknown linetypes - The acadrust DXF reader has converted ATTRIB/ATTDEF rotation to radians since f226110 (in every codec rev OCS pinned from 2026-08-08 on), while fix_dxf_dimension_rotations still applied to_radians a second time on load. Only Shape needs the conversion here. Surfaced by #1391's hunk. - Shape-file text styles are found by their is_shape_file flag instead of an empty name (also from #1391). - #1393's add_layer/modify_layer wrote an unresolved linetype name into the LAYER record; a name the drawing (plus the standard set) cannot supply is now refused, through one resolve_linetype helper. Co-Authored-By: Claude Fable 5.1 --- src/app/plugin_host.rs | 64 +++++++++++++++++------------------------- src/entities/shape.rs | 2 +- src/io/mod.rs | 16 +++-------- 3 files changed, 30 insertions(+), 52 deletions(-) diff --git a/src/app/plugin_host.rs b/src/app/plugin_host.rs index 5ac7204c2..0e08b5ae7 100644 --- a/src/app/plugin_host.rs +++ b/src/app/plugin_host.rs @@ -269,24 +269,14 @@ impl<'a> HostSession<'a> { return None; } - let resolved_lt = if let Some(ref lt_name) = config.linetype { - let matched = doc - .line_types - .iter() - .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) - .map(|lt| lt.name.clone()); - if let Some(m) = matched { - m - } else { - crate::io::linetypes::populate_document(doc); - doc.line_types - .iter() - .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) - .map(|lt| lt.name.clone()) - .unwrap_or_else(|| lt_name.clone()) - } - } else { - "Continuous".to_string() + let resolved_lt = match config.linetype { + // A name the drawing does not carry would leave the LAYER record + // pointing at no LTYPE, so it is refused rather than written. + Some(ref lt_name) => match resolve_linetype(doc, lt_name) { + Some(name) => name, + None => return None, + }, + None => "Continuous".to_string(), }; let mut layer = acadrust::tables::Layer::new(trimmed); @@ -325,27 +315,12 @@ impl<'a> HostSession<'a> { return false; } - let resolved_lt = if let Some(ref lt_name) = config.linetype { - let doc = self.document_mut(); - let matched = doc - .line_types - .iter() - .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) - .map(|lt| lt.name.clone()); - if let Some(m) = matched { - Some(m) - } else { - crate::io::linetypes::populate_document(doc); - Some( - doc.line_types - .iter() - .find(|lt| lt.name.eq_ignore_ascii_case(lt_name)) - .map(|lt| lt.name.clone()) - .unwrap_or_else(|| lt_name.clone()), - ) - } - } else { - None + let resolved_lt = match config.linetype { + Some(ref lt_name) => match resolve_linetype(self.document_mut(), lt_name) { + Some(name) => Some(name), + None => return false, + }, + None => None, }; let doc = self.document_mut(); @@ -397,6 +372,17 @@ impl<'a> HostSession<'a> { } } +/// The stored spelling of `name` in the drawing's linetype table, loading the +/// standard linetypes first when it is not there yet. `None` when the drawing +/// cannot supply it: a layer must not reference a linetype that does not exist. +fn resolve_linetype(doc: &mut acadrust::CadDocument, name: &str) -> Option { + let stored = |doc: &acadrust::CadDocument| doc.line_types.get(name).map(|lt| lt.name.clone()); + stored(doc).or_else(|| { + crate::io::linetypes::populate_document(doc); + stored(doc) + }) +} + /// The stable contract a plugin's `dispatch` sees. Each method forwards to the /// inherent `HostSession` method of the same name (inherent methods take /// resolution priority, so this is plain delegation, not recursion). The diff --git a/src/entities/shape.rs b/src/entities/shape.rs index 7b4b1b8f1..12cb0f4cc 100644 --- a/src/entities/shape.rs +++ b/src/entities/shape.rs @@ -119,7 +119,7 @@ fn shx_polylines( // glyph named like this shape. if !name.is_empty() { for style in document.text_styles.iter() { - if style.name.is_empty() && !style.font_file.trim().is_empty() { + if style.is_shape_file && !style.font_file.trim().is_empty() { if let Some(p) = try_style(style) { return Some(p); } diff --git a/src/io/mod.rs b/src/io/mod.rs index 7afc5868f..646c0d536 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -2368,21 +2368,13 @@ fn fix_viewport_status_flags(doc: &mut CadDocument) { } } -/// The acadrust DXF reader stores several rotation fields directly from DXF -/// group code 50 in degrees, while DWG and our own creation code store radians. -/// Apply to_radians() on load so tessellation can call cos/sin uniformly. +/// The acadrust DXF reader still stores Shape rotation directly from group +/// code 50 in degrees, while DWG and our own creation code store radians. +/// Dimension angles and ATTRIB/ATTDEF rotation are converted inside the +/// reader, so arms for them here would convert twice. fn fix_dxf_dimension_rotations(doc: &mut CadDocument) { for entity in doc.entities_mut() { match entity { - // Dimension angles (rotation / text / oblique) are converted - // degrees->radians inside the acadrust DXF reader now, so a - // dimension arm here would double-convert. - EntityType::AttributeDefinition(a) => { - a.rotation = a.rotation.to_radians(); - } - EntityType::AttributeEntity(a) => { - a.rotation = a.rotation.to_radians(); - } EntityType::Shape(s) => { s.rotation = s.rotation.to_radians(); } From 75e7f825cdf25e09d178495eda16de7a2d5fc82a Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Sun, 20 Sep 2026 20:30:46 +0300 Subject: [PATCH 08/18] docs(automation): make native.md match the code The page merged from #1401 described --serve and --mcp as one surface. Rewritten against src/app/automation.rs, src/app/control/mod.rs and src/mcp.rs: the --serve readiness banner and --port mode, the two request styles on that channel and how each treats a waiting command (command_busy versus replacement), measure being available headlessly while capture is not, --export writing DWG for every extension but .dxf, startup dialogs belonging to the editor behind --mcp only, MCP sessions being shared rather than one editor per connection, the MTEXT editor's own actions, and the legacy run reply fields (blocked_by, unconsumed) that only that path returns. Co-Authored-By: Claude Fable 5.1 --- docs/automation/native.md | 154 ++++++++++++++++++++++---------------- 1 file changed, 89 insertions(+), 65 deletions(-) diff --git a/docs/automation/native.md b/docs/automation/native.md index 3d3c9f9a7..9211f85c6 100644 --- a/docs/automation/native.md +++ b/docs/automation/native.md @@ -1,32 +1,82 @@ # Driving the native builds -Every native build exposes the same control core through two entry points: the MCP -server described in the [README](README.md) (`OpenCADStudio --mcp`), and a headless -JSON-line channel meant for scripts and CI (`OpenCADStudio --serve`). The operations, -request envelope and replies are the same; this page covers what is specific to the -native side. The browser entry is described in [web.md](web.md). +Every native build has three entry points besides the editor window: the MCP +server described in the [README](README.md) (`OpenCADStudio --mcp`), a headless +JSON-line channel for scripts and CI (`OpenCADStudio --serve`), and a one-shot +converter (`OpenCADStudio --export IN OUT`). The browser entry is described in +[web.md](web.md). ## Choosing a channel | | `--mcp` | `--serve` | |---|---|---| -| Transport | MCP over stdio | one JSON request per line on stdin, one reply per line on stdout | -| Editor | starts the desktop editor and exposes the live document | no window; works on its own document | -| Needs a display | yes — on a headless machine `xvfb-run` works, on software rendering the viewport may show the GPU warning dialog | no | -| `measure`, `ocs_capture`, `set_properties`, interactive steps | available | `measure` and `capture` are not; the remaining operations are | +| Transport | MCP (JSON-RPC over stdio) with the four tools of the README | one JSON request per line on stdin, one reply per line on stdout; `--port N` listens on `127.0.0.1:N` instead | +| Editor | attaches to a running desktop editor, or starts one when there is none (`ocs_sessions`, `launch_if_none`) | no window; a document session of its own that lives as long as the process | +| Needs a display | yes (`xvfb-run` works on a headless machine; software rendering may raise the GPU warning dialog) | no | +| Needs a writable `HOME` / `XDG_CONFIG_HOME` | yes — the session descriptors live in the user configuration directory; without one `ocs_sessions` answers `No user configuration directory` | no | +| Startup dialogs | those of the editor (below) | none | +| `capture` | `ocs_capture` | not available (`gui_required`): there is no window to capture | | Good for | an MCP client driving a real editor | batch jobs, CI, agents without a display | -`OpenCADStudio --export IN OUT` converts a drawing in one shot, without any session. It -accepts `.dwg` and `.dxf` targets only. +## `--serve` -## Requirements +The first line written is a readiness banner, before any request is read: -The process needs a writable `HOME` (or `XDG_CONFIG_HOME`) — without one, `ocs_sessions` -answers `No user configuration directory` and nothing else works. +```json +{"ok":true,"ready":true,"version":"2026.38"} +``` -## Startup dialogs +Over `--port N` the banner opens every connection; the listener serves one +client at a time and the document session persists across reconnects. -A fresh profile opens two dialogs before any drawing exists, and both of them block +Two request styles share the channel, told apart by the `protocol` field: + +| Style | Shape | Operations | +|---|---|---| +| protocol 1 | `{"protocol":1,"request_id":"…","document_id":N,"op":"…"}` — the same requests and replies as `ocs_execute` / `ocs_read`, described in the README | `new`, `open`, `save`, `activate`, `select`, `run`, `start`, `input`, `cancel`, `stop`, `action`, `property`, `set_properties`, `embed_image`, `undo`, `redo`; reads `state`, `capabilities`, `query`, `entities`, `records`, `record_schema`, `layers`, `header`, `properties`, `measure`, `history`, `commands`, `events`, `operation` | +| legacy | `{"op":"…"}` without `protocol` | `new`, `open`, `run`, `entities`, `query`, `records`, `record_schema`, `capabilities`, `layers`, `header`, `select`, `save`, `undo`, `redo` | + +The two styles differ in how a waiting command is treated. A protocol-1 `run` +or `start` while a command is still active is refused with `command_busy`: +continue it with `input` or drop it with `cancel`. A legacy `run` line ends the +waiting command and starts the new one. + +### The legacy `run` line + +`{"op":"run","cmd":"LINE 0,0 10,10"}` feeds the tokens after the command name +to the prompts in order and then presses Enter. The reply is + +```json +{"ok":true,"cmd":"CIRCLE 0,0 5 9","status":"completed","blocked_by":null,"entities":1,"added":1,"unconsumed":["9"]} +``` + +| Field | Meaning | +|---|---| +| `status` | `completed`, or `waiting_input` when the line left something open | +| `blocked_by` | what *this line* left open: `"command"`, `"text_editor"`, `"mtext_editor"`, `"modal:"`, or `null`. A surface that was already open when the line started is not attributed to it | +| `entities`, `added` | entity count after the line, and the difference to before | +| `unconsumed` | tokens no prompt asked for, in order (`["9"]` above: the radius step ended the command); `[]` when everything was consumed | + +`waiting_input` means input is still required, not that nothing happened: +`SPLINE` and `ARC` keep accepting input after the entity exists, and +`zoom_extents` reports it after the view has already moved. + +`TEXT 0,0 5 0 hello world` creates one `Text` entity holding `hello world`: the +tail of the line is typed into the in-place editor, committed, and the command +is closed so it does not stay armed for a second line. A `TEXT` line without +content answers `waiting_input` with `blocked_by: "text_editor"`. + +Known gap (#1390): `MTEXT`, `HATCH` and `POLYGON` answer `completed` with +`added: 0` on this path — they need interactive surfaces (the rich-text editor, +a pattern choice, a boundary pick) that a single line has no tokens to fill. + +## `--mcp` + +The MCP server is a client of the desktop editor: `ocs_sessions` lists the live +editor sessions found in the configuration directory and starts one when none +is running, so several MCP connections can share one editor and its state. + +A fresh profile opens two dialogs before any drawing exists, and both block document creation: | Step | `ocs_sessions` reports | @@ -35,73 +85,47 @@ document creation: | after `close_modal` | `modal: "DonationPrompt"` | | after a second `close_modal` | `modal: null` | -While a dialog is open, `{"op":"new"}` answers `ok` but no tab is created, so there is -no `document_id` to work with. Close the dialogs first: +While a dialog is open, `{"op":"new"}` answers `ok` but no tab is created. +Close the dialogs first: ```python while (info := sessions())["modal"]: execute({"op": "action", "name": "close_modal", "document_id": info["document_id"]}) ``` -The donation prompt appears once per application version per profile, so an automated -profile meets it once per version rather than on every run. See -[web.md](web.md) for the same pattern on the browser side. +The donation prompt appears once per application version per profile, so an +automated profile meets it once per version rather than on every run. -## Targeting a document +`new` makes the new drawing the active document. Requests carry `document_id`; +take it from the reply's state and pass it on the following requests, as the +README describes. -Reads and mutations target the active tab. `new` creates a document but does not move -the active tab, so pass the `document_id` from the reply explicitly — without it, reads -answer empty and it looks like nothing was created. `activate` switches the active tab. -A client connection to `--mcp` starts a new editor instance, so session state does not -carry over between connections. +## `--export` -## Feeding a whole command line - -`run` takes one complete command line, including the answers to every prompt, and then -terminates the command as if Enter were pressed. A prompt cannot be continued on a -following line — each line is parsed as a fresh command line, so a command left waiting -is replaced by the next one (send `{"op":"cancel"}` if you want to drop it explicitly). - -`waiting_input` means input is still required — not that nothing happened. `SPLINE` and -`ARC`, for example, keep accepting further input even after the entity exists, and -`zoom_extents` reports it while the view has already changed. Send `{"op":"cancel"}` to -close the command when the line is done with it. - -Two reply fields describe what happened to the line: - -| Field | Meaning | -|---|---| -| `unconsumed` | Tokens that no prompt ever asked for, in order. `CIRCLE 0,0 5 9` reports `["9"]`, because the radius step ends the command; a fully consumed line reports `[]`. | -| `blocked_by` | What *this line* left open: `"command"`, `"text_editor"`, `"mtext_editor"`, `"modal:"`, or `null`. Set whenever `status` is `waiting_input`. | - -`TEXT 0,0 5 0 hello world` therefore creates one `Text` entity that holds -`hello world`, reports `completed` with `added: 1`, and leaves nothing unconsumed. The -content step runs in the in-place editor, which the feeder commits and then closes so -that the command does not stay armed for a second line. +`OpenCADStudio --export IN OUT` loads `IN` and writes `OUT` without a window or +a session. The output format follows the extension: `.dxf` writes DXF, any other +extension writes DWG at the document's own version. The exit code is 0 on +success and 1 when the input cannot be read or the output cannot be written. ## Interactive steps -For commands whose answers cannot be written on one line, use `start` and then `input`. -`start` replies with `state.command`, whose `accepts`, `options`, `prompt` and +For commands whose answers cannot be written on one line, use `start` and then +`input`. `start` replies with `state.command`, whose `accepts`, `options` and `input_example` say what the current step wants: | Step | Request | Note | |---|---|---| -| a point | `{"op":"input","kind":"point","point":[x,y,0],"space":"wcs"}` | the point is an array; separate `x`/`y` fields are rejected | +| a point | `{"op":"input","kind":"point","point":[x,y,0],"space":"wcs"}` | the point is an array; `space` is `wcs` (default), `ucs` or `relative` | | a typed value (height, rotation) | `{"op":"input","kind":"text","text":"5"}` | digits go through `text`, not `token`; `{"op":"input","kind":"enter"}` accepts the default | | a keyword (`J`, `ST`, `C`) | `{"op":"input","kind":"token","text":"C"}` | the payload field is named `text` | -| the text of `TEXT`/`MTEXT` | `{"op":"action","name":"text_input","value":"hello"}` then `{"op":"action","name":"text_commit"}` | the command has already ended at that point and the editor owns the input | +| an object | `{"op":"input","kind":"entity","handle":"HANDLE","point":[x,y,0]}` | `kind: "structure"` for a sub-entity pick, `kind: "selection"` to hand over the current selection | +| the content of `TEXT` | `{"op":"action","name":"text_input","value":"hello"}` then `{"op":"action","name":"text_commit"}` | the command has already ended; the in-place editor owns the input | +| the content of `MTEXT` | `{"op":"action","name":"mtext_insert","value":"hello"}` then `{"op":"action","name":"mtext_commit"}` | the rich-text editor has its own actions; the `TEXT` ones answer `editor_closed` | ## Native-only operations -| Operation | Device | -|---|---| -| `measure` | `ocs_read` with `op: "measure"` and `parameters.handles` returns kernel-computed length, area, bounds and mass properties. | -| `ocs_capture` | Returns the viewport or the whole window. Either an inline base64 image, or a PNG written under the temporary directory with the reply carrying its `path`. | -| `set_properties` | Mutates record properties. It requires `collection`, and colours are the serialised enum — `{"Index": 1}` or `"ByLayer"`; a colour name such as `"Red"` is rejected with `invalid_value`. | - -## Known gaps - -On the `--serve` path, `MTEXT`, `HATCH` and `POLYGON` report `completed` with -`added: 0`: they need interactive surfaces (a pattern choice, a boundary selection) that -a single command line has no tokens to fill. `TEXT` no longer belongs to that list. +| Operation | `--serve` (protocol 1) | `--mcp` | +|---|---|---| +| measure | `{"protocol":1,"op":"measure","handles":["2A"]}` — kernel-computed length, area, bounds and mass properties; no window needed | `ocs_read` with `op: "measure"` and `parameters.handles` | +| capture | `{"protocol":1,"op":"capture","path":"out.png"}` — needs the editor window, so it answers `gui_required` here | `ocs_capture` with `scope` (`viewport` or `window`) and `max_dimension` | +| `set_properties` | requires `collection`; colours are the serialised enum — `{"Index": 1}` or `"ByLayer"`; a colour name such as `"Red"` is rejected with `invalid_value` | same request through `ocs_execute` | From a1629fd073b8208c5c7edb8e5634125db1f878f1 Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Sun, 20 Sep 2026 21:12:07 +0300 Subject: [PATCH 09/18] fix: closing-segment markers on closed polylines; case-insensitive page-setup names - resolve_point, set_polyline_vertex and the solver's point_for_marker treat the marker one past the last vertex of a closed polyline as vertex 0, the way line_segment already did. Equal (#1366), the axis pre-alignment (#1365) and the transform pin (#1361) address a picked segment's end that way, so on the closing segment they either refused the pick or pinned nothing. - Page-setup dictionary keys compare case-insensitively: page_setup_save replaces "site" when "Site" exists instead of adding a second entry (PSETUPIN counted it as replaced, #1344), delete and rename match the same way, and a rename that only changes case is allowed. Co-Authored-By: Claude Fable 5.1 --- src/scene/page_setup.rs | 35 ++++++++++++++++++++++++----- src/scene/parametric_constraints.rs | 27 +++++++++++++++++++--- src/scene/parametric_solve.rs | 12 ++++++++-- 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/src/scene/page_setup.rs b/src/scene/page_setup.rs index af3926e75..193264d13 100644 --- a/src/scene/page_setup.rs +++ b/src/scene/page_setup.rs @@ -175,7 +175,23 @@ impl Scene { let ObjectType::Dictionary(d) = self.document.objects.get(&dh)? else { return None; }; - d.entries.iter().find(|(k, _)| k == name).map(|(_, h)| *h) + d.entries + .iter() + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(_, h)| *h) + } + + /// The dictionary's own spelling of `name` (page-setup names compare + /// case-insensitively, like every named-object dictionary key). + fn page_setup_key(&self, name: &str) -> Option { + let dh = self.plotsettings_dict_handle()?; + let ObjectType::Dictionary(d) = self.document.objects.get(&dh)? else { + return None; + }; + d.entries + .iter() + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(k, _)| k.clone()) } /// Create or update the named page setup from `ps` (its `page_name` and @@ -183,7 +199,9 @@ impl Scene { /// are inserted and registered in the dictionary. pub fn page_setup_save(&mut self, name: &str, mut ps: PlotSettings) { let dict_handle = self.ensure_plotsettings_dict(); - ps.page_name = name.to_string(); + // Replacing keeps the entry's stored spelling so the dictionary key + // and the object's page_name stay one name. + ps.page_name = self.page_setup_key(name).unwrap_or_else(|| name.to_string()); ps.owner = dict_handle; if let Some(h) = self.page_setup_handle(name) { ps.handle = h; @@ -206,8 +224,12 @@ impl Scene { let handle = if let Some(ObjectType::Dictionary(d)) = self.document.objects.get_mut(&dict_handle) { - let h = d.entries.iter().find(|(k, _)| k == name).map(|(_, h)| *h); - d.entries.retain(|(k, _)| k != name); + let h = d + .entries + .iter() + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(_, h)| *h); + d.entries.retain(|(k, _)| !k.eq_ignore_ascii_case(name)); h } else { None @@ -223,7 +245,8 @@ impl Scene { if old == new || new.trim().is_empty() { return; } - if self.page_setup_handle(new).is_some() { + // A different spelling of the same name is a rename, not a collision. + if !old.eq_ignore_ascii_case(new) && self.page_setup_handle(new).is_some() { return; // name collision } let Some(dict_handle) = self.plotsettings_dict_handle() else { @@ -231,7 +254,7 @@ impl Scene { }; let mut renamed = None; if let Some(ObjectType::Dictionary(d)) = self.document.objects.get_mut(&dict_handle) { - if let Some(e) = d.entries.iter_mut().find(|(k, _)| k == old) { + if let Some(e) = d.entries.iter_mut().find(|(k, _)| k.eq_ignore_ascii_case(old)) { e.0 = new.to_string(); renamed = Some(e.1); } diff --git a/src/scene/parametric_constraints.rs b/src/scene/parametric_constraints.rs index 0dc086c20..0ab068550 100644 --- a/src/scene/parametric_constraints.rs +++ b/src/scene/parametric_constraints.rs @@ -244,6 +244,9 @@ pub(crate) fn set_polyline_vertex( x: f64, y: f64, ) -> bool { + let Some(index) = polyline_vertex_index(entity, index) else { + return false; + }; match entity { acadrust::EntityType::LwPolyline(polyline) => polyline .vertices @@ -800,9 +803,27 @@ pub(crate) fn resolve_point(entity: &acadrust::EntityType, marker: i32) -> Optio if marker < 0 { return None; } - super::dimension_assoc::source_points(entity) - .get(marker as usize) - .copied() + let points = super::dimension_assoc::source_points(entity); + // The closing segment of a closed polyline runs back to vertex 0, so its + // end marker (one past the last vertex) names that vertex — the same wrap + // the solver's `line_segment` applies. + let index = match polyline_vertex_index(entity, marker as usize) { + Some(index) => index, + None => marker as usize, + }; + points.get(index).copied() +} + +/// `marker` as a vertex index of a polyline, wrapping the closing segment's +/// end (one past the last vertex) onto vertex 0 when the polyline is closed. +/// `None` for anything that is not a 2D polyline. +pub(crate) fn polyline_vertex_index(entity: &acadrust::EntityType, marker: usize) -> Option { + let (count, closed) = match entity { + acadrust::EntityType::LwPolyline(polyline) => (polyline.vertices.len(), polyline.is_closed), + acadrust::EntityType::Polyline2D(polyline) => (polyline.vertices.len(), polyline.is_closed()), + _ => return None, + }; + Some(if closed && marker == count && count > 0 { 0 } else { marker }) } /// Below this squared distance (1e-6 world units), two points count as diff --git a/src/scene/parametric_solve.rs b/src/scene/parametric_solve.rs index a0c7a1792..59a5d30ca 100644 --- a/src/scene/parametric_solve.rs +++ b/src/scene/parametric_solve.rs @@ -87,8 +87,16 @@ impl EntityGeom { (EntityGeom::Line(l), 0) => Some(l.p1), (EntityGeom::Line(l), 1) => Some(l.p2), (EntityGeom::Ray(l), 0) => Some(l.p1), - (EntityGeom::Polyline { points, .. }, marker) if marker >= 0 => { - points.get(marker as usize).copied() + (EntityGeom::Polyline { points, closed, .. }, marker) if marker >= 0 => { + // The closing segment's end marker (one past the last vertex) + // is vertex 0 on a closed polyline, as in `line_segment`. + let index = marker as usize; + let index = if *closed && index == points.len() && !points.is_empty() { + 0 + } else { + index + }; + points.get(index).copied() } (EntityGeom::Circle(c), -3) => Some(c.center), (EntityGeom::Arc(a), 0) => Some(a.start), From e83c8a28a599e4f5bc3019ae1f4cedd66914dda8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 16:17:58 +0000 Subject: [PATCH 10/18] fix(trim): skip polyline segments with no plan-view shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sample_entity_xy` walks a polyline's exploded segments and drops the vertex each segment shares with the previous one. A segment whose curve plane is upright — any non-horizontal run of a 3D polyline — has no XY curve, so `entity_curve_xy` declines it and the segment samples to nothing. With nothing accumulated yet, the seam test compared the two empty ends as equal and then sliced the empty sample from index 1: panicked at src/modules/draw/modify/trim.rs:3982:42: range start index 1 out of range for slice of length 0 On the web build that panic unwinds through winit's event-loop runner and leaves its state borrowed, which is the "RefCell already borrowed" follow-on in the same report. Skip an empty sample instead, which leaves the seam handling to the segments that do have a plan-view shape. Fixes HakanSeven12/OpenCADStudio#1318 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TQzYZsXBwPZ5HwBaGitEUV (cherry picked from commit ac9a6ce91ad7ad13adf79ba5cb494fdcf0bd4071) --- src/modules/draw/modify/trim.rs | 56 +++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/src/modules/draw/modify/trim.rs b/src/modules/draw/modify/trim.rs index b18573986..31f29e3cd 100644 --- a/src/modules/draw/modify/trim.rs +++ b/src/modules/draw/modify/trim.rs @@ -3810,6 +3810,55 @@ mod tests { other => panic!("expected LwPolyline, got {other:?}"), } } + + /// #1318 repro: a 3D polyline whose first segment is not horizontal has no + /// plan-view shape, so it samples to nothing. The seam test then compared + /// two empty ends as equal and sliced the empty sample from index 1. + #[test] + fn sampling_skips_segments_with_no_plan_shape() { + use acadrust::entities::{Polyline3D, Vertex3DPolyline}; + + let mut pl = Polyline3D::new(); + pl.vertices = vec![ + // Vertical: an upright curve plane, dropped by `entity_curve_xy`. + Vertex3DPolyline::from_xyz(0.0, 0.0, 0.0), + Vertex3DPolyline::from_xyz(0.0, 0.0, 10.0), + // Horizontal, so this one does sample. + Vertex3DPolyline::from_xyz(10.0, 0.0, 10.0), + ]; + + let pts = sample_entity_xy(&EntityType::Polyline3D(pl)); + assert!( + !pts.is_empty(), + "the horizontal segment still has to be sampled" + ); + assert!( + pts.iter().all(|p| p.iter().all(|c| c.is_finite())), + "sampled points stay finite: {pts:?}" + ); + } + + /// The same skip must not swallow a leading segment that does sample, and + /// must keep dropping the duplicated seam vertex between two of them. + #[test] + fn sampling_still_joins_segments_at_their_seam() { + use acadrust::entities::{Polyline3D, Vertex3DPolyline}; + + let mut pl = Polyline3D::new(); + pl.vertices = vec![ + Vertex3DPolyline::from_xyz(0.0, 0.0, 0.0), + Vertex3DPolyline::from_xyz(10.0, 0.0, 0.0), + Vertex3DPolyline::from_xyz(10.0, 10.0, 0.0), + ]; + + let pts = sample_entity_xy(&EntityType::Polyline3D(pl)); + let seam = [10.0, 0.0]; + assert_eq!( + pts.iter().filter(|p| **p == seam).count(), + 1, + "the shared vertex appears once, not twice: {pts:?}" + ); + } } // ══════════════════════════════════════════════════════════════════════════ @@ -3978,6 +4027,13 @@ fn sample_entity_xy(e: &EntityType) -> Vec<[f64; 2]> { let mut pts: Vec<[f64; 2]> = Vec::new(); for seg in crate::modules::draw::modify::explode::explode_polyline_segments(e) { let sp = sample_entity_xy(&seg); + // A segment with no plan-view shape samples to nothing — an + // edge-on plane has no XY curve, so `entity_curve_xy` declines it. + // Skipping it here also keeps the seam test below from comparing + // two `None`s and then slicing an empty sample (#1318). + if sp.is_empty() { + continue; + } if pts.last() == sp.first() { pts.extend_from_slice(&sp[1..]); } else { From 41101efaa5fdcc83be7422d31378e7fabe9cbf0d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:45:18 +0000 Subject: [PATCH 11/18] fix(trim): skip empty preview polyline segment samples Co-authored-by: dasmatus <330471626+dasmatus@users.noreply.github.com> (cherry picked from commit fb13a7aaa6b1505d749c384cbdbbaa80bc040c4b) --- src/modules/draw/modify/trim.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/modules/draw/modify/trim.rs b/src/modules/draw/modify/trim.rs index 31f29e3cd..f6347ca6e 100644 --- a/src/modules/draw/modify/trim.rs +++ b/src/modules/draw/modify/trim.rs @@ -3859,6 +3859,23 @@ mod tests { "the shared vertex appears once, not twice: {pts:?}" ); } + + #[test] + fn preview_sampling_skips_edge_on_polyline2d_arc_segments() { + use acadrust::entities::{Polyline2D, Vertex2D}; + + let mut pl = Polyline2D::new(); + let mut start = Vertex2D::new(Vector3::new(0.0, 0.0, 0.0)); + start.bulge = 1.0; + pl.vertices = vec![start, Vertex2D::new(Vector3::new(2.0, 0.0, 0.0))]; + pl.normal = Vector3::new(1.0, 0.0, 0.0); + + let pts = preview_sample_xy(&EntityType::Polyline2D(pl)); + assert!( + pts.is_empty(), + "edge-on arc segments are skipped instead of panicking: {pts:?}" + ); + } } // ══════════════════════════════════════════════════════════════════════════ @@ -4086,6 +4103,9 @@ fn preview_sample_xy(e: &EntityType) -> Vec<[f64; 2]> { } _ => sample_entity_xy(&seg), }; + if sp.is_empty() { + continue; + } if pts.last() == sp.first() { pts.extend_from_slice(&sp[1..]); } else { From 033319a4e3c3423cfcf8c837db433a8e0e7c99b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 16:26:07 +0000 Subject: [PATCH 12/18] feat(otrack): draw both vectors of an intersection lock An OTRACK crossing is the meeting of two tracking vectors, but `OtrackHit` carried only the one a typed distance is measured along, so the overlay had a single guide to draw. Snapping to the crossing of two vectors worked and looked like an unexplained point on one line. Carry the crossed ray out of `otrack_snap` as `OtrackHit::cross`, keep it on the app beside `otrack_active`, and let the overlay take a list of guides rather than one. `otrack_active` still drives typed-distance entry on its own, so distance entry is unchanged. Fixes HakanSeven12/OpenCADStudio#1313 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TQzYZsXBwPZ5HwBaGitEUV (cherry picked from commit 2f6caacece2522904b3fd899333b708eba3ef3bb) --- src/app/command_driver.rs | 1 + src/app/mod.rs | 6 ++ src/app/update/mod.rs | 3 + src/app/update/viewport.rs | 3 + src/app/view/mod.rs | 27 ++++--- src/snap.rs | 149 +++++++++++++++++++++++++++++++++++-- src/ui/overlay.rs | 17 +++-- 7 files changed, 183 insertions(+), 23 deletions(-) diff --git a/src/app/command_driver.rs b/src/app/command_driver.rs index 4e0f19ef0..0e21a8e6e 100644 --- a/src/app/command_driver.rs +++ b/src/app/command_driver.rs @@ -104,6 +104,7 @@ impl OpenCADStudio { self.snapper.from_point = None; self.snapper.clear_tracking(); self.otrack_active = None; + self.otrack_cross = None; self.otrack_kind = None; self.axis_lock_dir = None; self.dyn_user_reshaped = false; diff --git a/src/app/mod.rs b/src/app/mod.rs index 0d2a683e9..fd1c73551 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -502,6 +502,11 @@ pub(super) struct OpenCADStudio { /// cursor is on a tracking ray. Lets a typed distance place a point along /// the ray from the tracking point (issue #69). `None` when not aligned. otrack_active: Option<(glam::DVec3, glam::DVec3)>, + /// The vector `otrack_active` crosses, when the lock is the meeting of two + /// tracking vectors. Drawn beside the first so the user can see that the + /// point is their intersection; a typed distance still runs along + /// `otrack_active` alone. `None` for a single-ray alignment. (#1313) + otrack_cross: Option<(glam::DVec3, glam::DVec3)>, /// Active OTRACK ray kind, separate from typed-distance geometry. otrack_kind: Option, /// Whether Tangent snap was enabled before a tangent-pick command started. @@ -3868,6 +3873,7 @@ impl OpenCADStudio { annotation_auto_scale: -4, last_saved_config: None, otrack_active: None, + otrack_cross: None, otrack_kind: None, clean_screen: false, quick_properties: false, diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 3e9408c9e..e93a218a8 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -366,6 +366,7 @@ impl OpenCADStudio { { self.snapper.clear_tracking(); self.otrack_active = None; + self.otrack_cross = None; self.otrack_kind = None; } if let Some((started, tab_id, before)) = perf_edit_before { @@ -402,6 +403,7 @@ impl OpenCADStudio { pub(in crate::app) fn reset_tracking_after_point(&mut self) { self.snapper.clear_tracking(); self.otrack_active = None; + self.otrack_cross = None; self.otrack_kind = None; } @@ -3982,6 +3984,7 @@ impl OpenCADStudio { if !self.snapper.otrack_enabled { self.snapper.clear_tracking(); self.otrack_active = None; + self.otrack_cross = None; self.otrack_kind = None; } Task::none() diff --git a/src/app/update/viewport.rs b/src/app/update/viewport.rs index f316976fd..c5279e5a8 100644 --- a/src/app/update/viewport.rs +++ b/src/app/update/viewport.rs @@ -1479,6 +1479,7 @@ impl OpenCADStudio { Some((base, dir)) }); self.otrack_active = otrack_hit.map(|hit| (hit.base, hit.dir)).or(drafting_guide); + self.otrack_cross = otrack_hit.and_then(|hit| hit.cross); self.otrack_kind = otrack_hit.map(|hit| hit.kind); @@ -2190,6 +2191,7 @@ impl OpenCADStudio { None }; self.otrack_active = otrack_hit.map(|h| (h.base, h.dir)); + self.otrack_cross = otrack_hit.and_then(|h| h.cross); self.otrack_kind = otrack_hit.map(|h| h.kind); // Parallel snap: with nothing else snapped or tracked, lock @@ -2208,6 +2210,7 @@ impl OpenCADStudio { (self.last_point, self.snapper.parallel_ref) { self.otrack_active = Some((base, dir.as_dvec3())); + self.otrack_cross = None; } self.tabs[i].snap_result = Some(par); } diff --git a/src/app/view/mod.rs b/src/app/view/mod.rs index 52d4e18cb..65a74c0df 100644 --- a/src/app/view/mod.rs +++ b/src/app/view/mod.rs @@ -770,15 +770,22 @@ bg={bg_ms:.1}ms n={view_count}" // The active alignment vector: a dashed guide from the acquired // tracking point through the locked cursor, so the user sees the // extension / tracking line they are snapped to (#219). - let otrack_line: Option<(iced::Point, iced::Point)> = - match (otrack_proj, self.otrack_active) { - (Some((view_rot, eye, ob)), Some((base, _dir))) => { - let b = ost_project(base, view_rot, eye, ob); - let a = ost_project(tab.last_cursor_world, view_rot, eye, ob); - (b.x.is_finite() && a.x.is_finite()).then_some((b, a)) - } - _ => None, - }; + // An intersection lock is the meeting of two tracking vectors, so + // both are drawn — one guide alone hides what the point is (#1313). + let otrack_lines: Vec<(iced::Point, iced::Point)> = match otrack_proj { + Some((view_rot, eye, ob)) => { + let a = ost_project(tab.last_cursor_world, view_rot, eye, ob); + self.otrack_active + .into_iter() + .chain(self.otrack_cross) + .filter_map(|(base, _dir)| { + let b = ost_project(base, view_rot, eye, ob); + (b.x.is_finite() && a.x.is_finite()).then_some((b, a)) + }) + .collect() + } + None => vec![], + }; // The acquired Parallel-snap reference, marked on its line (#277). let parallel_ref_marker: Option = match (otrack_proj, self.snapper.parallel_ref) { @@ -894,7 +901,7 @@ bg={bg_ms:.1}ms n={view_count}" grip_clip, ucs_icons, ost_points, - otrack_line, + otrack_lines, parallel_ref_marker, // ViewCube hover region matches the drawn cube — gone when hidden. !is_paper && viewcube_visible, diff --git a/src/snap.rs b/src/snap.rs index 05e9055fb..9afe2ad9d 100644 --- a/src/snap.rs +++ b/src/snap.rs @@ -177,6 +177,12 @@ pub struct OtrackHit { pub dir: DVec3, /// The tracking point the ray emanates from. pub base: DVec3, + /// The second ray of an intersection lock, as `(base, outward direction)`. + /// A crossing is the meeting of two tracking vectors, and the user has to + /// see both of them to read what the lock means; `base`/`dir` above carry + /// only the one a typed distance is measured along. `None` for a + /// single-ray alignment, which has no second vector. (#1313) + pub cross: Option<(DVec3, DVec3)>, pub kind: TrackingKind, } @@ -917,19 +923,35 @@ impl Snapper { if sd < r && best_x.as_ref().map_or(true, |(bd, _)| sd < *bd) { // Report an acquired tracking ray (not an auxiliary // last_point ray) as base/dir for typed-distance entry. - let ot = if rays[i].group != POLAR_GROUP && rays[i].group != ORTHO_GROUP { - &rays[i] + let (ot, other) = if rays[i].group != POLAR_GROUP + && rays[i].group != ORTHO_GROUP + { + (&rays[i], &rays[j]) } else { - &rays[j] + (&rays[j], &rays[i]) }; - let t = (x.x - ot.origin.x) * ot.dir.x + (x.y - ot.origin.y) * ot.dir.y; - let dir_out = if t >= 0.0 { ot.dir } else { -ot.dir }; + // Point each ray the way the crossing lies from its own + // origin, so the guide drawn for it runs through the lock + // rather than away from it. + let outward = |ray: &Ray| { + let t = (x.x - ray.origin.x) * ray.dir.x + + (x.y - ray.origin.y) * ray.dir.y; + if t >= 0.0 { + ray.dir + } else { + -ray.dir + } + }; + let dir_out = outward(ot); best_x = Some(( sd, OtrackHit { aligned: x, dir: dir_out, base: ot.origin, + // The vector the reported one crosses. Both are + // drawn, so the intersection reads as one. (#1313) + cross: Some((other.origin, outward(other))), kind: ot.kind, } )); @@ -970,6 +992,8 @@ impl Snapper { aligned, dir: dir_out, base: ray.origin, + // A single-ray alignment has no second vector. + cross: None, kind: ray.kind, }, )); @@ -3660,6 +3684,121 @@ mod ext_tests { assert!(none.is_none(), "no base point → no base→corner alignment"); } + /// #1313: an intersection lock must report both of the vectors it is the + /// crossing of, so the overlay can draw both. Reporting only the one a + /// typed distance runs along leaves the user with a single guide and no + /// sign of what the point actually is. + #[test] + fn intersection_lock_reports_both_crossing_vectors() { + let mut s = Snapper::default(); + s.otrack_enabled = true; + s.osnap_radius_px = 10.0; + // Two acquired corners. With no polar step each offers a horizontal and + // a vertical ray, so their rays cross at (10, 0) and at (0, 5). + let first = DVec3::new(0.0, 0.0, 0.0); + let second = DVec3::new(10.0, 5.0, 0.0); + for corner in [first, second] { + s.tracking_points.push(corner); + s.tracking_dirs.push(Vec::new()); + } + + let view_rot = Mat4::from_scale(Vec3::splat(0.0001)); + let eye = glam::DVec3::ZERO; + let bounds = Rectangle { + x: 0.0, + y: 0.0, + width: 1000.0, + height: 1000.0, + }; + + // Cursor a hair off the crossing of the first corner's horizontal ray + // and the second corner's vertical one. + let crossing = DVec3::new(10.0, 0.0, 0.0); + let hit = s + .otrack_snap( + crossing + DVec3::new(0.02, 0.02, 0.0), + view_rot, + eye, + bounds, + None, + None, + None, + false, + DVec3::X, + DVec3::Y, + ) + .expect("the two rays cross inside the aperture"); + assert!( + (hit.aligned - crossing).length() < 1e-9, + "locked off the crossing: {:?}", + hit.aligned + ); + + let (cross_base, cross_dir) = hit.cross.expect("a crossing reports its second vector"); + let bases = [hit.base, cross_base]; + for corner in [first, second] { + assert!( + bases.iter().any(|b| (*b - corner).length() < 1e-9), + "{corner:?} is not one of the two reported vectors: {bases:?}" + ); + } + + // Each vector runs from its own corner through the crossing, pointing + // at it — the guides are drawn along these. + for (base, dir) in [(hit.base, hit.dir), (cross_base, cross_dir)] { + let off = crossing - base; + assert!( + (off.x * dir.y - off.y * dir.x).abs() < 1e-9, + "the crossing is off the vector from {base:?} along {dir:?}" + ); + assert!( + off.dot(dir) > 0.0, + "vector from {base:?} points away from the crossing" + ); + } + } + + /// The second vector belongs to a crossing alone: a plain single-ray + /// alignment has nothing to cross, and must not draw a second guide. + #[test] + fn single_ray_alignment_reports_no_crossing_vector() { + let mut s = Snapper::default(); + s.otrack_enabled = true; + s.osnap_radius_px = 10.0; + let corner = DVec3::new(10.0, 5.0, 0.0); + s.tracking_points.push(corner); + s.tracking_dirs.push(Vec::new()); + + let view_rot = Mat4::from_scale(Vec3::splat(0.0001)); + let eye = glam::DVec3::ZERO; + let bounds = Rectangle { + x: 0.0, + y: 0.0, + width: 1000.0, + height: 1000.0, + }; + + let hit = s + .otrack_snap( + DVec3::new(60.0, 5.02, 0.0), + view_rot, + eye, + bounds, + None, + None, + None, + false, + DVec3::X, + DVec3::Y, + ) + .expect("the corner's horizontal ray catches the cursor"); + assert!( + hit.cross.is_none(), + "a single-ray alignment reported a crossing vector: {:?}", + hit.cross + ); + } + #[test] fn tangent_points_are_perpendicular_to_the_radius() { let c = Vec3::new(0.0, 0.0, 0.0); diff --git a/src/ui/overlay.rs b/src/ui/overlay.rs index a1c7f2e02..74a828a09 100644 --- a/src/ui/overlay.rs +++ b/src/ui/overlay.rs @@ -962,7 +962,7 @@ pub fn selection_overlay<'a>( grip_clip: Option, ucs_icons: Vec, ost_points: Vec, - otrack_line: Option<(Point, Point)>, + otrack_lines: Vec<(Point, Point)>, parallel_ref_marker: Option, show_viewcube: bool, dividers: Vec, @@ -988,7 +988,7 @@ pub fn selection_overlay<'a>( grip_clip, ucs_icons, ost_points, - otrack_line, + otrack_lines, parallel_ref_marker, show_viewcube, dividers, @@ -1029,11 +1029,12 @@ struct SelectionCanvas { /// entry carries hover/selected (grips). ucs_icons: Vec, ost_points: Vec, - /// Active OTRACK alignment: (acquired tracking point, locked cursor), both - /// in screen space. Drawn as a dashed guide extended a little past the - /// cursor so the extension / tracking line the user snapped to is visible. - /// (#219) - otrack_line: Option<(Point, Point)>, + /// Active OTRACK alignments, each (acquired tracking point, locked cursor) + /// in screen space. Drawn as dashed guides extended a little past the + /// cursor so the extension / tracking line the user snapped to is visible + /// (#219). An intersection lock contributes both of its crossing vectors, + /// so the point reads as their meeting rather than a lone guide (#1313). + otrack_lines: Vec<(Point, Point)>, /// The acquired Parallel-snap reference point (screen), marked with a small /// ∥ glyph so the user sees which line is the parallel reference. (#277) parallel_ref_marker: Option, @@ -2013,7 +2014,7 @@ impl canvas::Program for SelectionCanvas { // real angle from the acquired point through the lock and a little // beyond, dashed so it reads as a construction guide. This covers the // ortho (0°/90°), polar, and edge-extension cases uniformly (#219). - if let Some((base, tip)) = self.otrack_line { + for &(base, tip) in &self.otrack_lines { let dx = tip.x - base.x; let dy = tip.y - base.y; let len = (dx * dx + dy * dy).sqrt(); From 5d4b70721483de0a480b107302a3629e5e4440b8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:36:47 +0000 Subject: [PATCH 13/18] fix review feedback for OTRACK projection and OtrackHit API Co-authored-by: dichhead <330471626+dichhead@users.noreply.github.com> (cherry picked from commit 97f94a2bfe6fe58354d6713b78a00de688f12745) --- src/app/view/mod.rs | 6 +++++- src/snap.rs | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/app/view/mod.rs b/src/app/view/mod.rs index 65a74c0df..dc6679743 100644 --- a/src/app/view/mod.rs +++ b/src/app/view/mod.rs @@ -780,7 +780,11 @@ bg={bg_ms:.1}ms n={view_count}" .chain(self.otrack_cross) .filter_map(|(base, _dir)| { let b = ost_project(base, view_rot, eye, ob); - (b.x.is_finite() && a.x.is_finite()).then_some((b, a)) + (b.x.is_finite() + && b.y.is_finite() + && a.x.is_finite() + && a.y.is_finite()) + .then_some((b, a)) }) .collect() } diff --git a/src/snap.rs b/src/snap.rs index 9afe2ad9d..7b13f8ab1 100644 --- a/src/snap.rs +++ b/src/snap.rs @@ -170,6 +170,7 @@ pub(crate) fn wire_source(wire: &WireModel) -> Option Date: Thu, 17 Sep 2026 16:31:03 +0000 Subject: [PATCH 14/18] feat(insert): grip each block attribute Selecting a block showed one grip, at its insertion point, so there was no handle to pick an attribute up by: an attribute could only be moved by moving the whole block. Give the INSERT one grip per attribute, taken from the attribute's own `Grippable` implementation, at ids `1 + index`. A drag on one moves that attribute alone, through the same `apply_grip` the attribute already has (which carries `alignment_point` along, so non-left alignments stay put relative to the text). Grip 0 keeps translating the block and every attribute with it (#255). A constant attribute belongs to the block definition and an invisible one is not drawn, so neither offers a grip; a locked-position attribute declines on its own. Fixes HakanSeven12/OpenCADStudio#1259 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TQzYZsXBwPZ5HwBaGitEUV (cherry picked from commit 7eb966f6a2e65d57653ac06e9472a11a349b0614) --- src/entities/insert.rs | 121 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 2 deletions(-) diff --git a/src/entities/insert.rs b/src/entities/insert.rs index 37f7dcd7f..04157fae7 100644 --- a/src/entities/insert.rs +++ b/src/entities/insert.rs @@ -5,6 +5,7 @@ use glam::Vec3; use crate::command::EntityTransform; use crate::entities::common::{edit_angle_prop as edit_angle, edit_prop as edit, parse_f64, ro_prop as ro, square_grip}; +use crate::entities::traits::Grippable; use crate::scene::model::object::{GripApply, GripDef, PropSection, PropValue, Property}; use crate::scene::model::wire_model::WireModel; @@ -12,11 +13,38 @@ use crate::scene::cache::block_cache; use crate::scene::convert::tessellate; use crate::scene::view::render; +/// Grip ids from here up address the INSERT's attributes — `BASE + i` is +/// `attributes[i]`. Grip 0 stays the block's own insertion point. +const ATTRIBUTE_GRIP_BASE: usize = 1; + +/// Whether an attribute is one the user can pick up and reposition. A constant +/// attribute belongs to the block definition rather than this insert, and an +/// invisible one is not drawn, so neither has a grip to offer. The attribute +/// itself declines a locked position. +fn attribute_is_movable(att: &acadrust::entities::AttributeEntity) -> bool { + !att.flags.constant && !att.flags.invisible +} + fn grips(ins: &Insert) -> Vec { // `insert_point` is in the OCS defined by `normal`; the grip must sit at // the world placement, so map it through the OCS. Identity for +Z. let w = Matrix3::arbitrary_axis(ins.normal) * ins.insert_point; - vec![square_grip(0, glam::DVec3::new(w.x, w.y, w.z))] + let mut out = vec![square_grip(0, glam::DVec3::new(w.x, w.y, w.z))]; + // One grip per attribute, so an attribute can be moved off the place the + // block definition put it without dragging the whole block (#1259). + // Attributes are stored beside the block in world space — the same space + // `apply_grip` already translates them in. + out.extend(ins.attributes.iter().enumerate().filter_map(|(i, att)| { + if !attribute_is_movable(att) { + return None; + } + let grip = Grippable::grips(att).into_iter().next()?; + Some(GripDef { + id: ATTRIBUTE_GRIP_BASE + i, + ..grip + }) + })); + out } fn properties(ins: &Insert) -> Vec { @@ -108,7 +136,14 @@ fn apply_geom_prop(ins: &mut Insert, field: &str, value: &str) { } } -fn apply_grip(ins: &mut Insert, _grip_id: usize, apply: GripApply) { +fn apply_grip(ins: &mut Insert, grip_id: usize, apply: GripApply) { + // An attribute grip moves that attribute alone; the block stays put. + if let Some(index) = grip_id.checked_sub(ATTRIBUTE_GRIP_BASE) { + if let Some(att) = ins.attributes.get_mut(index).filter(|a| attribute_is_movable(a)) { + Grippable::apply_grip(att, 0, apply); + } + return; + } // The grip works in world space, but `insert_point` is stored in the OCS // defined by `normal`. Round-trip through the OCS so dragging a block // whose extrusion direction isn't +Z moves along world axes. Identity OCS @@ -328,3 +363,85 @@ pub(crate) fn append_insert_attribute_wires( } } } + +#[cfg(test)] +mod tests { + use super::*; + use acadrust::entities::AttributeEntity; + + fn attribute(tag: &str, x: f64, y: f64) -> AttributeEntity { + let mut att = AttributeEntity::new(tag.to_string(), format!("{tag}-value")); + att.insertion_point = Vector3::new(x, y, 0.0); + att.alignment_point = Vector3::new(x, y, 0.0); + att + } + + fn block_with(attributes: Vec) -> Insert { + let mut ins = Insert::new("BLOCK", Vector3::new(0.0, 0.0, 0.0)); + ins.attributes = attributes; + ins + } + + /// #1259: selecting a block showed only the insertion grip, so there was no + /// handle to pick an attribute up by and nowhere to drop it. + #[test] + fn each_attribute_offers_its_own_grip() { + let ins = block_with(vec![attribute("TAG_A", 3.0, 4.0), attribute("TAG_B", 6.0, 8.0)]); + + let g = grips(&ins); + assert_eq!(g.len(), 3, "insertion point plus one grip per attribute"); + assert_eq!(g[0].world, glam::DVec3::new(0.0, 0.0, 0.0)); + assert_eq!(g[1].world, glam::DVec3::new(3.0, 4.0, 0.0)); + assert_eq!(g[2].world, glam::DVec3::new(6.0, 8.0, 0.0)); + } + + /// Dragging an attribute grip moves that attribute and nothing else — the + /// block and its other attributes stay where they were. + #[test] + fn attribute_grip_moves_only_that_attribute() { + let mut ins = block_with(vec![attribute("TAG_A", 3.0, 4.0), attribute("TAG_B", 6.0, 8.0)]); + + let moved = grips(&ins)[1].id; + apply_grip(&mut ins, moved, GripApply::Absolute(glam::DVec3::new(30.0, 40.0, 0.0))); + + assert_eq!(ins.insert_point, Vector3::new(0.0, 0.0, 0.0)); + assert_eq!(ins.attributes[0].insertion_point, Vector3::new(30.0, 40.0, 0.0)); + assert_eq!(ins.attributes[0].alignment_point, Vector3::new(30.0, 40.0, 0.0)); + assert_eq!(ins.attributes[1].insertion_point, Vector3::new(6.0, 8.0, 0.0)); + } + + /// The insertion grip keeps carrying the attributes along with the block, + /// which is what the attribute grips are an alternative to, not a + /// replacement for (#255). + #[test] + fn insertion_grip_still_carries_the_attributes() { + let mut ins = block_with(vec![attribute("TAG_A", 3.0, 4.0)]); + + apply_grip(&mut ins, 0, GripApply::Translate(glam::DVec3::new(10.0, 0.0, 0.0))); + + assert_eq!(ins.insert_point, Vector3::new(10.0, 0.0, 0.0)); + assert_eq!(ins.attributes[0].insertion_point, Vector3::new(13.0, 4.0, 0.0)); + } + + /// A constant attribute belongs to the block definition and an invisible + /// one is not drawn, so neither gets a grip — and a grip id must still + /// address the attribute it was built from once some are skipped. + #[test] + fn skipped_attributes_neither_grip_nor_shift_the_others() { + let mut constant = attribute("TAG_CONST", 1.0, 1.0); + constant.flags.constant = true; + let mut invisible = attribute("TAG_HIDDEN", 2.0, 2.0); + invisible.flags.invisible = true; + let mut locked = attribute("TAG_LOCKED", 4.0, 4.0); + locked.lock_position = true; + let mut ins = block_with(vec![constant, invisible, locked, attribute("TAG_OK", 5.0, 5.0)]); + + let g = grips(&ins); + assert_eq!(g.len(), 2, "only the movable attribute is gripped: {g:?}"); + assert_eq!(g[1].world, glam::DVec3::new(5.0, 5.0, 0.0)); + + apply_grip(&mut ins, g[1].id, GripApply::Absolute(glam::DVec3::new(50.0, 50.0, 0.0))); + assert_eq!(ins.attributes[3].insertion_point, Vector3::new(50.0, 50.0, 0.0)); + assert_eq!(ins.attributes[0].insertion_point, Vector3::new(1.0, 1.0, 0.0)); + } +} From bd89507a6cc60f1da863b777ac44acdcc4427a7a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:43:32 +0000 Subject: [PATCH 15/18] fix insert attribute grip visibility Co-authored-by: dasmatus <330471626+dasmatus@users.noreply.github.com> (cherry picked from commit 68cc414e4a98373821c8a7e7fe63fb4b18e27793) --- src/app/properties.rs | 7 ++ src/entities/insert.rs | 194 +++++++++++++++++++++++++++++++++-------- 2 files changed, 163 insertions(+), 38 deletions(-) diff --git a/src/app/properties.rs b/src/app/properties.rs index c0ed6ce64..b96b37e04 100644 --- a/src/app/properties.rs +++ b/src/app/properties.rs @@ -2683,6 +2683,13 @@ handles={handles_ms:.1} panel={:.1} ribbon={ribbon_ms:.1} tail={:.1} selected={} annotation_scale_handle, ); let mut entity_grips = dispatch::grips(contextual.as_ref()); + if let acadrust::EntityType::Insert(insert) = contextual.as_ref() { + entity_grips = crate::entities::insert::visible_attribute_grips( + &self.tabs[i].scene.document, + insert, + self.tabs[i].scene.annotation_scale, + ); + } if crate::scene::model::solid_history::has_specialized_primitive_properties( &self.tabs[i].scene.document, handle, diff --git a/src/entities/insert.rs b/src/entities/insert.rs index 04157fae7..59873c3b0 100644 --- a/src/entities/insert.rs +++ b/src/entities/insert.rs @@ -1,16 +1,18 @@ use acadrust::entities::Insert; use acadrust::types::{Matrix3, Transform, Vector3}; -use acadrust::{EntityType, Handle}; +use acadrust::{CadDocument, EntityType, Handle}; use glam::Vec3; use crate::command::EntityTransform; -use crate::entities::common::{edit_angle_prop as edit_angle, edit_prop as edit, parse_f64, ro_prop as ro, square_grip}; +use crate::entities::common::{ + edit_angle_prop as edit_angle, edit_prop as edit, parse_f64, ro_prop as ro, square_grip, +}; use crate::entities::traits::Grippable; -use crate::scene::model::object::{GripApply, GripDef, PropSection, PropValue, Property}; -use crate::scene::model::wire_model::WireModel; use crate::scene::cache::block_cache; use crate::scene::convert::tessellate; +use crate::scene::model::object::{GripApply, GripDef, PropSection, PropValue, Property}; +use crate::scene::model::wire_model::WireModel; use crate::scene::view::render; /// Grip ids from here up address the INSERT's attributes — `BASE + i` is @@ -25,6 +27,59 @@ fn attribute_is_movable(att: &acadrust::entities::AttributeEntity) -> bool { !att.flags.constant && !att.flags.invisible } +fn attribute_is_render_visible( + document: &CadDocument, + visibility: i16, + attribute: &acadrust::entities::AttributeEntity, +) -> bool { + let layer_visible = document + .layers + .get(&attribute.common.layer) + .is_none_or(|layer| !layer.flags.off && !layer.flags.frozen); + layer_visible + && visibility != 0 + && (visibility == 2 || (!attribute.common.invisible && !attribute.flags.invisible)) +} + +fn insert_attribute_block_scale( + insert: &Insert, + annotation_scale: f32, + scale_policy: crate::scene::BlockScalePolicy, +) -> f64 { + if scale_policy == crate::scene::BlockScalePolicy::FromInsert + && insert + .common + .extended_data + .get_record("AcAnnotativeData") + .is_some() + { + annotation_scale as f64 + } else { + 1.0 + } +} + +fn scale_attribute_for_insert( + insert: &Insert, + attribute: &mut acadrust::entities::AttributeEntity, + block_scale: f64, +) { + if (block_scale - 1.0).abs() <= 1.0e-6 { + return; + } + let insertion = insert.insert_point; + let scale_about = |point: Vector3| { + Vector3::new( + insertion.x + (point.x - insertion.x) * block_scale, + insertion.y + (point.y - insertion.y) * block_scale, + insertion.z + (point.z - insertion.z) * block_scale, + ) + }; + attribute.height *= block_scale; + attribute.insertion_point = scale_about(attribute.insertion_point); + attribute.alignment_point = scale_about(attribute.alignment_point); +} + fn grips(ins: &Insert) -> Vec { // `insert_point` is in the OCS defined by `normal`; the grip must sit at // the world placement, so map it through the OCS. Identity for +Z. @@ -47,6 +102,42 @@ fn grips(ins: &Insert) -> Vec { out } +pub(crate) fn visible_attribute_grips( + document: &CadDocument, + insert: &Insert, + annotation_scale: f32, +) -> Vec { + let w = Matrix3::arbitrary_axis(insert.normal) * insert.insert_point; + let mut out = vec![square_grip(0, glam::DVec3::new(w.x, w.y, w.z))]; + let visibility = document.header.attribute_visibility; + let block_scale = insert_attribute_block_scale( + insert, + annotation_scale, + crate::scene::BlockScalePolicy::FromInsert, + ); + out.extend( + insert + .attributes + .iter() + .enumerate() + .filter_map(|(i, attribute)| { + if !attribute_is_movable(attribute) + || !attribute_is_render_visible(document, visibility, attribute) + { + return None; + } + let mut attribute = attribute.clone(); + scale_attribute_for_insert(insert, &mut attribute, block_scale); + let grip = Grippable::grips(&attribute).into_iter().next()?; + Some(GripDef { + id: ATTRIBUTE_GRIP_BASE + i, + ..grip + }) + }), + ); + out +} + fn properties(ins: &Insert) -> Vec { let annotative = ins .common @@ -230,45 +321,14 @@ pub(crate) fn insert_attribute_entities( if visibility == 0 || insert.attributes.is_empty() { return Vec::new(); } - let block_scale = if scale_policy - == crate::scene::BlockScalePolicy::FromInsert - && insert - .common - .extended_data - .get_record("AcAnnotativeData") - .is_some() - { - annotation_scale as f64 - } else { - 1.0 - }; - let insertion = insert.insert_point; - let scale_about = |point: Vector3| { - Vector3::new( - insertion.x + (point.x - insertion.x) * block_scale, - insertion.y + (point.y - insertion.y) * block_scale, - insertion.z + (point.z - insertion.z) * block_scale, - ) - }; + let block_scale = insert_attribute_block_scale(insert, annotation_scale, scale_policy); insert .attributes .iter() - .filter(|attribute| { - let layer_visible = document - .layers - .get(&attribute.common.layer) - .is_none_or(|layer| !layer.flags.off && !layer.flags.frozen); - layer_visible - && (visibility == 2 - || (!attribute.common.invisible && !attribute.flags.invisible)) - }) + .filter(|attribute| attribute_is_render_visible(document, visibility, attribute)) .map(|attribute| { let mut attribute = attribute.clone(); - if (block_scale - 1.0).abs() > 1.0e-6 { - attribute.height *= block_scale; - attribute.insertion_point = scale_about(attribute.insertion_point); - attribute.alignment_point = scale_about(attribute.alignment_point); - } + scale_attribute_for_insert(insert, &mut attribute, block_scale); EntityType::AttributeEntity(attribute) }) .collect() @@ -368,6 +428,8 @@ pub(crate) fn append_insert_attribute_wires( mod tests { use super::*; use acadrust::entities::AttributeEntity; + use acadrust::tables::Layer; + use acadrust::xdata::ExtendedDataRecord; fn attribute(tag: &str, x: f64, y: f64) -> AttributeEntity { let mut att = AttributeEntity::new(tag.to_string(), format!("{tag}-value")); @@ -382,6 +444,14 @@ mod tests { ins } + fn document_with_layers(layers: Vec) -> CadDocument { + let mut document = CadDocument::new(); + for layer in layers { + document.layers.add_or_replace(layer); + } + document + } + /// #1259: selecting a block showed only the insertion grip, so there was no /// handle to pick an attribute up by and nowhere to drop it. #[test] @@ -444,4 +514,52 @@ mod tests { assert_eq!(ins.attributes[3].insertion_point, Vector3::new(50.0, 50.0, 0.0)); assert_eq!(ins.attributes[0].insertion_point, Vector3::new(1.0, 1.0, 0.0)); } + + #[test] + fn visible_attribute_grips_match_render_visibility() { + let mut off_layer = Layer::new("OFF"); + off_layer.flags.off = true; + let mut frozen_layer = Layer::new("FROZEN"); + frozen_layer.flags.frozen = true; + let mut document = document_with_layers(vec![off_layer, frozen_layer]); + document.header.attribute_visibility = 1; + + let mut common_invisible = attribute("TAG_COMMON_INVISIBLE", 1.0, 1.0); + common_invisible.common.invisible = true; + let mut off = attribute("TAG_OFF", 2.0, 2.0); + off.common.layer = "OFF".into(); + let mut frozen = attribute("TAG_FROZEN", 3.0, 3.0); + frozen.common.layer = "FROZEN".into(); + let visible = attribute("TAG_VISIBLE", 4.0, 4.0); + let ins = block_with(vec![common_invisible, off, frozen, visible]); + + let grips = visible_attribute_grips(&document, &ins, 1.0); + + assert_eq!(grips.len(), 2, "base grip plus only rendered attributes"); + assert_eq!(grips[1].id, ATTRIBUTE_GRIP_BASE + 3); + assert_eq!(grips[1].world, glam::DVec3::new(4.0, 4.0, 0.0)); + + document.header.attribute_visibility = 0; + assert_eq!( + visible_attribute_grips(&document, &ins, 1.0).len(), + 1, + "ATTMODE=0 should hide every attribute grip", + ); + } + + #[test] + fn visible_attribute_grips_scale_annotative_attributes() { + let document = CadDocument::new(); + let mut insert = block_with(vec![attribute("TAG", 11.0, 10.0)]); + insert.insert_point = Vector3::new(10.0, 10.0, 0.0); + insert + .common + .extended_data + .add_record(ExtendedDataRecord::new("AcAnnotativeData")); + + let grips = visible_attribute_grips(&document, &insert, 2.0); + + assert_eq!(grips.len(), 2); + assert_eq!(grips[1].world, glam::DVec3::new(12.0, 10.0, 0.0)); + } } From 4559c8a2d8c32f9f8c93e804085a9dcdc0c5a895 Mon Sep 17 00:00:00 2001 From: dasmatus Date: Sat, 19 Sep 2026 17:51:17 +0200 Subject: [PATCH 16/18] Refactor attribute filtering logic in insert.rs Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> (cherry picked from commit a8f75e969d4737d9f331c9356714d7393ca044f7) --- src/entities/insert.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/entities/insert.rs b/src/entities/insert.rs index 59873c3b0..b35209a1a 100644 --- a/src/entities/insert.rs +++ b/src/entities/insert.rs @@ -121,9 +121,10 @@ pub(crate) fn visible_attribute_grips( .iter() .enumerate() .filter_map(|(i, attribute)| { - if !attribute_is_movable(attribute) + if attribute.flags.constant + || attribute.lock_position + || attribute.flags.locked_position || !attribute_is_render_visible(document, visibility, attribute) - { return None; } let mut attribute = attribute.clone(); From 2b92fc5b0901ff70de4e6bf49c8e734c59f327fa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:57:57 +0000 Subject: [PATCH 17/18] Fix missing brace in insert attribute visibility filter Co-authored-by: dasmatus <330471626+dasmatus@users.noreply.github.com> (cherry picked from commit 6cf16d8621e236cee8623227b6853352a3ec48ae) --- src/entities/insert.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/entities/insert.rs b/src/entities/insert.rs index b35209a1a..343df25d3 100644 --- a/src/entities/insert.rs +++ b/src/entities/insert.rs @@ -125,6 +125,7 @@ pub(crate) fn visible_attribute_grips( || attribute.lock_position || attribute.flags.locked_position || !attribute_is_render_visible(document, visibility, attribute) + { return None; } let mut attribute = attribute.clone(); From fd2062bdec95780d0d7d58356397a44c0c740a5b Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Sun, 20 Sep 2026 21:31:54 +0300 Subject: [PATCH 18/18] fix(insert): attribute grips follow ATTMODE 2 and annotative scale Follow-up to the block-attribute grips picked from #1386. - Movability no longer includes the invisible flag: an attribute ATTMODE 2 displays is grippable, so the grip the display-aware list shows can be dragged. The document-less grips() still skips invisible attributes, since it cannot consult ATTMODE. - The grip sits on the displayed attribute, which an annotative insert scales about its insertion point; Scene::apply_grip now unscales the dragged target the same way before the attribute stores it, so the grip no longer jumps to twice the drag at scale 2. Co-Authored-By: Claude Fable 5.1 --- src/entities/insert.rs | 45 ++++++++++++++++++++++++++++++++++-------- src/scene/modify.rs | 11 +++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/src/entities/insert.rs b/src/entities/insert.rs index 343df25d3..5fe92b024 100644 --- a/src/entities/insert.rs +++ b/src/entities/insert.rs @@ -20,11 +20,39 @@ use crate::scene::view::render; const ATTRIBUTE_GRIP_BASE: usize = 1; /// Whether an attribute is one the user can pick up and reposition. A constant -/// attribute belongs to the block definition rather than this insert, and an -/// invisible one is not drawn, so neither has a grip to offer. The attribute -/// itself declines a locked position. +/// attribute belongs to the block definition rather than this insert, and a +/// locked position cannot be dragged. Visibility is a separate question: an +/// invisible attribute that ATTMODE 2 displays is grippable while it shows. fn attribute_is_movable(att: &acadrust::entities::AttributeEntity) -> bool { - !att.flags.constant && !att.flags.invisible + !att.flags.constant && !att.lock_position && !att.flags.locked_position +} + +/// A dragged attribute grip, mapped back to the attribute's stored position. +/// The grip is placed on the displayed attribute, which an annotative insert +/// scales about its insertion point; the target has to be unscaled the same +/// way or the attribute lands `scale` times too far and the grip jumps. +pub(crate) fn unscale_attribute_grip( + insert: &Insert, + annotation_scale: f32, + grip_id: usize, + apply: GripApply, +) -> GripApply { + if grip_id < ATTRIBUTE_GRIP_BASE { + return apply; + } + let block_scale = insert_attribute_block_scale( + insert, + annotation_scale, + crate::scene::BlockScalePolicy::FromInsert, + ); + if (block_scale - 1.0).abs() <= 1.0e-6 { + return apply; + } + let origin = glam::DVec3::new(insert.insert_point.x, insert.insert_point.y, insert.insert_point.z); + match apply { + GripApply::Absolute(target) => GripApply::Absolute(origin + (target - origin) / block_scale), + GripApply::Translate(delta) => GripApply::Translate(delta / block_scale), + } } fn attribute_is_render_visible( @@ -89,8 +117,11 @@ fn grips(ins: &Insert) -> Vec { // block definition put it without dragging the whole block (#1259). // Attributes are stored beside the block in world space — the same space // `apply_grip` already translates them in. + // Without the document there is no ATTMODE to consult, so only an + // attribute that is drawn on its own terms gets a grip here; + // `visible_attribute_grips` is the display-aware list. out.extend(ins.attributes.iter().enumerate().filter_map(|(i, att)| { - if !attribute_is_movable(att) { + if !attribute_is_movable(att) || att.flags.invisible || att.common.invisible { return None; } let grip = Grippable::grips(att).into_iter().next()?; @@ -121,9 +152,7 @@ pub(crate) fn visible_attribute_grips( .iter() .enumerate() .filter_map(|(i, attribute)| { - if attribute.flags.constant - || attribute.lock_position - || attribute.flags.locked_position + if !attribute_is_movable(attribute) || !attribute_is_render_visible(document, visibility, attribute) { return None; diff --git a/src/scene/modify.rs b/src/scene/modify.rs index e4ae6e69c..0cecabf48 100644 --- a/src/scene/modify.rs +++ b/src/scene/modify.rs @@ -1443,6 +1443,17 @@ impl Scene { if self.is_layer_locked(handle) { return; } + // An attribute grip sits on the displayed (annotation-scaled) + // attribute; the stored one is unscaled. + let apply = match self.document.get_entity(handle) { + Some(EntityType::Insert(insert)) => crate::entities::insert::unscale_attribute_grip( + insert, + self.annotation_scale, + grip_id, + apply, + ), + _ => apply, + }; if self.apply_solid_history_grip(handle, grip_id, apply.clone()) { return; }