Add physics variant selection to asset converters - #6881
Closed
hujc7 wants to merge 15 commits into
Closed
Conversation
The URDF and MJCF importers emit physics as payloads behind a "Physics" variant set and leave it unselected, so the generated USD composes with geometry only. The converters already author a "physx" selection to compensate, but bail out when the asset offers no such variant: a URDF whose joints are all fixed yields only "none" and "physics", so those assets still convert without joints, articulation roots, or mass. Resolve the requested variant against what the asset actually offers, falling back to the backend-neutral "physics" variant that both "physx" and "mujoco" sublayer. Expose the request as physics_variant on the converter config so that a Newton user can ask for "mujoco" at conversion time, and "none" stays available as an explicit opt-out.
…er-physics-variant
The "physics" variant holds the backend-portable description: standard UsdPhysics joints, articulation roots and mass, plus the Newton schemas. The "physx" and "mujoco" variants sublayer it and add solver tuning, so selecting one of them bakes a backend choice into the converted asset. Default to "physics" instead, which leaves the asset usable on either backend and leaves the backend choice to whoever knows it. Nothing is lost by this: every variant stays in the generated USD file, so a caller can request one at conversion time or override the selection at spawn.
The variant names were module-level string constants, which put the valid
values out of reach of callers. Expose them as a nested PhysicsVariant
string enum on the converter config instead, so a caller can name a
variant directly and the values document themselves.
Serializing an enum member needs class_to_dict to treat it as a leaf.
Enum members carry a __dict__ of enum internals, so the generic branch
recursed into it and emitted {_value_, _name_, _sort_order_} in place of
the value -- which would have landed in every converted asset's
config.yaml and in the hash that decides whether to reconvert.
The standalone URDF/MJCF importer instructions sat inside the legacy installer section, so users following the recommended uv path never saw them. The wheel is not specific to any installation method, so promote it to its own section that all of them reach. The instructions also predated the wheel being published, and told the reader to substitute a package index URL for it. It is on PyPI now, so install it by name.
…er-physics-variant
The importers disagree on which physics variant to select. The Isaac Sim importer extensions leave the variant set unselected, while the standalone importer wheel selects "physx". The converter only authored a selection when none was present, so on the standalone path physics_variant was silently discarded and every asset came out as "physx". Author the configured variant whenever it differs from what is already selected, so the same config produces the same asset on either path.
…er-physics-variant
Falling back to another variant handed the caller an asset authored for a different backend than they asked for, silently. A caller that names "physx" wants PhysX tuning; substituting the portable variant produces an asset that loads and simulates, just not as configured. Raise instead, naming the variants the asset does offer. Assets with no "Physics" variant set at all are still left alone, since there is nothing to select.
The installation doc cleanup upstream moved the standalone importer section from the installation index into the import how-to and deleted the legacy installer include. Keep upstream's location and drop the duplicate section, then reapply the install command fix there: the wheel is on PyPI, so the package index substitution it still described is unnecessary.
Review of the physics variant work surfaced four defects. class_to_dict serializes enum members as their value, but update_class_from_dict type-checked the raw scalar against the enum class, so reloading any config holding an enum default raised. This reached beyond the converters: RecorderManagerBaseCfg round-trips through Hydra for every manager-based environment. Rebuild the member from its value instead, so the two functions are inverses again. The asset hash was recorded before conversion ran, so a conversion that raised still counted as cached and an identical retry skipped it and returned the asset. Record the hash only once the asset is complete. The nested PhysicsVariant enum was indexed once per converter cfg subclass, and the docs build treats warnings as errors, so it failed with three duplicate-object warnings. Document it on the base class only. The physics_variant docstring said conversion raises for an absent variant, which does not hold for a flat asset carrying no variant set.
The previous commit swept in a re-resolution of the lockfile. This branch changes no dependencies, so the lockfile belongs unchanged.
USD accepts a variant selection naming a variant the set does not offer, and the prim then composes as if nothing were selected. For the "Physics" variant set that silently drops the joints, articulation roots and mass properties the set exists to carry, so the asset spawns as plain geometry with no diagnostic -- the same symptom this branch fixes on the conversion side. Raise for a "Physics" set that is absent or lacks the requested variant, and keep the existing warn-and-continue for every other set. Those were introduced for optional variants such as a payload or a colour, where one spawn configuration is expected to cover assets that do not all expose the set.
…nt' into jichuanh/converter-physics-variant
Collaborator
Author
|
Folding this into #6935. That PR already contains these commits, and splitting the review across two PRs was costing more than it saved — the physics variant selection and the importer packaging both hinge on the same question of which importer serves a conversion. Everything here is in #6935 unchanged:
Closing in favour of #6935. |
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
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 URDF and MJCF importers emit their physics description as payloads behind a
"Physics"variant set on the generated asset, and leave that variant set unselected. USD has no implicit default, so an unselected variant set contributes nothing to composition and the converted USD opens with geometry only — no joints, no articulation root, no mass properties.The converters already author a
"physx"selection to compensate, but return early when the asset offers no such variant. A URDF whose joints are all fixed yields["none", "physics"]and no"physx", so those assets still convert physics-free.What changed
AssetConverterBaseCfg.physics_variant(new, defaults to"physics") chooses which variant the converter selects on the generated USD file.Every variant stays in the generated USD file, so the selection is not destructive — it only decides what composes when the asset is opened without saying anything.
UsdFileCfg.variantsstill overrides it at spawn time.Why
"physics"is the defaultThe variants are a shared base plus two supersets:
physx.usdaandmujoco.usdaboth carrysubLayers = [@./physics.usda@]. Measured on the converted Franka:physicsUsdPhysicsjoints / articulation root / mass, plusNewtonMimicAPI,NewtonArticulationRootAPI,NewtonCollisionAPIphysxPhysxJointAPIandphysxJoint:maxJointVelocityper jointmujocoMjcActuatorprim per jointThe base is the backend-portable layer by construction: the importer routes schemas matching
Physics.*andNewton.*into it,Physx.*into the PhysX variant, andMjc.*into the MuJoCo variant. Everything structural lives in the base — the importer no longer authors PhysX mimic joints or articulation roots — so the backend variants carry solver tuning only.Defaulting to
"physics"therefore produces an asset that runs on either backend, and leaves the backend choice to the caller who knows which one they want. Selecting"physx"or"mujoco"bakes that choice into the asset, which is the right thing to do explicitly and the wrong thing to do by default.Testing
Isaac Sim 6.1.0-alpha.35, Kit 110.3.0:
test_physics_variant_overrideandtest_physics_variant_raises_when_requested_absentfailThe new tests cover the default selection, an explicit
"mujoco"override, and the failure raised for an asset that offers no"physx"variant.Tracked as nvbug 6527494.