Bsb/new model workflow - #54
Draft
bsb808 wants to merge 33 commits into
Draft
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six inline comments, answered in place rather than deferred. Textures embedded versus external, and how much version resolution to track, became decisions rather than assertions. The coordinate-system bullets gained explanations of UV and of the sRGB versus linear split, the second tied to the Gazebo bug where every PBR texture was treated as color data and meshes rendered too dark. The +Z forward question drove the largest change. Reading the specification closely shows the sentence is stated about the asset rather than each mesh, carries no normative keyword, and says nothing about where a mesh origin belongs. So the review no longer frames our convention as violating anything. Orientation and origin placement are now stated as open, with the trade-offs and, for origin, with what the library measurably does: centered in plan, referenced to the mounting plane vertically, on all but two parts. That contradicts parts.md, the older README and the draft spec, each differently. The stability questions are answered directly. Gazebo's glTF handling is not stable, the record shows continuous correction since 2022, and patch-level drift already changed behavior under this audit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Carlos's original, unmodified, so the revision that follows has a baseline to diff against and the starting point is recoverable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reproduced verbatim from the browser artifacts, each carrying a header naming the errors found in it and pointing at the section of the review that re-checked it. Committed because the review already links to them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cept Section 0.1 now opens with the design goal the workflow rests on: build on published standards, glTF 2.0, REP 103, URDF and SDF, and define project convention only where no standard reaches. Two obligations follow. Departures are explicit and written where the affected person reads them, and where a standard says nothing we say so rather than implying an authority that does not exist. Section 1.9 works the origin question as the case where both obligations apply. Whether meshes share a frame and where that frame's zero sits have opposite answers: the first is guaranteed by the node hierarchy, the second is not addressed anywhere in glTF. Primitives carry no transform, so the seven primitives of the BlueROV2 chassis share one origin by construction, and that part's problems are material and naming rather than frame. parts.md gains the vocabulary section explaining why this library says part, mesh and assembly rather than asset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Renamed from asset-spec.md, since "asset" spans meshes, textures, rigs and scenes and glTF uses it for a whole file, so it cannot be used precisely. The original draft is recoverable at 5d808b1. The document now separates rules from reasoning. Rules live here; the evidence and the record of what was measured stay in the review, and the durable rationale is carried alongside each rule as an Implementation Note. Four conventions are borrowed from the glTF specification's own document conventions, each answering a problem the audit found. BCP 14 keywords, so a reader can tell a requirement from a preference, which the previous draft could not express. Informative language marked as such, so rationale can sit next to a rule without looking like one. A declared audience, since the modeler and the integrator are responsible for different checks. And a terminology section that governs, which is what makes the vocabulary decision binding rather than advisory. Open questions are stated as Undecided blocks rather than omitted, so the holes are visible to the people they affect instead of being discoverable only by asking. Ten of them, each naming the decision that would settle it, indexed in section 13. A delivery cannot fail to conform on an undecided point. Section 1.3 marks which content is project-specific, since the format subset, the frame rules and the material requirements generalize beyond this repo while naming, delivery paths and toolchain versions do not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Mesh" prejudged an open question. Whether a part is one glTF mesh, one node or several is undecided, so naming the document after the term presumed an answer. It also forced the terminology section to redefine a standard term, which sits badly with a specification whose stated design goal is to rest on external standards. "Model" now names the deliverable, the file and its contents, and "mesh" keeps its glTF meaning throughout. The remaining uses of "mesh" are all in that sense, or ordinary English as in mesh guards. Note for later: "model" collides with SDF <model> and Gazebo models, which mean a whole simulated entity with links and joints. The local usage in bluerobotics_parts/models/<part>/ matches the sense used here, so the collision is tolerable, but a future generalization outside this project should revisit it. The review's references to the old path are updated to say the draft was committed unmodified and then superseded, rather than pointing at a file that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restoring asset-spec.md. Committing it and then overwriting it in place was the wrong reading of "get it into git as-is": the draft is a source document being audited, in the same category as the two artifact reports, and those were kept as files. Section 3 of the review quotes it item by item, so having it present is what makes that audit checkable without a git command. It carries a header marking it superseded and pointing at model-spec.md, matching the pattern used on the two reproduced reports. Carlos's text below that note is unmodified. The spec and the review now link to the file instead of citing a commit hash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five items, all in the opening. The description of the first half is rewritten in plain terms: it tests what we already believe, by setting out what counts as correct, explaining how a glTF file is assembled and what Gazebo and RViz do with one, then checking the draft and the delivered files against that. That also settles the boundary problem, since the previous wording called sections 0 to 6 the audit when sections 0 to 2 establish the basis rather than audit anything. The description of the second half loses two pieces of scaffolding that announced a contrast instead of stating content. The glTF specification and REP 103 join the inputs list, with the extension registry and the validator alongside the first. REP 103 checked as asked: Standard Units of Measure and Coordinate Conventions, the source of x forward, y left, z up. The sources-of-truth table is transposed onto Truth, Assessment, How we do it now, and Proposed method. The two new rows are the point of the change. Today the format is never checked, intended appearance is judged from Blender's viewport rather than the exported file, and the targets are covered ad hoc by three people with three sets of tools. That is the Multiple sources of truth issue restated as a gap, with a named tool and owner proposed for each column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The validator and the model-spec check both go to CI, beside the base color guard already in the parts test, with the workflow fetching the validator binary since it has no apt package. Not agents: a conformance gate has to give the same answer every time. The Sample Viewer link turns out to matter more than a link usually does. The hosted viewer runs the Khronos validator inline, so one drag and drop answers both the format and the appearance question, which makes the modeler-side check a single action on every delivery. Budgets become a fourth row rather than a separate section. It is a different kind of row and says so: the other three measure against a published specification, a reference renderer and the tools we ship, while glTF sets no limit on geometry or texture size and neither does Gazebo, so we are the only authority. Measured rather than asserted, and the measurement answers the question that prompted it. Textures are 59 percent of every byte stored, so with an all-in-one GLB the lever is texture resolution, not vertex count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
glb_inventory.py parses the JSON chunk, reads the accessors and decodes the embedded images to report per part what is geometry and what is texture, the resolution of each map, and the texel density and UV fill that say whether a pixel count fits the surface it covers. GLB_INVENTORY.md is its output plus the reading, as the evidence behind the T4 budget row of the visual asset pipeline review and the input to decisions 4 and 5. Baseline commit for review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answers the tag on the hierarchy sentence, replaces "the Prims and Mats columns turn on them" with what those columns actually count, and adds the note that every glTF cross-reference is an integer array index, so "points at" has a concrete meaning everywhere it appears. Adds a mesh entry, since decision 15 is an argument about what "one mesh" means and the term was used but never defined, and restates the material entry with the material as its subject. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review comments, both about imprecise vocabulary. "Channel" was doing two jobs: the material's inputs, and the R, G, B, A components of a pixel. The table collided them directly, heading one column "Channel" in the first sense while the same row said "the blue channel of" in the second. The specification's word for the first sense is property, which it uses when introducing the model as "defined by the following properties", so the document now says property for the six material inputs and channel only for a component of a pixel, and states that rule rather than leaving it to be inferred. "Material" was used without being defined. It now carries the specification's definition and, more usefully, the containment chain from primitive to material to texture to image and sampler, which is what the question was really asking for. Reading-time estimates removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Third review round as left in the working tree, plus the author's terminology block. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ories Section 1 restructured around shading model = reflection model + parameterization, with format as the other choice. Physically plausible (Lewis 1994) separated from physically based; measurable-parameters clause dropped. Synonym table for the package by source. 3.4 corrected: metallic-roughness descends from Burley 2012, specular-glossiness is the older workflow. Emission added to the empirical coefficients. References filled in. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ments Baseline for the review round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Baseline for the next round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Baseline for the next round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds an annotated reading list in priority order, distinguishes BRDF from BTDF and BSDF, names the shading model glTF Appendix B defines, and adds a rendered comparison figure with its generator in docs/_tools. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Baseline for the next round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answers what the BRDF's value actually is (a density, not a bounded ratio), that it is per wavelength, and that lobes add. States how far glTF's pinning of the shading model goes and where it stops. Drops SDF and URDF as off topic and rewrites section 4 around expressible against implemented. Adds notes to every remaining subsection in 5 through 10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Writes out the BRDF as a ratio with its inputs, output and parameters named. Says which shading models carry more than one lobe. Answers the probability density analogy and the question of whether lobes simply add, with the Fresnel and metallic weighting glTF actually uses. Points at 8.4 for how three numbers stand in for a spectrum. Scopes the claim about what a conformant glTF file may contain to the core. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ssed Grounds the BRDF ratio in radiometric quantities and cites Marschner and Shirley chapter 18 as a whole, noting that photometry in 18.3 is the companion subject and that nothing here is photometric. Says how a direction is expressed, two angles on paper and a unit vector in code, and that the polar angle is measured from the normal rather than the surface. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The primer was general background on shading models and glTF, with nothing specific to this library in it. It now lives at posts/graphics/rendering-primer in the carrel repository, where its drafting history and sources are recorded in NOTES.md beside it. Project-specific material is unaffected: model-spec.md states the rules, VISUAL_ASSET_PIPELINE_REVIEW.md holds the evidence, and pbr-and-gltf.md remains as the short project-facing primer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Those are copyrighted textbooks consulted while drafting, kept locally and never an asset of this repository. Without this rule a git add -A would commit 23 MB of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Coming Soon