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

@@ -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;

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<PathData> {

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<Facet>, paint: &Paint, opacity: f32, blend: BlendMode, colored: bool) -> Node {

crates/engine/src/cmd/effectcmd.rs

@@ -487,7 +487,7 @@ fn lighting_preset(s: &mut Session, p: &Value) -> Result<Value> {
 /// `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<Value> {
     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<Value> {
         }
         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<Value> {
             if let (Some(cur), Value::Object(new)) = (prospective.as_object_mut(), effects::upgrade_params(to, &params)) {
                 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, &params) } 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| {

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]