A glTF document reads on its own - #332
Merged
Merged
Conversation
Until now a document's buffers table was never read at all: every view was assumed to point into the container's own chunk, and a view naming any other buffer was refused. That is one of the two shapes a glTF asset comes in, and it is not the self-contained one. So the buffers table is read. A buffer with no source is the container's chunk, and only the first may be - the specification leaves any other sourceless buffer undefined, and undefined is refused here rather than guessed at, because the alternative is handing a view the first buffer's bytes and calling the result geometry. A buffer may instead embed its payload as a data URI, which must declare one of the two media types the specification allows and is decoded in place. Anything else names a second file, which this crate will not open. A buffer is its resource cut to the length it declares. The specification allows the resource to be longer and says only the first byteLength bytes belong to the buffer, and that is not a technicality: the container pads its binary chunk to a four-byte boundary, so the chunk is routinely longer than the buffer inside it. Cutting is what stops a view reaching past the buffer into that padding and being handed bytes the document never claimed. A resource shorter than its buffer is a disagreement between the document and its payload, and is refused naming both numbers. A view now carries the buffer it points at, the way an accessor already carried the view it reads through, and the three tables travel as one value rather than as three arguments threaded through every function that only passes them along. The suite's own fixtures were documents that declared views over a buffers table they never wrote, which the old reader never looked at. They declare it now, and two tests that were one fault are two: a view past its buffer is a fault in a document that disagrees with itself, and a buffer past its resource is a document that disagrees with its chunk.
…r it A glTF asset comes in two shapes: a document wrapped in a container beside a chunk of geometry, or that same document alone, carrying its geometry as payloads embedded in itself. Only the first could be read. Now `read` takes either, choosing on the four-byte magic, and everything below that line is identical - which is what building the buffers table first was for. Detection stops answering with the fallback format for a document. The new arm parses and asks for the asset object with a version string that the specification requires of every document, because "starts with a brace" would claim every configuration file in the world, and a prefix test is the defect the polygon reader's magic check was tightened to cure. It sits after the magic checks and before the keyword ones, where it cannot take another format's files. The cost is a parse before the reader parses again, and that is the right price for not answering confidently about the wrong format. A behaviour change worth naming: bytes whose magic is wrong are no longer a container refusal. They are not a container at all, so the reader tries the remaining shape and the document layer is what refuses. The test claiming a container fault arrives as a container fault now uses a fixture that is one - magic intact, version broken - and a second test pins the new dispatch. The corpus had no document seeds at all, so the whole second shape was reachable only by a mutation that destroyed the magic, and a mutation that destroys the magic usually destroys everything after it. Five seeds start inside that path. Adding them took the corpus's only container refusal with them, because the wrong-magic seed changed sides; a seed with the magic intact and the version broken closes that, and the required list now names the buffer layer's refusals so the same hole cannot open quietly.
Four reviews against this branch, and the checks were green throughout. A bufferView's buffer is a required member with no default of its own, and it was being defaulted to zero. The schema lists it and byteLength as required and gives neither a default, while byteOffset beside it does have one - which is why the mistake reads as plausible. It was harmless while every buffer but the first was refused, because absent and zero named the same bytes, and it became a wrong answer inside this very branch: a view naming no buffer was handed the first one's bytes, which is the failure the change describes itself as refusing. Every fixture, every seed and the tool's own test carried the non-conformant spelling and now name their buffer. Nothing tested it. The member appeared once in the whole suite, as zero, so a reader that dropped the index entirely would have passed every test, every property and the replay gate. There is a document with two embedded buffers of distinguishable content now, and forcing every view to the first buffer fails it. Detection ran an unbounded parse ahead of every bounded check. The JSON reader validates the whole slice as text before it looks at a value, so a large polygon or wavefront file paid a full scan to be told it was not JSON - and every other arm of that dispatch is deliberately bounded and says so. A document's root is an object, so when the first byte that is not whitespace is not a brace the answer was already no; asking that first costs nothing and changes nothing. Reading a table was quadratic twice over: an index into a value walks from the head, and the refusal for a missing row was built eagerly, so counting the table happened on every successful lookup too. The value type's own documentation says a layer wanting random access should build its table in one pass. Three of them do now. A media type is not case sensitive, and the decoder one file over already forgives the marker's case under the rule that a difference which cannot change an output byte is not a difference. The buffer's media type is compared the same way now. Tests that asserted less than their names: the owned half of the buffer cut never cut anything, because every embedded payload decoded to exactly its declared length; the same-document-in-a-container test wrapped it beside nothing, because the helper writes no chunk for an empty payload; the document detector had no test of its own; and a document with views and no buffers table changed from reading to refusing with nothing naming it. The corpus gained seeds for the two refusals nothing provoked, a floor so it cannot lose its only document that reads, and file extensions derived from the bytes rather than assumed, since half these seeds are documents and the container extension said otherwise. Seventeen documentation defects, including a module doc that still said this reader does not call the decoder it now calls.
Member
Author
|
Four review passes rejected this, while every check was green — so the merge waited on them, and the last commit is what came of it. The one that mattered: a Nothing tested it. Also fixed: a detection arm that ran an unbounded parse ahead of every bounded check; table reads that were quadratic twice over, against the value type's own documented advice; a case-sensitive media type in a crate whose sibling decoder already forgives case for the same reason; four tests asserting less than their names claimed; two refusals with no corpus seed; and seventeen documentation defects, including a module doc that still said this reader does not call the decoder it now calls. Local gates on the new head: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A glTF asset comes in two shapes: a document wrapped in a container beside a chunk of geometry, or that same document alone, carrying its geometry as payloads embedded in itself. Only the first could be read. Both can now, and everything below the four-byte magic that chooses between them is identical.
The buffers table is read for the first time. A buffer with no source is the container's own chunk, and only the first may be — the specification leaves any other sourceless buffer undefined, and undefined is refused here rather than guessed at, because the alternative is handing a view the first buffer's bytes and calling the result geometry. A buffer may instead embed its payload as a
data:URI, which must declare one of the two media types the specification allows and is decoded in place. Anything else names a second file, which this crate will not open.A buffer is its resource cut to the length it declares. The specification allows the resource to be longer and says only the first
byteLengthbytes belong to the buffer — and that is not a technicality: the container pads its binary chunk to a four-byte boundary, so the chunk is routinely longer than the buffer inside it. Cutting is what stops a view reaching past the buffer into that padding and being handed bytes the document never claimed. A resource shorter than its buffer is a disagreement between document and payload, refused naming both numbers.Detection stops answering with the fallback format for a document. The new arm parses and asks for the
assetobject with aversionstring the specification requires, because "starts with{" would claim every configuration file in the world — a prefix test is the defect the PLY magic check was tightened to cure. It sits after the magic checks and before the keyword ones, where it cannot take another format's files.A behaviour change worth naming: bytes whose magic is wrong are no longer a container refusal. They are not a container at all, so the reader tries the remaining shape and the document layer refuses. The test claiming "a container fault arrives as a container fault" now uses a fixture that is one — magic intact, version broken — and a second test pins the new dispatch.
Three things caught by guards rather than by inspection, which is the part worth reporting:
bufferViewsover abufferstable they never wrote. Invalid glTF that the old reader got away with because it never looked. Twenty-one corrected here.Five new refusals, each with a sentence in the census that would not compile without one. Corpus 26 seeds reaching 11 outcomes, up from 20 and 8, with floors raised to match.
Local gates:
cargo fmt --all --checkclean,cargo clippy --workspace --all-targets -D warningsclean,cargo test --workspace264 suites / 2,947 passed / 0 failed,renew checkreports the workspace healthy.