Usd import rework - #358
Draft
jeffamstutz wants to merge 27 commits into
Draft
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.
|
|
||
| A decoded image resident in a TSD Scene is stored in ANARI orientation: the | ||
| array's row 0 is the bottom row of the picture, so texture coordinate `(0, 0)` | ||
| addresses the lower-left corner. Importers hand ANARI texture coordinates in |
Collaborator
There was a problem hiding this comment.
As of today, I think the opposite is mostly true in TSD. Images are loaded so that the first scanline is the top row of the image (that's how stb_image returns its content by default). The exception to that is HDRI where we do need to flip them (stbi_set_flip_vertically_on_load(true)).
In the former sampler case, image orientation is not specified by ANARI, aligning on the broad Vulkan's guidance: sampling origin points at the begining of the first scanline.
The latter HDRI case is kind of a de-facto standard where all device support HDRIs adhere, even if under specified by the ANARI specification (AFAIK).
Framebuffers in ANARI are bottom-up (OpenGL convention), but does not imply anything on images samplers.
So, overall, ANARI is not forcing any image orientation. TSD is trying to, on import. Not sure what led Claude to this bottom-up assumption.
Collaborator
Author
There was a problem hiding this comment.
ANARI explicitly cites Vulkan samplers for behavior, so it is definitely top-left. I had this reversed with which is GL vs. Vulkan. I can update to this convention.
Collaborator
There was a problem hiding this comment.
Just to confirm, my understanding is that:
- Framebuffers do have an orientation. They need so they can ultimately be blit on-screen. ANARI follows OpenGL convention on that part.
- Images and textures generally don't. ANARI follows Vulkan here. The only constraint imposed by Vulkan is that (0, 0) maps to the first texel of the first scanline. Whether this is the top or the bottom of the image only depends on how UVs are wired.
Side note, deferring to Vulkan in the case of ANARI specification makes it really hard to actually dig out meaningful information.
| **Block-compressed DDS is the one exception.** BC blocks are 4×4, so a | ||
| vertical flip requires decode and re-encode, which defeats the point of | ||
| `compressedImage2D`. Recommendation: keep DDS texels as authored and mark the | ||
| `Image` so `makeImageSampler` folds a `v`-flip into that sampler's |
Collaborator
There was a problem hiding this comment.
DDS is defined as top-down, and flipping it would need to re-encode the file, in the most complex compressed case (BC6 and BC7 IIRC).
So, yes, UV is the decisive tool here. But that again is to be put in perspective with the importer actually pulling that image in.
| @@ -0,0 +1,44 @@ | |||
| # Report UDIM tile sets as unsupported rather than approximate them | |||
|
|
|||
| A MaterialX `filename` input whose path contains a `<UDIM>` marker names a set | |||
Collaborator
There was a problem hiding this comment.
Nit-picking: Not sure how this statement above impacts the implementation, UDIM is not MaterialX only, but about textures no matter the material using them.
Collaborator
Author
There was a problem hiding this comment.
This comes from MaterialX content forcing this to be a thing with certain USD scenes. We encounter these as they come, so it's a matter of trying to work on concrete scenes with tiled textures that we address them. Whenever that occurs, the ADR will be updated accordingly.
| > format's convention where they differ. | ||
|
|
||
| Decoders declare the row order their library produces; the import layer | ||
| normalizes. No importer flips anything itself. |
Collaborator
There was a problem hiding this comment.
And yet, the importer is the one knowing the image orientation (usually top-down through stb_image) and how this aligns with matching UVs.
Thinking of the importer story a bit more, even if today's code is most likely a bit brittle WRT that topic, this proposal feels like over-engineered, and the ANARI assumption being bottom-up is incorrect.
Doing a quick survey, it seems that:
- GLTF explicitely state that image are expected to be top-down (https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#_wrapping) with the first scanline being the top of the image, and UV will be such that top-left is (0, 0).
- Same with the Obj file format
- USD actual goes the other way, assuming bottom-up (https://openusd.org/release/spec_usdpreviewsurface.html#texture-reader), so USD loading a image would need either UV being V flipped, or the image coming from stb_image to be flipped on load (the latter having an effect on sampler caching if the orientation is not part of the caching deambiguation)
(I have no idea about OIIO behaviour in that regard)
Collaborator
Author
There was a problem hiding this comment.
This will be addressed with the corrected ANARI orientation.
| // What a decoder produced, before it is normalized and handed to a Scene. | ||
| // Decoders fill this in and declare the row order they wrote; nothing outside | ||
| // this file decides orientation for them. | ||
| struct DecodedImage |
Collaborator
There was a problem hiding this comment.
Nit-pick: feels to me like the name DecodedImage is overloaded here. Image would be enough.
ANARI addresses texture coordinate (0, 0) at an image's upper-left corner. The previous commit on this branch read it as the lower-left and stored every array bottom-up, so this reverses the contract and every uv conversion that followed from it. Row 0 of a decoded array is the picture's top row again, which is what stb, tinyexr, OIIO, and tinygltf all hand back, so nothing is reordered on the texture path at all now. The direction each importer converts reverses with it: - ImageCache stores TOP_DOWN, and the order is asked for per ImageSource rather than fixed for the whole tree. - glTF stops flipping v on vertex.attributeN and on the raw accessor data it re-reads for tangents; it is already v-down. - OBJ, USD, and PBRT flip v; ASSIMP asks for aiProcess_FlipUVs again. - SceneToUSD writes arrays straight to PNG and EXR again. - calcTangentsForTriangleMesh defaults flipTexCoordY back to true. - PBRT's height-derived normal map takes its v gradient negated again. A format that carries a uv transform of its own has it conjugated by the vertex flip rather than flipped twice: vs*v + (1 - vs - vd). That restores PBRT's term and adds the equivalent to USD's UsdTransform2d and ASSIMP's aiUVTransform, neither of which had one. ASSIMP's also packed its translation into the w row of the matrix, where ANARI never reads it, so that translation was silently dropped; it moves to inOffset. USD flips v on the primvar a material reads as its texture coordinates rather than on every float2 primvar, so a primvar in a spare attribute slot is still bound as authored. A Surface reading a shared vertex primvar as uv gets its own reversed copy. An hdri light's radiance is mapped over the sphere by the light rather than addressed by an image sampler, so import_HDRI and the USD dome light ask the cache for BOTTOM_UP -- the order that path has always been given -- and are unchanged. Block-compressed DDS decodes top-down, which is what a sampler wants, so its v-flip no longer fires. The mechanism stays, keyed on the cache having failed to reorder the texels rather than on the format, and a test reaches it by asking for the other row order. The characterization suite is green: the end-to-end assertion that a quad's top corner samples the picture's top row holds for all five importers. Studio's array preview gets a free fix, since ImGui draws row 0 at the top. Co-Authored-By: Claude Fable 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.