From 583ba084b67637614cf6060054085223ec141ffb Mon Sep 17 00:00:00 2001 From: JonRC Date: Mon, 2 Nov 2026 22:50:04 -0300 Subject: [PATCH] Fix #2917: material.set names the Material Options a layer doesn't have A 3D model layer keeps its file's materials, so it has no Base Color, Metallic, Roughness or Emissive. material.set refused them with "no Material Options property matched (..., baseColor, metallic, roughness, ...)", naming the parameters it had just refused, and next to a parameter the layer has it dropped them and reported success. A parameter that none of the given layers has now fails the command (nothing is set), naming it and the properties the first layer has. A value of the wrong type is its own error. The parameter list says which layer kinds take which. --- crates/engine/src/cmd/model3d.rs | 98 ++++++++++++++++++++++-------- crates/engine/src/tests/model3d.rs | 37 +++++++++++ 2 files changed, 111 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/cmd/model3d.rs b/crates/engine/src/cmd/model3d.rs index 2cc4d34e6..f4e62f6b7 100644 --- a/crates/engine/src/cmd/model3d.rs +++ b/crates/engine/src/cmd/model3d.rs @@ -171,9 +171,29 @@ fn kind_p(p: &Value, cmd: &str) -> Result { PrimitiveKind::parse(k).ok_or_else(|| bad(cmd, format!("kind: cube|sphere|plane|torus|cone|cylinder, not `{k}`"))) } -/// Parse Material Options parameters onto a layer at comp time `t`. -fn apply_material(l: &mut Layer, p: &Value, t: Tick, cmd: &str) -> Result { - let mut n = 0; +/// The Material Options parameters [`apply_material`] reads. Which ones a layer has depends on +/// its kind (see [`material_mismatch`]). +const MATERIAL_PARAMS: [&str; 14] = [ + "castsShadows", + "acceptsShadows", + "acceptsLights", + "appearsInReflections", + "baseColor", + "metallic", + "roughness", + "emissive", + "lightTransmission", + "ambient", + "diffuse", + "specularIntensity", + "specularShininess", + "metal", +]; + +/// Parse Material Options parameters onto a layer at comp time `t`. The parameters the layer +/// had a property for. +fn apply_material(l: &mut Layer, p: &Value, t: Tick, cmd: &str) -> Result> { + let mut set = Vec::new(); let tri = |v: &Value| -> Option { match v { Value::Bool(b) => Some(*b as u32), @@ -187,28 +207,55 @@ fn apply_material(l: &mut Layer, p: &Value, t: Tick, cmd: &str) -> Result _ => None, } }; + let mut put = |l: &mut Layer, k: &'static str, v: KV| { + if set_at(l, &format!("materialOptions/{k}"), t, v) { + set.push(k); + } + }; for k in ["castsShadows", "acceptsShadows"] { if let Some(v) = p.get(k) { let e = tri(v).ok_or_else(|| bad(cmd, format!("{k}: off|on|only")))?; - n += set_at(l, &format!("materialOptions/{k}"), t, KV::Enum(e)) as usize; + put(l, k, KV::Enum(e)); } } + // A value of the wrong type is an error, not a parameter left out. for k in ["acceptsLights", "appearsInReflections"] { - if let Some(v) = b_p(p, k) { - n += set_at(l, &format!("materialOptions/{k}"), t, KV::Bool(v)) as usize; + if p.get(k).is_some() { + let v = b_p(p, k).ok_or_else(|| bad(cmd, format!("{k}: true|false")))?; + put(l, k, KV::Bool(v)); } } for k in ["lightTransmission", "ambient", "diffuse", "specularIntensity", "specularShininess", "metal", "metallic", "roughness"] { - if let Some(v) = f_p(p, k) { - n += set_at(l, &format!("materialOptions/{k}"), t, KV::Scalar(v)) as usize; + if p.get(k).is_some() { + let v = f_p(p, k).ok_or_else(|| bad(cmd, format!("{k}: a number")))?; + put(l, k, KV::Scalar(v)); } } for k in ["baseColor", "emissive"] { - if let Some(c) = color_p(p, k) { - n += set_at(l, &format!("materialOptions/{k}"), t, KV::Color([c[0] as f64, c[1] as f64, c[2] as f64, 1.0])) as usize; + if p.get(k).is_some() { + let c = color_p(p, k).ok_or_else(|| bad(cmd, format!("{k}: \"#rrggbb\" or [r, g, b]")))?; + put(l, k, KV::Color([c[0] as f64, c[1] as f64, c[2] as f64, 1.0])); } } - Ok(n) + Ok(set) +} + +/// The error for Material Options parameters `missing` that none of the layers has: what the +/// first layer has instead, and which layers the missing ones belong to. +fn material_mismatch(l: Option<&Layer>, missing: &[&str]) -> EngineError { + let names = missing.iter().map(|k| format!("`{k}`")).collect::>().join(", "); + let Some(l) = l else { return bad("material.set", format!("no layer has {names}")) }; + let Some(g) = l.props.sub("materialOptions") else { return bad("material.set", format!("`{}` has no Material Options", l.name)) }; + let has: Vec<&str> = g.children.iter().map(|c| c.match_id()).collect(); + let why = if matches!(l.source, LayerSource::Model { .. }) { ": a 3D model layer keeps its model file's materials" } else { "" }; + bad( + "material.set", + format!( + "`{}` has no {names}{why}. Its Material Options: {}. baseColor, metallic, roughness and emissive are for 3D primitive layers; lightTransmission, ambient, diffuse, specularIntensity, specularShininess and metal for footage, solid, text, shape and precomp layers", + l.name, + has.join(", ") + ), + ) } fn new_primitive(s: &mut Session, p: &Value) -> Result { @@ -273,19 +320,22 @@ fn material_set(s: &mut Session, p: &Value) -> Result { } let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?.clone(); let t = super::time_p(s, p, Some(&comp)); - let n = s.edit("Material Options", None, |proj, _| { - let mut n = 0; - for id in &ids { - n += apply_material(layer_mut(proj, cid, *id)?, p, t, "material.set")?; - } - Ok(n) - })?; - if n == 0 { - return Err(bad( - "material.set", - "no Material Options property matched (castsShadows, acceptsShadows, acceptsLights, baseColor, metallic, roughness, …)", - )); + if !MATERIAL_PARAMS.iter().any(|k| p.get(*k).is_some()) { + return Err(bad("material.set", format!("no Material Options parameter given ({})", MATERIAL_PARAMS.join(", ")))); } + // A parameter none of the layers has fails the whole command (the edit is dropped), so + // it is never skipped without a word. + let n = s.edit("Material Options", None, |proj, _| { + let mut set = Vec::new(); + for id in &ids { + set.extend(apply_material(layer_mut(proj, cid, *id)?, p, t, "material.set")?); + } + let missing: Vec<&str> = MATERIAL_PARAMS.into_iter().filter(|k| p.get(*k).is_some() && !set.contains(k)).collect(); + if !missing.is_empty() { + return Err(material_mismatch(ids.first().and_then(|id| proj.comp(cid)?.layer(*id)), &missing)); + } + Ok(set.len()) + })?; Ok(json!({"set": n})) } @@ -411,7 +461,7 @@ pub fn specs() -> Vec { "Material Options", [], None, - "{layers?, castsShadows?: off|on|only, acceptsShadows?: off|on|only, acceptsLights?, appearsInReflections?, lightTransmission?, ambient?, diffuse?, specularIntensity?, specularShininess?, metal?, baseColor?, metallic?, roughness?, emissive?, time?}", + "{layers?, time?, castsShadows?: off|on|only, acceptsShadows?: off|on|only, acceptsLights?, appearsInReflections? (model and primitive layers), baseColor?, metallic?, roughness?, emissive? (primitive layers; a model layer keeps its file's materials), lightTransmission?, ambient?, diffuse?, specularIntensity?, specularShininess?, metal? (footage, solid, text, shape and precomp layers)}; a parameter none of the layers has is an error", has_layers, material_set ), diff --git a/crates/engine/src/tests/model3d.rs b/crates/engine/src/tests/model3d.rs index 40a165799..548e93b20 100644 --- a/crates/engine/src/tests/model3d.rs +++ b/crates/engine/src/tests/model3d.rs @@ -232,6 +232,43 @@ fn material_commands_and_serde() { assert_eq!(&back, &*s.project); } +/// #2917: a 3D model layer keeps its file's materials, so it has no Base Color, Metallic or +/// Roughness. material.set refused them with "no Material Options property matched (…, +/// baseColor, metallic, roughness, …)", naming the very parameters it refused, and next to a +/// parameter the layer does have it dropped them without a word. +#[test] +fn material_set_names_the_parameters_a_layer_lacks() { + let mut s = session(); + let r = s.execute("file.import", json!({"paths": [fixture("quad.glb")]})).unwrap(); + let item = r["items"][0].as_u64().unwrap(); + let m = s.execute("layer.addItem", json!({"item": item})).unwrap()["layer"].as_u64().unwrap(); + let e = s.execute_checked("material.set", json!({"layers": [m], "baseColor": "#3366ff", "roughness": 20})).unwrap_err().to_string(); + assert!(e.contains("`baseColor`, `roughness`") && e.contains("model file") && e.contains("appearsInReflections"), "{e}"); + assert!(!e.contains("`metallic`"), "{e}"); + // A parameter the layer has doesn't hide one it lacks, and nothing is set. + assert!(s.execute_checked("material.set", json!({"layers": [m], "castsShadows": "on", "metallic": 50})).is_err()); + assert_eq!(layer(&s, m).props.prop("materialOptions/castsShadows").unwrap().value, KV::Enum(0)); + s.execute_checked("material.set", json!({"layers": [m], "castsShadows": "on"})).unwrap(); + assert_eq!(layer(&s, m).props.prop("materialOptions/castsShadows").unwrap().value, KV::Enum(1)); + // Across layers, a parameter goes to the ones that have it. + let cube = s.execute("layer.newPrimitive", json!({"kind": "cube"})).unwrap()["layer"].as_u64().unwrap(); + assert_eq!(s.execute_checked("material.set", json!({"layers": [m, cube], "roughness": 20})).unwrap()["set"], 1); + assert_eq!(layer(&s, cube).props.prop("materialOptions/roughness").unwrap().value.as_f64(), 20.0); + // A solid has the classic shading, not the primitive's material. + let solid = s.execute("layer.newSolid", json!({"width": 64, "height": 32})).unwrap()["layer"].as_u64().unwrap(); + let e = s.execute_checked("material.set", json!({"layers": [solid], "emissive": "#ffffff"})).unwrap_err().to_string(); + assert!(e.contains("`emissive`") && e.contains("specularShininess"), "{e}"); + assert!(s.execute_checked("material.set", json!({"layers": [m]})).is_err()); + // The parameter list still names every parameter, or agents' calls are refused before they run. + let spec = crate::command_specs().iter().find(|c| c.id == "material.set").unwrap(); + let keys = crate::commands::accepted_params(spec.params).unwrap(); + let want = "layers time castsShadows acceptsShadows acceptsLights appearsInReflections baseColor metallic roughness emissive \ + lightTransmission ambient diffuse specularIntensity specularShininess metal"; + for k in want.split_whitespace() { + assert!(keys.iter().any(|a| a == k), "{k}: {keys:?}"); + } +} + /// #264: only Advanced 3D draws models and primitives, and new comps are Classic 3D, so an /// imported model was invisible. Adding one switches the comp to Advanced 3D (in the same undo /// step, with a toast), as After Effects does.