M8: vector.edit.position moves points in one pass; refusal, empty-selection and scrub tests

The handler grouped equal offsets by a linear scan per point, quadratic when every point moves
by its own offset. Only a single move covering every point carries the regions' paint frames,
so it now checks in one pass whether all offsets are equal: if so the points move as one,
otherwise each moves by its own offset. The result is the same as before.

Tests: booleans, a valid x with a refused y, and the overflow case are refused with no undo
step; nothing selected is refused; a shared Y scrubs every point in one undo step and a Mixed
X doesn't scrub. selection_position's doc comment names its one user, vector.edit.bounds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Model-Effort: Opus 5.5 drove and wrote
This commit is contained in:
Trevor Brown
2026-11-08 14:44:47 -05:00
co-authored by Claude Opus 5.5
parent 62e7e3a41a
commit dc841f8548
4 changed files with 59 additions and 16 deletions
+13 -12
View File
@@ -762,7 +762,7 @@ pub fn selected_parent_points(doc: &Document, ve: &VectorEdit) -> Vec<Point> {
selected_points(net, ve).into_iter().filter_map(|v| net.vertices.get(v)).map(|v| t * v.point()).collect()
}
/// Selection bounds' top-left in the parent's coordinates (what the X/Y fields show).
/// Selection bounds' top-left in the parent's coordinates (the `x` and `y` of `vector.edit.bounds`).
pub fn selection_position(doc: &Document, ve: &VectorEdit) -> Option<Point> {
let net = network(doc, ve.id)?;
let t = doc.node(ve.id)?.transform;
@@ -1017,18 +1017,19 @@ pub fn specs() -> Vec<CommandSpec> {
if moves.is_empty() {
return Err(bad("vector.edit.position", "select points"));
}
// Points with the same offset move together, so moving every point by one offset
// still carries the regions' paint frames, as a drag of all of them does.
let mut groups: Vec<(Vec2, Vec<usize>)> = Vec::new();
for (i, d) in moves {
match groups.iter_mut().find(|(g, _)| (*g - d).hypot() < 1e-9) {
Some((_, v)) => v.push(i),
None => groups.push((d, vec![i])),
}
}
// Points that all share one offset move as one, so moving every point of a layer
// by one offset still carries the regions' paint frames, as a drag of all of them
// does. Otherwise no one move covers every point, so each point moves alone.
let d0 = moves[0].1;
let uniform = moves.iter().all(|(_, d)| (*d - d0).hypot() < 1e-9);
edit(s.doc_mut()?, ve.id, |net| {
for (d, v) in &groups {
ops::move_vertices(net, v, *d);
if uniform {
let all: Vec<usize> = moves.iter().map(|(i, _)| *i).collect();
ops::move_vertices(net, &all, d0);
} else {
for (i, d) in &moves {
ops::move_vertices(net, &[*i], *d);
}
}
})?;
Ok(json!({"ok": true}))
@@ -19,6 +19,10 @@ fn parent_xy(s: &Session) -> Vec<(f64, f64)> {
selected_parent_points(s.doc().unwrap(), &ve).iter().map(|p| (p.x, p.y)).collect()
}
fn undo_steps(s: &Session) -> usize {
s.ds().unwrap().history.undo.len()
}
fn near(a: f64, b: f64) -> bool {
(a - b).abs() < 1e-6
}
@@ -132,6 +136,7 @@ fn junk_is_refused_and_changes_nothing() {
let id = rect(&mut s);
let before = xs(&s, id);
ex(&mut s, "vector.edit.select", json!({"vertices": [0, 1]}));
let steps = undo_steps(&s);
for junk in [
json!({"x": {"div": 0}}),
json!({"x": {"pow": 2}}),
@@ -140,9 +145,27 @@ fn junk_is_refused_and_changes_nothing() {
json!({"y": {"add": "x"}}),
json!({"x": {}}),
json!({"x": [1]}),
json!({"x": true}),
json!({"x": {"add": true}}),
// A valid x with a refused y changes neither.
json!({"x": 5, "y": {"div": 0}}),
// Point 1's x of 100 overflows; point 0's 0 alone would not.
json!({"x": {"mul": 1e308}}),
] {
assert!(s.execute("vector.edit.position", &junk).is_err(), "{junk}");
}
assert_eq!(xs(&s, id), before);
assert_eq!(undo_steps(&s), steps, "a refused edit leaves no undo step");
}
#[test]
fn nothing_selected_is_refused() {
let mut s = Session::new();
let id = rect(&mut s);
ex(&mut s, "vector.edit.select", json!({"vertices": []}));
let (before, steps) = (xs(&s, id), undo_steps(&s));
assert!(parent_xy(&s).is_empty());
let err = s.execute("vector.edit.position", &json!({"x": 5})).unwrap_err();
assert!(err.to_string().contains("select points"), "{err}");
assert_eq!((xs(&s, id), undo_steps(&s)), (before, steps));
}
+2 -2
View File
@@ -234,7 +234,7 @@ fn geom(u: &Ui, n: NodeId) -> (f64, f64, f64, f64) {
(b.x0, b.y0, b.width(), b.height())
}
fn undo_steps(u: &Ui) -> usize {
pub(crate) fn undo_steps(u: &Ui) -> usize {
u.app.session.ds().unwrap().history.undo.len()
}
@@ -261,7 +261,7 @@ pub(crate) fn type_into(u: &mut Ui, name: &str, text: &str, end: Key) {
}
/// Scrub field `name` by its label: `dx` points with `modifiers` held, then let go.
fn scrub(u: &mut Ui, name: &str, dx: f32, modifiers: Modifiers) {
pub(crate) fn scrub(u: &mut Ui, name: &str, dx: f32, modifiers: Modifiers) {
let from = label(u, name);
drag(u, from, dx, modifiers);
}
+21 -2
View File
@@ -1,12 +1,12 @@
//! The vector edit panel's Point section: X and Y show the value the selected points share, or
//! "Mixed", and a typed value sets every selected point's own coordinate.
use egui::Key;
use egui::{Key, Modifiers};
use figcraft_doc::NodeId;
use figcraft_engine::Session;
use serde_json::json;
use crate::field_tests::{field, type_into};
use crate::field_tests::{field, scrub, type_into, undo_steps};
use crate::layers_drag_tests::Ui;
fn setup() -> (Ui, NodeId) {
@@ -58,3 +58,22 @@ fn one_point_shows_its_own_value() {
assert_eq!(field(&mut u, "X").1, format!("{}", p.0));
assert_eq!(field(&mut u, "Y").1, format!("{}", p.1));
}
#[test]
fn a_shared_value_scrubs_every_point_in_one_step_and_mixed_does_not_scrub() {
let (mut u, id) = setup();
// Points 0 and 1 share their y: a scrub moves both to the same new y, as one undo step.
u.app.session.execute("vector.edit.select", &json!({"vertices": [0, 1]})).unwrap();
let (before, steps) = (xs(&u, id), undo_steps(&u));
scrub(&mut u, "Y", 20.0, Modifiers::NONE);
let after = xs(&u, id);
assert!(after[0].1 > before[0].1, "the scrub moved y: {before:?} -> {after:?}");
assert_eq!(after[0].1, after[1].1, "both points take the scrubbed value");
assert_eq!((after[0].0, after[1].0), (before[0].0, before[1].0), "x is untouched");
assert_eq!(undo_steps(&u), steps + 1, "one undo step for the whole scrub");
// Their x differs (Mixed): there is no value to scrub from, so nothing moves.
assert_eq!(field(&mut u, "X").1, "Mixed");
let (before, steps) = (xs(&u, id), undo_steps(&u));
scrub(&mut u, "X", 20.0, Modifiers::NONE);
assert_eq!((xs(&u, id), undo_steps(&u)), (before, steps));
}