Merge PR #2924

crates/engine/src/cmd/model3d.rs

@@ -171,9 +171,29 @@ fn kind_p(p: &Value, cmd: &str) -> Result<PrimitiveKind> {
     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<usize> {
-    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<Vec<&'static str>> {
+    let mut set = Vec::new();
     let tri = |v: &Value| -> Option<u32> {
         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<usize>
             _ => 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::<Vec<_>>().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<Value> {
@@ -273,19 +320,22 @@ fn material_set(s: &mut Session, p: &Value) -> Result<Value> {
     }
     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<CommandSpec> {
             "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
         ),

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.