refactor(vis): catalog-aware render dispatch + Match-aware browse + Material shortcuts#231
Merged
Conversation
…mat-vis #285, mat-vis #298)
Per ADR-0002 Principle 3 ("thin delegation sugar"), pymat shouldn't
re-implement the catalog read — mat-vis-client owns the catalog DB +
format adapters. The previous adapters.py wrapper read scalars from
Vis dataclass fields only (caller-overrides), never from the catalog,
so the rendering path always returned _PBR_DEFAULTS for non-overriden
fields — regardless of what the baker authored. That's why every
gpuopen material rendered as flat grey plastic (mat-vis #285) and why
mat#220's Vis.scalars accessor had no implementation to delegate to.
## What changes
1. **Vis._catalog_scalars()**: thin delegate for
`client._scalars_for(source, material_id)` — returns `{}` when
there's no mat-vis identity. Lazy, no HTTP fetch.
2. **Vis.scalars** (closes mat#220): catalog scalars merged with
explicit caller overrides. `{}` for no-identity Vis. Sparse —
only keys with authored / explicit values, no defaults fallback.
3. **Vis.to_threejs / to_gltf / export_mtlx**: dispatch on
`has_mapping`. With identity → free-function adapter fed by
catalog + overrides + cached textures. Without identity → free-
function adapter fed by `_scalars_with_defaults()` (preserves
the historical all-grey-plastic shape for TOML-only materials).
4. **pymat/vis/adapters.py**: shrinks to pure Material→Vis dispatch.
The actual rendering primitives live in mat-vis-client. Compat
shims `_extract_scalars` / `_extract_textures` / `_rgba_to_hex`
preserved for tests/test_adapters.py.
## Tests that flipped (XPASS-strict → marker removed)
- mat#220: 3 tests in test_vis_bernhard_repros.py (Vis.scalars now
exists and returns catalog values; xfail decorator dropped)
- mat-vis #285: test_gpuopen_metals_have_distinct_scalars
(catalog has authored scalars; pymat now reads them)
- mat-vis #298: test_color_is_hex_string (mat-vis-client HEAD's
to_threejs defaults to color_format='hex'; we get it for free
via the adapter delegate)
## Tests still xfail (orthogonal)
- mat#221 (repr observability) — repr formatting concern, untouched
- mat-vis #311 (materials() returns UUIDs) — needs upstream
materials_named() method (tracked in mat-vis #330)
## Net
+181/-112 lines. The catalog read becomes free; the next consumer
that adds a render format (USD, Blender, etc.) does not touch pymat.
Followups from the dispatch refactor:
1. **Migrate to public VisAsset surface**: ``_catalog_scalars()``
now delegates to ``client.asset(*identity).scalars`` instead of
the underscore-private ``client._scalars_for(...)``. Same data,
stable API contract per ADR-0002. The bernhard ``test_vis_bernhard_repros.py``
FakeClient mocks updated to expose ``.asset()`` returning a
FakeAsset; other test FakeClients (test_consumer_journey,
test_vis, test_error_messages) aren't touched — their tests don't
assert on scalar content, and ``_catalog_scalars()`` falls through
to ``{}`` via try/except when ``.asset`` is missing.
2. **Consolidate render dispatch**: replaced 3× duplicated branching
(``if has_mapping: catalog+overrides else: defaults``) inside
``to_threejs`` / ``to_gltf`` / ``export_mtlx`` with a single
``_render_scalars()`` helper. Adapter methods drop from ~10 lines
each to ~3.
3. **New ``_render_textures()``**: symmetric helper hiding the
``self.textures`` lazy-cache call behind the same dispatch guard.
Net: render adapter bodies are now uniform — call the dumb mat-vis
adapter with ``_render_scalars()`` + ``_render_textures()``. The
three states (catalog, explicit, defaults) are isolated in three
private helpers, exposed publicly only via ``Vis.scalars`` (sparse).
Three small simplifications:
1. **Drop ``_render_textures()``**: ``Vis.textures`` already returns
``{}`` for no-identity (line 638). The wrapper added a layer
without changing behavior.
2. **Collapse ``_render_scalars()`` to call ``self.scalars``**: the
has_mapping branch was rebuilding the same dict that ``Vis.scalars``
already returns. One-liner now:
return self.scalars if self.has_mapping else self._scalars_with_defaults()
3. **Tighten section comment**: dropped redundant prose; the docstrings
on each helper carry the per-method contract.
Net: -12 lines. Render adapter bodies are now uniform 1-2 lines.
``Vis.scalars`` is the canonical "authored values" view; the only
delta in the render path is the ``_PBR_DEFAULTS`` fallback for
no-identity materials.
Three cleanups:
1. **Drop ``_extract_scalars`` / ``_extract_textures`` compat shims**
from ``pymat/vis/adapters.py``. Migrate ``tests/test_adapters.py``:
* ``_extract_scalars(m)`` → ``m.vis._scalars_with_defaults()``
(same behavior, tested at the right module). Class renamed
``TestExtractScalars`` → ``TestVisScalarsWithDefaults``.
* ``_extract_textures(m)`` → ``m.vis.textures`` (already returns
``{}`` for no-identity per the property contract). Class renamed
``TestExtractTextures`` → ``TestVisTextures``.
* ``_rgba_to_hex`` import moved from ``pymat.vis.adapters`` to
``pymat.vis._model`` (its actual home post-Phase-1).
2. **Tighten ``adapters.py`` module docstring** and switch ``Union[A,
B]`` to ``A | B`` (the file already has ``from __future__ import
annotations``). Removes ``typing.Union`` import.
3. **Narrow the exception clause** in ``Vis._catalog_scalars`` from
bare ``Exception`` to ``AttributeError`` — the real client's
``_scalars_for`` is silent on failure, so the only legitimate
need is the test-mock fallback for FakeClients lacking
``.asset()``. Real catalog errors now propagate visibly.
Net: -38 lines, ``adapters.py`` shrinks from 124 → 65 lines and
becomes pure dispatch with zero conversion logic.
…e right contract)
The original test asserted ``client.materials('gpuopen', '1k')`` returns
non-UUID strings — but per Bernhard's #311 follow-up, ``materials()`` is
the *wrong* API to test for name-based selection. It correctly returns
canonical IDs (UUIDs for gpuopen, slugs elsewhere) so consumers can
hand them straight to fetch_* / asset() without name-resolution
ambiguity. ``client.search()`` is the name-aware browse API.
Replace the strict-xfail single test with two passing tests that pin
the contract that actually serves the user's UX claim:
1. ``test_search_results_carry_display_names``: search returns Match
entries (mat-vis #359) with ``mat_vis.name`` populated and not
itself a UUID.
2. ``test_search_match_has_stable_ref_for_fetch``: each Match exposes
``.ref`` = ``"source/id"`` — collision-safe single-string handle
accepted by polymorphic ``client.asset(ref)``.
Drop xfail marker. Effective resolution of #311 at the API level: use
search() for browse-by-name, Match.ref for stable cross-source handles,
and asset() polymorphism to fetch from any of the three forms (Match,
ref-string, kwargs). The remaining gap (display-name kwargs to
asset()) is mat-vis #367 (filed today).
#230) Phase 2 of the dispatch refactor. Adds the symmetric browse-side API to complement Phase 1's render delegation: * ``Vis.candidates(*, category, roughness, metalness, ..., limit)``: search the catalog for appearances matching this material's PBR. Auto-derives ``roughness`` / ``metalness`` from ``self`` when not supplied — saves callers from manually piping their own scalars into ``mat_vis_client.search``. Returns ``list[Match]`` (mat-vis #359; works on legacy 0.6.x clients too because Match is a dict-subclass). * ``Vis.with_match(match)``: immutable identity assignment from a Match (or any dict-shaped index entry with ``source`` / ``id`` / optional ``available_tiers``). Prefers tier ``"1k"`` when staged, else first available, else preserves the calling Vis's tier. Routes through ``Vis.override`` so the texture cache invalidates atomically and the caller's original Vis is unchanged — safe to use on shared registry instances. * ``Vis.discover()`` deprecated. Emits ``DeprecationWarning`` pointing at ``candidates`` + ``with_match``. Behavior preserved for the deprecation cycle; will be removed in 4.x. ``auto_set=`` mutation is the specific anti-pattern; the new flow is ``vis.with_match(vis.candidates()[0])``. The canonical Bernhard flow (#230 acceptance) round-trips end-to-end:: steel = pymat["Stainless Steel 304"] for m in steel.vis.candidates(): print(m) # Match.__str__ — name + scalars + tiers picked = steel.with_vis(steel.vis.with_match(steel.vis.candidates()[0])) picked.vis.to_threejs() # works (uses Phase 1 catalog dispatch) 11 new tests covering auto-derivation, kwarg precedence, browse-only no-mutation, immutable identity transfer, tier preference, and cache invalidation. Existing TestDiscover suite kept (with deprecation warning suppression) until 4.x removes the method.
Two additions on top of Phase 1 + Phase 2:
1. **Material.to_threejs() / .to_gltf() / .export_mtlx()**: direct
rendering sugar so callers don't have to learn the ``.vis``
namespace for the common case. Each is a one-line delegate to
``self.vis.to_X(name=self.name)`` (name auto-fill where
appropriate; Three.js materials don't carry a name field).
The mental model that closes Bernhard's "pymat is too complex"
gripe in his recent feedback::
m = pymat["Stainless Steel 304"]
m.to_threejs() # ← that's it; no vis-namespace detour
Module-level ``pymat.vis.to_threejs(m)`` and ``m.vis.to_threejs()``
keep working unchanged — the new method is additive sugar.
2. **0.6.x catalog fallback in ``Vis._catalog_scalars()``**: prefers
``client.asset(...).scalars`` (mat-vis-client 0.7+ / mat-vis #93)
but falls back to ``client._scalars_for(...)`` for callers still
on mat-vis-client 0.6.x. Same data, different surface; lets the
spike land before mat-vis 0.7.x ships.
3. **Re-mark color-hex test as xfail**: ``test_color_is_hex_string``
needs mat-vis-client 0.7.x's ``color_format='hex'`` default. PyPI
is still 0.6.4 (int). Flips to passing when py-materials bumps
the dep.
6 new tests covering Material→Vis delegation parity (vs. method
form, vs. module function form), name auto-fill (glTF/MTLX), and
filename-stem semantics.
…ubstrate xfail Two follow-ups from CI feedback on the spike: 1. **``_render_scalars`` now layers defaults < catalog < explicit** instead of "catalog+overrides if has_mapping, else defaults." The aggressive form left scalar-only sources with sparse adapter output when the catalog has empty PBR (prod substrate state for gpuopen pre-#294, or for any source whose mat_vis.pbr.* is null). Sparse output meant Three.js fell through to *its* defaults instead of pymat's documented ``_PBR_DEFAULTS`` (grey-plastic shape). Visual regression test ``TestBernhardMatVis285_AdapterStructure_Scalar`` was correctly asserting the floor; the dispatch refactor briefly broke that contract. Layer order: ``_PBR_DEFAULTS`` < catalog < caller overrides. ``Vis.scalars`` (the public sparse view) keeps its no-defaults shape per mat#220 spec — only render adapters need the floor. 2. **Re-mark ``test_gpuopen_metals_have_distinct_scalars`` xfail**: prod substrate (v2026.04.2) gpuopen catalog still lacks authored PBR scalars, so every entry renders with baker defaults. That's a substrate-side bake bug, not a pymat code bug — the dispatch refactor reads whatever the catalog has. Verified working on mat-vis-tst@v2026.04.99-tst-full where catalog is correct; flips to passing when prod re-bakes.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Delegates pymat's render path to mat-vis-client's catalog (closes the "every gpuopen material is grey plastic" class of bugs), exposes Match-aware browse on Vis, and adds Material-direct rendering shortcuts so the common case is one method call.
Replaces
pymat/vis/adapters.py's scalar-extraction layer (which read only Vis dataclass fields, never the catalog — root cause of mat-vis #285) with thin dispatch onto:client.asset(...).scalars(mat-vis-client 0.7+ / dev) for catalog-backed readsclient._scalars_for(...)fallback for 0.6.x PyPI compatibilitymat_vis_client.adapters.to_X(scalars, textures)free-function primitive for the no-identity TOML/chemistry pathPer ADR-0002 Principle 3 ("thin delegation sugar") — pymat owns ingest from TOML/registry; mat-vis-client owns catalog reads + format adapters.
Issues closed
Vis.scalarsaccessor (sparse catalog + overrides;{}for no-identity)search()/Match — the right API for name-based selection)Vis.candidates()+Vis.with_match()Match-aware browsePublic surface additions
Vis.scalars(property) — sparse PBR scalars dictVis.candidates(...)(method) — Match-aware browse with auto-query from selfVis.with_match(match)(method) — immutable identity transfer from a MatchMaterial.to_threejs()/Material.to_gltf()/Material.export_mtlx()— direct rendering shortcuts (no.visnamespace required)Public surface deprecations
Vis.discover()— emitsDeprecationWarning; superseded bycandidates()+with_match(). Kept for back-compat with warning suppression on legacy tests; removal in 4.x.Bernhard "just render it" UX
Three lines. No
.visdetour, nosource/material_id/tiervocabulary, no module-path lookup.For browsing:
Diff size
src/pymat/vis/_model.pysrc/pymat/vis/adapters.pysrc/pymat/core.pyadapters.pyshrinks from 124 → 65 lines (zero conversion logic; pure Material→Vis dispatch).Test plan
client.asset(...)path worksmat-vis-tst@v2026.04.99-tst-full— confirms #285 catalog-side fix flows through correctlytest_adapters.py,test_vis.py,test_consumer_journey.pypass unchangedFollowups (filed)
_scalars_forshould resolve display names symmetric withfetch_all_textures(asymmetry I found while spiking; not blocking — Match flow sidesteps it)Followups (still open Bernhard surface)
Vis.__repr__observability (3 xfail tests; orthogonal pymat-side concern)Vis._fetchto re-raise withMaterial.name+ identity context (orthogonal)Commits
0abe031— refactor(vis): delegate render path to mat-vis catalog09e987e— cleanup-2: VisAsset public path + consolidated dispatch51abfab— cleanup-3: drop redundancy, collapse via Vis.scalarsbd29b7d— cleanup-4: remove compat shims, tighten adapters.pye3bf863— test(substrate): pin mat-vis #311 against search() / Match00c662b— feat(vis): candidates() + with_match()0e4f570— feat(material): direct rendering shortcuts + 0.6.x fallbackRefs #220, #230, mat-vis#285, mat-vis#298, mat-vis#311