Merge pull request #1504 from midasmatterhorn/fix/issue-1274-obj-sibling-textures

model: OBJ/MTL references from another machine find the file next to the model, and a missing one is logged

Cargo.lock

@@ -1653,6 +1653,7 @@ version = "0.18.0"
 dependencies = [
  "effectcraft-geom",
  "image",
+ "log",
  "miniz_oxide 0.9.1",
  "serde",
  "serde_json",

crates/media/src/pool.rs

@@ -1218,8 +1218,8 @@ mod tests {
         assert_eq!(px(&f, FloatCurve::Gamma24), g24);
     }
 
-    /// A model's material library is read beside it; one named by a path to another computer
-    /// is not read.
+    /// A model's material library is read beside it; one named by a path to another computer is
+    /// looked up under its own file name beside the model, never fetched from that machine (#1274).
     #[test]
     fn model_files_on_other_computers_are_not_read() {
         let dir = std::env::temp_dir().join(format!("ec-model-files-{}", std::process::id()));
@@ -1233,11 +1233,13 @@ mod tests {
             model.materials.iter().any(|m| m.base_color[..3] == [1.0, 0.0, 0.0])
         };
         assert!(red("beside.obj", "red.mtl"));
-        assert!(!red("unc.obj", r"\\192.0.2.1\share\red.mtl"));
-        assert!(!red("url.obj", "file://192.0.2.1/s/red.mtl"));
+        // 192.0.2.1 is a test-net address nothing here can reach, so these passes prove the
+        // material library came from beside the model, not from another computer.
+        assert!(red("unc.obj", r"\\192.0.2.1\share\red.mtl"));
+        assert!(red("url.obj", "file://192.0.2.1/s/red.mtl"));
         // Linux reads `//dir/red.mtl` as the local file.
         #[cfg(unix)]
-        assert!(!red("slashes.obj", &format!("/{}", dir.join("red.mtl").display())));
+        assert!(red("slashes.obj", &format!("/{}", dir.join("red.mtl").display())));
         let _ = std::fs::remove_dir_all(&dir);
     }
 }

crates/model/Cargo.toml

@@ -14,5 +14,6 @@ effectcraft-geom = { workspace = true }
 serde = { workspace = true }
 serde_json = { workspace = true }
 thiserror = { workspace = true }
+log = { workspace = true }
 image = { workspace = true }
 miniz_oxide = "0.9"

crates/model/src/obj.rs

@@ -14,18 +14,52 @@ use std::sync::Arc;
 
 use crate::{AlphaMode, Material, Mesh, Model, Primitive, Result, TexRef, Texture, perr};
 
+/// The last component of a referenced path, treating `\` as a separator too: a path from a
+/// machine of another kind (`C:\textures\checker.png`) still finds `checker.png` next to the
+/// model (#1274). `None` when the name carries no path at all.
+fn sibling_name(name: &str) -> Option<&str> {
+    let file = name.rsplit(['/', '\\']).next()?;
+    (!file.is_empty() && file.len() < name.len()).then_some(file)
+}
+
+/// A material reference resolved against the model's folder: the name as written (a name
+/// containing spaces is one name), then its file name (an absolute path from another machine,
+/// #1274), then the final token (the options convention, `-bm 1 file.png`).
+fn resolve_reference(resolve: &dyn Fn(&str) -> Option<Vec<u8>>, name: &str) -> Option<Vec<u8>> {
+    let mut tried: Vec<&str> = vec![];
+    for candidate in [name, sibling_name(name).unwrap_or(name), name.split_whitespace().last().unwrap_or(name)] {
+        if tried.contains(&candidate) {
+            continue;
+        }
+        tried.push(candidate);
+        if let Some(b) = resolve(candidate) {
+            return Some(b);
+        }
+    }
+    None
+}
+
 /// Parse MTL text into named materials (textures appended to `textures`).
 pub fn parse_mtl(text: &str, resolve: &dyn Fn(&str) -> Option<Vec<u8>>, textures: &mut Vec<Arc<Texture>>) -> Vec<Material> {
     let mut out: Vec<Material> = vec![];
     let mut loaded: HashMap<String, Option<usize>> = HashMap::new();
     let mut tex = |name: &str, textures: &mut Vec<Arc<Texture>>| -> Option<TexRef> {
-        // Options (`-bm 1 file.png`): the file name is the last token.
-        let file = name.split_whitespace().last()?.to_string();
-        let t = *loaded.entry(file.clone()).or_insert_with(|| {
-            let b = resolve(&file)?;
-            let t = Texture::decode(&b).ok()?;
-            textures.push(Arc::new(t));
-            Some(textures.len() - 1)
+        let key = name.trim().to_string();
+        let t = *loaded.entry(key.clone()).or_insert_with(|| {
+            let Some(b) = resolve_reference(resolve, &key) else {
+                log::warn!("model: texture `{key}` was not found next to the model");
+                return None;
+            };
+            match Texture::decode(&b) {
+                Ok(t) => {
+                    textures.push(Arc::new(t));
+                    Some(textures.len() - 1)
+                }
+                Err(e) => {
+                    log::warn!("model: cannot decode texture `{key}`: {e}");
+                    None
+                }
+            }
         });
         t.map(|texture| TexRef { texture, uv_set: 0 })
     };
@@ -147,13 +181,18 @@ pub fn parse(bytes: &[u8], resolve: &dyn Fn(&str) -> Option<Vec<u8>>) -> Result<
             }
             "mtllib" => {
                 let name = line["mtllib".len()..].trim();
-                for file in std::iter::once(name).chain(name.split_whitespace()) {
+                let mut found = false;
+                for file in std::iter::once(name).chain(name.split_whitespace()).chain(sibling_name(name)) {
                     if let Some(b) = resolve(file) {
                         let mut ms = parse_mtl(&String::from_utf8_lossy(&b), resolve, &mut textures);
                         materials.append(&mut ms);
+                        found = true;
                         break;
                     }
                 }
+                if !found && !name.is_empty() {
+                    log::warn!("model: material library `{name}` was not found next to the model");
+                }
             }
             "usemtl" => {
                 let name = line["usemtl".len()..].trim();

crates/model/src/tests.rs

@@ -119,6 +119,92 @@ fn obj_without_normals_gets_them() {
     assert!(obj::parse(b"v 0 0 0\n", &|_| None).is_err());
 }
 
+#[test]
+fn obj_textures_from_another_machine_find_the_file_next_to_the_model() {
+    // No committed media: the material file and its textures are written by the test itself.
+    let d = std::env::temp_dir().join(format!("ec-obj-sibling-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&d);
+    std::fs::create_dir_all(&d).unwrap();
+    let png = {
+        let img = image::RgbaImage::from_fn(2, 2, |x, y| if (x + y) % 2 == 0 { image::Rgba([255, 0, 0, 255]) } else { image::Rgba([0, 0, 255, 255]) });
+        let mut out = std::io::Cursor::new(Vec::new());
+        img.write_to(&mut out, image::ImageFormat::Png).unwrap();
+        out.into_inner()
+    };
+    std::fs::write(d.join("checker.png"), &png).unwrap();
+    std::fs::write(d.join("Diffuse Map.png"), &png).unwrap();
+    let mtl = "newmtl Mat\nKd 0.640000 0.640000 0.640000\nmap_Kd C:\\Users\\somebody\\Desktop\\textures\\checker.png\n\
+               newmtl Spaced\nKd 1.000000 1.000000 1.000000\nmap_Kd Diffuse Map.png\n\
+               newmtl Missing\nKd 0.500000 0.500000 0.500000\nmap_Kd nowhere.png\n";
+    let obj = "mtllib quad.mtl\nv -1 -1 0\nv 1 -1 0\nv 1 1 0\nv -1 1 0\nvt 0 0\nvt 1 0\nvt 1 1\nvt 0 1\n\
+               usemtl Mat\nf 1/1 2/2 3/3\nf 1/1 3/3 4/4\n";
+    // Like the media layer's resolver: a relative name joins the model's folder; an absolute
+    // path is only looked up where it points (here: nowhere else).
+    let resolve = |uri: &str| -> Option<Vec<u8>> {
+        let p = std::path::Path::new(uri);
+        if p.is_absolute() {
+            return None;
+        }
+        std::fs::read(d.join(p)).ok()
+    };
+    let resolver = |uri: &str| -> Option<Vec<u8>> { if uri == "quad.mtl" { Some(mtl.as_bytes().to_vec()) } else { resolve(uri) } };
+    let m = obj::parse(obj.as_bytes(), &resolver).unwrap();
+    assert_eq!(m.materials.len(), 3);
+    let mat = &m.materials[0];
+    let t = mat.base_color_tex.expect("the absolute map_Kd path finds checker.png next to the .obj");
+    let tex = &m.textures[t.texture];
+    assert_eq!((tex.width, tex.height), (2, 2));
+    assert_eq!(tex.data[0], [1.0, 0.0, 0.0, 1.0]);
+    assert!((mat.base_color[0] - obj::srgb_to_linear(0.64)).abs() < 1e-6, "Kd still tints the texture");
+    assert!(m.materials[1].base_color_tex.is_some(), "a texture name containing spaces is one name");
+    assert!(m.materials[2].base_color_tex.is_none(), "a genuinely missing texture keeps the Kd colour");
+    assert!((m.materials[2].base_color[0] - obj::srgb_to_linear(0.5)).abs() < 1e-6);
+    // The same fallback applies to the material library itself.
+    let here = obj.replace("mtllib quad.mtl", "mtllib C:\\build\\quad.mtl");
+    assert_eq!(obj::parse(here.as_bytes(), &resolver).unwrap().materials.len(), 3);
+    let _ = std::fs::remove_dir_all(&d);
+}
+
+#[test]
+fn obj_texture_prefers_the_complete_name_over_its_final_token() {
+    // Two textures sit beside the model: `Map.png` (the final whitespace token of the reference)
+    // and `Diffuse Map.png` (the complete name). The complete name must win, its basename next,
+    // the final token last (the `-bm 1 file.png` options convention).
+    let d = std::env::temp_dir().join(format!("ec-obj-full-name-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&d);
+    std::fs::create_dir_all(&d).unwrap();
+    let png = |w: u32| {
+        let img = image::RgbaImage::from_fn(w, 2, |_, _| image::Rgba([255, 0, 0, 255]));
+        let mut out = std::io::Cursor::new(Vec::new());
+        img.write_to(&mut out, image::ImageFormat::Png).unwrap();
+        out.into_inner()
+    };
+    std::fs::write(d.join("Map.png"), png(2)).unwrap();
+    std::fs::write(d.join("Diffuse Map.png"), png(4)).unwrap();
+    let mtl = "newmtl Spaced\nmap_Kd Diffuse Map.png\n\
+               newmtl Absolute\nmap_Kd C:\\tex\\Diffuse Map.png\n\
+               newmtl Options\nmap_Kd -bm 1 Map.png\n";
+    let obj = "mtllib quad.mtl\nv -1 -1 0\nv 1 -1 0\nv 1 1 0\nv -1 1 0\nvt 0 0\nvt 1 0\nvt 1 1\nvt 0 1\n\
+               usemtl Spaced\nf 1/1 2/2 3/3\nf 1/1 3/3 4/4\n";
+    // Like the media layer's resolver: a relative name joins the model's folder; an absolute
+    // path is only looked up where it points (here: nowhere else).
+    let resolve = |uri: &str| -> Option<Vec<u8>> {
+        let p = std::path::Path::new(uri);
+        if p.is_absolute() {
+            return None;
+        }
+        std::fs::read(d.join(p)).ok()
+    };
+    let resolver = |uri: &str| -> Option<Vec<u8>> { if uri == "quad.mtl" { Some(mtl.as_bytes().to_vec()) } else { resolve(uri) } };
+    let m = obj::parse(obj.as_bytes(), &resolver).unwrap();
+    assert_eq!(m.materials.len(), 3);
+    let size = |mat: &Material| mat.base_color_tex.map(|t| (m.textures[t.texture].width, m.textures[t.texture].height));
+    assert_eq!(size(&m.materials[0]), Some((4, 2)), "the complete `Diffuse Map.png` wins over its final token `Map.png`");
+    assert_eq!(size(&m.materials[1]), Some((4, 2)), "an absolute path's spaced basename beats its final token");
+    assert_eq!(size(&m.materials[2]), Some((2, 2)), "the `-bm 1 file.png` options convention still finds the final token");
+    let _ = std::fs::remove_dir_all(&d);
+}
+
 #[test]
 fn channel_interpolation_modes() {
     let mut c = Channel { node: 0, path: ChannelPath::Translation, interp: Interp::Step, times: vec![0.0, 1.0], values: vec![0.0, 0.0, 0.0, 10.0, 0.0, 0.0] };