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.