From 517e0b7ae85d36e26f045ec066cddcc53f234eaa Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 06:35:39 -0800 Subject: [PATCH] Effects: hiding a 3D effect still checks the values given with it effect.setParams with visible: false skipped every 3D parameter check, so a hidden edit could store a Classic depth of 2001, an unknown surface or bevel, an empty light list or a Revolve edge other than left or right. The values given are now checked by validate_three_d_params (the checks of validate_three_d that do not build the art); after a change of 3D type the values the effect keeps are checked too. The object still does not need to fit while the effect is hidden. --- crates/effects/src/lib.rs | 3 +- crates/effects/src/revolve.rs | 10 ++++- crates/effects/src/solids.rs | 20 ++++++++-- crates/engine/src/cmd/effectcmd.rs | 12 ++++-- crates/engine/src/cmd/effectcmd/tests.rs | 50 ++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 9 deletions(-) diff --git a/crates/effects/src/lib.rs b/crates/effects/src/lib.rs index 417292b52..bf9fba23d 100644 --- a/crates/effects/src/lib.rs +++ b/crates/effects/src/lib.rs @@ -85,7 +85,8 @@ pub use reshape::{ pub use revolve::{REVOLVE, has_revolve, revolve_art, revolve_options, validate_revolve}; pub use solids::{ CLASSIC_EXTRUDE, EXTRUDE, INFLATE, ROTATE, ROTATION_PRESETS, SHADING_COLORS, SOLIDS, THREE_D_TYPES, classic_extrude_options, classic_shading, - extrude_options, has_solid, inflate_options, is_three_d, solid_art, three_d_params_as, validate_solid, validate_three_d, view_options, + extrude_options, has_solid, inflate_options, is_three_d, solid_art, three_d_params_as, validate_solid, validate_three_d, validate_three_d_params, + view_options, }; pub use stylize::{ScribbleSetting, scribble_setting_id, scribble_settings}; pub use trap::TRAP; diff --git a/crates/effects/src/revolve.rs b/crates/effects/src/revolve.rs index b2528818b..9e7179941 100644 --- a/crates/effects/src/revolve.rs +++ b/crates/effects/src/revolve.rs @@ -66,19 +66,25 @@ pub fn revolve_options(p: &Value) -> Revolve { /// Validate before a command mutates the document. Loading external files is still bounded /// by the evaluator; unsupported source kinds remain unchanged when rendered. pub fn validate_revolve(n: &Node, p: &Value) -> Result<(), String> { - crate::lighting::validate(p)?; + validate_revolve_params(p)?; let path = profile(n).ok_or("Revolve currently requires a path or compound path. Select a profile drawn with the Pen tool.")?; let b = path.bounds().ok_or("Revolve requires a profile with at least two anchors")?; if !path.subpaths.iter().any(|sp| sp.anchors.len() >= 2) { return Err("Revolve requires at least two anchors".into()); } + vectorcraft_three_d::revolve(&path, b, revolve_options(p)).map(|_| ()) +} + +/// Check Revolve's parameter values `p` alone: its lights, `edge` and `cap`. +pub(crate) fn validate_revolve_params(p: &Value) -> Result<(), String> { + crate::lighting::validate(p)?; if p.get("edge").is_some_and(|v| v.as_str() != Some("left") && v.as_str() != Some("right")) { return Err("Revolve edge must be left or right".into()); } if p.get("cap").is_some_and(|v| v.as_bool().is_none()) { return Err("Revolve cap must be a boolean".into()); } - vectorcraft_three_d::revolve(&path, b, revolve_options(p)).map(|_| ()) + Ok(()) } fn profile(n: &Node) -> Option { diff --git a/crates/effects/src/solids.rs b/crates/effects/src/solids.rs index 818fd6c1b..c31dbe856 100644 --- a/crates/effects/src/solids.rs +++ b/crates/effects/src/solids.rs @@ -182,19 +182,26 @@ fn facets(id: &str, path: &PathData, rule: FillRule, bounds: Rect, p: &Value) -> } } -/// Validate before a command changes the document; the evaluator bounds loaded files too. -pub fn validate_solid(n: &Node, id: &str, p: &Value) -> Result<(), String> { +/// Check solid `id`'s parameter values `p` alone: the part of [`validate_solid`] that does not +/// need the object. +fn validate_solid_params(id: &str, p: &Value) -> Result<(), String> { if id == CLASSIC_EXTRUDE { classic::validate(p)?; } crate::lighting::validate(p)?; - let label = crate::effect_info(id).map_or("This effect", |e| e.label.trim_end_matches('…')); if let Some(v) = p.get("bevel") && !v.as_str().is_some_and(|s| s == "none" || BevelShape::parse(s).is_some()) { let names: Vec<_> = BevelShape::ALL.iter().map(|b| b.id()).collect(); return Err(format!("bevel must be none or one of {}", names.join(", "))); } + Ok(()) +} + +/// Validate before a command changes the document; the evaluator bounds loaded files too. +pub fn validate_solid(n: &Node, id: &str, p: &Value) -> Result<(), String> { + validate_solid_params(id, p)?; + let label = crate::effect_info(id).map_or("This effect", |e| e.label.trim_end_matches('…')); if is_container(n) { let shapes = members(n, label)?; let b = union_bounds(&shapes).ok_or_else(|| format!("{label} requires a group holding a shape with at least two anchors"))?; @@ -277,6 +284,13 @@ pub fn validate_three_d(n: &Node, id: &str, p: &Value) -> Result<(), String> { if id == crate::REVOLVE { crate::validate_revolve(n, p) } else { validate_solid(n, id, p) } } +/// Are 3D effect `id`'s parameter values `p` valid? The checks of [`validate_three_d`] that do not +/// build the art, for an edit that hides the effect: hidden, it draws nothing, so its object does +/// not need to fit. +pub fn validate_three_d_params(id: &str, p: &Value) -> Result<(), String> { + if id == crate::REVOLVE { crate::revolve::validate_revolve_params(p) } else { validate_solid_params(id, p) } +} + /// One surface: each facet a path shaded from `paint`, as a group with the paint's opacity and /// blend mode (so overlapping facets don't add up their transparency). fn surface(n: &Node, facets: Vec, paint: &Paint, opacity: f32, blend: BlendMode, colored: bool) -> Node { diff --git a/crates/engine/src/cmd/effectcmd.rs b/crates/engine/src/cmd/effectcmd.rs index 450dbe0e5..2b90dc6b4 100644 --- a/crates/engine/src/cmd/effectcmd.rs +++ b/crates/engine/src/cmd/effectcmd.rs @@ -487,7 +487,7 @@ fn lighting_preset(s: &mut Session, p: &Value) -> Result { /// `effect.setParams`: merges `params` into the effect at `index` of the targets' active item, and /// sets its Warp style (`warpStyle`), its 3D type (`threeDType`) or visibility (`visible`), as one /// undo step. They are checked as `effect.apply` checks them, and a visible 3D effect must still -/// fit. → `{ids}`. +/// fit; a hidden one has its given values checked. → `{ids}`. fn set_params(s: &mut Session, p: &Value) -> Result { const C: &str = "effect.setParams"; let index = index_param(p, "index", C)?; @@ -539,7 +539,6 @@ fn set_params(s: &mut Session, p: &Value) -> Result { } if let Some(e) = effect && effects::is_three_d(&e.id) - && visible != Some(false) { let to = kind.unwrap_or(&e.id); let mut prospective = @@ -547,7 +546,14 @@ fn set_params(s: &mut Session, p: &Value) -> Result { if let (Some(cur), Value::Object(new)) = (prospective.as_object_mut(), effects::upgrade_params(to, ¶ms)) { cur.extend(new); } - effects::validate_three_d(n, to, &prospective).map_err(|e| bad(C, e))?; + if visible != Some(false) { + effects::validate_three_d(n, to, &prospective).map_err(|e| bad(C, e))?; + } else { + // Hidden, the effect draws nothing, so its object need not fit. The values given are + // checked, and after a change of 3D type so is every value it keeps. + let checked = if to == e.id { effects::upgrade_params(to, ¶ms) } else { prospective }; + effects::validate_three_d_params(to, &checked).map_err(|e| bad(C, e))?; + } } } let ids = edit_effects(s, p, item, C, "Effect Options", &format!("{C}: no effect at index {index}"), index, |fx, index| { diff --git a/crates/engine/src/cmd/effectcmd/tests.rs b/crates/engine/src/cmd/effectcmd/tests.rs index be1dd443c..e3fca957b 100644 --- a/crates/engine/src/cmd/effectcmd/tests.rs +++ b/crates/engine/src/cmd/effectcmd/tests.rs @@ -46,6 +46,56 @@ fn revolve_commands_keep_profile_live_edit_options_expand_and_undo() { assert_eq!(effects_for_test(&s, id), before); } +/// `effect.setParams` checks the values it is given for a 3D effect when the same edit hides it: +/// a bad value is refused and the document is left as it was, and good values are stored. +#[test] +fn hiding_a_3d_effect_still_checks_its_new_values() { + let mut s = Session::new(); + s.execute("file.new", &json!({"width": 400, "height": 400})).unwrap(); + let rect = |s: &mut Session, x: u32| { + NodeId(s.execute("shape.rectangle", &json!({"x": x, "y": 100, "width": 60, "height": 60})).unwrap()["id"].as_u64().unwrap()) + }; + let classic = rect(&mut s, 20); + s.execute("effect.apply", &json!({"effect": effects::CLASSIC_EXTRUDE, "ids": [classic.0], "item": null})).unwrap(); + let extrude = rect(&mut s, 120); + s.execute("effect.apply", &json!({"effect": effects::EXTRUDE, "ids": [extrude.0], "item": null})).unwrap(); + let profile = NodeId(s.execute("path.create", &json!({"d": "M 300 40 L 330 60 L 330 140 L 300 160"})).unwrap()["id"].as_u64().unwrap()); + s.execute("effect.apply", &json!({"effect": effects::REVOLVE, "ids": [profile.0], "item": null})).unwrap(); + for (id, params, why) in [ + (classic, json!({"depth": 2001}), "`depth` must be between 0 and 2000"), + (classic, json!({"surface": "glossy"}), "`surface` must be one of"), + (classic, json!({"bevel": "wavy"}), "bevel must be none or one of"), + (extrude, json!({"lights": []}), "lights must contain one to ten lights"), + (extrude, json!({"lightColor": "red"}), "lightColor must be an opaque RGB color"), + (profile, json!({"edge": "top"}), "Revolve edge must be left or right"), + ] { + let before = s.doc().unwrap().doc.clone(); + let undo = s.doc().unwrap().history.undo.len(); + let p = json!({"ids": [id.0], "index": 0, "item": null, "params": params, "visible": false}); + let e = s.execute("effect.setParams", &p).unwrap_err().to_string(); + assert!(e.contains(why), "{params}: {e}"); + assert_eq!(s.doc().unwrap().doc, before, "{params}"); + assert_eq!(s.doc().unwrap().history.undo.len(), undo, "{params}"); + } + // A hidden switch of 3D type checks the values given with it for the new type. + let before = s.doc().unwrap().doc.clone(); + let p = json!({"ids": [extrude.0], "index": 0, "item": null, "threeDType": "inflate", "params": {"lights": []}, "visible": false}); + let e = s.execute("effect.setParams", &p).unwrap_err().to_string(); + assert!(e.contains("lights must contain one to ten lights"), "{e}"); + assert_eq!(s.doc().unwrap().doc, before); + // Hiding alone checks nothing. + s.execute("effect.setParams", &json!({"ids": [extrude.0], "index": 0, "item": null, "visible": false})).unwrap(); + assert!(!node(&s, extrude).appearance.effects[0].visible); + s.execute("effect.setParams", &json!({"ids": [classic.0], "index": 0, "item": null, "params": {"depth": 2000}, "visible": false})).unwrap(); + let e = &node(&s, classic).appearance.effects[0]; + assert_eq!((e.params["depth"].as_f64(), e.visible), (Some(2000.0), false)); + // A value the effect already holds is not checked again: a key it does not take, as a file + // can carry, does not stop a hidden edit of another value. + std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().doc).node_mut(classic).unwrap().appearance.effects[0].params["extra"] = json!(1); + s.execute("effect.setParams", &json!({"ids": [classic.0], "index": 0, "item": null, "params": {"depth": 10}, "visible": false})).unwrap(); + assert_eq!(node(&s, classic).appearance.effects[0].params["depth"].as_f64(), Some(10.0)); +} + /// Extrude & Bevel, Inflate and Rotate: applied by command, saved and reopened, edited, /// exported, expanded and undone; one 3D effect per object. #[test]