Fix the type annotations in Scene.

[alexmillane]

Summary

Cleanup type annotations in Scene

Detailed description

• The type annotations were wrong.
• This was confusing Claude leading it to produce incorrect code.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR fixes incorrect type annotations throughout Scene, replacing ad-hoc Asset | RigidObjectSet unions with the proper abstract base ObjectBase. It also corrects the fields type hint in get_events_cfg() from AssetCfg to EventTermCfg, adds the missing import, and cleans up unused imports. All changes are type-annotation-only — no runtime behavior changes.

Design Assessment

Design is sound. The old annotations had two problems:

1. list[Asset, RigidObjectSet] is invalid Python — list accepts a single type parameter, not two.
2. Using Asset | RigidObjectSet was an incomplete union that excluded Object and ObjectReference as direct arguments.

Using ObjectBase as the common type is the correct fix — it’s the abstract base class that Object, ObjectReference, and RigidObjectSet all inherit from. The import cleanup (removing now-unused Asset and RigidObjectSet imports) is appropriate.

Findings

🔵 Suggestion: _create_prim_from_asset type vs runtime assertion — isaaclab_arena/scene/scene.py:150,162

The function signature now declares asset: ObjectBase, but the body immediately narrows with assert isinstance(asset, Object). This is a minor inconsistency — the annotation accepts a broader type than the function actually handles. Consider either:

• Keeping the parameter typed as Object (since that’s what’s actually required), or
• Adding a # type: narrow comment to signal the intentional narrowing to static analyzers.

This is non-blocking since the function is private (_-prefixed) and only called from export_scene_to_usd which iterates scene.assets (all valid Object instances at runtime).

Test Coverage

This is a type-annotation-only cleanup — no runtime behavior changes. No new tests are needed.

CI Status

Pre-commit check is pending.

Verdict

Ship it 🚀

Clean, correct type annotation fixes. The ObjectBase hierarchy is the right abstraction for these annotations, the stale AssetCfg type hint in get_events_cfg is properly fixed, and unused imports are removed. The one minor _create_prim_from_asset annotation note is non-blocking.

[isaaclab-review-bot]

👍 The old list[Asset, RigidObjectSet] was invalid Python (list takes a single type parameter). list[ObjectBase] is the correct fix — it properly covers Object, ObjectReference, and RigidObjectSet through the class hierarchy.

[isaaclab-review-bot]

👍 Good catch — this method builds EventTermCfg fields (from asset.get_event_cfg()), not AssetCfg fields. The old AssetCfg annotation was misleading.

[isaaclab-review-bot]

🔵 Minor suggestion: The signature now accepts ObjectBase, but the body immediately asserts isinstance(asset, Object) (line 162). Since this private function only works with Object instances (it accesses asset.usd_path and asset.scale), you could keep the parameter typed as Object for tighter type safety, or add a brief comment noting the intentional narrowing. Non-blocking either way.

[greptile-apps]

Greptile Summary

This PR cleans up type annotations in Scene to prevent misleading annotations from confusing AI code generation tools. All remaining findings are P2 style/annotation-precision suggestions; _is_double_precision (line 196) still carries a -> bool | None return type even though the function can only ever return a bool, which is the same category of issue the PR set out to fix.

Confidence Score: 5/5

Safe to merge; all findings are annotation-precision P2 suggestions with no runtime impact.

No logic, correctness, or security issues are introduced. The one lingering annotation inaccuracy (bool | None) mirrors the PR's own stated goal and is a P2 style fix, which does not block merge.

isaaclab_arena/scene/scene.py — specifically line 196 for the remaining incorrect return type annotation.

Important Files Changed

Filename	Overview
isaaclab_arena/scene/scene.py	Type annotation cleanup PR; _is_double_precision still carries an incorrect `-> bool

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["_is_double_precision(op: UsdGeom.XformOp)"] --> B{op is falsy?}
    B -- Yes --> C["return False (bool)"]
    B -- No --> D["return op.GetPrecision() == PrecisionDouble (bool)"]
    C --> E["Annotated: bool | None ← should be bool"]
    D --> E

Loading

_{Reviews (2): Last reviewed commit: "Merge branch 'develop' into alex/cleanup..." | Re-trigger Greptile}