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 tightens type annotations in scene.py by replacing the now-removed Asset / RigidObjectSet union with the correct ObjectBase base class, and fixes the get_events_cfg fields tuple to use EventTermCfg instead of AssetCfg.

• _is_double_precision (line 206): annotated -> bool | None even though the function always returns bool — counter to the PR's stated goal.
• _create_prim_from_asset (line 160–172): updated to accept ObjectBase, but immediately asserts isinstance(asset, Object), so any ObjectReference or RigidObjectSet in the scene will raise AssertionError at export time. The annotation should be narrowed to Object to match the implementation.

Confidence Score: 4/5

Safe to merge after addressing the annotation/assertion mismatch in _create_prim_from_asset.

One P1 finding: _create_prim_from_asset is annotated ObjectBase but asserts Object, meaning export will crash for non-Object assets in the scene. One P2 finding: residual bool | None annotation. No P0 issues.

isaaclab_arena/scene/scene.py — specifically _create_prim_from_asset (lines 160–172) and _is_double_precision (line 206).

Important Files Changed

Filename	Overview
isaaclab_arena/scene/scene.py	Type annotations cleaned up throughout (Asset/RigidObjectSet → ObjectBase, EventTermCfg fix); _is_double_precision still carries a spurious `

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class ObjectBase {
        +name: str
        +get_event_cfg()
        +get_object_cfg()
    }
    class Object {
        +usd_path
        +object_type: ObjectType
        +initial_pose
        +scale
        +get_relations()
    }
    class ObjectReference {
        +parent_asset: ObjectBase
        +get_relations()
    }
    class RigidObjectSet
    ObjectBase <|-- Object
    ObjectBase <|-- ObjectReference
    ObjectBase <|-- RigidObjectSet

    class Scene {
        +assets: dict~str, ObjectBase~
        +add_asset(asset: ObjectBase)
        +add_assets(assets: list~ObjectBase~)
        +get_scene_cfg() Any
        +get_events_cfg() Any
        +export_to_usd()
    }
    Scene --> ObjectBase : stores

    class _create_prim_from_asset {
        <<function>>
        param asset: ObjectBase
        assert isinstance(asset, Object)
    }
    _create_prim_from_asset ..> Object : actually requires

Loading

Comments Outside Diff (1)

1. isaaclab_arena/scene/scene.py, line 160-172 (link)

   Type annotation broader than implementation allows

   The parameter is now typed as ObjectBase, which includes Object, ObjectReference, and RigidObjectSet — but line 172 immediately asserts isinstance(asset, Object). Any non-Object asset in the scene (e.g. an ObjectReference or RigidObjectSet) passed through export_scene_to_usd → _create_prim_from_asset will raise an AssertionError at runtime. The annotation should be narrowed to reflect the actual constraint, or the assertion should be replaced with a proper guard and skip/error message.

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