Adding Mass USD data classes and writers - #6263
Conversation
Add the additive mass schema-fragment framework mirroring the rigid-body pilot. Mass is a pure-UsdPhysics family with no backend split. - Add MassFragment marker and MassCfg fragment (physics:mass / physics:density) in core schemas_cfg. - Add apply_mass_properties writer applying UsdPhysics.MassAPI as the implicit anchor, then dispatching each fragment via its func. - Widen the RigidObjectSpawnerCfg mass_props slot to accept a MassFragment or list, with transition bridges at the shapes, meshes, from_files, and mesh_converter spawn sites. - Export the new public names and add a test plus changelog fragment. The legacy MassPropertiesCfg and define_/modify_mass_properties remain the canonical names and are left untouched.
No behavior change; collapse over-explained inline comments to terse intent.
Greptile SummaryThis PR adds the mass schema-fragment API: a
Confidence Score: 4/5Safe to merge if the single-MassFragment path is dropped from the type annotation or the four shims are updated to wrap it in a list before dispatching. The spawner_cfg.py (type annotation), and the four shims in shapes.py, from_files.py, meshes.py, and mesh_converter.py — any one of which should gate or wrap the single-fragment case. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["mass_props value"] --> B{Is None?}
B -- Yes --> Z[Skip / no-op]
B -- No --> C{isinstance list/tuple?}
C -- Yes --> D{all SchemaFragment?}
D -- Yes --> E["apply_mass_properties(prim_path, fragments)"]
D -- No --> F["define_mass_properties / modify_mass_properties\n(legacy MassPropertiesCfg path)"]
C -- No --> G{isinstance SchemaFragment?}
G -- Yes --> H["Falls to else branch\n→ define_mass_properties(MassCfg)\n→ _apply_namespaced_schemas\n→ ValueError: SchemaFragment has no _usd_namespace"]
G -- No --> F
E --> I["1. Apply UsdPhysics.MassAPI anchor\n2. For each fragment: call cfg.func\n → apply_namespaced skips func field"]
style H fill:#ffcccc,stroke:#cc0000
%%{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"}}}%%
flowchart TD
A["mass_props value"] --> B{Is None?}
B -- Yes --> Z[Skip / no-op]
B -- No --> C{isinstance list/tuple?}
C -- Yes --> D{all SchemaFragment?}
D -- Yes --> E["apply_mass_properties(prim_path, fragments)"]
D -- No --> F["define_mass_properties / modify_mass_properties\n(legacy MassPropertiesCfg path)"]
C -- No --> G{isinstance SchemaFragment?}
G -- Yes --> H["Falls to else branch\n→ define_mass_properties(MassCfg)\n→ _apply_namespaced_schemas\n→ ValueError: SchemaFragment has no _usd_namespace"]
G -- No --> F
E --> I["1. Apply UsdPhysics.MassAPI anchor\n2. For each fragment: call cfg.func\n → apply_namespaced skips func field"]
style H fill:#ffcccc,stroke:#cc0000
Reviews (2): Last reviewed commit: "Annotate mass writer fragments type and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def apply_mass_properties(prim_path: str, fragments, stage: Usd.Stage | None = None) -> bool: | ||
| """Apply a list of mass fragments to a prim. | ||
|
|
||
| Applies ``UsdPhysics.MassAPI`` as the implicit anchor (the defining schema for mass properties), | ||
| then dispatches each fragment via its :attr:`~isaaclab.sim.schemas.SchemaFragment.func`. | ||
|
|
||
| Args: | ||
| prim_path: The prim path to apply the mass schemas on. | ||
| fragments: An iterable of :class:`~isaaclab.sim.schemas.MassFragment` 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() | ||
| prim = stage.GetPrimAtPath(prim_path) | ||
| if not UsdPhysics.MassAPI(prim): | ||
| UsdPhysics.MassAPI.Apply(prim) | ||
| 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.
apply_mass_properties skips the prim.IsValid() guard that its companion apply_rigid_body_properties (line 459) explicitly includes. If an invalid prim_path is passed, the function proceeds to UsdPhysics.MassAPI(prim) on an invalid prim and then dispatches the fragment funcs, eventually returning True regardless — masking the error entirely. Additionally, unlike apply_rigid_body_properties which aggregates per-fragment return values into a success flag, this function always returns True even when a fragment's func reports failure.
| # note: we apply rigid properties in the end to later make the instanceable prim | ||
| # apply mass properties | ||
| if cfg.mass_props is not None: | ||
| schemas.define_mass_properties(prim_path, cfg.mass_props, stage=stage) | ||
| # transition shim, remove later: new fragment list -> apply_*; legacy single cfg -> define_* | ||
| mass_frags = cfg.mass_props if isinstance(cfg.mass_props, (list, tuple)) else [cfg.mass_props] | ||
| if mass_frags and all(isinstance(f, schemas.SchemaFragment) for f in mass_frags): | ||
| schemas.apply_mass_properties(prim_path, mass_frags, stage=stage) |
There was a problem hiding this comment.
define_mass_properties with wrong type
When cfg.mass_props is an empty list [], mass_frags becomes [], and if mass_frags and ... evaluates to False — so the else branch calls schemas.define_mass_properties(prim_path, cfg.mass_props, ...) with [] as the cfg argument. define_mass_properties expects a MassPropertiesCfg, so this produces a runtime error rather than a no-op. The same issue appears in the shims in from_files.py (line 353), meshes.py (line 441), and mesh_converter.py (line 183).
| """ | ||
|
|
||
|
|
||
| def apply_mass_properties(prim_path: str, fragments, stage: Usd.Stage | None = None) -> bool: |
There was a problem hiding this comment.
fragments parameter lacks a type annotation. apply_rigid_body_properties uses Iterable[schemas_cfg.RigidBodyFragment], which keeps signatures consistent and enables static analysis to catch type mismatches at the call site.
| def apply_mass_properties(prim_path: str, fragments, stage: Usd.Stage | None = None) -> bool: | |
| def apply_mass_properties( | |
| prim_path: str, fragments: Iterable[schemas_cfg.MassFragment], stage: Usd.Stage | None = None | |
| ) -> bool: |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| mass: float | None = None | ||
| """The mass of the rigid body [kg]. | ||
|
|
||
| Writes ``physics:mass`` via :class:`UsdPhysics.MassAPI`. | ||
|
|
||
| Note: | ||
| If non-zero, the mass is ignored and the density is used to compute the mass. | ||
| """ |
There was a problem hiding this comment.
Note for the mass field is ambiguous: "If non-zero, the mass is ignored" leaves "non-zero" unspecified (does it refer to mass or density?). The USD physics rule is that a non-zero density takes precedence and is used to derive mass. The same ambiguous note exists in MassPropertiesCfg but is copied here as-is.
| mass: float | None = None | |
| """The mass of the rigid body [kg]. | |
| Writes ``physics:mass`` via :class:`UsdPhysics.MassAPI`. | |
| Note: | |
| If non-zero, the mass is ignored and the density is used to compute the mass. | |
| """ | |
| mass: float | None = None | |
| """The mass of the rigid body [kg]. | |
| Writes ``physics:mass`` via :class:`UsdPhysics.MassAPI`. | |
| Note: | |
| If ``density`` is non-zero, it takes precedence and is used to compute the mass instead. | |
| """ |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
apply_mass_properties silently accepted invalid prim paths and always returned True. Add the prim-validity guard and aggregate per-fragment results so a reported failure is not masked by the always-applied MassAPI anchor, matching apply_rigid_body_properties. Route the spawn-writer mass shims on type rather than truthiness, so an empty mass_props list takes the fragment path and no-ops cleanly instead of being forwarded to the legacy writer as an unexpected list. Add regression tests covering the invalid-path raise, result aggregation, and the empty-list no-op spawn.
Add the missing Iterable[MassFragment] annotation on apply_mass_properties so its signature matches apply_rigid_body_properties and static analysis can flag call-site type mismatches. Clarify the MassCfg.mass docstring note: a non-zero density (not a non-zero mass) takes precedence and is used to compute the mass.
| """ | ||
|
|
||
| mass_props: schemas.MassPropertiesCfg | None = None | ||
| """Mass properties.""" | ||
| mass_props: schemas.MassPropertiesCfg | schemas.MassFragment | list[schemas.MassFragment] | None = None | ||
| """Mass properties. | ||
|
|
||
| Accepts either a single legacy :class:`~isaaclab.sim.schemas.MassPropertiesCfg` or a list of | ||
| :class:`~isaaclab.sim.schemas.MassFragment` fragments (e.g. ``[MassCfg(...)]``). When a fragment | ||
| list is given, ``UsdPhysics.MassAPI`` is applied as the implicit anchor and each fragment writes | ||
| its own namespace. | ||
| """ | ||
|
|
||
| rigid_props: schemas.RigidBodyBaseCfg | schemas.RigidBodyFragment | list[schemas.RigidBodyFragment] | None = None | ||
| """Rigid body properties. |
There was a problem hiding this comment.
MassFragment advertised but silently crashes at runtime
The type annotation adds schemas.MassFragment as a valid single-value option, but all four spawn shims (shapes.py, from_files.py, meshes.py, mesh_converter.py) route only isinstance(cfg.mass_props, (list, tuple)) through apply_mass_properties. A bare MassCfg(mass=1.0) (not wrapped in a list) falls into the else branch and is passed to define_mass_properties / modify_mass_properties, which calls _apply_namespaced_schemas. That helper builds cfg_dict from every dataclass field — including func inherited from SchemaFragment. Because func is non-None and _get_field_declaring_class resolves it to SchemaFragment (whose _usd_namespace = None), _apply_namespaced_schemas raises ValueError: SchemaFragment declares fields ['func'] but does not define '_usd_namespace'. No test exercises the single-fragment path.
Fix: either drop schemas.MassFragment from the union (since the docstring already says "a list of MassFragment fragments") or add a single-fragment guard in each shim, e.g. wrapping with [cfg.mass_props] before dispatching to apply_mass_properties.
The mass_props slot advertises a single MassFragment as a convenience form, but the spawn shims only routed list/tuple values through apply_mass_properties; a bare MassCfg fell to the legacy writer and raised ValueError. Normalize a single fragment to a list in all four shims (shapes, from_files, meshes, mesh_converter) so the convenience form works, and add a single-fragment spawn regression test.
mesh_converter imported the schemas module, which lacks SchemaFragment (defined in schemas_cfg, re-exported by the package). The transition shims' isinstance(f, schemas.SchemaFragment) checks raised AttributeError at convert time. Import the package instead, matching the other spawners.
# Description Adds the **mass** schema-fragment API. Unlike the other families, mass has **no backend split** — it is a single core fragment. - `MassFragment` marker + `MassCfg` (`physics:mass` / `physics:density`; `UsdPhysics.MassAPI` anchor) in `isaaclab`. - `apply_mass_properties` family writer (applies the `MassAPI` anchor, then dispatches each fragment via its `func`). - The spawner `mass_props` slot now also accepts a `MassCfg` / `list[MassCfg]`. This PR is purely **additive** and self-contained: it builds only on the single-namespace schema-fragment base (`SchemaFragment` + `apply_namespaced`) already in `develop`, existing call sites are untouched (a transition bridge routes legacy single cfgs to the existing `define_`/`modify_` writers), and it does **not** depend on any other open PR. ## Type of change - New feature (non-breaking change which adds functionality) ## Screenshots N/A — non-visual API change. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package (do **not** edit `CHANGELOG.rst` or bump `extension.toml` — CI handles that) - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
2 participants
Description
Adds the mass schema-fragment API. Unlike the other families, mass has no backend split — it is a single core fragment.
MassFragmentmarker +MassCfg(physics:mass/physics:density;UsdPhysics.MassAPIanchor) inisaaclab.apply_mass_propertiesfamily writer (applies theMassAPIanchor, then dispatches each fragment via itsfunc).mass_propsslot now also accepts aMassCfg/list[MassCfg].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