Usd import rework - #358
Merged
Merged
Conversation
Records the design for reworking USD Stage import, derived from a fresh look at TSD and USD together rather than from the existing importer. ADRs: - 0015: consume a Hydra scene index rather than traversing UsdGeom and UsdShade directly, so composition, purpose/visibility, instancing, material binding, primvar interpolation and skinning are resolved by OpenUSD instead of being reimplemented - 0016: bake prototype-internal transforms, since the render index never pushes a transform-array node's matrices onto the transform stack - 0017: deviate from usdview defaults for purpose and subdivision, and record why neither deviation is a bug - 0018: let imported scenes retain an open UsdStage so deforming geometry can be bound lazily instead of baked per frame Glossary: - Add a Foreign-Format Import section to the TSD I/O context, grouping the existing terms under Native Persistence, and reserve "Layer" and "Instance" for their TSD and ANARI meanings Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The USD importer rework consumes OpenUSD's own resolved scene through a Hydra scene index (ADR 0015), so tsd_io needs the imaging, scene-index filter, and image libraries, plus OpenSubdiv for subdivision refinement. TSD_USE_USD becomes a public compile definition in both its enabled and disabled forms so that consumers -- the test target in particular -- can compile conditionally against USD support. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the hand-written UsdGeom/UsdShade traversal with a converter over OpenUSD's own resolved scene (ADR 0015). Composition, Purpose and visibility resolution, native and point instancing, material binding, primvar interpolation, implicit shapes, NURBS approximation and skinning are resolved by OpenUSD before TSD sees them; TSD converts already-resolved prims into Scene objects and Layer nodes. The entry point gains a typed options value and returns an Import Report, so "content was silently dropped" becomes an assertable condition rather than something to notice by eye. Prims that do not become renderable content leave a named, disabled Placeholder Node at their place in the hierarchy, tagged with why, and are counted by reason in the report. What this changes for a user: - Point instancers import, as one transform-array node over shared Prototype objects; placements USD marks invisible are omitted. - USD Instances share one converted Prototype across placements, with each gprim's Prototype-relative transform baked into its vertex data (ADR 0016). - Guide and proxy Purpose are excluded by default and render is included (ADR 0017); exclusions are reported per Purpose and are configurable. - Prims resolving to invisible import as disabled nodes rather than vanishing. - Meshes tessellate through OpenUSD's topology-aware utilities, so non-convex polygons and holes are handled; per-face material subsets become several Surfaces sharing the mesh's vertex arrays. - The UV primvar comes from the material's own reader node instead of being assumed to be named "st"; texture colour space is honoured. - Unmaterialed prims take display colour and display opacity. - Light intensity accounts for exposure, normalization and colour temperature; shaped sphere and disk lights become spot lights; dome orientation is baked into the light's own direction and up. - Sphere, cone and cylinder stay analytic; capsule, cube and plane become meshes. - Subdivision surfaces are refined with OpenSubdiv, carrying vertex primvars through the same refinement and honouring creases, corners and holes. - Transform animation uses the authored sample times, densified only where a two-key spherical interpolation would collapse the rotation. - Deforming geometry imports one eager frame plus a file-backed binding that re-pulls from the retained Stage, and survives save and reload (ADR 0018). - Stage up-axis and unit scale are recorded on the import root; coordinates are left exactly as authored. Two open questions from the spec are settled here. Refinement applies only to meshes that explicitly author a subdivision scheme: USD's schema default is catmullClark for every mesh, so refining unauthored meshes would silently distort ordinary polygon meshes. And OpenUSD's imaging libraries carry no GL or X11 link dependency, so headless and CI builds are unaffected. The TSD dialect stays a claim-and-prune pre-pass: markers on the raw Stage claim whole subtrees, which are pruned from the resolved scene and routed to the existing EnSight, volume, transfer-function and render-settings handlers, so a carrier prim is never also converted as generic geometry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Correctness: - Cameras rescaled their bindings onto raw USD time codes while transform bindings used the Stage's normalized clock, so a Stage with both animated on different clocks. Both now go through one shared rescale. - Cameras and dome lights read their pose at UsdTimeCode's default rather than the Stage's start of time, reintroducing the very bug this rework set out to remove: values authored only as time samples did not resolve. - The deforming-geometry binding picked its frame off an even grid, undoing the authored spacing it had just stored. It now selects the authored sample the current time falls in. - Prototypes whose internal transforms are animated converted their geometry once per placement. Baking is what an animated Prototype gives up, not sharing: its gprims now convert once and each placement references them. - Transform-stack resets were recorded as an instance parameter nothing reads, so the parent transform still composed. A resetting node now cancels the accumulated ancestor transform, keeping its place in the hierarchy while landing where USD puts it. Completeness: - Subdivision refinement carries face-varying and uniform primvars through the same refinement instead of dropping them, so a textured subdivision mesh keeps its UVs -- and stops reporting a false skip that broke the empty skip-list invariant. Holes are mapped onto the refined faces. - Points, curves, and quadrics take the display-colour fallback that meshes already had, rather than falling back to TSD's default material. - Time-varying visibility and time-varying material values are now reported. Cleanups: one shared clock helper, one shared native-instancing root, one shared material-mode conversion (the Lua binding had grown its own copy), and shared helpers for position baking, width-to-radius conversion, and material binding lookup. Skip-reason counts derive from the enum instead of a parallel list that could silently fall out of date. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The OpenUSD install now ships hdMtlx, so the MaterialX emission mode does the thing the spec asked for: the document is generated from the resolved network by OpenUSD's own conversion rather than rebuilt by hand. The mode is gated on what the OpenUSD build actually has. CMake looks for the hdMtlx header and the MaterialX package and sets TSD_USD_HAS_MATERIALX accordingly; where either is missing the importer keeps reporting the mode as unavailable and emits portable materials, so installs without MaterialX still build. The flag is public for the same reason TSD_USE_USD is: the test guards itself on it. pxrConfig already points MaterialX_DIR at the build OpenUSD was linked against, so no extra hint is needed. One thing the implementation had to learn: MaterialX has no node definition for UsdPreviewSurface, and asking hdMtlx to convert such a terminal anyway produces a document that fails MaterialX's own validation -- a material node with no category. The converter now checks for a node definition first and falls back to the portable mapping, reporting that it did, rather than emitting a document no renderer can consume. Tests cover both paths: an authored MaterialX network passes through as an inline document whose material name selects into it, and a preview-surface network falls back with the fallback named in the Import Report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The importer has had a MaterialX emission mode since the previous commit, but nothing an application could reach set it: every import path in the tree calls import_USD with default options, and the only thing that could pass anything else was the Lua binding. A Stage whose materials are authored as MaterialX therefore imported as untextured defaults -- the OpenPBR terminal shares no input names with UsdPreviewSurface, so the portable mapping had nothing to read and every material fell back to a flat base colour. USD_MATX sits beside USD the way ASSIMP_FLAT sits beside ASSIMP: a second Importer Type naming the same reader with one option set. It is exposed wherever an Importer Type is named -- the -usd_matx command-line flag, both import dialogs, and SciVis Studio's project-file mapping so a project that loaded one reloads as one. Fixing the viewer's import dialog was a prerequisite rather than a bonus. It cast the combo index straight to ImporterType against a hand-maintained list of labels that had fallen one entry short of the enum, so every choice from SWC_SDF on named one importer and ran another -- picking "USD" imported TRK. Adding an entry would have shifted the skew rather than fixed it, so the labels now carry the type they select. The trailing "TSD" label went with it: no Importer Type backed it, it ran VOLUME_ANIMATION, and Scene Archives load through their own path. Two neighbouring cases were missing from SciVis Studio's toString, which is also what importerTypeFromString round-trips through: PBRT and SWC_SDF serialized as "NONE" and could not be reloaded. Tests pin the dispatch rather than only the option it sets, since the dispatch is the part that was missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OpenUSD's conversion writes each texture filename exactly as it was authored -- SdfAssetPath::GetAssetPath(), not the resolved path -- and hands back the texture nodes it wrote so the consumer can finish the job. Every Hydra render delegate does finish it; TSD did not. The document then travelled to the device as inline text, which has no file of its own for a relative path to be relative to, so a filename like "textures/wood.png" named nothing at all. The anchor is the one the rest of this importer already uses for textures: the resolved path where the Stage's resolver produced one, and the Stage's own directory otherwise. The fallback is not a nicety -- a UDIM path names a set of tiles rather than a file, so no resolver ever resolves it, and in the asset this was found with that is 92 of 117 texture inputs. A path that survives anchoring and still names nothing is now reported as TEXTURE_LOAD_FAILED against the material that asked for it. The device that opens the document later cannot say which prim wanted the file, and silent loss is the failure mode this whole area keeps producing. This is necessary but not yet sufficient: the paths that reach the device are now correct and absolute, verified against the asset, but the device's MDL resource resolution does not accept absolute host paths without a matching mdlResourceSearchPaths root, and has no UDIM expansion. Those are device-side and unaddressed here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correct paths in the document turned out not to be enough, because the device does not open the files at all. Its MaterialX material publishes every `filename` input under the input's document path and reads texels from an ANARI sampler bound to that name -- the device's own test says so plainly: "The file input carries no resolvable path; the bound sampler supplies the texels." Nothing bound one, so a MaterialX import produced no samplers at all while a preview-surface import of the same Stage produced them normally. The import now loads each texture through the same importTexture and cache the preview-surface path uses, so a texture shared between materials is read once, and binds the result under the input's MaterialX element path. The colour space comes from the document rather than being assumed: MaterialX names the encoding per input, and only an sRGB encoding is de-gamma'd. The absolute paths stay. They cost nothing, they are what any consumer that does resolve files would need, and for a tile set -- where no sampler can be bound -- they are all the document carries. Two limits are now reported rather than silent, both of which this asset hits: a tiled (UDIM) path names a set rather than a file and nothing in the stack expands one, and TIFF is not a format the image loader decodes. Both arrive as TEXTURE_LOAD_FAILED naming the file and the material that wanted it. Verified against the asset this started with: the device accepts every binding (no "sampler not bound" warnings), and of its 117 texture inputs 21 now bind, 89 are reported as tiled, and 7 as TIFF. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stb has no TIFF decoder, so `importTexture` fell through to it for `.tif`/`.tiff` and every such texture failed to load. On the OpenPBR Shader Playground asset that cost 7 of 117 MaterialX texture inputs (the `walls`, `iceCube` and `iceCubesInner` materials). Rather than take on a format-specific decoder, dispatch to OpenImageIO, which is already installed alongside the OpenUSD builds this importer targets and covers every other format it has a reader for. It is gated behind `TSD_USE_OIIO` (default OFF) like the other optional dependencies; with it off, `.tif` now reports that TSD was built without a decoder instead of surfacing an opaque stb decode failure. OIIO hands back raw normalized values where stb applies the sRGB transfer function via `stbi_ldr_to_hdr_gamma()`, so the decode is applied here, alpha untouched, to keep all texture paths on one contract. `TSD_USE_OIIO` is a PUBLIC definition so the test can assert the decoded path only where a decoder exists -- the same widening `TSD_USE_USD` already makes. Measured on the reference asset, with `TSD_USE_OIIO=ON`: skipped prims went from 96 texture-load failures to 89, and every one that remains is a `<UDIM>` tile set, which is a separate gap. Zero `sampler not bound` warnings from the device. Full suite 30/30.
anari_cpp maps only `const char *` onto ANARI_STRING, so
`ANARITypeFor<std::string>::value` is ANARI_UNKNOWN and every generic
Any template silently missed strings: `is<std::string>()` reduced to
`is(ANARI_UNKNOWN)` and was false for every value ever held, and
`Any a = std::string("hello")` did not compile at all -- despite being
the example in Any.hpp's own header comment.
The visible symptom was Lua: `ref:getParameter()` returned nil for
every ANARI_STRING parameter, because ParameterHelpers reaches for
`val.is<std::string>()`. That made scripted inspection of string
parameters -- `source` and `materialName` on imported USD materials
among them -- impossible.
Fix it at the root, in Any, so the two RenderBindings call sites with
the same shape are covered too. The specializations are local to Any
rather than a global ANARI_TYPEFOR_SPECIALIZATION, which would leak the
mapping into anari_cpp calls that would then memcpy a std::string into
device storage. `get<std::string>()` routes through `getString()`,
since `getAs<>()` static_asserts on ANARI_STRING for exactly that
reason.
Verified against a freshly built `tsdLua`: the new
`ref:setParameter with string` case fails on the pre-fix binary and
passes after. Unit coverage added at the Any level. Full suite 30/30.
89 of the OpenPBR Shader Playground asset's texture inputs are `<UDIM>` tile sets, and after TIFF decoding landed they are the entirety of the importer's remaining texture failures. The obvious next move -- lean on MDL's native `<UDIM>` support and let the device resolve the tiles -- does not work, and the reason is not visible from TSD, so record it. VisRTX does load MDL texture resources from disk (`SamplerRegistry::loadFromImage` stb-loads `textureDesc.url`, which corrects the "the device does not open texture files" reading -- that holds only for the sampler-bound MaterialX path). But `libmdl::Core::resolveResource` returns `get_element(0)->get_filename(0)`, one filename, while MDL's entity resolver returns one element per tile; all but the first are discarded, and `loadFromImage` then builds exactly one `Image2D`. Nothing under `devices/` carries a UDIM concept at all. So `mdlResourceSearchPaths` -- the seam that would clear the cosmetic `Failed to resolve texture resource` log noise -- would not change the outcome, and TSD-side tile expansion has nowhere to send the texels either: an ANARI sampler is a single image. Binding tile 1001 and dropping the rest was considered and rejected; it turns a reported gap into a silently wrong render for any mesh spanning more than one tile. The gap stays reported, with the anchored absolute path still written into the document so a consumer that gains UDIM support finds it well-formed. Revisit when ANARI grows a tiled or array sampler, or when the MDL runtime grows tile-indexed lookup.
Review of fca25dd turned up three ways the OpenImageIO decode diverged from the stb paths it claimed to match, each of which would have shown up only on a TIFF with alpha or a float TIFF -- shapes the 1x1 RGB8 fixture never exercised. Alpha was being gamma-corrected on 2-channel images. stb's rule is `if (comp & 1) n = comp; else n = comp-1` (stb_image.h:1573), so an even channel count ends in alpha and an odd one is all colour; `min(n, 3)` agrees for 1, 3 and 4 channels but not for grey+alpha. The comment claiming "alpha stays untouched, matching stb" was therefore false for exactly the case it was written about. OpenImageIO also premultiplies unassociated alpha into the colour channels by default, which stb never does; the new grey+alpha fixture caught it. Ask for the file's own values with `oiio:UnassociatedAlpha`. Third, float and half TIFFs already carry linear values, and stb only ever gamma-decodes integer input, so the curve is now applied only when the file's format is integral. Also from review: rename `srgbToLinearInPlace` to `applyGamma22InPlace`, since it applies pow(x, 2.2) rather than the true sRGB EOTF -- matching stb is right, the old name overclaimed; extract the channel-count to ANARI-type cascade, which was duplicated verbatim between the stb and OIIO decoders; drop three redundant `image->close()` calls, as the unique_ptr's deleter closes on every exit path; reorder the Any specializations so `is<std::string>()` precedes `get<std::string>()`, which removes an ordering hazard and the comment explaining it; record in io/CMakeLists.txt why OpenUSD's Hio was rejected in favour of depending on OpenImageIO directly; and list `TSD_USE_OIIO` in AGENTS.md alongside the other optional flags. Adds docs/usd-materialx-known-gaps.md for the MaterialX import observations that are understood but undiagnosed -- the single 'Surface' nodedef transcode failure, the geometry_opacity version skew, and the cosmetic MDL resolve noise -- so they survive the handoff. Full suite 30/30, 74 Lua cases pass, and the reference asset is unchanged: 89 skips, all UDIM, zero `sampler not bound`.
…oise Setting the parameter was tried against the OpenPBR Shader Playground asset and is blocked from both ends. Anchoring on the texture directories the importer already knows is a measured no-op -- MDL resolves a leading `/` root-relative, so the only root that can match a fully absolute host path is `/` itself. Registering `/` does silence all 109 failures, but it also resolves the 85 UDIM paths, and libmdl hands back tile 1001, which the device then binds: the render changes while the import report still claims the tile sets were skipped. That is the outcome ADR 0019 rejects. No code change -- the gap stays open, now with the routes out of it named and the dead one ruled out by measurement rather than left to be retried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One material out of 55 on the reference asset failed shader generation inside the device with `Could not find a nodedef for node 'Surface'`, then fell back to the default material with nothing in the Import Report to say so. `Surface` was a red herring: hdMtlx names the surface node after the shader prim, so every emitted document has one. The material is `/World/Looks/OJfoam`, and the defect is in the asset -- it connects `geometry_opacity`, declared `float` on `open_pbr_surface`, to a `color3` colorcorrect node. MaterialX matches a node to its definition on category, type and the exact set of inputs, so one mistyped port leaves the surface node resolving to no nodedef at all. MaterialX 1.39.6 rejects the source `.mtlx` standalone, so nothing between the Stage and codegen introduced it. This also reclassifies the `geometry_opacity` validation warning that was recorded as benign OpenPBR version skew. It is neither: it is MaterialX reporting this exact port, on the materials that author it rather than on all of them. TSD cannot fix the asset, so `documentResolves` checks every node against the standard libraries before emission and reports a failure as MATERIAL_RESOLUTION_FAILED naming the offending port, falling back to the portable mapping. The check sits after the texture pass and before sampler creation, which is load-bearing: checking first drops the material's tile-set reports, since the fallback reads the network by UsdPreviewSurface names and reports none of them -- ADR 0019's failure mode by the back door. Also adds `TSD_USD_MATERIALX_DUMP_DIR`, which writes each generated document out named after its material prim. That is what isolated this, and it is the tool to reach for when a device rejects a document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A Mesh with GeomSubset children converted to one Surface per subset, and each subset borrowed the parent's attributes through a hard-coded list of vertex.normal, vertex.attribute0, and vertex.color. Face-varying data is indexed by 3*triangle+corner against the mesh's whole triangulation, so a subset -- drawing only some of those triangles -- cannot borrow it. USD assets overwhelmingly author texture coordinates as faceVarying, so the subsets that actually render carried no UVs at all, and samplers read whatever attribute0 defaults to. Uniform data was lost the same way. Attributes are now expanded onto the triangulation once per mesh and gathered per Surface for the triangles that Surface draws. Vertex data still shares one Array across every Surface, since for it the gather is the identity. Two consequences of binding per Surface, both intended: - A subset resolves its own material's UV primvar name instead of taking whatever the mesh-level binding chose, and falls back to the mesh's answer and then to "st". - Faces no subset claims become a Surface under the mesh's own material. They were previously built into a parent geometry that was never surfaced: invisible, and dead objects in the scene. ADR 0020 records the design, including the pre-existing coarse-versus- refined face mismatch that subsets on subdivision meshes still have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HdMeshUtil::ComputeTriangulatedFaceVaryingPrimvar reports Unchanged, not Success, when every face is already a triangle, leaving the output value untouched. Treating that as failure silently dropped every face-varying primvar (UVs, normals) on pre-triangulated exports. Unchanged means the flattened input is already one value per triangle corner, so bind it directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TSD already stores every non-owning scene back-reference as a raw pointer --
Layer, AnariHandleCache, AnyObjectUsePtr, and the network messages all use
`Scene *m_scene{nullptr}` -- but the convention was only implicit in the code.
Write it down, with the lifetime rationale that motivates it: a reference
member deletes assignment and forces construction-time binding, which conflicts
with the movable-not-copyable lifetime declared everywhere else.
Scopes the rule to stored members so it does not contradict the File I/O
section, whose importer and exporter signatures take `Scene &` deliberately.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Seven decode paths in tsd_io each carry a private assumption about which row of a decoded image is row 0. They cancel for glTF, ASSIMP, and PBRT and do not cancel for OBJ and USD, whose textures render mirrored. Record the survey, the ANARI-orientation contract, the ImageCache API, the importer changes that must land with the flip, and the sequencing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Assert the two halves of a correct texture fetch separately: that a decoded array's row 0 is the picture's bottom row, and that the mesh corner at the top of the picture addresses the picture's top row. The suite is red on purpose. It confirms the survey in docs/tsd-io-image-import.md from local evidence rather than from source reading: glTF and PBRT pass the end-to-end assertion, OBJ and USD fail it, and the storage assertion fails everywhere because every decode path stores top-down today. The following commits make it green. Fixtures are synthesized into the temp directory, following the TGA in tests/test_UsdImport.cpp and the TIFF in tests/test_Importers.cpp. One 1x2 TGA serves every importer; its rows are stored bottom-up, so the storage assertion cannot pass by a decoder's byte order happening to agree with the contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Seven decode paths each did their own keying, Array creation, sampler construction, and orientation assumption. Move all of them into src/tsd/io/images: decoders report the row order their library produced, ImageCache normalizes and stores, and makeImageSampler builds the sampler. One cache key -- (id, colorSpace) -- replaces the three schemes in the tree, and the cache holds the Scene it caches for so a cached ArrayRef cannot reach a different one. No behavior change: SCENE_ROW_ORDER is TOP_DOWN, which is what all seven paths produced. The characterization suite fails in exactly the same three places as before this commit. Folded in while here, because they blocked the type change: - glTF's private decode path becomes acquireDecoded, keeping its native element types and per-texture sampler settings. Its dead flipNormalMapY parameter, which only ever reached the cache key, is gone from all 18 call sites. - PBRT's height-to-normal map stores through acquireDecoded. - ASSIMP's embedded textures key off the texture id directly, so makeTextureCacheKey has no callers left. ImageSource drops the displayName the proposal sketched: makeImageSampler already takes the sampler's name, and ImageSource identifies content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Row 0 of a decoded array is now the picture's bottom row, so texture coordinate (0, 0) addresses the lower-left corner, and importers hand ANARI texture coordinates in ANARI's own convention. This is the commit that fixes OBJ and USD textures rendering vertically mirrored. Every part of it has to land together, because flipping the arrays without the uv changes would just move the bug: - ImageCache stores BOTTOM_UP. - glTF flips v on vertex.attributeN, and on the raw accessor data it re-reads for tangent generation. - ASSIMP drops aiProcess_FlipUVs; its own output is already v-up. - PBRT drops `v = 1 - v` on trianglemesh and plymesh, and its uv transform loses the compensating `(1 - vs - vd)` term. - OBJ and USD change nothing and become correct. - SceneToUSD reverses rows writing PNG and EXR, both top-down formats. - calcTangentsForTriangleMesh defaults flipTexCoordY to false. Block-compressed DDS is the exception: 4x4 blocks cannot be row-reversed without decoding and re-encoding, so its texels stay as authored and makeImageSampler composes a v-flip into the sampler's inTransform and inOffset. That is why makeImageSampler now owns those two parameters outright and takes the importer's uv transform through SamplerSettings -- ASSIMP's 14 sites, USD's uvTransform, and PBRT's uscale/vscale all set them afterwards before, which would have silently dropped the flip. PBRT's height-derived normal map negates its v gradient, since the row axis it was taken over is now reversed relative to the fetching v. The characterization suite is green. It also gained the two cases this commit created the need for: the block-compressed flip, including that it composes onto a caller's transform rather than replacing it, and ASSIMP, which had no coverage at all and whose uv handling changed here. TSD_USE_ASSIMP becomes PUBLIC so the test target can see it, matching TSD_USE_OIIO and TSD_USE_USD. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HDRImage was the last decoder creating Scene arrays on its own, and the one place in the tree whose comment named an orientation -- the opposite of what every other path then produced. It keeps its decode, because it handles multipart EXR and forces three channels and the shared texture path does neither, but it now declares the row order it emits and its callers store through ImageCache::acquireDecoded. That gets HDRIs the same keying and Scene-scoped lifetime as any other image, and caching they had none of before. The USD dome light bakes its radiometry scale into the texels, so it keys on the colour as well as the file. The new test covers the .hdr path end to end. PBRT's equal-area-to- equirectangular conversion still consumes HDRImage's raw buffer directly rather than a cached Image; it is correct as it stands, and rewriting its frame of reference has no test to hold it, so it is left alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The one real defect: ASSIMP set `hasUvTransform` unconditionally, so every ASSIMP texture got an explicit identity inTransform/inOffset even where assimp authored no transform -- contradicting the flag's own documented contract. Standards: - `Image::operator bool` and `DecodedImage::operator bool` move out of the struct bodies into Inlined definitions sections (parent SS5). - `ImageCache` states its copy/move intent with the TypeMacros (SS6), and drops the private `lookup` returning `Image *` so one kind of lookup has one idiom (SS11 Fallible Returns). - `SamplerSettings`'s `hasUvTransform` bool and the value it guarded become one `std::optional<UvTransform>` (SS7). - clang-format had hoisted `<vector>` above the project headers in import_ASSIMP.cpp; restored. - `src/tsd/io/CONTEXT.md` gains the vocabulary this work introduced -- Image, Image Source, Image Cache, Row Order, Color Space. Correctness and clarity: - glTF image ids are scoped to their file, as the proposal specified. Harmless while each importer owns its cache; a landmine the moment one is shared. - `import_HDRI` checks the acquired Image before dereferencing it. - `gltfTexelType` drops a parameter it never read. - ADR 0014 said glTF and PBRT both flip v. PBRT stopped flipping in c4106e7; corrected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The proposal doc read entirely in the future tense, which is misleading now that most of it is in the tree. Add a status banner and a Status table, mark the Survey as describing the tree at 017f7d9, and note at each point where the implementation diverged from the plan: HDRImage kept its own flip, and the DDS recommendation was ratified but shaped as SamplerSettings rather than a helper callers must remember to call. Add a Remaining work section covering the six items in rough payoff order -- one cache per import rather than per importer, retiring the shims, PBRT's infinite light, HDRImage's flip, native element types, and the EXR/TIFF/equal-area test gaps -- each with what it needs and why it did not land here. Also record that ASSIMP binds no textures for OBJ. Its GL-style material branch reads no texture slot at all, so an OBJ with map_Kd imports untextured through ASSIMP and correctly through import_OBJ. Unrelated to this work, found by writing its tests, and it will mislead someone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Width-less BasisCurves and Points prims (common in Blender hair exports) previously left the geometry radius unset, inheriting ANARI's default of 1 world unit — hair strands imported as giant overlapping tubes. Fall back to an explicit radius of 1e-3 of the prim's own bounding-box diagonal so strands stay hair-like at any scene scale; authored widths convert exactly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jeffamstutz
force-pushed
the
usd-import-rework
branch
from
August 14, 2026 16:33
220a36a to
86a5501
Compare
jeffamstutz
marked this pull request as draft
August 14, 2026 16:35
tarcila
reviewed
Aug 14, 2026
tarcila
left a comment
tarcila
left a comment
Collaborator
There was a problem hiding this comment.
A few nit-pick comments and some questions WRT image orientation.
Side note that I was not able to build the branch with USD 25.05, it seems there is some OpenUSD 26.05 specifics.
The three texture shims took a `Scene &` beside the `ImageCache &` the image came from, so a caller could name a Scene the image had never reached -- the very thing an ImageCache holding its own Scene exists to prevent. They now take the cache alone and read `ImageCache::scene()`, which nothing had called until now. `makeImageSampler` gains an overload taking the cache for the same reason, so the two callers that acquire an image themselves -- glTF's `importGLTFTexture` and PBRT's `importHeightAsNormalMap` -- cannot name a second Scene either. With no Scene needed to build a Sampler, it fell out of four importer-local helpers that were only passing it along: ASSIMP's `importEmbeddedTexture` and PBRT's `bakeTexture`, `bakeTextureSlot`, and `resolveTexture`. Every ImageCache in the tree is constructed from the Scene that used to be passed beside it, so no call site changes behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Remaining work" item 2 of the image-import note called for deleting `importTexture` and its two siblings as pure forwarding. Now that they take the ImageCache alone, they are what keeps the cache's Scene the only Scene a caller can put a Sampler in, and they still carry the path normalisation and colour-space choice their call sites share. Record the decision and its reason in place of a plan the code has gone against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
import_HDRI decodes exactly one image per call, so the function-local ImageCache it routed radiance through could never take a hit: it keyed a single entry, copied the texels a second time into a DecodedImage, and was destroyed at scope exit. Its acquireDecoded row-order normalization was a no-op too, because HDRImage already emits the BOTTOM_UP order the ImageSource asked for. The array now comes straight from the Scene, as import_PBRT's infinite light has always built its own. Behaviour is unchanged: store() ended in the same createArray + setData, and the error path this drops was unreachable -- store() only fails on a null Scene or an empty decode, and HDRImage::import already rejects a zero-sized image. The comment left in its place records why this path skips the cache, and that ADR 0014 still describes an hdri light's radiance as reaching the light through ImageCache -- true of the UsdLights dome-light path, which keys on file plus radiometry and genuinely dedups, but no longer of this one. Whether that ADR and the "owner of every decoded Image" wording in io/CONTEXT.md should be amended is left for a follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The DDS scenario asserted the sampler's subtype and its uv transform, but nothing checked the two parameters that tell the device how to read a block stream. The Array behind a compressed image is a flat run of bytes, so `format` and `size` are the only description of the picture the sampler carries, and a wrong or missing one renders garbage without failing a test. Both values are what the path already produced; this only records them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ImageCache::store() wrote the block format and the picture's dimensions as stringly-keyed metadata on the texel Array, and makeImageSampler read them back untyped twenty lines later -- a side channel between two functions in one file, when Image was already the thing being passed between them. Both values now live on Image, where the ArrayRef and vFlipInSampler already did. blockCompressed collapses into the format being non-empty: the DDS decoder is the only producer of compressed texels and it yields no image at all for a format it does not recognize, so the two could never disagree. store() derives it the same way makeImageSampler does rather than copying the decoder's bool, which keeps the one Image it builds self-consistent. Nothing else read either metadata key -- serialization walks metadata generically, and the device's BC table in mdl/SamplerRegistry.cpp maps from MDL image formats rather than from these -- so the writes go away with the reads, and with them the file-scope ANARI_TYPEFOR_SPECIALIZATION that existed only to move the dimensions through an Any. The sampler's "size" parameter is now set untyped, which is the one place that needed the mapping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three readers of the UsdPreviewSurface network each open with the same TfToken-or-string unwrapping of a shader parameter. That access belongs to NetworkWalker, which already owns every other way the network is read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four branches of convertLight repeated the same three decisions -- the subtype's object, the prim's name, and where the radiometry's colour and brightness land. Only the brightness parameter actually differs, so it is the one thing the caller still passes. The dome light stays hand-built: it has no `color` parameter, and contorting the helper around that would cost more than the branch does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Traversal::visit() wrote every prim on the Stage into InstancerRegistry::nodeForPrimPath, a Stage-sized map serving a lookup that attachNativeInstances() makes only for its own placement paths -- and that Stages without native instancing never make at all. InstancerRegistry now discovers those placement paths up front, from the instancers under the propagated-prototypes root, and recordNode() keeps a node only when a placement will be attached to it. The map and the lookup are private to the registry, so the general traversal no longer reaches into the instancing feature's data structure directly.
This branch's additions landed between existing groups: UsdImport.cpp and the images/ sources between animation/ and exporters/, and the three animation/Usd*FileBinding.cpp inside the importers/detail/ block. Put each back with its own directory, top-level source first, as the rest of tsd's source lists are written. No source is added or removed.
The registry's placement-path discovery and attachNativeInstances() each walked the propagated-prototypes subtree and filtered instancer prims out of it themselves. The two have to agree on that set for a placement to land on its own node, so give them one nativeInstancerPaths() to share and let attachNativeInstances() keep only the filtering that is its own.
Splitting the USD import suite moved this baseline below the ImportedStage construction that runs the import, so the count was sampled after the fact and the REQUIRE compared it to itself. Take it from a fresh Scene, the way the sibling camera case already does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The split left the eight suite files and UsdTestFixtures.h unformatted, on the reasoning that formatting would bury the move. The move has landed, so run clang-format over them. The two hand-laid-out byte tables -- the TGA fixture and the EnSight coordinates -- are fenced off, since one value per line reads worse than the grid they describe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nanovdb::io::readGrid does not bound its own reads. On a file too short to hold a header it retries against a stream already at EOF and never returns -- an empty file, four bytes of text, and a truncated OpenVDB grid all hang it indefinitely, with no timeout to escape through. A file long enough to read a header out of throws cleanly, which is why this went unnoticed: a real OpenVDB grid is fine, an interrupted download of one is not. Read the 8-byte magic first and reject anything that is not one of the three NanoVDB numbers. Teaching import_spatial_field '.vdb' put this behind the viewer's drag-drop, so the hang is now reachable by dropping a stray file rather than only by naming one on the command line. An OpenVDB grid says so specifically. '.vdb' means NanoVDB on this path, which the extension gives no hint of, so the error names the converter that turns one into the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Silo scenario stood alone for a change that also put '.hdf5' within reach of a UsdVol field for the first time. Add its counterpart, and assert on both that the dispatcher's own "no loader for file type" fallback stayed quiet, so neither can pass on a file that reached no importer at all. Both now write the file they name. It only has to exist: a filePath that resolves to nothing stays the bare name the Stage authored, and import_FLASH reads no such name, so the scenario would have measured that rather than which importer the extension chose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fileOf() scanned for the last path separator and answered "" when there was none, so a bare 'volume.raw' -- the form anyone types for a file in the working directory -- reported no file at all. Six importers guard on that result before doing any work, which made them silent no-ops; the thirty-odd call sites that only wanted a display name got an empty one, including every Sampler an importer names by a scoped id rather than a path. Hand both halves to std::filesystem::path, which knows a path may carry no directory and, on Windows, that '/' separates as well as '\'. The one-character scan missed that too. pathOf() keeps its trailing separator, since callers concatenate a sibling file onto it, and keeps answering "" when there is no directory -- that answer was always right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six importers opened with `if (fileOf(path).empty()) return;`, which only ever fired for the bare filename fileOf() used to misread. With that fixed it can only mean a path that is empty or names a directory, and the work below it already copes: RAW reads no dimensions out of no name, FLASH and NVDB fail to open, and the display name is simply empty. import_XYZDP and import_E57XYZ needed a real check first. Both fread() straight off the fopen() result without testing it, so the filename guard was the only thing between them and a null FILE* -- and only by accident, since it never looked at whether the file existed. Check what they actually depend on. import_AGX keeps its guard: it is the one that says something. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
import_ENSIGHT picks a part's material as per-part binding > carrier binding > scalar colormap > default, and converting carrier bindings turned on two rungs that had never run. Which one wins over the colormap was left to whoever reads the if-chain. Cover it both ways over one dataset: with no carrier binding the part takes the colormap built from the field, and with one it takes the bound material instead. The dataset writer grows a scalar variable to have something for a colormap to be built from, and its two record writers move out to file scope so the variable file can use them too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pathOf() rebuilt its answer from parent_path() and appended the platform's own separator, so on Windows '/a/b/volume.raw' came back as '/a/b\' -- Windows accepts a forward slash, and the two halves no longer rejoined into the path that was passed in. Callers concatenate them to reach a sibling file, so that is the property that matters. Take the prefix fileOf() left behind instead. The separator is then whichever one the path was written with, on either platform, and the split is reversible by construction. Assert that directly rather than only through expected strings, since the expected strings were what quietly passed on Linux. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jeffamstutz
force-pushed
the
usd-import-rework
branch
from
August 19, 2026 15:16
2a316db to
3e49027
Compare
…cene makeImageSampler had two forms: the original taking a Scene, and an overload taking the ImageCache that produced the image, added so the texture shims could not pair an image with a Scene it never reached. Adding the second one left the first exported from ImageCache.hpp with exactly one caller in the tree -- the overload delegating to it -- so tsd_io still published an entry point that takes an arbitrary Scene, and there were two ways to build a Sampler where the point was to have one. Fold the body into the ImageCache form and delete the Scene one. Every call site already passed a cache; none of them change. The null-Scene answer moves into the merged function and stays what it was: a cache with no Scene holds no valid image either, so there is nothing to build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
convertLight took an `ImageCache &` and opened by discarding it. Its one image-shaped job, an infinite light's radiance, goes through loadInfiniteRadiance(), which converts equal-area to equirectangular and so binds an array that is not the decoded image and could not be keyed as one. The parameter was never going to be read. This is the same vestigial `Scene &`/`ImageCache &` weight the texture shims shed; it survived because the parameter was silenced rather than removed. convertMaterial and applyShapeAlpha keep both -- they create Materials, so the Scene is doing work -- and both callers still take their pair from the single `ImageCache texCache(&scene)` above them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`textureCache` is an ImageCache built from the `scene` member, so the aggregate carried the Scene pointer twice. Nothing copies an ImportContext today -- one is made in import_USD() and passed by reference everywhere below -- but a copy followed by a reassignment of `scene` is all it would take for the cache to keep naming the old Scene, and every Sampler the import made would land there while its Materials went elsewhere. Delete the copy operations so that cannot be written. That costs the struct its aggregate status, so the initialization at import_USD.cpp:315 now runs through a constructor taking the same eight members in the same order -- the call site is unchanged, braces and all. `importAnimationIndex` loses the comment explaining why it could not be private; it still isn't, because nothing else about how this struct is used has changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The texture shims' note counted "nine call sites"; there are ten in `tsd/src` (OBJ 1, ASSIMP 3, PBRT 3, UsdMaterials 3) plus six in the tests. The nine came from a task document that listed only two of UsdMaterials' three. Since nothing in the sentence needs a number, drop it -- a count is a fact that goes stale on the next call site, and the point of writing the note down was that a wrong number is debt with a label on it. The same section said makeImageSampler "gained an overload taking the cache"; there is no overload now, only the cache form. The function-local ImageCache inventory in Remaining work had drifted: three of its five line numbers were off, and import_HDRI no longer builds a cache at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ADR 0014 named an hdri light's radiance as an image that asks ImageCache for the order its consumer wants, and io/CONTEXT.md called the cache "the owner of every decoded Image for one scene". Since 0caf746 that is not what the tree does: import_HDRI builds its radiance array directly, because it decodes exactly one image per call and a cache scoped to the call can never be hit. Narrow both to the images the cache owns, and say plainly that a light-bound radiance array may be built without it. The contract itself does not change -- a sampled image is still stored top-down -- and the one path that still routes radiance through the cache, UsdLights, still asks for bottom-up on its ImageSource, which is now named as the example. The ADR's closing sentence said no decoder emits bottom-up rows, which read as false: HDRImage does. It is not one of io/images/detail's decoders, which is what the paragraph was about, so the ambiguity is resolved rather than the sentence deleted -- and the conclusion it supports still holds. No row reversal runs in the tree today, because HDRImage's one cached consumer asks for the order it already produced. The in-code note in import_HDRI said PBRT's infinite light binds its radiance directly "for the same reason". It does not: it resamples equal-area to equirectangular, so what it binds is not the decoded image and could not be keyed as one. That is a different reason and worth being the one written down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The light converter decides per branch which ANARI parameter carries a light's brightness -- `irradiance` for a distant light, `intensity` for rect and sphere, `scale` for a dome -- and since 904e3dd three of those names are arguments passed to a shared helper rather than literals at the site that means them. A wrong name still produces a light of the right subtype, so nothing that existed caught it: coverage was point and spot only. Three scenarios, one per untested branch, each asserting the name as well as the value: - distant: `irradiance` carries it and `intensity` is unset, and `normalize` leaves the intensity alone, because a distant light subtends no area to divide by. - rect: the quad's corner and two edge vectors describe the authored width and height, and `normalize` divides by their product. - dome: brightness rides on `scale`, `color` stays unset, and the colour arrives baked into a synthesized 1x1 radiance -- which is why this branch cannot go through the shared helper, and the assertion that says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A single over-long argument list, unformatted since 0ccece9. No code change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Root STYLEGUIDE §6 asks every type to say what copying it means. InstancerRegistry said nothing, so it was copyable by default -- harmless today, since it holds only two containers and no pointer into the scene index its constructor reads, but "harmless" was left for the reader to work out. It is built once per import and passed by reference, so the answer is that copying it is not intended: not copyable, movable by default. Co-Authored-By: Claude Opus 5 <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
2 participants
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.
No description provided.