From d2c85f4411c26b8876c4a7f25d8b5d98e8e3db25 Mon Sep 17 00:00:00 2001 From: CagdasErturk Date: Wed, 9 Sep 2026 14:45:16 +0300 Subject: [PATCH 1/6] feat(cli): an imported model's materials are reported and its images are written where you say `asset-import` read geometry and threw away everything else a glTF document said about itself. It reports the materials now, and writes the images into a directory the caller names. `--images ` is the asking. Without it nothing is written and everything is still reported, so a caller learns a model carries textures before deciding where they go -- a command that scattered them beside the blob because the input happened to hold some would be writing files nobody asked for. The directory is created, because the flag names a destination for a set of files whose size the caller cannot know in advance; `--out` names one file whose parent they already chose. Each image is written as `image-` after the document's own address for it, not after the name the document gave it: a name in a file is not a thing that should decide where bytes land on disk, and the index is what a material's texture reference points at, so the JSON and the directory can be matched without guessing. The extension comes from the media type, and that is the one judgement this arm makes about one. It is a judgement about a *name*: nothing here decodes anything, a type it cannot name may be perfectly readable by something else, and calling a JPEG `.png` would be inventing a fact the document never stated. `image/png` and `image/jpeg` are the two the format's own schema names; anything else, or nothing at all, is `UnknownMediaType` -- and only ever when `--images` asked for a file to be named. Without the flag the type is still just reported. `gltf::tables` is new beside `gltf::read`: it does the same container dispatch for the material and image tables that `read` does for geometry. Two callers had that dispatch written out already and each had it subtly different; a third copy in the tool was not worth having. Its images own their bytes, which a borrowed form could not -- the parsed document lives inside the call and nothing pointing into it can be handed back. The two commands still compose and neither learns about the other: import writes files, pack collects them. No pack is written here, and no image is decoded here or below. A format that carries no images answers with two empty tables rather than a refusal. Asking an STL for its textures is a fair question with a true answer. Also corrects the mesh crate's own description of how an image's media type is chosen, which still described a comparison that no longer happens. --- crates/mesh/README.md | 31 +++- crates/mesh/src/gltf.rs | 71 ++++++++ crates/mesh/tests/gltf.rs | 59 +++++++ tools/cli/README.md | 53 +++++- tools/cli/src/cli.rs | 24 +++ tools/cli/src/main.rs | 333 ++++++++++++++++++++++++++++++++++++-- tools/cli/tests/cli.rs | 222 +++++++++++++++++++++++++ 7 files changed, 767 insertions(+), 26 deletions(-) diff --git a/crates/mesh/README.md b/crates/mesh/README.md index ac285846..e7ba12ac 100644 --- a/crates/mesh/README.md +++ b/crates/mesh/README.md @@ -372,14 +372,29 @@ between them would be answering a question the document did not settle. A `uri` is a payload this reader decodes or a second file it will not open — the same pair of answers a buffer's `uri` gets. -**One name for the bytes, not two.** An image may state its type in -`mimeType`, in its payload's URI, or in both; the format requires one -beside a view, because a view carries bytes and nothing about them. When -both are present and differ, the document is refused rather than the -contradiction being handed on — the same trade the payload decoder -makes when it refuses a resource two texts could spell. An absent type is -not a disagreement: a URI may omit one, and that is the document having -said nothing rather than having said something else. +**`mimeType` is the document's answer when it gives one.** An image may +state its type beside itself, in its payload's URI, or in both; the +format requires one beside a view, because a view carries bytes and +nothing about them. The two are never compared. The format relates +neither to the other — its rule is that a payload's media type match its +*content*, which nothing here can check because nothing here decodes — +so refusing a disagreement would refuse conformant documents, and a PNG +carried as `application/octet-stream` is ordinary. The one the format +makes mandatory wins, and the payload's is what is left when there is no +other. + +**An empty statement is an absence, on both sides.** A payload may carry +no type, and `mimeType` may be written `""` because the schema ends in a +permissive string. Both are a document saying nothing about its bytes, +so both read as none — the reported type is never an empty string, which +is a value every caller would have to know to treat as absent. + +`gltf::tables` reads the materials and the images together from either +shape of the asset, doing the container dispatch that `gltf::read` does +for geometry. Its images own their bytes, because the parsed document +lives inside the call and nothing pointing into it can be handed back; a +caller that wants to avoid that copy holds the parse itself and calls +`gltf::images`. ## `data:` URIs, and why a decoder is strict about spelling diff --git a/crates/mesh/src/gltf.rs b/crates/mesh/src/gltf.rs index d0e0b67e..a8d0d9b6 100644 --- a/crates/mesh/src/gltf.rs +++ b/crates/mesh/src/gltf.rs @@ -878,6 +878,77 @@ pub fn images<'s>(root: Value<'_>, source: &'s Source<'_>) -> Result { + /// Take ownership of the bytes, so the image outlives the document. + /// + /// **The only way out of the borrow, and it is a copy where the + /// bytes came from a buffer.** An image read from a `bufferView` + /// borrows the document's own memory; a caller that wants to hold + /// it after the document is dropped -- to write it to a file, say -- + /// has to pay for that once. An image decoded from a payload already + /// owns its bytes and pays nothing. + #[must_use] + pub fn into_owned(self) -> Image<'static> { + Image { + name: self.name, + media_type: self.media_type, + bytes: Cow::Owned(self.bytes.into_owned()), + } + } +} + +/// What a document says beyond its geometry. +/// +/// **Two tables that travel together because one caller wants both.** +/// A tool reporting what it imported needs the materials and the images +/// at once, and the alternative -- asking for each separately -- makes +/// the caller build the container dispatch and the buffer table twice. +#[derive(Clone, Debug, Default, PartialEq)] +pub struct Tables { + /// Every material, in the vocabulary the format states them in. + pub materials: Vec, + /// Every image, holding its own bytes. + pub images: Vec>, +} + +/// Read what a document says beyond its geometry, in either shape. +/// +/// **The sibling of [`read`], and it exists for the same reason.** A +/// binary glTF wraps its document in a container beside a chunk; a +/// `.gltf` is that document on its own. Every caller of these tables +/// would otherwise write that dispatch itself -- and the two that +/// already exist got it wrong in different ways before this function +/// did it once. +/// +/// **The images own their bytes**, which a borrowed form could not: the +/// parsed document lives inside this call and cannot be handed back +/// beside things that point into it. A caller that wants to avoid the +/// copy has [`images`] and can hold the parse itself. +/// +/// # Errors +/// +/// A [`GltfError`] naming the layer that refused and carrying its +/// numbers. +pub fn tables(bytes: &[u8]) -> Result { + let (document, chunk) = if glb::looks_like(bytes) { + let container = glb::read(bytes).map_err(GltfError::Container)?; + (container.json, container.binary) + } else { + (bytes, None) + }; + + let json = parse(document)?; + let root = json.root(); + let source = Source::of(root, chunk)?; + Ok(Tables { + materials: materials(root)?, + images: images(root, &source)? + .into_iter() + .map(Image::into_owned) + .collect(), + }) +} + /// Read the document's materials, in the vocabulary glTF states them. /// /// A material object has no required members, so an empty one is legal diff --git a/crates/mesh/tests/gltf.rs b/crates/mesh/tests/gltf.rs index df68dcf2..3fde7ed0 100644 --- a/crates/mesh/tests/gltf.rs +++ b/crates/mesh/tests/gltf.rs @@ -1117,6 +1117,65 @@ fn an_image_that_states_no_type_anywhere_reports_none() { assert_eq!(&*read[0].bytes, &[1, 2, 3, 4]); } +/// **The tables read from either shape of the same asset.** +/// +/// The whole reason this entry point exists: a caller would otherwise +/// write the container dispatch itself, and the two places that already +/// had it written each got it wrong in a different way. +#[test] +fn the_tables_read_from_a_document_and_from_a_container() { + let text = r#"{"asset":{"version":"2.0"}, +"materials":[{"name":"brass","metallicFactor":1.0,"roughnessFactor":0.25}], +"images":[{"name":"grain","uri":"data:image/png;base64,AQIDBA=="}]}"#; + + let alone = gltf::tables(text.as_bytes()).expect("a document on its own"); + assert_eq!(alone.materials.len(), 1); + assert_eq!(alone.materials[0].name.as_deref(), Some("brass")); + assert_eq!(alone.images.len(), 1); + assert_eq!(alone.images[0].name.as_deref(), Some("grain")); + assert_eq!(&*alone.images[0].bytes, &[1, 2, 3, 4]); + + // The same document wrapped, which the layers below cannot tell + // apart and this one must. + let wrapped = gltf::tables(&container(text, &[])).expect("the same document, wrapped"); + assert_eq!(wrapped, alone); +} + +/// **An image out of a container's chunk owns its bytes afterwards.** +/// +/// It is borrowed while the document is alive and this hands it back +/// after the document is gone, so the copy is the whole point rather +/// than an inefficiency: without it the value could not be returned at +/// all. +#[test] +fn an_image_stored_in_a_chunk_survives_the_document() { + let text = r#"{"asset":{"version":"2.0"}, +"buffers":[{"byteLength":4}], +"bufferViews":[{"buffer":0,"byteLength":4}], +"images":[{"bufferView":0,"mimeType":"image/png"}]}"#; + + let read = gltf::tables(&container(text, &[9, 8, 7, 6])).expect("one image, from the chunk"); + assert_eq!(read.images.len(), 1); + assert_eq!(&*read.images[0].bytes, &[9, 8, 7, 6]); + assert_eq!(read.images[0].media_type.as_deref(), Some("image/png")); +} + +/// A document with neither table has neither, which is not a refusal. +#[test] +fn a_document_with_no_tables_has_none() { + let read = gltf::tables(br#"{"asset":{"version":"2.0"}}"#).expect("nothing is not a refusal"); + assert_eq!(read, gltf::Tables::default()); +} + +/// **A refusal from either table is the whole call's refusal**, named by +/// the layer that made it rather than by this one. +#[test] +fn a_table_that_refuses_refuses_the_call() { + let refused = gltf::tables(br#"{"asset":{"version":"2.0"},"images":[{"uri":"grain.png"}]}"#) + .expect_err("a second file is not opened"); + assert_eq!(refused, GltfError::ExternalResource); +} + /// **A URI spelled with escapes is the URI it spells.** /// /// JSON lets a document write `/` as `\/`, and a `data:` payload is diff --git a/tools/cli/README.md b/tools/cli/README.md index 428b4380..9bccfbcb 100644 --- a/tools/cli/README.md +++ b/tools/cli/README.md @@ -41,6 +41,8 @@ options: the model file to read --out (ui-compile, asset-import; required) where the compiled document, or the canonical mesh, is written + --images (asset-import only) write the model's own images into + this directory; without it they are counted, not written --verify (asset-inspect only) check each entry against its digest --emit (determinism only) write this target's digests here --compare (determinism only, repeatable) a target report to compare @@ -193,27 +195,62 @@ fallback and refused by name, since it describes surfaces rather than their shape and would otherwise be reported as a truncated mesh: true, and no help to anyone. -STL, PLY and OBJ are read; glTF is not, yet. Nothing is written but -geometry, so a model's materials do not travel with it through this arm. +STL, PLY, OBJ and glTF are read -- both shapes of glTF, a `.gltf` +document carrying its own payloads and a `.glb` container. + +**A glTF model's materials and images are reported, and its images are +written only where you say.** + +``` +renew asset-import --from tree.gltf --out tree.msh --images build/textures/ +renew asset-pack --from build/ --pack game.rpk +``` + +The two commands compose and neither learns about the other: import +writes files, pack collects them. `--images` names a directory, which is +created if it is not there, and each file is named `image-` after the +document's own address for it -- not after the name the document gave +it, because a name in a file is not a thing that should decide where +bytes land on disk. The extension comes from the media type the document +stated: `image/png` and `image/jpeg`, the two the format's own schema +names. + +**Without `--images` nothing is written and everything is still +reported.** A command that scattered textures beside the blob because +the input happened to carry some would be writing files nobody asked +for; the flag is the asking, and the count is there either way so a +caller can learn there are images before deciding where they go. + +**No image is decoded, here or below.** A media type is reported and not +weighed -- until something has to name a file for it, which is the one +judgement this arm makes and the one place a type it cannot name is a +refusal. That refusal is about the name, not the bytes: they may be +perfectly readable by something else. `asset-import --json` adds, to the envelope every subcommand shares, the `format` detected, the `triangles` read, one boolean per optional -stream, the `bytes` written and the `out` path. **On a refusal it carries the variant's name +stream, the `bytes` written, the `out` path, the `materials` and +`images` the document carried, and the `images_written` paths. **On a refusal it carries the variant's name in `refusal` as well as the sentence in `stderr`**, because a message is for a person and a name is for a program: the sentences are meant to improve, and a script keying on one breaks when they do. -**Two of those names are this tool's own rather than a reader's**, and -the distinction is worth a script knowing. `NotGeometry` says the file +**Three of those names are this tool's own rather than a reader's**, and +the distinction is worth a script knowing. `UnknownMediaType` says the +document stated a type this tool cannot name a file extension for, or +stated none at all — a verdict about naming a file rather than about the +bytes, which may be perfectly readable by something else, and one that +only ever appears when `--images` asked for a file to be named. +`NotGeometry` says the file was read fine and describes materials rather than shape; `SameFile` says `--from` and `--out` name one file, which is refused before the file is opened, because writing the blob there would destroy the only thing that could produce it again. Every other name comes from the reader that refused, so it is a verdict about the file's contents. -`asset-import` reads meshes and nothing else. There is no image or -audio import, because a real one needs a decoder for each and one that -only copied bytes would be worse than its absence. +There is no audio import, and no image *decoding*: a real one needs a +decoder per format, and this arm carries image bytes without ever +looking inside them. ## The module inventory diff --git a/tools/cli/src/cli.rs b/tools/cli/src/cli.rs index 9c456372..63e2caea 100644 --- a/tools/cli/src/cli.rs +++ b/tools/cli/src/cli.rs @@ -191,6 +191,16 @@ pub struct Invocation { /// `ui-compile` and `asset-import` (parse enforces, and requires): /// where the compiled document, or the canonical mesh, is written. pub out: Option, + /// `asset-import` only (parse enforces): the directory an imported + /// model's images are written into. + /// + /// **Absent means none are written.** A model can carry its textures + /// inside itself, and a command that scattered them beside the blob + /// because the input happened to hold some would be writing files + /// nobody asked for. The flag is the asking, and without it the + /// images are still counted and reported -- so a caller can find out + /// there are any before deciding where they go. + pub images: Option, /// `asset-inspect` only (parse enforces): also check every payload /// against its recorded digest. Off by default because it reads every /// byte, where listing reads only the table. @@ -353,6 +363,7 @@ pub fn parse(arguments: &[String]) -> Result { let mut pack: Option = None; let mut from: Option = None; let mut out: Option = None; + let mut images: Option = None; let mut verify = false; let mut compare: Vec = Vec::new(); let mut features: Vec = Vec::new(); @@ -444,6 +455,10 @@ pub fn parse(arguments: &[String]) -> Result { let path = rest.next().ok_or(ParseError::MissingValue("--out"))?; out = Some(path.clone()); } + "--images" => { + let path = rest.next().ok_or(ParseError::MissingValue("--images"))?; + images = Some(path.clone()); + } "--from" => { let path = rest.next().ok_or(ParseError::MissingValue("--from"))?; from = Some(path.clone()); @@ -490,6 +505,7 @@ pub fn parse(arguments: &[String]) -> Result { pack.as_deref(), from.as_deref(), out.as_deref(), + images.as_deref(), verify, )?; check_determinism_mode(command, emit.as_deref(), &compare, target.as_deref())?; @@ -506,6 +522,7 @@ pub fn parse(arguments: &[String]) -> Result { pack, from, out, + images, verify, compare, emit, @@ -605,6 +622,7 @@ fn check_file_combination( pack: Option<&str>, from: Option<&str>, out: Option<&str>, + images: Option<&str>, verify: bool, ) -> Result<(), ParseError> { let is_pack = command == Some(Command::AssetPack); @@ -626,6 +644,9 @@ fn check_file_combination( if verify && !is_inspect { return Err(ParseError::UnexpectedArgument("--verify".to_string())); } + if images.is_some() && !is_import { + return Err(ParseError::UnexpectedArgument("--images".to_string())); + } // Then what each subcommand cannot work without. Both paths are the // whole input: guessing one would be worse than refusing. @@ -793,6 +814,8 @@ pub fn usage() -> String { " the model file to read\n", " --out (ui-compile, asset-import; required) where the\n", " compiled document, or the canonical mesh, is written\n", + " --images (asset-import only) write the model's own images into\n", + " this directory; without it they are counted, not written\n", " --verify (asset-inspect only) check each entry against its digest\n", " --emit (determinism only) write this target's digests here\n", " --compare (determinism only, repeatable) a target report to compare\n", @@ -846,6 +869,7 @@ mod tests { pack: None, from: None, out: None, + images: None, verify: false, compare: Vec::new(), emit: None, diff --git a/tools/cli/src/main.rs b/tools/cli/src/main.rs index 10e51d1d..672478a9 100644 --- a/tools/cli/src/main.rs +++ b/tools/cli/src/main.rs @@ -66,6 +66,7 @@ fn run(invocation: &Invocation) -> ExitCode { Command::AssetImport => run_asset_import( invocation.from.as_deref().unwrap_or_default(), invocation.out.as_deref().unwrap_or_default(), + invocation.images.as_deref(), invocation.json, ), // Parsing guarantees both paths, as it does for the pack. @@ -514,7 +515,215 @@ fn names_one_file(from: &Path, out: &Path) -> bool { .is_ok_and(|resolved| resolved.join(name) == source) } -fn run_asset_import(from: &str, out_path: &str, json_mode: bool) -> ExitCode { +/// A refusal on its way out of the image-writing helpers. +/// +/// **Carried rather than emitted**, because the helpers do not know +/// whether the caller asked for JSON and the one place that does is +/// already written. +struct ImportFailure { + message: String, + refusal: Option<&'static str>, +} + +/// The file extension a media type implies, if this tool can name one. +/// +/// **The one judgement this command makes about a media type**, and it +/// is a judgement about a *name*, not about the bytes: nothing here +/// decodes anything, and a type this returns `None` for may be perfectly +/// readable by something else. The reader below reports a type without +/// weighing it, and that stays true -- this weighs it only because a +/// file on disk has to be called something, and calling a JPEG `.png` +/// would be this tool inventing a fact the document never stated. +/// +/// The two named are the two the format's own schema names. A third is +/// added the day a document carries one and a caller needs the file. +fn extension_for(media_type: &str) -> Option<&'static str> { + // Compared without case, as the reader compares a buffer's: a media + // type is not case sensitive, and two parts of one tool should not + // disagree about that. + if media_type.eq_ignore_ascii_case("image/png") { + Some("png") + } else if media_type.eq_ignore_ascii_case("image/jpeg") { + Some("jpg") + } else { + None + } +} + +/// Write each image into `directory`, and answer with the paths written. +/// +/// **Named by index, not by the document's name for them.** An image's +/// `name` is whatever the file said and may be empty, repeated, or a +/// path -- and a tool that turned it into a filename would be letting a +/// document choose where bytes land on disk. The index is the document's +/// own address for the image, so a caller reading the JSON can match a +/// material's texture reference to a file without guessing. +fn write_images( + images: &[renew_mesh::gltf::Image<'static>], + directory: &str, +) -> Result, ImportFailure> { + let root = Path::new(directory); + // The flag names a destination for a set of files whose size the + // caller cannot know in advance, so making it is part of honouring + // it -- unlike `--out`, which names one file whose parent the + // caller already chose. + if let Err(error) = std::fs::create_dir_all(root) { + return Err(ImportFailure { + message: format!("cannot create {directory}: {error}"), + refusal: None, + }); + } + + let mut written = Vec::with_capacity(images.len()); + for (index, image) in images.iter().enumerate() { + // **Refused before anything is written.** A run that wrote four + // images and then stopped at the fifth would leave the caller + // deciding which half of a directory to trust; the loop is short + // enough that checking as it goes still fails on the first one. + let Some(stated) = image.media_type.as_deref() else { + return Err(ImportFailure { + message: format!( + "image {index} states no media type, so this cannot name a file for it" + ), + refusal: Some("UnknownMediaType"), + }); + }; + let Some(extension) = extension_for(stated) else { + return Err(ImportFailure { + message: format!( + "image {index} is `{stated}`, and this cannot name a file for that -- \ + the bytes may be fine, the name is the problem" + ), + refusal: Some("UnknownMediaType"), + }); + }; + + let path = root.join(format!("image-{index}.{extension}")); + if let Err(error) = std::fs::write(&path, &*image.bytes) { + return Err(ImportFailure { + message: format!("cannot write {}: {error}", path.display()), + refusal: None, + }); + } + written.push(path.display().to_string()); + } + Ok(written) +} + +/// One material, in the vocabulary the format states it in. +/// +/// **Every member, including the ones that are defaults.** A reader of +/// this JSON cannot tell a document that said nothing from one that said +/// the default, and the difference does not matter to a caller choosing +/// a shader -- but a missing key would make them write the defaults +/// themselves, in a second place, from a specification they would have +/// to go and read. +fn material_json(material: &renew_mesh::pbr::Material) -> Value { + // **The record's f32 widened, not reformatted.** Every one of these + // came out of a document as a decimal and went into an `f32`; going + // back out through `f64` is the only widening that adds no digits + // the file did not have. + let number = |value: f32| Value::Float(f64::from(value)); + let numbers = + |values: &[f32]| Value::Array(values.iter().copied().map(number).collect::>()); + + let mut fields = vec![ + ( + "name".to_string(), + material + .name + .as_ref() + .map_or(Value::Null, |name| Value::String(name.clone())), + ), + ("base_color".to_string(), numbers(&material.base_color)), + ("metallic".to_string(), number(material.metallic)), + ("roughness".to_string(), number(material.roughness)), + ("emissive".to_string(), numbers(&material.emissive)), + ( + "double_sided".to_string(), + Value::Bool(material.double_sided), + ), + ]; + // **The mode and its cutoff travel together**, because the format + // makes the cutoff meaningless without the mode and refuses one + // written beside no mode at all. + let (mode, cutoff) = match material.alpha { + renew_mesh::pbr::Alpha::Opaque => ("OPAQUE", Value::Null), + renew_mesh::pbr::Alpha::Mask { cutoff } => ("MASK", number(cutoff)), + renew_mesh::pbr::Alpha::Blend => ("BLEND", Value::Null), + }; + fields.push(("alpha_mode".to_string(), Value::String(mode.to_string()))); + fields.push(("alpha_cutoff".to_string(), cutoff)); + fields.push(( + "textures".to_string(), + Value::Array( + [ + ("base_color", material.base_color_map), + ("metallic_roughness", material.metallic_roughness_map), + ("normal", material.normal_map.map(|map| map.map)), + ("occlusion", material.occlusion_map.map(|map| map.map)), + ("emissive", material.emissive_map), + ] + .into_iter() + .filter_map(|(role, reference)| { + // **Only the maps the document named.** A null per absent + // role would make five keys that are almost always null, + // and a caller looking for what a material references + // would filter them right back out. + reference.map(|reference| { + Value::Object(vec![ + ("role".to_string(), Value::String(role.to_string())), + ( + "texture".to_string(), + Value::Number(i64::from(reference.texture)), + ), + ( + "uv_set".to_string(), + Value::Number(i64::from(reference.uv_set)), + ), + ]) + }) + }) + .collect(), + ), + )); + Value::Object(fields) +} + +/// One image, as what it is rather than as its bytes. +/// +/// The bytes themselves are never put in the envelope: they are a +/// texture, the JSON is a report, and base64 in a status line would make +/// a megabyte of output nobody reads. +fn image_json(image: &renew_mesh::gltf::Image<'static>) -> Value { + Value::Object(vec![ + ( + "name".to_string(), + image + .name + .as_ref() + .map_or(Value::Null, |name| Value::String(name.clone())), + ), + ( + "media_type".to_string(), + image + .media_type + .as_ref() + .map_or(Value::Null, |stated| Value::String(stated.clone())), + ), + ( + "bytes".to_string(), + Value::Number(i64::try_from(image.bytes.len()).unwrap_or(i64::MAX)), + ), + ]) +} + +fn run_asset_import( + from: &str, + out_path: &str, + images_dir: Option<&str>, + json_mode: bool, +) -> ExitCode { let started = Instant::now(); // **Checked before the file is opened, because reading first is what @@ -577,6 +786,29 @@ fn run_asset_import(from: &str, out_path: &str, json_mode: bool) -> ExitCode { } }; + // **What the document says beyond its shape**, read from the same + // bytes before they are dropped. Only glTF states materials in this + // vocabulary and only glTF carries its images inside itself, so + // every other format answers with two empty tables rather than with + // a refusal -- a caller asking what an STL's textures are is not + // making a mistake, it is getting the true answer. + let tables = match found { + renew_mesh::format::Format::Gltf | renew_mesh::format::Format::Glb => { + match renew_mesh::gltf::tables(&bytes) { + Ok(tables) => tables, + Err(refusal) => { + return import_failure( + &format!("{from}: {refusal}"), + Some(refusal.name()), + json_mode, + started, + ); + } + } + } + _ => renew_mesh::gltf::Tables::default(), + }; + // The file is not read again after this, and it can be as large as // the model: holding it across the write was a quarter of this // command's peak for nothing. @@ -594,38 +826,119 @@ fn run_asset_import(from: &str, out_path: &str, json_mode: bool) -> ExitCode { ); } - let triangles = i64::try_from(mesh.triangles()).unwrap_or(i64::MAX); - let size = i64::try_from(blob.len()).unwrap_or(i64::MAX); - if json_mode { + // **Written after the blob, and only where the caller said.** The + // flag is the asking; without it the images are still counted, so a + // caller learns there are some before deciding where they go. + let written = match images_dir { + None => Vec::new(), + Some(directory) => match write_images(&tables.images, directory) { + Ok(written) => written, + Err(failure) => { + return import_failure(&failure.message, failure.refusal, json_mode, started); + } + }, + }; + + report_import(&Import { + mesh: &mesh, + tables: &tables, + written: &written, + format, + out_path, + blob_bytes: blob.len(), + json_mode, + started, + }) +} + +/// Everything one successful import has to say for itself. +/// +/// **A record rather than eight parameters**, which is what the list had +/// grown to: three of them were paths or flags of the same type, and a +/// caller swapping two would have compiled. +struct Import<'a> { + mesh: &'a renew_mesh::Mesh, + tables: &'a renew_mesh::gltf::Tables, + written: &'a [String], + format: &'a str, + out_path: &'a str, + blob_bytes: usize, + json_mode: bool, + started: Instant, +} + +/// Say what was imported, in whichever form was asked for. +/// +/// Split from the import itself because reading a model and describing +/// one are different jobs, and the description is the half that grows +/// every time the reader below learns a new table. +fn report_import(report: &Import<'_>) -> ExitCode { + let triangles = i64::try_from(report.mesh.triangles()).unwrap_or(i64::MAX); + let size = i64::try_from(report.blob_bytes).unwrap_or(i64::MAX); + let (format, out_path) = (report.format, report.out_path); + if report.json_mode { // `envelope_base` puts `schema_version` first already, which is // what D11 asks of a public JSON surface. A second one here // would be a duplicate key in the object, and a reader taking // whichever it met first would be right by luck. - let mut fields = envelope_base("asset-import", "ok", 0, started, ""); - fields.push(("format".to_string(), Value::String(format.to_string()))); + let mut fields = envelope_base("asset-import", "ok", 0, report.started, ""); + fields.push(( + "format".to_string(), + Value::String(report.format.to_string()), + )); fields.push(("triangles".to_string(), Value::Number(triangles))); // Which optional streams survived the read, so a caller can tell // a lit mesh from a bare one without opening the blob. fields.push(( "face_normals".to_string(), - Value::Bool(!mesh.face_normals.is_empty()), + Value::Bool(!report.mesh.face_normals.is_empty()), )); fields.push(( "corner_normals".to_string(), - Value::Bool(!mesh.corner_normals.is_empty()), + Value::Bool(!report.mesh.corner_normals.is_empty()), )); fields.push(( "corner_texcoords".to_string(), - Value::Bool(!mesh.corner_texcoords.is_empty()), + Value::Bool(!report.mesh.corner_texcoords.is_empty()), )); fields.push(("bytes".to_string(), Value::Number(size))); - fields.push(("out".to_string(), Value::String(out_path.to_string()))); + fields.push(( + "out".to_string(), + Value::String(report.out_path.to_string()), + )); + // **Reported whether or not any were written**, which is the + // point of counting them separately from writing them. + fields.push(( + "materials".to_string(), + Value::Array(report.tables.materials.iter().map(material_json).collect()), + )); + fields.push(( + "images".to_string(), + Value::Array(report.tables.images.iter().map(image_json).collect()), + )); + fields.push(( + "images_written".to_string(), + Value::Array( + report + .written + .iter() + .map(|path| Value::String(path.clone())) + .collect(), + ), + )); fields.push(("refusal".to_string(), Value::Null)); emit_stdout_line(&Value::Object(fields).render()); } else { emit_stdout(&format!( "read {triangles} triangles of {format} into {out_path} ({size} bytes)\n" )); + let (materials, images) = (report.tables.materials.len(), report.tables.images.len()); + if materials > 0 || images > 0 { + emit_stdout(&format!(" {materials} materials, {images} images\n")); + } + for path in report.written { + emit_stdout(&format!(" wrote {path}\n")); + } } ExitCode::SUCCESS } diff --git a/tools/cli/tests/cli.rs b/tools/cli/tests/cli.rs index bb8eb738..e712ddea 100644 --- a/tools/cli/tests/cli.rs +++ b/tools/cli/tests/cli.rs @@ -2107,6 +2107,228 @@ const A_TRIANGLE_AS_STL: &str = "solid one\n\ vertex 0 0 0\n vertex 1 0 0\n vertex 0 1 0\n\ endloop\nendfacet\nendsolid one\n"; +/// A glTF document carrying one material and two images. +/// +/// Written here rather than borrowed, as every fixture in this tranche +/// is. The two payloads are four bytes each: `AQIDBA==` and `BQYHCA==`. +fn textured_document() -> &'static [u8] { + br#"{"asset":{"version":"2.0"},"scenes":[{"nodes":[0]}], +"nodes":[{"mesh":0}],"meshes":[{"primitives":[{"attributes":{"POSITION":0},"material":0}]}], +"accessors":[{"bufferView":0,"componentType":5126,"count":3,"type":"VEC3"}], +"buffers":[{"byteLength":36,"uri":"data:application/octet-stream;base64,AAAAAAAAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAgD8AAAAA"}], +"bufferViews":[{"buffer":0,"byteLength":36}], +"textures":[{"source":0}], +"materials":[{"name":"brass","pbrMetallicRoughness":{"baseColorFactor":[0.5,0.25,0.125,1.0], +"metallicFactor":1.0,"roughnessFactor":0.25,"baseColorTexture":{"index":0}}, +"emissiveFactor":[0.0,0.0,0.25],"alphaMode":"MASK","alphaCutoff":0.75,"doubleSided":true}], +"images":[{"name":"grain","uri":"data:image/png;base64,AQIDBA=="}, +{"uri":"data:image/jpeg;base64,BQYHCA=="}]}"# +} + +/// **Materials and images are reported whether or not they are written.** +/// +/// The count is the half a caller needs to decide anything: without it +/// they would have to ask for the files in order to learn there are any. +#[test] +fn asset_import_reports_the_tables_without_writing_them() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-tables")?; + let model = directory.join("scene.gltf"); + let blob = directory.join("out.msh"); + fs::write(&model, textured_document())?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &blob.to_string_lossy(), + ])?; + assert!(output.status.success(), "a textured document imports"); + let reported = String::from_utf8_lossy(&output.stdout); + validate_json(reported.trim()).expect("one valid document"); + + assert!( + reported.contains("\"name\":\"brass\""), + "the material is named as the document named it: {reported:?}" + ); + assert!( + reported.contains("\"alpha_mode\":\"MASK\"") && reported.contains("\"alpha_cutoff\":0.75"), + "the mode and its cutoff travel together: {reported:?}" + ); + assert!( + reported.contains("\"role\":\"base_color\""), + "and the map it references is named by role: {reported:?}" + ); + assert!( + reported.contains("\"media_type\":\"image/png\"") + && reported.contains("\"media_type\":\"image/jpeg\""), + "both images are reported with the types the document stated: {reported:?}" + ); + assert!( + reported.contains("\"images_written\":[]"), + "and none were written, because none were asked for: {reported:?}" + ); + + // **The claim the whole flag rests on.** Nothing but the blob. + let mut left: Vec = fs::read_dir(&directory)? + .filter_map(|entry| Some(entry.ok()?.file_name().to_string_lossy().into_owned())) + .collect(); + left.sort(); + assert_eq!(left, ["out.msh", "scene.gltf"], "no file nobody asked for"); + Ok(()) +} + +/// **`--images` writes them, named by index and typed by media type.** +#[test] +fn asset_import_writes_images_where_it_is_told() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-images")?; + let model = directory.join("scene.gltf"); + let blob = directory.join("out.msh"); + let textures = directory.join("textures"); + fs::write(&model, textured_document())?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &blob.to_string_lossy(), + "--images", + &textures.to_string_lossy(), + ])?; + assert!( + output.status.success(), + "the images are written: {}", + String::from_utf8_lossy(&output.stdout) + ); + + // The directory is made, because the flag asked for a destination + // whose size the caller could not know. + let mut written: Vec = fs::read_dir(&textures)? + .filter_map(|entry| Some(entry.ok()?.file_name().to_string_lossy().into_owned())) + .collect(); + written.sort(); + assert_eq!( + written, + ["image-0.png", "image-1.jpg"], + "named by the document's own address for them, typed by what it said they are" + ); + assert_eq!(fs::read(textures.join("image-0.png"))?, [1, 2, 3, 4]); + assert_eq!(fs::read(textures.join("image-1.jpg"))?, [5, 6, 7, 8]); + Ok(()) +} + +/// **A media type this tool cannot name a file for is refused, and the +/// refusal says the name is the problem rather than the bytes.** +#[test] +fn asset_import_refuses_to_invent_a_file_extension() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-unnameable")?; + let model = directory.join("scene.gltf"); + fs::write( + &model, + br#"{"asset":{"version":"2.0"},"scenes":[{"nodes":[0]}], +"nodes":[{"mesh":0}],"meshes":[{"primitives":[{"attributes":{"POSITION":0}}]}], +"accessors":[{"bufferView":0,"componentType":5126,"count":3,"type":"VEC3"}], +"buffers":[{"byteLength":36,"uri":"data:application/octet-stream;base64,AAAAAAAAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAgD8AAAAA"}], +"bufferViews":[{"buffer":0,"byteLength":36}], +"images":[{"uri":"data:image/tiff;base64,AQIDBA=="}]}"#, + )?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + "--images", + &directory.join("textures").to_string_lossy(), + ])?; + assert!(!output.status.success(), "it cannot name that file"); + let reported = String::from_utf8_lossy(&output.stdout); + assert!( + reported.contains("\"refusal\":\"UnknownMediaType\""), + "named for a program: {reported:?}" + ); + assert!( + reported.contains("the bytes may be fine, the name is the problem"), + "and said for a person: {reported:?}" + ); + + // **The same document imports without the flag.** The type is + // reported and not judged until something has to name a file. + let allowed = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + ])?; + assert!( + allowed.status.success(), + "a type nobody has to name is not a refusal: {}", + String::from_utf8_lossy(&allowed.stdout) + ); + Ok(()) +} + +/// **A format that carries no images answers with none, not a refusal.** +/// +/// Asking an STL for its textures is a fair question with a true answer. +#[test] +fn asset_import_reports_empty_tables_for_a_format_without_them() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-untextured")?; + let model = directory.join("model.stl"); + // One binary-STL triangle: an 80-byte header, a count, and one facet. + let mut stl = vec![0_u8; 80]; + stl.extend_from_slice(&1_u32.to_le_bytes()); + for value in [ + 0.0_f32, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, + ] { + stl.extend_from_slice(&value.to_le_bytes()); + } + stl.extend_from_slice(&0_u16.to_le_bytes()); + fs::write(&model, &stl)?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + "--images", + &directory.join("textures").to_string_lossy(), + ])?; + assert!( + output.status.success(), + "an STL with no images is not a broken STL: {}", + String::from_utf8_lossy(&output.stdout) + ); + let reported = String::from_utf8_lossy(&output.stdout); + assert!( + reported.contains("\"materials\":[]") && reported.contains("\"images\":[]"), + "both tables are empty and both are reported: {reported:?}" + ); + Ok(()) +} + +/// `--images` belongs to `asset-import` and to nothing else. +#[test] +fn images_is_refused_on_another_subcommand() -> std::io::Result<()> { + let output = run(&["asset-pack", "--images", "textures"])?; + assert!(!output.status.success(), "it is not that command's flag"); + let said = String::from_utf8_lossy(&output.stderr); + assert!( + said.contains("--images"), + "and the caller hears about the flag they typed: {said:?}" + ); + Ok(()) +} + /// `asset-import` end to end: the blob lands where `--out` says, the /// format is reported as what it is, and the reader that owns it accepts /// the result. From a925bd5f58b108c02f4a635e611dc545226db156 Mon Sep 17 00:00:00 2001 From: CagdasErturk Date: Wed, 9 Sep 2026 14:56:31 +0300 Subject: [PATCH 2/6] test(cli): the arms the import grew, and the exempt block that moved under them Six sites the new import code reached and nothing executed, each given a test rather than an exemption: the two filesystem failures (`--images` pointed at an existing file, so the directory cannot be made; and at a directory already holding `image-0.png`, so the file cannot be written), the half of `UnknownMediaType` that fires when a document names no type at all, the arm that carries a table's refusal out of an import whose geometry read perfectly, the `OPAQUE` and `BLEND` alpha modes -- the first fixture only used `MASK` -- and the whole human-readable output arm, which is a second piece of code that would otherwise ship to everyone who does not pass `--json` having never run once. The one exempt block in this file is the determinism emit half, and the import grew the code above it, so all fifty-two of its lines moved by exactly 313. Checked rather than assumed: every old position maps to a reported-uncovered position with that same delta, and nothing else in the block changed. --- coverage-exemptions.toml | 2 +- tools/cli/tests/cli.rs | 232 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 233 insertions(+), 1 deletion(-) diff --git a/coverage-exemptions.toml b/coverage-exemptions.toml index dd706d4b..5b719fb2 100644 --- a/coverage-exemptions.toml +++ b/coverage-exemptions.toml @@ -171,7 +171,7 @@ reason = "The arm that clears a voice whose sound index is missing. A voice is o [[exempt]] file = "tools/cli/src/main.rs" -lines = [1766, 1767, 1769, 1770, 1771, 1772, 1773, 1784, 1788, 1789, 1790, 1792, 1796, 1810, 1811, 1812, 1814, 1815, 1816, 1817, 1818, 1819, 1820, 1821, 1822, 1823, 1824, 1825, 1827, 1831, 1832, 1855, 1873, 1874, 1875, 1876, 1883, 1884, 1885, 1886, 1887, 1888, 1889, 1892, 1896, 1897, 1898, 1902, 2282, 2283, 2284, 2285] +lines = [2079, 2080, 2082, 2083, 2084, 2085, 2086, 2097, 2101, 2102, 2103, 2105, 2109, 2123, 2124, 2125, 2127, 2128, 2129, 2130, 2131, 2132, 2133, 2134, 2135, 2136, 2137, 2138, 2140, 2144, 2145, 2168, 2186, 2187, 2188, 2189, 2196, 2197, 2198, 2199, 2200, 2201, 2202, 2205, 2209, 2210, 2211, 2215, 2595, 2596, 2597, 2598] reason = "The determinism emit half past its first child, plus the two arms that answer for a target this process is not running on. Reaching the emit arms from a test would mean building and running pinned runs under instrumentation: the report-reading arms need the first pinned run (renew-ui, no arguments) to compile and succeed, and the leg construction and write at the tail need all eleven - spread across six workspace packages, four of them samples and two engine crates - to succeed. The success path is not untested: the three determinism legs execute it on Linux, Windows and macOS on every push, and a failure there is what a broken emit looks like. Everything testable without a subprocess has been moved out - digests_from_output has seven cases beside it, digest_name is the one spelling both sides call, emit_note is unit-tested on both branches, pinned_invocation asserts the --target pass-through with no device, and the emit-red path is driven end to end by tests/targets.rs in both output modes. What is left is process orchestration, the leg written when every child succeeded, and the arms no gating push can reach: a compiler that answered and failed when asked its own version, a child that cannot start at all, a child whose report is unreadable, a leg file that cannot be written, one pinned run claiming a digest name another already used, and a working directory that has ceased to exist beneath the process. The --target arms are their own case: the refusal for a triple the table cannot name is reached by nothing at all, because every lane passes either a known triple or none, and it exists so a target added to CI and forgotten in that table fails loudly instead of emitting a leg labelled by a guess; the arm that labels a leg from its triple is executed only by the Android emulator lane, which is advisory and cannot redden main, so it is held by a lane whose red only a reader sees - which is the honest description until that row is signed and the lane gates." [[exempt]] diff --git a/tools/cli/tests/cli.rs b/tools/cli/tests/cli.rs index e712ddea..8784b86b 100644 --- a/tools/cli/tests/cli.rs +++ b/tools/cli/tests/cli.rs @@ -2107,6 +2107,238 @@ const A_TRIANGLE_AS_STL: &str = "solid one\n\ vertex 0 0 0\n vertex 1 0 0\n vertex 0 1 0\n\ endloop\nendfacet\nendsolid one\n"; +/// A glTF document with one triangle and whatever tables are asked for. +/// +/// The same triangle every fixture here uses, so a test that is about a +/// material or an image is not also about geometry. +fn document_with(tables: &str) -> Vec { + format!( + r#"{{"asset":{{"version":"2.0"}},"scenes":[{{"nodes":[0]}}], +"nodes":[{{"mesh":0}}],"meshes":[{{"primitives":[{{"attributes":{{"POSITION":0}}}}]}}], +"accessors":[{{"bufferView":0,"componentType":5126,"count":3,"type":"VEC3"}}], +"buffers":[{{"byteLength":36,"uri":"data:application/octet-stream;base64,AAAAAAAAAAAAAAAAAACAPwAAAAAAAAAAAAAAAAAAgD8AAAAA"}}], +"bufferViews":[{{"buffer":0,"byteLength":36}}]{tables}}}"# + ) + .into_bytes() +} + +/// **An image that states no type at all is refused for saying nothing**, +/// which is the other half of `UnknownMediaType`. +/// +/// The first half is a type this tool cannot name. This is a document +/// that named none: `data:;base64,` is a payload with an empty media +/// type, and the reader reports the absence rather than inventing RFC +/// 2397's default. +#[test] +fn asset_import_refuses_an_image_that_names_no_type() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-untyped")?; + let model = directory.join("scene.gltf"); + fs::write( + &model, + document_with(r#","images":[{"uri":"data:;base64,AQIDBA=="}]"#), + )?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + "--images", + &directory.join("textures").to_string_lossy(), + ])?; + assert!(!output.status.success(), "nothing said is not a name"); + let reported = String::from_utf8_lossy(&output.stdout); + assert!( + reported.contains("\"refusal\":\"UnknownMediaType\"") + && reported.contains("states no media type"), + "and the sentence says which half it is: {reported:?}" + ); + Ok(()) +} + +/// **A table that refuses refuses the import**, even where the geometry +/// beside it reads perfectly. +/// +/// The two halves of a document are read separately, so this is the one +/// case that proves the second half is read at all: the triangle is +/// fine and the image names a second file. +#[test] +fn asset_import_carries_a_table_refusal_out() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-table-refusal")?; + let model = directory.join("scene.gltf"); + fs::write(&model, document_with(r#","images":[{"uri":"grain.png"}]"#))?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + ])?; + assert!( + !output.status.success(), + "a file this reader will not open is a refusal, geometry or no geometry" + ); + let reported = String::from_utf8_lossy(&output.stdout); + assert!( + reported.contains("\"refusal\":\"ExternalResource\""), + "named by the layer that refused rather than by this tool: {reported:?}" + ); + Ok(()) +} + +/// **A destination that cannot be made is reported as itself**, not as a +/// refusal about the model. +#[test] +fn asset_import_says_when_it_cannot_make_the_directory() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-blocked-dir")?; + let model = directory.join("scene.gltf"); + fs::write( + &model, + document_with(r#","images":[{"uri":"data:image/png;base64,AQIDBA=="}]"#), + )?; + // A file where the directory would go, so making it cannot succeed. + let blocked = directory.join("textures"); + fs::write(&blocked, b"not a directory")?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + "--images", + &blocked.to_string_lossy(), + ])?; + assert!(!output.status.success(), "there is a file in the way"); + let reported = String::from_utf8_lossy(&output.stdout); + assert!( + reported.contains("cannot create"), + "and it says so as a filesystem problem: {reported:?}" + ); + assert!( + reported.contains("\"refusal\":null"), + "with no refusal name, because no reader refused: {reported:?}" + ); + Ok(()) +} + +/// **A file that cannot be written is reported as itself too.** +#[test] +fn asset_import_says_when_it_cannot_write_an_image() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-blocked-file")?; + let model = directory.join("scene.gltf"); + fs::write( + &model, + document_with(r#","images":[{"uri":"data:image/png;base64,AQIDBA=="}]"#), + )?; + // A directory where the first image's file would go. + let textures = directory.join("textures"); + fs::create_dir_all(textures.join("image-0.png"))?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + "--images", + &textures.to_string_lossy(), + ])?; + assert!( + !output.status.success(), + "that name is taken by a directory" + ); + let reported = String::from_utf8_lossy(&output.stdout); + assert!( + reported.contains("cannot write") && reported.contains("image-0.png"), + "naming the file it could not write: {reported:?}" + ); + Ok(()) +} + +/// **All three alpha modes are reported as the format spells them**, and +/// the cutoff is null for the two that do not have one. +#[test] +fn asset_import_reports_every_alpha_mode() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-alpha")?; + let model = directory.join("scene.gltf"); + fs::write( + &model, + document_with( + r#","materials":[{"name":"plain"},{"name":"cut","alphaMode":"MASK","alphaCutoff":0.25}, +{"name":"glass","alphaMode":"BLEND"}]"#, + ), + )?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + ])?; + assert!(output.status.success(), "three materials import"); + let reported = String::from_utf8_lossy(&output.stdout); + validate_json(reported.trim()).expect("one valid document"); + for mode in ["OPAQUE", "MASK", "BLEND"] { + assert!( + reported.contains(&format!("\"alpha_mode\":\"{mode}\"")), + "{mode} is reported: {reported:?}" + ); + } + assert_eq!( + reported.matches("\"alpha_cutoff\":null").count(), + 2, + "and only the masked one carries a cutoff: {reported:?}" + ); + Ok(()) +} + +/// **The human-readable form says the same things the JSON does.** +/// +/// Two output modes are two pieces of code, and a tool whose prose arm +/// was never run would ship a panic to whoever did not pass `--json`. +#[test] +fn asset_import_says_what_it_wrote_in_prose_too() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-prose-tables")?; + let model = directory.join("scene.gltf"); + let textures = directory.join("textures"); + fs::write( + &model, + document_with( + r#","materials":[{"name":"brass"}],"images":[{"uri":"data:image/png;base64,AQIDBA=="}]"#, + ), + )?; + + let output = run(&[ + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + "--images", + &textures.to_string_lossy(), + ])?; + assert!(output.status.success(), "it imports without --json too"); + let said = String::from_utf8_lossy(&output.stdout); + assert!( + said.contains("1 materials, 1 images"), + "the counts are said: {said:?}" + ); + assert!( + said.contains("wrote") && said.contains("image-0.png"), + "and so is each file: {said:?}" + ); + Ok(()) +} + /// A glTF document carrying one material and two images. /// /// Written here rather than borrowed, as every fixture in this tranche From e4e8188928909e25395f00e5422be922e9da0723 Mon Sep 17 00:00:00 2001 From: CagdasErturk Date: Wed, 9 Sep 2026 15:08:51 +0300 Subject: [PATCH 3/6] fix(cli): a model's metadata is reported, not a gate on reading the model The import read a glTF's material and image tables whether or not `--images` asked for them, and the image reader refuses a `uri` naming a second file. So a document with embedded geometry and ordinary sibling `.png` textures -- the commonest shape a real glTF has -- stopped importing, for a reason that says nothing about whether its geometry is sound. Reading a model is what the command is for. A table that will not read is reported now: `materials`, `textures` and `images` come back null with `tables_refusal` carrying the refusal's name and its sentence, and the blob is written. It stays fatal when `--images` was given, because then the caller asked for the thing that cannot be delivered. Nothing is written until every image's name is settled. The loop checked one image and then wrote it, so a document whose third image had a media type this cannot name left the first two on disk beside a blob -- the half-written directory the comment beside it claimed was impossible. A material names a texture and a texture names an image, which is a step the report skipped: a caller pairing a material to a file by index was wrong whenever a texture's index was not its image's. `gltf::textures` reads that table and the envelope carries it. The material report also dropped a normal map's scale and an occlusion map's strength -- the two values a default cannot recover once a document has stated them -- while the doc above it argued that nothing is dropped. `--images ""` named the working directory, and `create_dir_all("")` succeeds, so it scattered a model's textures wherever the process happened to be standing. Refused where the other path rules live. `Format::tables` sits beside `Format::read` and is exhaustive, so the tool no longer matches on the format enum behind a wildcard and a format added later has to answer the question before it compiles. Its `None` means "this format does not state these in this vocabulary", which is why the fields are null rather than empty for an OBJ: Wavefront's material model is a different thing, and an empty array would be saying something false about the file. `ExternalResource` said a document kept its *geometry* somewhere else, which was true when only a buffer could raise it. An image raises it too. The corpus image floor parsed each seed as JSON directly, so every binary glTF in it was skipped silently and no container seed ever reached the image table. --- crates/mesh/README.md | 21 +-- crates/mesh/src/format.rs | 24 ++++ crates/mesh/src/gltf.rs | 77 ++++++++-- crates/mesh/tests/corpus_replay.rs | 17 ++- tools/cli/README.md | 54 +++++-- tools/cli/src/cli.rs | 7 + tools/cli/src/json.rs | 22 +-- tools/cli/src/main.rs | 219 +++++++++++++++++++++-------- tools/cli/tests/cli.rs | 192 ++++++++++++++++++++++--- tools/cli/tests/source_hygiene.rs | 2 +- 10 files changed, 498 insertions(+), 137 deletions(-) diff --git a/crates/mesh/README.md b/crates/mesh/README.md index e7ba12ac..5df2b837 100644 --- a/crates/mesh/README.md +++ b/crates/mesh/README.md @@ -372,22 +372,11 @@ between them would be answering a question the document did not settle. A `uri` is a payload this reader decodes or a second file it will not open — the same pair of answers a buffer's `uri` gets. -**`mimeType` is the document's answer when it gives one.** An image may -state its type beside itself, in its payload's URI, or in both; the -format requires one beside a view, because a view carries bytes and -nothing about them. The two are never compared. The format relates -neither to the other — its rule is that a payload's media type match its -*content*, which nothing here can check because nothing here decodes — -so refusing a disagreement would refuse conformant documents, and a PNG -carried as `application/octet-stream` is ordinary. The one the format -makes mandatory wins, and the payload's is what is left when there is no -other. - -**An empty statement is an absence, on both sides.** A payload may carry -no type, and `mimeType` may be written `""` because the schema ends in a -permissive string. Both are a document saying nothing about its bytes, -so both read as none — the reported type is never an empty string, which -is a value every caller would have to know to treat as absent. +**`mimeType` wins where a document states one, and the two labels are +never compared** — the format relates neither to the other, and an +absence on either side reads as an absence rather than as an empty +string. The reasoning is on `gltf::Image::media_type`, which is where it +belongs and where it will stay correct. `gltf::tables` reads the materials and the images together from either shape of the asset, doing the container dispatch that `gltf::read` does diff --git a/crates/mesh/src/format.rs b/crates/mesh/src/format.rs index 0bfe758e..fbb8bcc9 100644 --- a/crates/mesh/src/format.rs +++ b/crates/mesh/src/format.rs @@ -141,6 +141,30 @@ impl Format { Self::Mtl => None, } } + + /// Read what this format says beyond its geometry, or `None` if it + /// says nothing this crate has a vocabulary for. + /// + /// **`None` is not an error, and it is not "no materials".** It is + /// "this format does not state them in the vocabulary this returns". + /// OBJ and MTL do carry materials, in a different model entirely -- + /// Wavefront's, which `mtl::read` answers with and which is not + /// convertible into this one without inventing values. A caller that + /// reports an empty table for an OBJ would be saying something false + /// about the file. + /// + /// Exhaustive on purpose, like [`read`](Self::read): a format added + /// to this enum has to answer this question before it compiles, + /// which is the compile-time check a wildcard would throw away. + #[must_use] + pub fn tables(self, bytes: &[u8]) -> Option> { + match self { + Self::Glb | Self::Gltf => { + Some(gltf::tables(bytes).map_err(|refusal| MeshError::Gltf(Box::new(refusal)))) + } + Self::Obj | Self::Mtl | Self::Stl | Self::Ply | Self::Blob => None, + } + } } /// Which format these bytes are. diff --git a/crates/mesh/src/gltf.rs b/crates/mesh/src/gltf.rs index a8d0d9b6..9eaa708a 100644 --- a/crates/mesh/src/gltf.rs +++ b/crates/mesh/src/gltf.rs @@ -132,12 +132,16 @@ pub enum GltfError { count: usize, }, - /// A buffer this reader will not go and get. + /// A resource this reader will not go and get: a buffer, or an image. /// /// Refused rather than ignored: a document whose geometry lives in a /// second file describes a model this cannot assemble, and returning /// what it *can* assemble would be returning half a model without - /// saying so. + /// saying so. The same holds for an image, which is why the two + /// share a refusal -- but **the caller's answer differs**, because a + /// missing texture leaves a whole model where a missing buffer + /// leaves none, so a caller that only wanted geometry is entitled to + /// carry on past this one. ExternalResource, /// A payload embedded in the document that will not decode. @@ -316,7 +320,7 @@ impl core::fmt::Display for GltfError { } => write!(f, "`{table}[{index}]` of a table holding {count}"), Self::ExternalResource => write!( f, - "this document keeps its geometry somewhere else, and this reader takes bytes" + "this document keeps a resource somewhere else, and this reader takes bytes" ), Self::Payload(refusal) => write!(f, "an embedded payload will not decode: {refusal}"), Self::BufferWithoutSource { buffer } => write!( @@ -704,11 +708,13 @@ pub struct Image<'a> { /// document's answer, and the URI's is what is left when it gives /// none. /// - /// **`None` is the document having said nothing**, which is not the - /// same as `Some("")` — a URI may carry an empty media type, and - /// the decoder below reports that rather than applying RFC 2397's - /// default, so that a caller needing an explicit type can see there - /// was none. + /// **`None` is the document having said nothing, whichever side said + /// it.** A `mimeType` written `""` and a payload carrying no type at + /// all both normalise to `None`, so this is never `Some("")` — a + /// value every caller would otherwise have to know to treat as + /// absent. The decoder below reports a payload's missing type as an + /// empty string rather than applying RFC 2397's default, and this is + /// where that becomes an absence. /// /// **Reported, never judged.** Which types are readable is a fact /// about what the caller is doing with the bytes, and this layer @@ -881,8 +887,8 @@ pub fn images<'s>(root: Value<'_>, source: &'s Source<'_>) -> Result { /// Take ownership of the bytes, so the image outlives the document. /// - /// **The only way out of the borrow, and it is a copy where the - /// bytes came from a buffer.** An image read from a `bufferView` + /// **The way out of the borrow that does not rebuild the value, and + /// it is a copy where the bytes came from a buffer.** An image read from a `bufferView` /// borrows the document's own memory; a caller that wants to hold /// it after the document is dropped -- to write it to a file, say -- /// has to pay for that once. An image decoded from a payload already @@ -907,6 +913,15 @@ impl Image<'_> { pub struct Tables { /// Every material, in the vocabulary the format states them in. pub materials: Vec, + /// Which image each texture draws its bytes from, where it says. + /// + /// **The step between a material and an image, which is a step.** A + /// material names a *texture*, and a texture names a *source* — so a + /// caller holding a material's `TextureRef` and a list of images + /// cannot pair them without this. `source` is optional in the + /// format, because an extension may supply the image instead, and a + /// texture that names none reads as `None` rather than as zero. + pub textures: Vec>, /// Every image, holding its own bytes. pub images: Vec>, } @@ -915,10 +930,13 @@ pub struct Tables { /// /// **The sibling of [`read`], and it exists for the same reason.** A /// binary glTF wraps its document in a container beside a chunk; a -/// `.gltf` is that document on its own. Every caller of these tables -/// would otherwise write that dispatch itself -- and the two that -/// already exist got it wrong in different ways before this function -/// did it once. +/// `.gltf` is that document on its own, and a caller that wants the +/// tables out of either without holding the parse would otherwise write +/// that dispatch itself. +/// +/// It does not replace [`images`] for a caller that can hold the parse: +/// the images here own their bytes, so which source each came from is +/// no longer visible in the value. /// /// **The images own their bytes**, which a borrowed form could not: the /// parsed document lives inside this call and cannot be handed back @@ -942,6 +960,7 @@ pub fn tables(bytes: &[u8]) -> Result { let source = Source::of(root, chunk)?; Ok(Tables { materials: materials(root)?, + textures: textures(root)?, images: images(root, &source)? .into_iter() .map(Image::into_owned) @@ -949,6 +968,36 @@ pub fn tables(bytes: &[u8]) -> Result { }) } +/// Which image each texture draws its bytes from. +/// +/// **Only `source`, because that is the only member anything here can +/// follow.** A texture also names a sampler, and a sampler is filtering +/// and wrapping — facts for whoever draws with it, and nothing this +/// crate has a home for yet. +/// +/// # Errors +/// +/// A [`GltfError`]: `Document` for a table or an entry of the wrong +/// kind, or a `source` that is not a number. +pub fn textures(root: Value<'_>) -> Result>, GltfError> { + let Some(table) = root.get("textures") else { + return Ok(Vec::new()); + }; + + let mut out = Vec::new(); + for entry in table.elements().map_err(GltfError::Document)? { + // An object, for the reason every other table checks: every + // member of a number answers absent, so a texture written `5` + // would read as one naming no source. + entry.entries().map_err(GltfError::Document)?; + out.push(match entry.get("source") { + None => None, + Some(source) => Some(source.as_u32()?), + }); + } + Ok(out) +} + /// Read the document's materials, in the vocabulary glTF states them. /// /// A material object has no required members, so an empty one is legal diff --git a/crates/mesh/tests/corpus_replay.rs b/crates/mesh/tests/corpus_replay.rs index 60d0a26f..dad38a7c 100644 --- a/crates/mesh/tests/corpus_replay.rs +++ b/crates/mesh/tests/corpus_replay.rs @@ -1315,11 +1315,24 @@ fn the_image_layer_is_exercised(distinct: &BTreeSet>) { let (mut from_view, mut from_payload) = (0_usize, 0_usize); let mut reached: BTreeSet<&'static str> = BTreeSet::new(); for bytes in distinct { - let Ok(json) = gltf::parse(bytes) else { + // **Either shape, which this gate used to get wrong.** A + // container's bytes are not JSON, so parsing them directly + // failed and the seed was skipped -- silently, which is the + // worst way for a gate to miss something. Every `.glb` in the + // corpus went past this check without being looked at. + let (document, chunk) = if glb::looks_like(bytes) { + match glb::read(bytes) { + Ok(container) => (container.json, container.binary), + Err(_) => continue, + } + } else { + (&bytes[..], None) + }; + let Ok(json) = gltf::parse(document) else { continue; }; let root = json.root(); - let Ok(source) = gltf::Source::of(root, None) else { + let Ok(source) = gltf::Source::of(root, chunk) else { continue; }; match gltf::images(root, &source) { diff --git a/tools/cli/README.md b/tools/cli/README.md index 9bccfbcb..f852c395 100644 --- a/tools/cli/README.md +++ b/tools/cli/README.md @@ -195,8 +195,9 @@ fallback and refused by name, since it describes surfaces rather than their shape and would otherwise be reported as a truncated mesh: true, and no help to anyone. -STL, PLY, OBJ and glTF are read -- both shapes of glTF, a `.gltf` -document carrying its own payloads and a `.glb` container. +STL, PLY, OBJ, glTF and this crate's own `.msh` blob are read -- both +shapes of glTF, a `.gltf` document carrying its own payloads and a +`.glb` container. **A glTF model's materials and images are reported, and its images are written only where you say.** @@ -208,18 +209,36 @@ renew asset-pack --from build/ --pack game.rpk The two commands compose and neither learns about the other: import writes files, pack collects them. `--images` names a directory, which is -created if it is not there, and each file is named `image-` after the -document's own address for it -- not after the name the document gave -it, because a name in a file is not a thing that should decide where -bytes land on disk. The extension comes from the media type the document -stated: `image/png` and `image/jpeg`, the two the format's own schema -names. - -**Without `--images` nothing is written and everything is still -reported.** A command that scattered textures beside the blob because -the input happened to carry some would be writing files nobody asked -for; the flag is the asking, and the count is there either way so a -caller can learn there are images before deciding where they go. +created if there is at least one image to put in it. Each file is named +`image-.` -- the index is the document's own address for the +image, not the name the document gave it, because a name in a file is +not a thing that should decide where bytes land on disk. The extension +comes from the media type: `image/png` becomes `.png` and `image/jpeg` +becomes `.jpg`, those being the two the format's own schema names. + +A material names a *texture* and a texture names a *source*, so the +envelope carries the `textures` table too -- without it a caller holding +a material and a directory of files cannot pair them. + +**Without `--images` nothing is written and everything the document +carries inside itself is still reported.** A command that scattered +textures beside the blob because the input happened to carry some would +be writing files nobody asked for; the flag is the asking, and the +report is there either way so a caller can learn there are images before +deciding where they go. + +**A glTF that keeps its textures in files beside it is not refused.** +This reader does not open a second file, so it reports no tables and +says so in `tables_refusal` -- the geometry is still read and the blob +is still written, because whether a texture is reachable says nothing +about whether the shape is sound. Passing `--images` for such a model +*is* a refusal: then the caller asked for the thing that cannot be +delivered. + +**Files already in the directory are left alone.** Two models imported +into one directory leave the union of their images, so a caller that +globs it gets both. `images_written` in the envelope names exactly what +this run wrote, and is the list to trust. **No image is decoded, here or below.** A media type is reported and not weighed -- until something has to name a file for it, which is the one @@ -230,7 +249,12 @@ perfectly readable by something else. `asset-import --json` adds, to the envelope every subcommand shares, the `format` detected, the `triangles` read, one boolean per optional stream, the `bytes` written, the `out` path, the `materials` and -`images` the document carried, and the `images_written` paths. **On a refusal it carries the variant's name +`images` the document carried, the `textures` table that joins them, +`tables_refusal` when those could not be read, and the `images_written` +paths. `materials`, `textures` and `images` are `null` rather than empty +for a format that does not state them in this vocabulary -- an OBJ +carries materials in Wavefront's model, which this arm does not convert +into, so an empty array there would be saying something false. **On a refusal it carries the variant's name in `refusal` as well as the sentence in `stderr`**, because a message is for a person and a name is for a program: the sentences are meant to improve, and a script keying on one breaks when they do. diff --git a/tools/cli/src/cli.rs b/tools/cli/src/cli.rs index 63e2caea..cb4daedf 100644 --- a/tools/cli/src/cli.rs +++ b/tools/cli/src/cli.rs @@ -647,6 +647,13 @@ fn check_file_combination( if images.is_some() && !is_import { return Err(ParseError::UnexpectedArgument("--images".to_string())); } + // **An empty path is the working directory**, and `create_dir_all("")` + // succeeds, so `--images ""` would quietly scatter a model's textures + // wherever the process happens to be standing. Refused where the + // other path rules live rather than deep in the writer. + if images.is_some_and(str::is_empty) { + return Err(ParseError::MissingValue("--images")); + } // Then what each subcommand cannot work without. Both paths are the // whole input: guessing one would be worse than refusing. diff --git a/tools/cli/src/json.rs b/tools/cli/src/json.rs index 5cbb187f..1d4b38e5 100644 --- a/tools/cli/src/json.rs +++ b/tools/cli/src/json.rs @@ -90,17 +90,19 @@ impl Value { // back as `Number(2)` because the integer branch takes any // lexeme without `.`/`e`. The value survives that trip but // its type does not, so a parsed float re-emitted once - // comes back as an integer. Nothing renders a parsed float - // today -- the parser is the only thing that builds one -- - // which is why this never showed up in output. It still - // makes `parse(render(v)) == v` false for a value the - // parser itself can produce, so the suffix goes on here - // rather than the property being weakened to match. + // comes back as an integer. That makes + // `parse(render(v)) == v` false for a value the parser + // itself can produce, so the suffix goes on here rather + // than the property being weakened to match. // - // Finite only: the parser refuses non-finite numbers (JSON - // cannot spell them), so a non-finite `Float` is already - // unconstructible from input and `"inf.0"` would be no - // more valid than `"inf"`. + // Finite only. The parser refuses non-finite numbers, + // which JSON cannot spell, so one cannot arrive that way. + // **`asset-import` is now a second source**, reporting a + // material's factors, and it holds because every one of + // those is range-checked by the reader before it gets + // here -- a chain of checks in another crate rather than + // unconstructibility, which is worth saying out loud + // because it is the weaker guarantee. if number.is_finite() && !text.contains(['.', 'e', 'E']) { out.push_str(".0"); } diff --git a/tools/cli/src/main.rs b/tools/cli/src/main.rs index 672478a9..cc705e58 100644 --- a/tools/cli/src/main.rs +++ b/tools/cli/src/main.rs @@ -517,7 +517,7 @@ fn names_one_file(from: &Path, out: &Path) -> bool { /// A refusal on its way out of the image-writing helpers. /// -/// **Carried rather than emitted**, because the helpers do not know +/// **Carried rather than emitted**, because `write_images` does not know /// whether the caller asked for JSON and the one place that does is /// already written. struct ImportFailure { @@ -562,24 +562,13 @@ fn write_images( images: &[renew_mesh::gltf::Image<'static>], directory: &str, ) -> Result, ImportFailure> { - let root = Path::new(directory); - // The flag names a destination for a set of files whose size the - // caller cannot know in advance, so making it is part of honouring - // it -- unlike `--out`, which names one file whose parent the - // caller already chose. - if let Err(error) = std::fs::create_dir_all(root) { - return Err(ImportFailure { - message: format!("cannot create {directory}: {error}"), - refusal: None, - }); - } - - let mut written = Vec::with_capacity(images.len()); + // **Every name settled before the first byte is written.** Checking + // as the loop went would leave a caller deciding which half of a + // directory to trust: four files written, the fifth refused, and a + // blob on disk beside them. The names are two literals and an index, + // so the pass costs nothing. + let mut names = Vec::with_capacity(images.len()); for (index, image) in images.iter().enumerate() { - // **Refused before anything is written.** A run that wrote four - // images and then stopped at the fifth would leave the caller - // deciding which half of a directory to trust; the loop is short - // enough that checking as it goes still fails on the first one. let Some(stated) = image.media_type.as_deref() else { return Err(ImportFailure { message: format!( @@ -597,8 +586,31 @@ fn write_images( refusal: Some("UnknownMediaType"), }); }; + names.push(format!("image-{index}.{extension}")); + } - let path = root.join(format!("image-{index}.{extension}")); + // **Nothing at all when there is nothing**, not even the directory. + // A model with no images should leave no trace of having been asked + // for them, which is the same rule the absent flag obeys. + if names.is_empty() { + return Ok(Vec::new()); + } + + let root = Path::new(directory); + // The flag names a destination for a set of files whose size the + // caller cannot know in advance, so making it is part of honouring + // it -- unlike `--out`, which names one file whose parent the + // caller already chose. + if let Err(error) = std::fs::create_dir_all(root) { + return Err(ImportFailure { + message: format!("cannot create {directory}: {error}"), + refusal: None, + }); + } + + let mut written = Vec::with_capacity(names.len()); + for (name, image) in names.iter().zip(images) { + let path = root.join(name); if let Err(error) = std::fs::write(&path, &*image.bytes) { return Err(ImportFailure { message: format!("cannot write {}: {error}", path.display()), @@ -617,7 +629,9 @@ fn write_images( /// the default, and the difference does not matter to a caller choosing /// a shader -- but a missing key would make them write the defaults /// themselves, in a second place, from a specification they would have -/// to go and read. +/// to go and read. That argument bites hardest on a normal map's scale +/// and an occlusion map's strength, which is why those two ride on their +/// own map entries rather than being flattened away with the rest of it. fn material_json(material: &renew_mesh::pbr::Material) -> Value { // **The record's f32 widened, not reformatted.** Every one of these // came out of a document as a decimal and went into an `f32`; going @@ -658,20 +672,32 @@ fn material_json(material: &renew_mesh::pbr::Material) -> Value { "textures".to_string(), Value::Array( [ - ("base_color", material.base_color_map), - ("metallic_roughness", material.metallic_roughness_map), - ("normal", material.normal_map.map(|map| map.map)), - ("occlusion", material.occlusion_map.map(|map| map.map)), - ("emissive", material.emissive_map), + ("base_color", material.base_color_map, None), + ("metallic_roughness", material.metallic_roughness_map, None), + // **The two that carry a number of their own**, and the + // two the format cannot supply from a default once a + // document has stated them: a normal map's scale and an + // occlusion map's strength. + ( + "normal", + material.normal_map.map(|map| map.map), + material.normal_map.map(|map| ("scale", map.scale)), + ), + ( + "occlusion", + material.occlusion_map.map(|map| map.map), + material.occlusion_map.map(|map| ("strength", map.strength)), + ), + ("emissive", material.emissive_map, None), ] .into_iter() - .filter_map(|(role, reference)| { + .filter_map(|(role, reference, extra)| { // **Only the maps the document named.** A null per absent // role would make five keys that are almost always null, // and a caller looking for what a material references // would filter them right back out. reference.map(|reference| { - Value::Object(vec![ + let mut entry = vec![ ("role".to_string(), Value::String(role.to_string())), ( "texture".to_string(), @@ -681,7 +707,11 @@ fn material_json(material: &renew_mesh::pbr::Material) -> Value { "uv_set".to_string(), Value::Number(i64::from(reference.uv_set)), ), - ]) + ]; + if let Some((key, value)) = extra { + entry.push((key.to_string(), number(value))); + } + Value::Object(entry) }) }) .collect(), @@ -758,6 +788,13 @@ fn run_asset_import( let found = renew_mesh::format::detect(&bytes); let format = found.name(); + // Set when a table refused and the refusal was not fatal, so the + // envelope can say why it is reporting nothing rather than leaving + // a caller to read an absence as an emptiness. **The name and the + // sentence both**, for the same reason the failure envelope carries + // both: a name is for a program and a sentence is for a person, and + // this one never reaches `stderr` because the run succeeded. + let mut unreadable: Option<(&'static str, String)> = None; let Some(read) = found.read(&bytes) else { // A material library is not a broken mesh, and saying so is this // tool's judgement rather than a reader's refusal: no reader was @@ -787,26 +824,34 @@ fn run_asset_import( }; // **What the document says beyond its shape**, read from the same - // bytes before they are dropped. Only glTF states materials in this - // vocabulary and only glTF carries its images inside itself, so - // every other format answers with two empty tables rather than with - // a refusal -- a caller asking what an STL's textures are is not - // making a mistake, it is getting the true answer. - let tables = match found { - renew_mesh::format::Format::Gltf | renew_mesh::format::Format::Glb => { - match renew_mesh::gltf::tables(&bytes) { - Ok(tables) => tables, - Err(refusal) => { - return import_failure( - &format!("{from}: {refusal}"), - Some(refusal.name()), - json_mode, - started, - ); - } + // bytes before they are dropped. The format decides whether there is + // an answer at all: a format that states no materials in this + // vocabulary answers `None`, which is not the same as answering that + // it has none. + // + // **A table that will not read does not take the geometry down with + // it, unless the caller asked for the images.** Reading a model is + // what this command is for, and the commonest glTF in the world + // keeps its textures in files beside itself -- which this reader + // will not open, and which has nothing to do with whether the + // geometry is sound. So the refusal is *reported* and the import + // proceeds. Where `--images` was given the caller asked for + // something that cannot be delivered, and then it is fatal. + let tables = match found.tables(&bytes) { + None => None, + Some(Ok(tables)) => Some(tables), + Some(Err(refusal)) => { + if images_dir.is_some() { + return import_failure( + &format!("{from}: {refusal}"), + Some(refusal.name()), + json_mode, + started, + ); } + unreadable = Some((refusal.name(), refusal.to_string())); + None } - _ => renew_mesh::gltf::Tables::default(), }; // The file is not read again after this, and it can be as large as @@ -831,7 +876,10 @@ fn run_asset_import( // caller learns there are some before deciding where they go. let written = match images_dir { None => Vec::new(), - Some(directory) => match write_images(&tables.images, directory) { + Some(directory) => match write_images( + tables.as_ref().map_or(&[][..], |tables| &tables.images), + directory, + ) { Ok(written) => written, Err(failure) => { return import_failure(&failure.message, failure.refusal, json_mode, started); @@ -841,7 +889,8 @@ fn run_asset_import( report_import(&Import { mesh: &mesh, - tables: &tables, + tables: tables.as_ref(), + unreadable: unreadable.as_ref(), written: &written, format, out_path, @@ -853,12 +902,18 @@ fn run_asset_import( /// Everything one successful import has to say for itself. /// -/// **A record rather than eight parameters**, which is what the list had -/// grown to: three of them were paths or flags of the same type, and a -/// caller swapping two would have compiled. +/// **A record rather than a long parameter list**, which is what it had +/// grown to: `format` and `out_path` are both `&str`, and a caller +/// swapping them would have compiled. struct Import<'a> { mesh: &'a renew_mesh::Mesh, - tables: &'a renew_mesh::gltf::Tables, + /// What the document said beyond its shape, where this format states + /// such things at all and the tables read. + tables: Option<&'a renew_mesh::gltf::Tables>, + /// Why the tables are absent, when they are absent because a table + /// refused rather than because the format has none: the refusal's + /// name and its sentence. + unreadable: Option<&'a (&'static str, String)>, written: &'a [String], format: &'a str, out_path: &'a str, @@ -878,7 +933,7 @@ fn report_import(report: &Import<'_>) -> ExitCode { let (format, out_path) = (report.format, report.out_path); if report.json_mode { // `envelope_base` puts `schema_version` first already, which is - // what D11 asks of a public JSON surface. A second one here + // what a versioned machine-readable surface owes. A second here // would be a duplicate key in the object, and a reader taking // whichever it met first would be right by luck. let mut fields = envelope_base("asset-import", "ok", 0, report.started, ""); @@ -908,13 +963,42 @@ fn report_import(report: &Import<'_>) -> ExitCode { )); // **Reported whether or not any were written**, which is the // point of counting them separately from writing them. + // **Null is not empty.** A format that does not state materials + // in this vocabulary has not got none of them -- OBJ carries + // them in Wavefront's model, which is a different thing this arm + // does not convert into. And a table that refused has an answer + // nobody could read. Both are `null`, with `tables_refusal` + // telling the two apart. + let (materials, textures, images) = match report.tables { + None => (Value::Null, Value::Null, Value::Null), + Some(tables) => ( + Value::Array(tables.materials.iter().map(material_json).collect()), + Value::Array( + tables + .textures + .iter() + .map(|source| { + source.map_or(Value::Null, |source| Value::Number(i64::from(source))) + }) + .collect(), + ), + Value::Array(tables.images.iter().map(image_json).collect()), + ), + }; + fields.push(("materials".to_string(), materials)); + // **The step between a material and an image**, which is a step: + // a material names a texture and a texture names a source, so + // without this a caller holding both lists cannot pair them. + fields.push(("textures".to_string(), textures)); + fields.push(("images".to_string(), images)); fields.push(( - "materials".to_string(), - Value::Array(report.tables.materials.iter().map(material_json).collect()), - )); - fields.push(( - "images".to_string(), - Value::Array(report.tables.images.iter().map(image_json).collect()), + "tables_refusal".to_string(), + report.unreadable.map_or(Value::Null, |(name, detail)| { + Value::Object(vec![ + ("name".to_string(), Value::String((*name).to_string())), + ("detail".to_string(), Value::String(detail.clone())), + ]) + }), )); fields.push(( "images_written".to_string(), @@ -932,9 +1016,20 @@ fn report_import(report: &Import<'_>) -> ExitCode { emit_stdout(&format!( "read {triangles} triangles of {format} into {out_path} ({size} bytes)\n" )); - let (materials, images) = (report.tables.materials.len(), report.tables.images.len()); - if materials > 0 || images > 0 { - emit_stdout(&format!(" {materials} materials, {images} images\n")); + if let Some(tables) = report.tables { + let (materials, images) = (tables.materials.len(), tables.images.len()); + if materials > 0 || images > 0 { + emit_stdout(&format!( + " {materials} materials, {images} images +" + )); + } + } + if let Some((_, detail)) = report.unreadable { + emit_stdout(&format!( + " its materials and images were not read: {detail} +" + )); } for path in report.written { emit_stdout(&format!(" wrote {path}\n")); diff --git a/tools/cli/tests/cli.rs b/tools/cli/tests/cli.rs index 8784b86b..5f34eb87 100644 --- a/tools/cli/tests/cli.rs +++ b/tools/cli/tests/cli.rs @@ -2122,6 +2122,125 @@ fn document_with(tables: &str) -> Vec { .into_bytes() } +/// **A refusal part-way through leaves no half-written directory.** +/// +/// Every name is settled before the first byte is written, so a document +/// whose third image cannot be named writes none of the first two. The +/// old shape checked and wrote in one pass, which left a caller deciding +/// which half of a directory to trust. +#[test] +fn asset_import_writes_no_image_if_any_cannot_be_named() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-all-or-none")?; + let model = directory.join("scene.gltf"); + let textures = directory.join("textures"); + fs::write( + &model, + document_with( + r#","images":[{"uri":"data:image/png;base64,AQIDBA=="}, +{"uri":"data:image/jpeg;base64,BQYHCA=="},{"uri":"data:image/tiff;base64,CQoLDA=="}]"#, + ), + )?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + "--images", + &textures.to_string_lossy(), + ])?; + assert!(!output.status.success(), "the third cannot be named"); + assert!( + String::from_utf8_lossy(&output.stdout).contains("image 2 is `image/tiff`"), + "and it says which one" + ); + assert!( + !textures.exists(), + "the two that could be named are not written either, and the \ + directory they would have gone in is not made" + ); + Ok(()) +} + +/// **A texture's source is reported, because a material names a texture +/// and a texture names an image.** +/// +/// Without the middle table a caller holding a material's reference and +/// a directory of files has to guess, and the guess is wrong whenever a +/// texture's index is not its image's. +#[test] +fn asset_import_reports_which_image_each_texture_draws_from() -> std::io::Result<()> { + let directory = scratch_directory("asset-import-textures")?; + let model = directory.join("scene.gltf"); + fs::write( + &model, + document_with( + r#","textures":[{"source":1}], +"materials":[{"pbrMetallicRoughness":{"baseColorTexture":{"index":0}}, +"normalTexture":{"index":0,"scale":3.5},"occlusionTexture":{"index":0,"strength":0.25}}], +"images":[{"uri":"data:image/jpeg;base64,BQYHCA=="}, +{"uri":"data:image/png;base64,AQIDBA=="}]"#, + ), + )?; + + let output = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &directory.join("out.msh").to_string_lossy(), + ])?; + assert!( + output.status.success(), + "it imports: {}", + String::from_utf8_lossy(&output.stdout) + ); + let reported = String::from_utf8_lossy(&output.stdout); + validate_json(reported.trim()).expect("one valid document"); + + // Texture 0 draws from image 1, so a caller pairing them by index + // alone would reach for the wrong file. + assert!( + reported.contains("\"textures\":[1]"), + "the join is reported: {reported:?}" + ); + // **The two members a default cannot supply once a document states + // them**, which the material report used to drop. + assert!( + reported.contains("\"scale\":3.5"), + "a normal map's scale survives: {reported:?}" + ); + assert!( + reported.contains("\"strength\":0.25"), + "and an occlusion map's strength: {reported:?}" + ); + Ok(()) +} + +/// `--images` with an empty path names the working directory, and is +/// refused rather than scattering a model's textures into it. +#[test] +fn an_empty_images_path_is_refused() -> std::io::Result<()> { + let output = run(&[ + "asset-import", + "--from", + "m.gltf", + "--out", + "m.msh", + "--images", + "", + ])?; + assert!(!output.status.success(), "an empty path is not a directory"); + assert!( + String::from_utf8_lossy(&output.stderr).contains("--images"), + "and the caller hears about the flag they typed" + ); + Ok(()) +} + /// **An image that states no type at all is refused for saying nothing**, /// which is the other half of `UnknownMediaType`. /// @@ -2158,16 +2277,19 @@ fn asset_import_refuses_an_image_that_names_no_type() -> std::io::Result<()> { Ok(()) } -/// **A table that refuses refuses the import**, even where the geometry -/// beside it reads perfectly. +/// **A table that will not read does not take the geometry with it.** /// -/// The two halves of a document are read separately, so this is the one -/// case that proves the second half is read at all: the triangle is -/// fine and the image names a second file. -#[test] -fn asset_import_carries_a_table_refusal_out() -> std::io::Result<()> { +/// The commonest glTF in the world keeps its textures in files beside +/// itself. This reader will not open a second file, which is a fact +/// about the textures and says nothing about whether the geometry is +/// sound -- so the model imports, and the envelope says why it is +/// reporting no tables rather than leaving an absence to be read as an +/// emptiness. +#[test] +fn a_table_that_will_not_read_does_not_stop_the_import() -> std::io::Result<()> { let directory = scratch_directory("asset-import-table-refusal")?; let model = directory.join("scene.gltf"); + let blob = directory.join("out.msh"); fs::write(&model, document_with(r#","images":[{"uri":"grain.png"}]"#))?; let output = run(&[ @@ -2176,16 +2298,38 @@ fn asset_import_carries_a_table_refusal_out() -> std::io::Result<()> { "--from", &model.to_string_lossy(), "--out", - &directory.join("out.msh").to_string_lossy(), + &blob.to_string_lossy(), ])?; assert!( - !output.status.success(), - "a file this reader will not open is a refusal, geometry or no geometry" + output.status.success(), + "the geometry is embedded and sound: {}", + String::from_utf8_lossy(&output.stdout) ); + assert!(blob.exists(), "and the blob it was asked for is written"); let reported = String::from_utf8_lossy(&output.stdout); assert!( - reported.contains("\"refusal\":\"ExternalResource\""), - "named by the layer that refused rather than by this tool: {reported:?}" + reported.contains("\"name\":\"Gltf\"") + && reported.contains("keeps a resource somewhere else") + && reported.contains("\"materials\":null"), + "with the reason said rather than an empty table implied: {reported:?}" + ); + + // **Asking for the images makes it fatal**, because then the caller + // asked for the thing that cannot be delivered. + let asked = run(&[ + "--json", + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &blob.to_string_lossy(), + "--images", + &directory.join("textures").to_string_lossy(), + ])?; + assert!(!asked.status.success(), "there are no images to write"); + assert!( + String::from_utf8_lossy(&asked.stdout).contains("\"refusal\":\"Gltf\""), + "named the way this tool names every reader refusal" ); Ok(()) } @@ -2507,11 +2651,15 @@ fn asset_import_refuses_to_invent_a_file_extension() -> std::io::Result<()> { Ok(()) } -/// **A format that carries no images answers with none, not a refusal.** +/// **A format that states none of this answers `null`, not `[]`.** /// -/// Asking an STL for its textures is a fair question with a true answer. +/// Asking an STL for its textures is a fair question, and the true +/// answer is not "it has none" -- OBJ carries materials in Wavefront's +/// model, which this arm does not convert into, so an empty array would +/// be saying something false about the file. `null` says the question +/// was not answered here. #[test] -fn asset_import_reports_empty_tables_for_a_format_without_them() -> std::io::Result<()> { +fn asset_import_reports_no_tables_for_a_format_without_them() -> std::io::Result<()> { let directory = scratch_directory("asset-import-untextured")?; let model = directory.join("model.stl"); // One binary-STL triangle: an 80-byte header, a count, and one facet. @@ -2542,8 +2690,18 @@ fn asset_import_reports_empty_tables_for_a_format_without_them() -> std::io::Res ); let reported = String::from_utf8_lossy(&output.stdout); assert!( - reported.contains("\"materials\":[]") && reported.contains("\"images\":[]"), - "both tables are empty and both are reported: {reported:?}" + reported.contains("\"materials\":null") && reported.contains("\"images\":null"), + "not answered here, rather than answered as none: {reported:?}" + ); + assert!( + reported.contains("\"tables_refusal\":null"), + "and nothing refused -- the format simply does not state them: {reported:?}" + ); + // **The directory is not made for a model with no images**, which is + // the same rule the absent flag obeys. + assert!( + !directory.join("textures").exists(), + "no directory nobody had a use for" ); Ok(()) } diff --git a/tools/cli/tests/source_hygiene.rs b/tools/cli/tests/source_hygiene.rs index 8088734c..697e4996 100644 --- a/tools/cli/tests/source_hygiene.rs +++ b/tools/cli/tests/source_hygiene.rs @@ -268,7 +268,7 @@ fn line_of(bytes: &[u8], offset: usize) -> usize { /// turns the two characters backslash-r into a carriage return, and /// backslash-t into a tab, inside whatever literal was being written. /// -/// It happened here, twice in one session. A sample's README gained a +/// It happened here, twice. A sample's README gained a /// PowerShell block whose `$env:USERPROFILE\run.log` had become /// `$env:USERPROFILE` + CR + `un.log`, and whose `.\target\debug\glide.exe` /// had become `.` + TAB + `arget\debug\glide.exe`. It reached a merged From 40313de4cc194ac5b8764175435c867c4f2cd34e Mon Sep 17 00:00:00 2001 From: CagdasErturk Date: Wed, 9 Sep 2026 15:15:45 +0300 Subject: [PATCH 4/6] test(mesh): the texture table gets its own tests, and the exempt block follows the code `gltf::textures` reads which image each texture draws from, and shipped without a test of its own. Four now: a source that is named, a texture that names none -- `None` rather than zero, because the format leaves `source` optional for an extension to supply and defaulting it would point every such texture at whichever image happened to be first -- an absent table, and the two wrong-kind refusals. The unreadable-tables message had a test that passed `--json`, so its human-readable half had never run. Two output modes are two pieces of code. The one exempt block in the command's source is the determinism emit half, and the code above it moved again, so its fifty-two lines shift by ninety-five. Checked rather than assumed: each recorded line was located by its own text and every one is byte-identical at that offset. --- coverage-exemptions.toml | 2 +- crates/mesh/tests/gltf.rs | 53 +++++++++++++++++++++++++++++++++++++++ tools/cli/tests/cli.rs | 16 ++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/coverage-exemptions.toml b/coverage-exemptions.toml index 5b719fb2..122e7b9d 100644 --- a/coverage-exemptions.toml +++ b/coverage-exemptions.toml @@ -171,7 +171,7 @@ reason = "The arm that clears a voice whose sound index is missing. A voice is o [[exempt]] file = "tools/cli/src/main.rs" -lines = [2079, 2080, 2082, 2083, 2084, 2085, 2086, 2097, 2101, 2102, 2103, 2105, 2109, 2123, 2124, 2125, 2127, 2128, 2129, 2130, 2131, 2132, 2133, 2134, 2135, 2136, 2137, 2138, 2140, 2144, 2145, 2168, 2186, 2187, 2188, 2189, 2196, 2197, 2198, 2199, 2200, 2201, 2202, 2205, 2209, 2210, 2211, 2215, 2595, 2596, 2597, 2598] +lines = [2174, 2175, 2177, 2178, 2179, 2180, 2181, 2192, 2196, 2197, 2198, 2200, 2204, 2218, 2219, 2220, 2222, 2223, 2224, 2225, 2226, 2227, 2228, 2229, 2230, 2231, 2232, 2233, 2235, 2239, 2240, 2263, 2281, 2282, 2283, 2284, 2291, 2292, 2293, 2294, 2295, 2296, 2297, 2300, 2304, 2305, 2306, 2310, 2690, 2691, 2692, 2693] reason = "The determinism emit half past its first child, plus the two arms that answer for a target this process is not running on. Reaching the emit arms from a test would mean building and running pinned runs under instrumentation: the report-reading arms need the first pinned run (renew-ui, no arguments) to compile and succeed, and the leg construction and write at the tail need all eleven - spread across six workspace packages, four of them samples and two engine crates - to succeed. The success path is not untested: the three determinism legs execute it on Linux, Windows and macOS on every push, and a failure there is what a broken emit looks like. Everything testable without a subprocess has been moved out - digests_from_output has seven cases beside it, digest_name is the one spelling both sides call, emit_note is unit-tested on both branches, pinned_invocation asserts the --target pass-through with no device, and the emit-red path is driven end to end by tests/targets.rs in both output modes. What is left is process orchestration, the leg written when every child succeeded, and the arms no gating push can reach: a compiler that answered and failed when asked its own version, a child that cannot start at all, a child whose report is unreadable, a leg file that cannot be written, one pinned run claiming a digest name another already used, and a working directory that has ceased to exist beneath the process. The --target arms are their own case: the refusal for a triple the table cannot name is reached by nothing at all, because every lane passes either a known triple or none, and it exists so a target added to CI and forgotten in that table fails loudly instead of emitting a leg labelled by a guess; the arm that labels a leg from its triple is executed only by the Android emulator lane, which is advisory and cannot redden main, so it is held by a lane whose red only a reader sees - which is the honest description until that row is signed and the lane gates." [[exempt]] diff --git a/crates/mesh/tests/gltf.rs b/crates/mesh/tests/gltf.rs index 3fde7ed0..ad16764a 100644 --- a/crates/mesh/tests/gltf.rs +++ b/crates/mesh/tests/gltf.rs @@ -1117,6 +1117,57 @@ fn an_image_that_states_no_type_anywhere_reports_none() { assert_eq!(&*read[0].bytes, &[1, 2, 3, 4]); } +/// **A texture says which image it draws from, and that is a step.** +/// +/// A material names a texture and a texture names a source, so the two +/// indices are not the same number and a caller pairing them directly +/// is wrong whenever they differ. This is the table that joins them. +#[test] +fn a_texture_names_the_image_it_draws_from() { + let json = document(r#"{ "textures": [{ "source": 2 }, { "sampler": 0 }, { "source": 0 }] }"#); + let read = gltf::textures(json.root()).expect("three textures"); + assert_eq!(read, [Some(2), None, Some(0)]); +} + +/// **A texture with no source is `None`, not zero.** +/// +/// The format leaves `source` optional because an extension may supply +/// the image instead. Defaulting it to zero would point every such +/// texture at whichever image happened to be first. +#[test] +fn a_texture_without_a_source_is_not_texture_zero() { + let json = document(r#"{ "textures": [{}] }"#); + assert_eq!(gltf::textures(json.root()).expect("one texture"), [None]); +} + +/// A document with no texture table has none, which is not a refusal. +#[test] +fn a_document_with_no_textures_has_none() { + let json = document(r#"{ "asset": { "version": "2.0" } }"#); + assert!(gltf::textures(json.root()).expect("no textures").is_empty()); +} + +/// **A texture is an object**, and one that is not is refused for that +/// rather than read as naming no source. +#[test] +fn a_texture_that_is_not_an_object_is_refused() { + let json = document(r#"{ "textures": [5] }"#); + assert_eq!( + gltf::textures(json.root()) + .expect_err("a number is not a texture") + .name(), + "Document" + ); + + let source = document(r#"{ "textures": [{ "source": "first" }] }"#); + assert_eq!( + gltf::textures(source.root()) + .expect_err("a name is not an index") + .name(), + "Document" + ); +} + /// **The tables read from either shape of the same asset.** /// /// The whole reason this entry point exists: a caller would otherwise @@ -1126,11 +1177,13 @@ fn an_image_that_states_no_type_anywhere_reports_none() { fn the_tables_read_from_a_document_and_from_a_container() { let text = r#"{"asset":{"version":"2.0"}, "materials":[{"name":"brass","metallicFactor":1.0,"roughnessFactor":0.25}], +"textures":[{"source":0}], "images":[{"name":"grain","uri":"data:image/png;base64,AQIDBA=="}]}"#; let alone = gltf::tables(text.as_bytes()).expect("a document on its own"); assert_eq!(alone.materials.len(), 1); assert_eq!(alone.materials[0].name.as_deref(), Some("brass")); + assert_eq!(alone.textures, [Some(0)]); assert_eq!(alone.images.len(), 1); assert_eq!(alone.images[0].name.as_deref(), Some("grain")); assert_eq!(&*alone.images[0].bytes, &[1, 2, 3, 4]); diff --git a/tools/cli/tests/cli.rs b/tools/cli/tests/cli.rs index 5f34eb87..1b619c99 100644 --- a/tools/cli/tests/cli.rs +++ b/tools/cli/tests/cli.rs @@ -2331,6 +2331,22 @@ fn a_table_that_will_not_read_does_not_stop_the_import() -> std::io::Result<()> String::from_utf8_lossy(&asked.stdout).contains("\"refusal\":\"Gltf\""), "named the way this tool names every reader refusal" ); + + // **And the prose arm says it too.** Two output modes are two pieces + // of code, and a caller who does not pass `--json` is entitled to + // learn that the tables were not read rather than to see nothing. + let spoken = run(&[ + "asset-import", + "--from", + &model.to_string_lossy(), + "--out", + &blob.to_string_lossy(), + ])?; + assert!(spoken.status.success(), "it imports without --json too"); + assert!( + String::from_utf8_lossy(&spoken.stdout).contains("its materials and images were not read"), + "and says why it is reporting none" + ); Ok(()) } From 55222a935c018e8314ed3dae44d82f6914ab4beb Mon Sep 17 00:00:00 2001 From: CagdasErturk Date: Wed, 9 Sep 2026 15:23:49 +0300 Subject: [PATCH 5/6] fix(mesh): owning an image's bytes is bounded, and asked for Reading a document's tables copied every image out of it. An image stored in a buffer view borrows the document, so owning it copies -- and nothing in the format says two images must name two views. A document that points a thousand of them at one shared megabyte costs about thirty bytes an entry to write and a gigabyte to hold. Measured: 375 KB in, 1,099 MB allocated, and the curve is quadratic in the input's length, so an eight-megabyte document reaches hundreds of gigabytes. The command took 437 ms and a gigabyte over an import that used to take 5 ms and three megabytes, and exited successfully. This crate already had the rule: amplification is the danger, not allocation. Geometry answers to a ceiling and so does the material table. Images now answer to the same one, counted in bytes rather than entries because one image can be the whole ceiling on its own and a thousand tiny ones are harmless. The copy is also asked for rather than assumed. It was being paid by callers who only wanted to know what a model carries -- on a four-megabyte texture it is 99% of the call's time and 99.9% of its bytes, and the import paid it whether or not any file was going to be written. `ImageBytes` states the choice at the call site, and the import asks to count unless it was told where to put the files. `Stored` replaces the owning half of `Image`: it states its length separately from its bytes, so a report never needs the bytes in order to say how big something is. Two tests could not fail. Both asserted that a refusal's text mentions `--images`, and every parse error prints the usage block, which lists `--images` -- so they held however the guard behaved. They assert the sentence the rule produces now. And the material and image report is compared by value. Every number in it could have been dropped, negated or swapped without a test noticing: metallic read from roughness, an image's length reported as zero, every texture role relabelled, the texture index replaced with a constant. --- crates/mesh/src/format.rs | 15 +++-- crates/mesh/src/gltf.rs | 110 +++++++++++++++++++++++++-------- crates/mesh/src/lib.rs | 20 ++++++ crates/mesh/tests/gltf.rs | 69 ++++++++++++++++++--- tools/cli/src/main.rs | 126 ++++++++++++++++++++++++++------------ tools/cli/tests/cli.rs | 43 +++++++++++-- 6 files changed, 301 insertions(+), 82 deletions(-) diff --git a/crates/mesh/src/format.rs b/crates/mesh/src/format.rs index fbb8bcc9..83069fde 100644 --- a/crates/mesh/src/format.rs +++ b/crates/mesh/src/format.rs @@ -156,12 +156,19 @@ impl Format { /// Exhaustive on purpose, like [`read`](Self::read): a format added /// to this enum has to answer this question before it compiles, /// which is the compile-time check a wildcard would throw away. + /// + /// `wanted` decides whether an image's bytes are copied out of the + /// document or only measured; see [`gltf::ImageBytes`]. #[must_use] - pub fn tables(self, bytes: &[u8]) -> Option> { + pub fn tables( + self, + bytes: &[u8], + wanted: gltf::ImageBytes, + ) -> Option> { match self { - Self::Glb | Self::Gltf => { - Some(gltf::tables(bytes).map_err(|refusal| MeshError::Gltf(Box::new(refusal)))) - } + Self::Glb | Self::Gltf => Some( + gltf::tables(bytes, wanted).map_err(|refusal| MeshError::Gltf(Box::new(refusal))), + ), Self::Obj | Self::Mtl | Self::Stl | Self::Ply | Self::Blob => None, } } diff --git a/crates/mesh/src/gltf.rs b/crates/mesh/src/gltf.rs index 9eaa708a..73cea4de 100644 --- a/crates/mesh/src/gltf.rs +++ b/crates/mesh/src/gltf.rs @@ -884,25 +884,6 @@ pub fn images<'s>(root: Value<'_>, source: &'s Source<'_>) -> Result { - /// Take ownership of the bytes, so the image outlives the document. - /// - /// **The way out of the borrow that does not rebuild the value, and - /// it is a copy where the bytes came from a buffer.** An image read from a `bufferView` - /// borrows the document's own memory; a caller that wants to hold - /// it after the document is dropped -- to write it to a file, say -- - /// has to pay for that once. An image decoded from a payload already - /// owns its bytes and pays nothing. - #[must_use] - pub fn into_owned(self) -> Image<'static> { - Image { - name: self.name, - media_type: self.media_type, - bytes: Cow::Owned(self.bytes.into_owned()), - } - } -} - /// What a document says beyond its geometry. /// /// **Two tables that travel together because one caller wants both.** @@ -922,8 +903,55 @@ pub struct Tables { /// format, because an extension may supply the image instead, and a /// texture that names none reads as `None` rather than as zero. pub textures: Vec>, - /// Every image, holding its own bytes. - pub images: Vec>, + /// Every image the document carries. + pub images: Vec, +} + +/// An image a caller can hold after the document is gone. +/// +/// **Separate from [`Image`] because owning is a different thing from +/// borrowing, not a mode of it.** An `Image` points into the document +/// that produced it and costs nothing; this outlives that document, and +/// for an image stored in a `bufferView` that means a copy. Making it a +/// second type rather than a flag on the first keeps the cost where a +/// reader can see it. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct Stored { + /// What the document called it, if it called it anything. + pub name: Option, + /// The media type the document states for these bytes, if it states + /// one. See [`Image::media_type`] for how the two possible + /// statements are reconciled. + pub media_type: Option, + /// How long the image is, whether or not its bytes were kept. + /// + /// **The length is free and the bytes are not.** A caller reporting + /// what a model carries wants this and nothing else, so it is stated + /// separately rather than being read off a `bytes` that may not be + /// there. + pub len: usize, + /// The bytes, when [`ImageBytes::Kept`] asked for them. + pub bytes: Option>, +} + +/// Whether a caller wants an image's bytes or only the fact of it. +/// +/// **A copy is the only way out of the borrow, so it has to be asked +/// for.** An image read from a `bufferView` points into the document, +/// and a `Tables` outlives the parse that produced it -- so keeping the +/// bytes means copying them. A caller reporting what a model carries +/// needs the name, the type and the length, and none of those need the +/// bytes; a caller writing files needs all of it. +/// +/// Stated at the call site rather than inferred, because the cost is +/// the whole cost of the call: on a four-megabyte texture the copy +/// measured at 99% of the time and 99.9% of the bytes. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ImageBytes { + /// Copy them, because they are going somewhere. + Kept, + /// Report the length and drop them. + Counted, } /// Read what a document says beyond its geometry, in either shape. @@ -947,7 +975,7 @@ pub struct Tables { /// /// A [`GltfError`] naming the layer that refused and carrying its /// numbers. -pub fn tables(bytes: &[u8]) -> Result { +pub fn tables(bytes: &[u8], wanted: ImageBytes) -> Result { let (document, chunk) = if glb::looks_like(bytes) { let container = glb::read(bytes).map_err(GltfError::Container)?; (container.json, container.binary) @@ -958,13 +986,43 @@ pub fn tables(bytes: &[u8]) -> Result { let json = parse(document)?; let root = json.root(); let source = Source::of(root, chunk)?; + // **Owning the bytes is the one thing here that can amplify**, and + // it is bounded by the same ceiling geometry and materials answer + // to. An image from a `bufferView` borrows until this line; a + // document that points a thousand images at one shared megabyte + // costs two bytes an entry to write and a gigabyte to hold, which + // is the shape the rest of this crate already refuses. + let mut held = 0_usize; + let mut owned = Vec::new(); + for image in images(root, &source)? { + let len = image.bytes.len(); + let bytes = match wanted { + ImageBytes::Counted => None, + ImageBytes::Kept => { + // **The one line here that can amplify, and the ceiling + // it answers to.** Nothing says two images must name two + // views: a document may point a thousand of them at one + // shared megabyte, paying about thirty bytes an entry to + // do it. Measured before this existed, that reached + // nearly three thousand times the input and grew as its + // square. + crate::refuse_over_image_ceiling(held, len).map_err(GltfError::Geometry)?; + held += len; + Some(image.bytes.into_owned()) + } + }; + owned.push(Stored { + name: image.name, + media_type: image.media_type, + len, + bytes, + }); + } + Ok(Tables { materials: materials(root)?, textures: textures(root)?, - images: images(root, &source)? - .into_iter() - .map(Image::into_owned) - .collect(), + images: owned, }) } diff --git a/crates/mesh/src/lib.rs b/crates/mesh/src/lib.rs index ae176eb7..c80d4faf 100644 --- a/crates/mesh/src/lib.rs +++ b/crates/mesh/src/lib.rs @@ -147,6 +147,26 @@ pub(crate) fn refuse_over_material_ceiling(have: usize) -> Result<(), MeshError> Ok(()) } +/// Refuse before the next image's bytes are copied rather than after. +/// +/// **The amplification rule applied to the one place that copies.** An +/// image read out of a `bufferView` borrows the document's own memory, +/// and taking ownership of it copies. Nothing says two images must name +/// two views: a document may point a thousand images at one megabyte and +/// pay two bytes an entry for it, which measured at **over a thousand +/// times the input** before this existed. The bytes are what to count, +/// not the entries, because one image can be the whole ceiling on its +/// own and a thousand tiny ones are harmless. +pub(crate) fn refuse_over_image_ceiling(have: usize, adding: usize) -> Result<(), MeshError> { + if have.saturating_add(adding) > MAX_GEOMETRY_BYTES { + return Err(MeshError::TooLarge { + field: "images", + value: MAX_GEOMETRY_BYTES as u64, + }); + } + Ok(()) +} + /// Refuse before the geometry arrives rather than after it. /// /// `have` is what has been emitted, `adding` what the next face would diff --git a/crates/mesh/tests/gltf.rs b/crates/mesh/tests/gltf.rs index ad16764a..0176cb5e 100644 --- a/crates/mesh/tests/gltf.rs +++ b/crates/mesh/tests/gltf.rs @@ -1180,17 +1180,19 @@ fn the_tables_read_from_a_document_and_from_a_container() { "textures":[{"source":0}], "images":[{"name":"grain","uri":"data:image/png;base64,AQIDBA=="}]}"#; - let alone = gltf::tables(text.as_bytes()).expect("a document on its own"); + let alone = + gltf::tables(text.as_bytes(), gltf::ImageBytes::Kept).expect("a document on its own"); assert_eq!(alone.materials.len(), 1); assert_eq!(alone.materials[0].name.as_deref(), Some("brass")); assert_eq!(alone.textures, [Some(0)]); assert_eq!(alone.images.len(), 1); assert_eq!(alone.images[0].name.as_deref(), Some("grain")); - assert_eq!(&*alone.images[0].bytes, &[1, 2, 3, 4]); + assert_eq!(alone.images[0].bytes.as_deref(), Some(&[1, 2, 3, 4][..])); // The same document wrapped, which the layers below cannot tell // apart and this one must. - let wrapped = gltf::tables(&container(text, &[])).expect("the same document, wrapped"); + let wrapped = gltf::tables(&container(text, &[]), gltf::ImageBytes::Kept) + .expect("the same document, wrapped"); assert_eq!(wrapped, alone); } @@ -1207,16 +1209,64 @@ fn an_image_stored_in_a_chunk_survives_the_document() { "bufferViews":[{"buffer":0,"byteLength":4}], "images":[{"bufferView":0,"mimeType":"image/png"}]}"#; - let read = gltf::tables(&container(text, &[9, 8, 7, 6])).expect("one image, from the chunk"); + let read = gltf::tables(&container(text, &[9, 8, 7, 6]), gltf::ImageBytes::Kept) + .expect("one image, from the chunk"); assert_eq!(read.images.len(), 1); - assert_eq!(&*read.images[0].bytes, &[9, 8, 7, 6]); + assert_eq!(read.images[0].bytes.as_deref(), Some(&[9, 8, 7, 6][..])); assert_eq!(read.images[0].media_type.as_deref(), Some("image/png")); } +/// **Many images may name one view, and asking for their bytes copies +/// each one.** +/// +/// Nothing in the format says two images must name two views. A +/// document that points a thousand of them at one shared region pays +/// about thirty bytes an entry to write and a gigabyte to hold, which +/// measured at nearly three thousand times the input and grew as its +/// square. Counting them instead costs nothing, and that is what a +/// caller reporting a model wants. +#[test] +fn images_that_share_one_view_are_counted_without_being_copied() { + let mut aliased = String::from( + r#"{"asset":{"version":"2.0"}, +"buffers":[{"byteLength":4}], +"bufferViews":[{"buffer":0,"byteLength":4}], +"images":["#, + ); + for index in 0..64 { + if index > 0 { + aliased.push(','); + } + aliased.push_str(r#"{"bufferView":0,"mimeType":"image/png"}"#); + } + aliased.push_str("]}"); + let packed = container(&aliased, &[1, 2, 3, 4]); + + let counted = gltf::tables(&packed, gltf::ImageBytes::Counted).expect("counted"); + assert_eq!(counted.images.len(), 64); + for image in &counted.images { + // **The length is known and the bytes are not held.** A caller + // reporting what a model carries needs exactly this much. + assert_eq!(image.len, 4); + assert_eq!(image.bytes, None); + } + + let kept = gltf::tables(&packed, gltf::ImageBytes::Kept).expect("kept"); + for image in &kept.images { + assert_eq!(image.bytes.as_deref(), Some(&[1, 2, 3, 4][..])); + } + // The two answer the same about everything but the bytes. + assert_eq!( + counted.images.iter().map(|image| image.len).sum::(), + kept.images.iter().map(|image| image.len).sum::() + ); +} + /// A document with neither table has neither, which is not a refusal. #[test] fn a_document_with_no_tables_has_none() { - let read = gltf::tables(br#"{"asset":{"version":"2.0"}}"#).expect("nothing is not a refusal"); + let read = gltf::tables(br#"{"asset":{"version":"2.0"}}"#, gltf::ImageBytes::Kept) + .expect("nothing is not a refusal"); assert_eq!(read, gltf::Tables::default()); } @@ -1224,8 +1274,11 @@ fn a_document_with_no_tables_has_none() { /// the layer that made it rather than by this one. #[test] fn a_table_that_refuses_refuses_the_call() { - let refused = gltf::tables(br#"{"asset":{"version":"2.0"},"images":[{"uri":"grain.png"}]}"#) - .expect_err("a second file is not opened"); + let refused = gltf::tables( + br#"{"asset":{"version":"2.0"},"images":[{"uri":"grain.png"}]}"#, + gltf::ImageBytes::Kept, + ) + .expect_err("a second file is not opened"); assert_eq!(refused, GltfError::ExternalResource); } diff --git a/tools/cli/src/main.rs b/tools/cli/src/main.rs index cc705e58..c5f90f13 100644 --- a/tools/cli/src/main.rs +++ b/tools/cli/src/main.rs @@ -559,7 +559,7 @@ fn extension_for(media_type: &str) -> Option<&'static str> { /// own address for the image, so a caller reading the JSON can match a /// material's texture reference to a file without guessing. fn write_images( - images: &[renew_mesh::gltf::Image<'static>], + images: &[renew_mesh::gltf::Stored], directory: &str, ) -> Result, ImportFailure> { // **Every name settled before the first byte is written.** Checking @@ -610,8 +610,19 @@ fn write_images( let mut written = Vec::with_capacity(names.len()); for (name, image) in names.iter().zip(images) { + // **Asked for and therefore present.** `ImageBytes::Kept` is + // what puts them here, and it is chosen by the same flag that + // reaches this function -- so an absence would be a caller + // asking for files without asking for bytes, which is a mistake + // in this file rather than anything a document can cause. + let Some(bytes) = image.bytes.as_deref() else { + return Err(ImportFailure { + message: format!("image {name} was measured rather than read"), + refusal: None, + }); + }; let path = root.join(name); - if let Err(error) = std::fs::write(&path, &*image.bytes) { + if let Err(error) = std::fs::write(&path, bytes) { return Err(ImportFailure { message: format!("cannot write {}: {error}", path.display()), refusal: None, @@ -725,7 +736,7 @@ fn material_json(material: &renew_mesh::pbr::Material) -> Value { /// The bytes themselves are never put in the envelope: they are a /// texture, the JSON is a report, and base64 in a status line would make /// a megabyte of output nobody reads. -fn image_json(image: &renew_mesh::gltf::Image<'static>) -> Value { +fn image_json(image: &renew_mesh::gltf::Stored) -> Value { Value::Object(vec![ ( "name".to_string(), @@ -743,11 +754,73 @@ fn image_json(image: &renew_mesh::gltf::Image<'static>) -> Value { ), ( "bytes".to_string(), - Value::Number(i64::try_from(image.bytes.len()).unwrap_or(i64::MAX)), + Value::Number(i64::try_from(image.len).unwrap_or(i64::MAX)), ), ]) } +/// What a document says beyond its shape, and why it might say nothing. +struct Beyond { + tables: Option, + unreadable: Option<(&'static str, String)>, +} + +/// Read the material and image tables, or explain their absence. +/// +/// **The format decides whether there is an answer at all**: one that +/// states no materials in this vocabulary answers `None`, which is not +/// the same as answering that it has none. +/// +/// **A table that will not read does not take the geometry down with +/// it, unless the caller asked for the images.** Reading a model is what +/// this command is for, and the commonest glTF in the world keeps its +/// textures in files beside itself -- which this reader will not open, +/// and which says nothing about whether the geometry is sound. So the +/// refusal is reported and the import proceeds. Where `--images` was +/// given the caller asked for something that cannot be delivered, and +/// then it is fatal. +/// +/// # Errors +/// +/// The reader's own refusal, when the caller asked for images and the +/// tables holding them would not read. +fn beyond_geometry( + found: renew_mesh::format::Format, + bytes: &[u8], + images_dir: Option<&str>, +) -> Result { + // **The bytes are copied only where they are going somewhere.** An + // image stored in a buffer view points into the document, so keeping + // it means copying it -- and a caller who did not ask for files has + // no use for the copy. On a four-megabyte texture that copy measured + // at ninety-nine per cent of the whole call. + let wanted = if images_dir.is_some() { + renew_mesh::gltf::ImageBytes::Kept + } else { + renew_mesh::gltf::ImageBytes::Counted + }; + + match found.tables(bytes, wanted) { + None => Ok(Beyond { + tables: None, + unreadable: None, + }), + Some(Ok(tables)) => Ok(Beyond { + tables: Some(tables), + unreadable: None, + }), + Some(Err(refusal)) => { + if images_dir.is_some() { + return Err(refusal); + } + Ok(Beyond { + tables: None, + unreadable: Some((refusal.name(), refusal.to_string())), + }) + } + } +} + fn run_asset_import( from: &str, out_path: &str, @@ -788,13 +861,6 @@ fn run_asset_import( let found = renew_mesh::format::detect(&bytes); let format = found.name(); - // Set when a table refused and the refusal was not fatal, so the - // envelope can say why it is reporting nothing rather than leaving - // a caller to read an absence as an emptiness. **The name and the - // sentence both**, for the same reason the failure envelope carries - // both: a name is for a program and a sentence is for a person, and - // this one never reaches `stderr` because the run succeeded. - let mut unreadable: Option<(&'static str, String)> = None; let Some(read) = found.read(&bytes) else { // A material library is not a broken mesh, and saying so is this // tool's judgement rather than a reader's refusal: no reader was @@ -823,36 +889,18 @@ fn run_asset_import( } }; - // **What the document says beyond its shape**, read from the same - // bytes before they are dropped. The format decides whether there is - // an answer at all: a format that states no materials in this - // vocabulary answers `None`, which is not the same as answering that - // it has none. - // - // **A table that will not read does not take the geometry down with - // it, unless the caller asked for the images.** Reading a model is - // what this command is for, and the commonest glTF in the world - // keeps its textures in files beside itself -- which this reader - // will not open, and which has nothing to do with whether the - // geometry is sound. So the refusal is *reported* and the import - // proceeds. Where `--images` was given the caller asked for - // something that cannot be delivered, and then it is fatal. - let tables = match found.tables(&bytes) { - None => None, - Some(Ok(tables)) => Some(tables), - Some(Err(refusal)) => { - if images_dir.is_some() { - return import_failure( - &format!("{from}: {refusal}"), - Some(refusal.name()), - json_mode, - started, - ); - } - unreadable = Some((refusal.name(), refusal.to_string())); - None + let beyond = match beyond_geometry(found, &bytes, images_dir) { + Ok(beyond) => beyond, + Err(refusal) => { + return import_failure( + &format!("{from}: {refusal}"), + Some(refusal.name()), + json_mode, + started, + ); } }; + let (tables, unreadable) = (beyond.tables, beyond.unreadable); // The file is not read again after this, and it can be as large as // the model: holding it across the write was a quarter of this diff --git a/tools/cli/tests/cli.rs b/tools/cli/tests/cli.rs index 1b619c99..dfdc0516 100644 --- a/tools/cli/tests/cli.rs +++ b/tools/cli/tests/cli.rs @@ -2234,9 +2234,12 @@ fn an_empty_images_path_is_refused() -> std::io::Result<()> { "", ])?; assert!(!output.status.success(), "an empty path is not a directory"); + let said = String::from_utf8_lossy(&output.stderr); + // Same trap as the rule above: the usage block always names the + // flag, so only the sentence proves the guard ran. assert!( - String::from_utf8_lossy(&output.stderr).contains("--images"), - "and the caller hears about the flag they typed" + said.contains("`--images` needs a value"), + "the caller hears why, not just the usage text: {said:?}" ); Ok(()) } @@ -2557,6 +2560,31 @@ fn asset_import_reports_the_tables_without_writing_them() -> std::io::Result<()> && reported.contains("\"media_type\":\"image/jpeg\""), "both images are reported with the types the document stated: {reported:?}" ); + + // **The whole payload, by value.** Asserting on a hand-picked + // substring leaves every number free: a report that swapped metallic + // for roughness, negated `double_sided`, relabelled every texture + // role, or said a four-byte image was zero bytes long would pass a + // test that only looked for a name. So the material and the images + // are compared as written, in full. + let material = concat!( + r#"{"name":"brass","base_color":[0.5,0.25,0.125,1.0],"metallic":1.0,"#, + r#""roughness":0.25,"emissive":[0.0,0.0,0.25],"double_sided":true,"#, + r#""alpha_mode":"MASK","alpha_cutoff":0.75,"#, + r#""textures":[{"role":"base_color","texture":0,"uv_set":0}]}"# + ); + assert!( + reported.contains(material), + "the material reads back exactly as the document stated it: {reported:?}" + ); + let carried = concat!( + r#""images":[{"name":"grain","media_type":"image/png","bytes":4},"#, + r#"{"name":null,"media_type":"image/jpeg","bytes":4}]"# + ); + assert!( + reported.contains(carried), + "and so do both images, names and lengths included: {reported:?}" + ); assert!( reported.contains("\"images_written\":[]"), "and none were written, because none were asked for: {reported:?}" @@ -2728,9 +2756,14 @@ fn images_is_refused_on_another_subcommand() -> std::io::Result<()> { let output = run(&["asset-pack", "--images", "textures"])?; assert!(!output.status.success(), "it is not that command's flag"); let said = String::from_utf8_lossy(&output.stderr); - assert!( - said.contains("--images"), - "and the caller hears about the flag they typed: {said:?}" + // **Not `contains("--images")`.** Every parse error prints the usage + // block, and the usage block lists `--images`, so that assertion + // passes however the guard behaves -- which is a test that cannot + // fail. The sentence naming the unexpected argument is the thing + // this rule actually produces. + assert!( + said.contains("unexpected argument `--images`"), + "the caller hears about the flag they typed, not the usage text: {said:?}" ); Ok(()) } From fa66e4f9c94fef2e80866db3f10d25e2e0e6e7e0 Mon Sep 17 00:00:00 2001 From: CagdasErturk Date: Wed, 9 Sep 2026 15:39:44 +0300 Subject: [PATCH 6/6] test(mesh): the owning table reader is fuzzed, and its ceiling is tested at the boundary Nothing called `gltf::tables`. The target read the borrowing half and wrote its own container dispatch, so the one path that copies an image was never attacked -- which is how a document that points many images at one buffer view got as far as it did. It is called now, from the top so that a document whose container refuses in the borrowing half still reaches it, and the two modes are held against each other: the same materials, textures and count, and per image the same name, type and length, with an image that kept bytes keeping exactly as many as it measured. Counting is asserted to hold no bytes at all, which is the property the distinction exists for. A seed for the shape, since nothing else in the corpus reaches that arithmetic: four images over one view. The image ceiling gets the boundary test its siblings have. The copy that exactly fills it is allowed, one byte past is refused by name, and a sum that saturates is over the ceiling rather than back under it -- lengths come out of the document, and two near the pointer's width would otherwise wrap to an acceptance. The writer's arm for an image whose bytes were measured rather than kept is gone. The caller asks for bytes and files together, so nothing could reach it, and an arm nothing can run is worse than no arm: it reads as a case that has been handled. The pass builds the names and the bytes together instead. The exempt block in the command's source moved again with the code above it. Located by content and verified line by line at the new offset. --- coverage-exemptions.toml | 2 +- crates/mesh/README.md | 2 +- crates/mesh/examples/make_gltf_corpus.rs | 13 ++++ crates/mesh/src/lib.rs | 37 ++++++++++- crates/mesh/tests/corpus_replay.rs | 2 +- fuzz/corpus/gltf_read/image-shared-view.gltf | 4 ++ fuzz/fuzz_targets/gltf_read.rs | 68 ++++++++++++++++++++ tools/cli/src/main.rs | 29 ++++----- 8 files changed, 137 insertions(+), 20 deletions(-) create mode 100644 fuzz/corpus/gltf_read/image-shared-view.gltf diff --git a/coverage-exemptions.toml b/coverage-exemptions.toml index 122e7b9d..4b5ab2cd 100644 --- a/coverage-exemptions.toml +++ b/coverage-exemptions.toml @@ -171,7 +171,7 @@ reason = "The arm that clears a voice whose sound index is missing. A voice is o [[exempt]] file = "tools/cli/src/main.rs" -lines = [2174, 2175, 2177, 2178, 2179, 2180, 2181, 2192, 2196, 2197, 2198, 2200, 2204, 2218, 2219, 2220, 2222, 2223, 2224, 2225, 2226, 2227, 2228, 2229, 2230, 2231, 2232, 2233, 2235, 2239, 2240, 2263, 2281, 2282, 2283, 2284, 2291, 2292, 2293, 2294, 2295, 2296, 2297, 2300, 2304, 2305, 2306, 2310, 2690, 2691, 2692, 2693] +lines = [2219, 2220, 2222, 2223, 2224, 2225, 2226, 2237, 2241, 2242, 2243, 2245, 2249, 2263, 2264, 2265, 2267, 2268, 2269, 2270, 2271, 2272, 2273, 2274, 2275, 2276, 2277, 2278, 2280, 2284, 2285, 2308, 2326, 2327, 2328, 2329, 2336, 2337, 2338, 2339, 2340, 2341, 2342, 2345, 2349, 2350, 2351, 2355, 2735, 2736, 2737, 2738] reason = "The determinism emit half past its first child, plus the two arms that answer for a target this process is not running on. Reaching the emit arms from a test would mean building and running pinned runs under instrumentation: the report-reading arms need the first pinned run (renew-ui, no arguments) to compile and succeed, and the leg construction and write at the tail need all eleven - spread across six workspace packages, four of them samples and two engine crates - to succeed. The success path is not untested: the three determinism legs execute it on Linux, Windows and macOS on every push, and a failure there is what a broken emit looks like. Everything testable without a subprocess has been moved out - digests_from_output has seven cases beside it, digest_name is the one spelling both sides call, emit_note is unit-tested on both branches, pinned_invocation asserts the --target pass-through with no device, and the emit-red path is driven end to end by tests/targets.rs in both output modes. What is left is process orchestration, the leg written when every child succeeded, and the arms no gating push can reach: a compiler that answered and failed when asked its own version, a child that cannot start at all, a child whose report is unreadable, a leg file that cannot be written, one pinned run claiming a digest name another already used, and a working directory that has ceased to exist beneath the process. The --target arms are their own case: the refusal for a triple the table cannot name is reached by nothing at all, because every lane passes either a known triple or none, and it exists so a target added to CI and forgotten in that table fails loudly instead of emitting a leg labelled by a guess; the arm that labels a leg from its triple is executed only by the Android emulator lane, which is advisory and cannot redden main, so it is held by a lane whose red only a reader sees - which is the honest description until that row is signed and the lane gates." [[exempt]] diff --git a/crates/mesh/README.md b/crates/mesh/README.md index 5df2b837..2221cc06 100644 --- a/crates/mesh/README.md +++ b/crates/mesh/README.md @@ -206,7 +206,7 @@ the 25 STL seeds, `examples/make_ply_corpus.rs` the 24 PLY ones, `examples/make_mtl_corpus.rs` the 21 MTL ones and `examples/make_glb_corpus.rs` the 20 container ones and `examples/make_accessor_corpus.rs` the 21 accessor ones and -`examples/make_gltf_corpus.rs` the 43 document-and-container ones; between them +`examples/make_gltf_corpus.rs` the 44 document-and-container ones; between them every committed seed is built here rather than found. **The blob's 25 seeds need no such argument at all**, because the format is this crate's own and `examples/make_blob_corpus.rs` gets every byte from `blob::write`. **For OBJ that rule bites diff --git a/crates/mesh/examples/make_gltf_corpus.rs b/crates/mesh/examples/make_gltf_corpus.rs index 7c071f23..dba1dc82 100644 --- a/crates/mesh/examples/make_gltf_corpus.rs +++ b/crates/mesh/examples/make_gltf_corpus.rs @@ -408,6 +408,19 @@ fn image_seeds() -> Vec<(String, Vec)> { "image-payload-broken".to_owned(), with(r#"[{"uri":"data:image/png;base64,AQID!A=="}]"#), ), + // **Several images over one view, which is the shape that + // amplifies.** Nothing in the format says two images may not + // name the same region, so the entries are cheap to write and + // each one costs a copy to hold. Kept as a seed because the + // arithmetic it provokes is not reachable from any other shape + // here. + ( + "image-shared-view".to_owned(), + stored( + r#"[{"bufferView":0,"mimeType":"image/png"},{"bufferView":0,"mimeType":"image/png"}, +{"bufferView":0,"mimeType":"image/jpeg"},{"bufferView":0,"mimeType":"image/png"}]"#, + ), + ), // A second file, which this crate will not open. ( "image-names-a-file".to_owned(), diff --git a/crates/mesh/src/lib.rs b/crates/mesh/src/lib.rs index c80d4faf..0923c2aa 100644 --- a/crates/mesh/src/lib.rs +++ b/crates/mesh/src/lib.rs @@ -507,7 +507,8 @@ mod tests { #[cfg(test)] mod ceiling_tests { use super::{ - MAX_MATERIALS, MAX_POSITIONS, MeshError, refuse_over_ceiling, refuse_over_material_ceiling, + MAX_GEOMETRY_BYTES, MAX_MATERIALS, MAX_POSITIONS, MeshError, refuse_over_ceiling, + refuse_over_image_ceiling, refuse_over_material_ceiling, }; /// **The material ceiling refuses at the boundary rather than past @@ -532,6 +533,40 @@ mod ceiling_tests { ); } + /// **The image ceiling counts bytes rather than images**, and + /// refuses at the boundary rather than past it. + /// + /// Counting entries would be the wrong rule twice over: one image + /// can be the whole ceiling on its own, and a thousand four-byte + /// ones are harmless. What a document controls is how many entries + /// point at the same bytes, and it is the copying that costs. + /// + /// Probed by deleting the check: red, a copy past the ceiling is + /// accepted. Probed by loosening `>` to `>=`: red, the copy that + /// exactly fills the ceiling is refused when it fits. + #[test] + fn the_image_ceiling_counts_the_bytes_copied_so_far() { + refuse_over_image_ceiling(MAX_GEOMETRY_BYTES - 4, 4) + .expect("a copy that exactly fills the ceiling is not over it"); + + assert_eq!( + refuse_over_image_ceiling(MAX_GEOMETRY_BYTES - 4, 5), + Err(MeshError::TooLarge { + field: "images", + value: MAX_GEOMETRY_BYTES as u64, + }), + "one byte past it is over it, and it says which table by name" + ); + + // **The sum cannot wrap.** A document states its own lengths, so + // two of them near the pointer's width would answer `Ok` under + // wrapping arithmetic and let the copy through. + assert!( + refuse_over_image_ceiling(usize::MAX, usize::MAX).is_err(), + "a sum that saturates is over the ceiling, not back under it" + ); + } + /// **The ceiling is on the product, and it refuses at the boundary /// rather than past it.** /// diff --git a/crates/mesh/tests/corpus_replay.rs b/crates/mesh/tests/corpus_replay.rs index dad38a7c..051e829d 100644 --- a/crates/mesh/tests/corpus_replay.rs +++ b/crates/mesh/tests/corpus_replay.rs @@ -1209,7 +1209,7 @@ fn accessor_census() { // beside the crate, where a wedged run is a failed test. // --------------------------------------------------------------------- -const GLTF_LOW_WATER: usize = 41; +const GLTF_LOW_WATER: usize = 42; /// How many distinct outcomes the glTF seeds must still reach. /// diff --git a/fuzz/corpus/gltf_read/image-shared-view.gltf b/fuzz/corpus/gltf_read/image-shared-view.gltf new file mode 100644 index 00000000..16af3815 --- /dev/null +++ b/fuzz/corpus/gltf_read/image-shared-view.gltf @@ -0,0 +1,4 @@ +{"asset":{"version":"2.0"}, +"buffers":[{"byteLength":4,"uri":"data:application/octet-stream;base64,AQIDBA=="}], +"bufferViews":[{"buffer":0,"byteLength":4}],"images":[{"bufferView":0,"mimeType":"image/png"},{"bufferView":0,"mimeType":"image/png"}, +{"bufferView":0,"mimeType":"image/jpeg"},{"bufferView":0,"mimeType":"image/png"}]} \ No newline at end of file diff --git a/fuzz/fuzz_targets/gltf_read.rs b/fuzz/fuzz_targets/gltf_read.rs index fde4ab0e..2fcff874 100644 --- a/fuzz/fuzz_targets/gltf_read.rs +++ b/fuzz/fuzz_targets/gltf_read.rs @@ -49,6 +49,16 @@ fuzz_target!(|data: &[u8]| { // trust is likeliest to be. tables_answer(data); + // **The owning entry point, from the raw bytes, doing its own + // dispatch.** Everything above holds a parse and borrows out of it; + // `tables` is what a caller uses when it cannot, and it is the only + // path that *copies* an image. That copy is what a document can + // amplify -- nothing says two images may not name one buffer view, + // so a thousand entries can point at one megabyte and the bytes come + // back a thousand times over. The fuzzer's own memory limit is what + // catches that, and it can only catch it if something calls this. + owned_tables_answer(data); + let Ok(mesh) = gltf::read(data) else { // A refusal is an answer. Which refusal is the suite's business // beside the crate; that the call returned at all is this @@ -256,3 +266,61 @@ fn tables_answer(data: &[u8]) { let again = gltf::materials(root).expect("what read once reads again"); assert_eq!(again, materials, "the same bytes read to the same materials"); } + +/// Read the owning tables, and hold them to the borrowing ones. +/// +/// Separate from the block above because it starts from bytes rather +/// than from a parse: it repeats the container dispatch on purpose, so +/// that the dispatch itself is attacked and not merely the readers +/// underneath it. +fn owned_tables_answer(data: &[u8]) { + let Ok(counted) = gltf::tables(data, gltf::ImageBytes::Counted) else { + // A refusal is an answer, as everywhere else in this target. + return; + }; + + // **Counting states the length without holding the bytes**, which is + // the whole point of the distinction: a caller reporting what a + // model carries pays nothing for images it will never write. + for image in &counted.images { + assert!( + image.bytes.is_none(), + "an image was counted and its bytes were kept anyway" + ); + } + + let Ok(kept) = gltf::tables(data, gltf::ImageBytes::Kept) else { + // Keeping can refuse where counting does not: the copy answers to + // a ceiling and the count does not need to. + return; + }; + + assert_eq!( + kept.materials, counted.materials, + "the same bytes read to the same materials" + ); + assert_eq!( + kept.textures, counted.textures, + "and to the same texture table" + ); + assert_eq!( + kept.images.len(), + counted.images.len(), + "and to the same number of images" + ); + for (kept, counted) in kept.images.iter().zip(&counted.images) { + assert_eq!(kept.name, counted.name); + assert_eq!(kept.media_type, counted.media_type); + // **The length is the same whether or not the bytes were kept**, + // which is what lets a report be built from the cheap half. + assert_eq!( + kept.len, counted.len, + "one image measured two different lengths" + ); + assert_eq!( + kept.bytes.as_ref().map(Vec::len), + Some(kept.len), + "an image kept a different number of bytes than it measured" + ); + } +} diff --git a/tools/cli/src/main.rs b/tools/cli/src/main.rs index c5f90f13..bc69412f 100644 --- a/tools/cli/src/main.rs +++ b/tools/cli/src/main.rs @@ -567,7 +567,7 @@ fn write_images( // directory to trust: four files written, the fifth refused, and a // blob on disk beside them. The names are two literals and an index, // so the pass costs nothing. - let mut names = Vec::with_capacity(images.len()); + let mut ready: Vec<(String, &[u8])> = Vec::with_capacity(images.len()); for (index, image) in images.iter().enumerate() { let Some(stated) = image.media_type.as_deref() else { return Err(ImportFailure { @@ -586,13 +586,21 @@ fn write_images( refusal: Some("UnknownMediaType"), }); }; - names.push(format!("image-{index}.{extension}")); + // **Present because the caller asked for them.** + // `ImageBytes::Kept` is what puts bytes here and it is chosen by + // the same flag that reaches this function, so this is a + // narrowing rather than a question -- and written as one, since + // an arm for the case that cannot arise is an arm nothing can + // ever run. + if let Some(bytes) = image.bytes.as_deref() { + ready.push((format!("image-{index}.{extension}"), bytes)); + } } // **Nothing at all when there is nothing**, not even the directory. // A model with no images should leave no trace of having been asked // for them, which is the same rule the absent flag obeys. - if names.is_empty() { + if ready.is_empty() { return Ok(Vec::new()); } @@ -608,19 +616,8 @@ fn write_images( }); } - let mut written = Vec::with_capacity(names.len()); - for (name, image) in names.iter().zip(images) { - // **Asked for and therefore present.** `ImageBytes::Kept` is - // what puts them here, and it is chosen by the same flag that - // reaches this function -- so an absence would be a caller - // asking for files without asking for bytes, which is a mistake - // in this file rather than anything a document can cause. - let Some(bytes) = image.bytes.as_deref() else { - return Err(ImportFailure { - message: format!("image {name} was measured rather than read"), - refusal: None, - }); - }; + let mut written = Vec::with_capacity(ready.len()); + for (name, bytes) in &ready { let path = root.join(name); if let Err(error) = std::fs::write(&path, bytes) { return Err(ImportFailure {