Adding Tendon USD data classes and writers - #6265
Conversation
Add the additive schema-fragment framework for the tendons family, mirroring the merged rigid-body pilot. Core gains the FixedTendonFragment / SpatialTendonFragment markers and the apply_fixed_tendon_properties / apply_spatial_tendon_properties family writers. Tendons are a tune-not-apply family, so the writers dispatch each fragment's func without applying a new anchor schema. PhysX gains PhysxFixedTendonCfg / PhysxSpatialTendonCfg data-carrier fragments that override func with thin apply_fixed_tendon / apply_spatial_tendon wrappers delegating to the existing multi-instance modify_fixed_/spatial_tendon_properties writers. The from_files spawn slots are widened to accept fragment lists with a transition bridge; legacy cfgs and writers stay intact. The legacy writer's mjc:* branch is kept inline with a future-split-candidate note.
No behavior change; collapse over-explained inline comments to terse intent.
Greptile SummaryThis PR introduces the tendon schema-fragment API, adding
Confidence Score: 5/5Purely additive, self-contained change with no modifications to existing call sites; legacy paths are untouched and the new fragment dispatch is well-tested. All previously flagged issues (untyped fragments parameter, always-True return, empty-list fallthrough to legacy writer) have been addressed in this version. The core dispatch logic, subtree traversal, prim-validity guard, and aggregated return value are all correct. The test suite covers metadata defaults, multi-instance writes, subtree descent, aggregated failure propagation, legacy/fragment behavioral equivalence, and the empty-list no-op spawn case. No new functional defects were found. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Spawner as _spawn_from_usd_file
participant Shim as Transition Shim
participant Core as apply_fixed_tendon_properties<br/>(isaaclab core)
participant PhysX as apply_fixed_tendon<br/>(isaaclab_physx)
participant Newton as apply_mujoco_fixed_tendon<br/>(isaaclab_newton)
participant USD as USD Stage
Spawner->>Shim: fixed_tendons_props (any type)
alt is SchemaFragment (single)
Shim->>Shim: wrap in list
end
alt list/tuple of SchemaFragment
Shim->>Core: apply_fixed_tendon_properties(prim_path, fragments)
Core->>USD: validate prim (raise ValueError if invalid)
loop each fragment
Core->>Core: resolve cfg.func (callable or string_to_callable)
alt PhysxFixedTendonCfg
Core->>PhysX: apply_fixed_tendon(cfg, prim_path, stage)
PhysX->>USD: PrimRange subtree traversal
PhysX->>USD: write PhysxTendonAxisRootAPI:inst:field
else MujocoFixedTendonCfg
Core->>Newton: apply_mujoco_fixed_tendon(cfg, prim_path, stage)
Newton->>USD: PrimRange subtree (MjcTendon type filter)
Newton->>USD: write mjc:camelCase(field)
end
end
Core-->>Spawner: bool (True if all fragments succeeded)
else legacy PhysxFixedTendonPropertiesCfg
Shim->>USD: modify_fixed_tendon_properties(prim_path, cfg)
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Spawner as _spawn_from_usd_file
participant Shim as Transition Shim
participant Core as apply_fixed_tendon_properties<br/>(isaaclab core)
participant PhysX as apply_fixed_tendon<br/>(isaaclab_physx)
participant Newton as apply_mujoco_fixed_tendon<br/>(isaaclab_newton)
participant USD as USD Stage
Spawner->>Shim: fixed_tendons_props (any type)
alt is SchemaFragment (single)
Shim->>Shim: wrap in list
end
alt list/tuple of SchemaFragment
Shim->>Core: apply_fixed_tendon_properties(prim_path, fragments)
Core->>USD: validate prim (raise ValueError if invalid)
loop each fragment
Core->>Core: resolve cfg.func (callable or string_to_callable)
alt PhysxFixedTendonCfg
Core->>PhysX: apply_fixed_tendon(cfg, prim_path, stage)
PhysX->>USD: PrimRange subtree traversal
PhysX->>USD: write PhysxTendonAxisRootAPI:inst:field
else MujocoFixedTendonCfg
Core->>Newton: apply_mujoco_fixed_tendon(cfg, prim_path, stage)
Newton->>USD: PrimRange subtree (MjcTendon type filter)
Newton->>USD: write mjc:camelCase(field)
end
end
Core-->>Spawner: bool (True if all fragments succeeded)
else legacy PhysxFixedTendonPropertiesCfg
Shim->>USD: modify_fixed_tendon_properties(prim_path, cfg)
end
Reviews (5): Last reviewed commit: "Address tendon PR review: empty-list shi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def apply_fixed_tendon_properties(prim_path: str, fragments, stage: Usd.Stage | None = None) -> bool: | ||
| """Apply a list of fixed-tendon fragments to a prim. | ||
|
|
||
| Fixed tendons are a *tune-not-apply* family: the applied ``PhysxTendonAxisRootAPI`` | ||
| multi-instance schemas already exist on the prim (authored in the source asset). This writer | ||
| therefore applies no anchor schema; it only dispatches each fragment via its | ||
| :attr:`~isaaclab.sim.schemas.SchemaFragment.func`, which tunes the existing instances. | ||
| Backend fragments carry backend-specific funcs, so core never imports a backend. | ||
|
|
||
| Args: | ||
| prim_path: The prim path to apply the fixed-tendon schemas on. | ||
| fragments: An iterable of :class:`~isaaclab.sim.schemas.FixedTendonFragment` instances. | ||
| stage: The stage where to find the prim. Defaults to None, in which case the current | ||
| stage is used. | ||
|
|
||
| Returns: | ||
| True if the properties were successfully set. | ||
| """ | ||
| if stage is None: | ||
| stage = get_current_stage() | ||
| for cfg in fragments: | ||
| func = cfg.func if callable(cfg.func) else string_to_callable(cfg.func) | ||
| func(cfg, prim_path, stage) | ||
| return True |
There was a problem hiding this comment.
fragments parameter is untyped and the function always returns True
Both apply_fixed_tendon_properties and apply_spatial_tendon_properties (line 1029) accept fragments without a type annotation, and unconditionally return True even when every func(cfg, prim_path, stage) call internally logs a warning and writes nothing (because the prim has no matching PhysxTendonAxisRootAPI schema). A caller checking the return value for success will always receive True, masking the case where the tendon schemas don't exist on the target prim. Adding Iterable[FixedTendonFragment] as the type and propagating failures (or at minimum a more honest docstring) would match the contract implied by the -> bool return type.
| def _strip_fragment_fields(cfg) -> dict: | ||
| """Collect a fragment's non-``None`` data fields, excluding the ``func`` plumbing field. | ||
|
|
||
| Args: | ||
| cfg: The fragment instance to read fields from. | ||
|
|
||
| Returns: | ||
| A mapping of set field names to their values, suitable for building a legacy cfg. | ||
| """ | ||
| return { | ||
| f.name: getattr(cfg, f.name) | ||
| for f in dataclasses.fields(cfg) | ||
| if f.name != "func" and getattr(cfg, f.name) is not None | ||
| } |
There was a problem hiding this comment.
_strip_fragment_fields is tightly coupled to the current field parity
_strip_fragment_fields drops None values and excludes func, then passes the remaining keys directly as **kwargs to PhysxFixedTendonPropertiesCfg / PhysxSpatialTendonPropertiesCfg. If a future subclass of PhysxFixedTendonCfg adds a field that doesn't exist in the legacy cfg, this will raise an unguarded TypeError at call-time with no hint about the root cause. Consider accepting a target class and intersecting the field names, or at least documenting the coupling explicitly so future authors know to keep both field sets in sync.
Bring apply_fixed_tendon_properties and apply_spatial_tendon_properties in line with sibling apply_* writers: raise ValueError on an invalid prim path instead of silently skipping, and AND each fragment's return value so a reported failure is not masked. Also annotate modify_fixed_tendon_properties and modify_spatial_tendon_properties as superseded by the fragment path; they are retained for back-compat with the transitional Physx*TendonPropertiesCfg configs until callers migrate. Tests added to test_tendon_fragments.py cover both the guard and the aggregation logic.
Replace the delegating bodies of apply_fixed_tendon and
apply_spatial_tendon with direct multi-instance enumeration over
PhysxTendonAxisRootAPI and PhysxTendonAttachment{Root,Leaf}API
respectively. Remove the modify_fixed_tendon_properties /
modify_spatial_tendon_properties imports and the legacy-cfg
reconstruction; add safe_set_attribute_on_usd_prim, get_current_stage,
and to_camel_case imports. Add test_apply_fixed_tendon_writes_all_instances
as a two-instance regression guard. All 11 tendon tests pass.
Introduces MujocoFixedTendonCfg (a FixedTendonFragment subclass with _usd_namespace="mjc") and apply_mujoco_fixed_tendon in a new isaaclab_newton/sim/schemas/schemas.py. The applier gates on MjcTendon prim type (returns False otherwise) and writes mjc:stiffness / mjc:damping via the existing safe_set_attribute_on_usd_prim path. Exports are wired through __init__.pyi (alphabetically sorted). The spawner fixed_tendons_props slot already admitted FixedTendonFragment | list[FixedTendonFragment] — no widening needed.
mesh_converter imported the schemas module, which lacks SchemaFragment (defined in schemas_cfg, re-exported by the package). The transition shim's isinstance(f, schemas.SchemaFragment) check raised AttributeError at convert time. Import the package instead, matching the other spawners.
Docstrings: add backend-composition note and Raises: ValueError to apply_fixed_tendon_properties / apply_spatial_tendon_properties; reword backward-compat NOTE comments to drop internal staging jargon; replace cross-package Sphinx role in physx applier docstring with prose; reword MujocoFixedTendonCfg composition sentence. Type annotations: annotate cfg params on apply_mujoco_fixed_tendon, apply_fixed_tendon, and apply_spatial_tendon with their fragment types via local schemas_cfg imports. Exports: move apply_mujoco_fixed_tendon before TitleCase classes in isaaclab_newton __init__.pyi __all__ (functions before classes). Changelogs: add isaaclab core fragment (ValueError + aggregated return); append migration guidance to physx Changed entry; convert plain literal to Sphinx :func: role in newton fragment. Tests: add test_apply_spatial_tendon_writes_all_instances (Root+Leaf); add raises-on-invalid-prim tests for both physx and newton appliers; extend non-mjc-prim test with HasAttribute check; add func-exclusion assertions to spatial write test and mjc positive write test.
# Conflicts: # source/isaaclab/isaaclab/sim/__init__.pyi # source/isaaclab/isaaclab/sim/schemas/__init__.pyi
Tendon schemas (PhysxTendonAxisRootAPI / PhysxTendonAttachment{Root,Leaf}API,
MjcTendon) are authored on descendant joint prims, not the prim_path the
spawner targets. The fragment appliers inspected only the single target prim,
so tendons on child joints were silently skipped (a regression from the
legacy apply_nested writers). Descend the whole subtree via Usd.PrimRange and
tune every prim carrying the schema. Add regression tests applying at a root
with the schema on a child joint (fixed and spatial).
The tendon appliers match their multi-instance schemas explicitly and do not use _usd_namespace, yet MujocoFixedTendonCfg advertised _usd_namespace = 'mjc' (the PhysX fragments inherited None implicitly). Set _usd_namespace = None explicitly on all three tendon fragments with a comment, so the intent is visible and the generic apply_namespaced guard would fire if one were ever mis-routed. Also refresh the PhysX fragment docstrings that still described the old delegating applier (now direct multi-instance enumeration).
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| if fixed_tendon_frags and all(isinstance(f, schemas.SchemaFragment) for f in fixed_tendon_frags): | ||
| schemas.apply_fixed_tendon_properties(prim_path, fixed_tendon_frags) | ||
| else: | ||
| schemas.modify_fixed_tendon_properties(prim_path, cfg.fixed_tendons_props) |
There was a problem hiding this comment.
When cfg.fixed_tendons_props = [] the condition if fixed_tendon_frags and all(...) short-circuits to False (an empty list is falsy), so the else branch calls modify_fixed_tendon_properties(prim_path, []). That function expects a PhysxFixedTendonPropertiesCfg dataclass, not a list, and will raise a cryptic AttributeError at runtime. The type annotation for fixed_tendons_props permits list[FixedTendonFragment], which includes []. Routing on the type of the original value (rather than the truthiness of the wrapped list) avoids the fallthrough. The same bug exists in the spatial block below.
| if fixed_tendon_frags and all(isinstance(f, schemas.SchemaFragment) for f in fixed_tendon_frags): | |
| schemas.apply_fixed_tendon_properties(prim_path, fixed_tendon_frags) | |
| else: | |
| schemas.modify_fixed_tendon_properties(prim_path, cfg.fixed_tendons_props) | |
| if isinstance(cfg.fixed_tendons_props, (schemas.SchemaFragment, list, tuple)): | |
| schemas.apply_fixed_tendon_properties(prim_path, fixed_tendon_frags) | |
| else: | |
| schemas.modify_fixed_tendon_properties(prim_path, cfg.fixed_tendons_props) |
| if spatial_tendon_frags and all(isinstance(f, schemas.SchemaFragment) for f in spatial_tendon_frags): | ||
| schemas.apply_spatial_tendon_properties(prim_path, spatial_tendon_frags) | ||
| else: | ||
| schemas.modify_spatial_tendon_properties(prim_path, cfg.spatial_tendons_props) |
There was a problem hiding this comment.
Mirrors the fixed-tendon bug above: spatial_tendons_props = [] (type-valid) makes if spatial_tendon_frags and all(...) falsy, routing to modify_spatial_tendon_properties(prim_path, []) which expects a PhysxSpatialTendonPropertiesCfg, not a list.
| if spatial_tendon_frags and all(isinstance(f, schemas.SchemaFragment) for f in spatial_tendon_frags): | |
| schemas.apply_spatial_tendon_properties(prim_path, spatial_tendon_frags) | |
| else: | |
| schemas.modify_spatial_tendon_properties(prim_path, cfg.spatial_tendons_props) | |
| if isinstance(cfg.spatial_tendons_props, (schemas.SchemaFragment, list, tuple)): | |
| schemas.apply_spatial_tendon_properties(prim_path, spatial_tendon_frags) | |
| else: | |
| schemas.modify_spatial_tendon_properties(prim_path, cfg.spatial_tendons_props) |
apply_fixed_tendon and apply_spatial_tendon were identical except for the schema marker(s) they match. Extract _tune_multi_instance_tendon(markers) and make both thin wrappers passing their markers, removing the duplicated descend-and-tune logic.
Pins that apply_fixed_tendon_properties authors the same attributes as the legacy modify_fixed_tendon_properties writer, on a synthetic root + descendant -joint structure mirroring the Shadow Hand (so it runs without asset-server access). Guards against the two code paths drifting and re-covers the descend-to-child-prims behavior.
Route tendon spawner slots on type (not truthiness) so an empty fixed/ spatial tendon list takes the fragment path as a no-op instead of crashing the legacy modify_*_tendon_properties writer with a list (mirrors the mass shim fix). Annotate the apply_*_tendon_properties fragments params with Iterable[...Fragment]. Add an empty-list spawn regression test and refresh the _strip_fragment_fields docstring (no longer builds a legacy cfg).
…-tendons # Conflicts: # source/isaaclab/isaaclab/sim/__init__.pyi # source/isaaclab/isaaclab/sim/schemas/__init__.pyi # source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py
|
One small docs/API clarity point: I would reword this to match the core writer docs: tendon fragments are backend-specific and should be composed across the appropriate target prims/roots, not advertised as generally mixable in one list for one target unless the caller intentionally accepts a |
2 participants
Description
Adds the tendons schema-fragment API.
FixedTendonFragment/SpatialTendonFragmentmarkers +PhysxFixedTendonCfg/PhysxSpatialTendonCfginisaaclab_physx. These are data carriers that overridefuncto the existing multi-instancemodify_fixed_tendon_properties/modify_spatial_tendon_propertieswriters (tune-not-apply), since tendon attributes live under multi-instancePhysxTendonAxisRootAPI:*schemas that the genericapply_namespacedwriter cannot handle.fixed_tendons_props/spatial_tendons_propsslots now also accept fragment lists.This PR is purely additive and self-contained: it builds only on the single-namespace schema-fragment base (
SchemaFragment+apply_namespaced) already indevelop, existing call sites are untouched (a transition bridge routes legacy single cfgs to the existingdefine_/modify_writers), and it does not depend on any other open PR.Type of change
Screenshots
N/A — non-visual API change.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there