[BUG FIX] Make Surface shortcut resolution idempotent#2761
Open
Kashu7100 wants to merge 3 commits intoGenesis-Embodied-AI:mainfrom
Open
[BUG FIX] Make Surface shortcut resolution idempotent#2761Kashu7100 wants to merge 3 commits intoGenesis-Embodied-AI:mainfrom
Kashu7100 wants to merge 3 commits intoGenesis-Embodied-AI:mainfrom
Conversation
The ``model_validator(mode="after")`` resolvers on ``Surface`` and ``Glass`` route shortcut fields (``color``, ``opacity``, ``roughness``, ``metallic``, ``emissive``, ``thickness``) into their corresponding ``*_texture`` fields, then raise if both are populated. They never cleared the shortcut after applying it, so the instance ended up with both populated. This worked when constructing a surface in isolation, but raised ``GenesisException: 'color' and 'diffuse_texture' cannot both be set.`` the moment the surface was nested inside another Pydantic model — e.g. ``MySurfaceWrapper(surface=gs.surfaces.Rough(color=...))`` — because Pydantic re-runs after-mode validators on nested instances and the second pass tripped the conflict check. Fix: clear each shortcut after routing it into the texture field. The ``default_roughness`` sync moves to the top of ``_resolve_shortcuts`` because the loop that consumes ``self.roughness`` now nulls it. Adds tests covering idempotent re-validation across Plastic-family, Glass, BSDF, and Emit surfaces; ``default_roughness`` sync ordering; and that first-construct conflict detection still raises. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch from clearing the shortcut fields after resolution to a private ``_shortcuts_resolved`` flag that gates ``Surface._resolve_shortcuts``. The previous approach (nulling ``self.color``, etc.) preserved idempotency but broke downstream consumers that read these fields: e.g., the Nyx scene exporter at gs_nyx_plugin populates ``mat.albedoColor`` from ``surface.color`` directly, so colored entities rendered as white once the shortcut was nulled. The flag preserves the shortcut fields verbatim while still gating the "both set" conflict check so re-validation (nested in another Pydantic model) is a no-op. Also consolidate the Glass ``thickness`` shortcut into the same loop in ``Surface._resolve_shortcuts`` (gated by ``texture_field in self.model_fields`` so non-Glass surfaces skip it). ``Glass._post_init`` keeps only its idempotent texture-shape normalization. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
tests are failing due to fetch issue - not related to this PR. |
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
The
model_validator(mode="after")resolvers onSurface(_resolve_shortcuts) andGlass(_post_init) route shortcut fields (color,opacity,roughness,metallic,emissive,thickness) into their corresponding*_texturefields and then raise if both are populated. They never clear the shortcut after applying it, so the instance ends up with both fields set.This works when constructing a surface in isolation, but raises
GenesisException: 'color' and 'diffuse_texture' cannot both be set.the moment the surface is nested inside another Pydantic model (downstream frameworks that wrap Genesis options for declarative scene configs commonly hit this). Pydantic 2 re-runs after-mode validators on nested instances even withrevalidate_instances="never"— the second pass sees bothcoloranddiffuse_texturepopulated and trips the conflict check.Repro
Fix
Clear each shortcut after routing it into the texture field, so re-running the validator is a no-op. The
default_roughnesssync moves to the top of_resolve_shortcutsbecause the loop that consumesself.roughnessnow nulls it.A repo-wide grep confirmed no other code reads
surface.<shortcut>after construction — these fields haveField(exclude=True, repr=False)and are documented as input-only shortcut parameters.Test plan
gs.surfaces.Rough(color=...),Glass(color=..., thickness=...),BSDF(color=..., roughness=..., metallic=...),Emission(color=...)round-trip safely through nested Pydantic models.default_roughnesssync still mirrors theroughnessshortcut, and an explicitdefault_roughnessstill wins over the shortcut.Rough(color=..., diffuse_texture=...),Glass(thickness=..., thickness_texture=...)).🤖 Generated with Claude Code