Fixes drone navigation, camera benchmark, and kitless shutdown errors

[kellyguo11]

Description

Environment shutdown fix (isaaclab/envs/) All three env classes (DirectRLEnv, DirectMARLEnv, ManagerBasedEnv) had a race condition in del: the lazy import sys inside the destructor could fail or produce a spurious traceback once Python's import machinery had already finalized at interpreter shutdown. The fix captures sys.is_finalizing as a default argument at class definition time (before finalization), so the check always works safely.

Drone ARL navigation default env count (isaaclab_tasks/) Reduced the default num_envs for Isaac-Navigation-3DObstacles-ARL-Robot-1-v0 from 4096/8192 to 1024 in both the env config and the rl_games PPO config. Users can override with --num_envs or env.scene.num_envs.

Camera benchmark fix (scripts/benchmarks/benchmark_cameras.py) Moved prim creation and prim_path assignment to after the camera config object is built, so class_type is resolved from the instantiated config rather than the unresolved camera_cfg callable. Also uses the config's class_type (not the original camera_cfg.class_type) when calling the constructor, fixing a subtle bug when the config overrides the class.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[isaaclab-review-bot]

PR Review: Fixes drone navigation, camera benchmark, and kitless shutdown errors

This PR addresses three distinct issues. Here's my analysis:

1. Environment Shutdown Fix ✅

Files: direct_marl_env.py, direct_rl_env.py, manager_based_env.py

The fix for the __del__ destructor race condition is well-designed. The pattern:

def __del__(self, _sys_is_finalizing=sys.is_finalizing):
    if not _sys_is_finalizing():
        self.close()

This is a known Python idiom for capturing references at function definition time rather than at call time. During interpreter shutdown, module-level names (like sys) can become None, causing AttributeError or TypeError when accessed in __del__. By capturing sys.is_finalizing as a default argument, the reference is bound when the class is defined, not when the destructor runs.

Minor observation: The import sys at the top is now consistent across all three files. Good.

2. Drone ARL Navigation Default Env Count ✅

Files: navigation_env_cfg.py, rl_games_rough_ppo_cfg.yaml

Reducing default num_envs from 4096/8192 → 1024 is a reasonable usability improvement. The changelog clearly documents that users can override via --num_envs or env.scene.num_envs.

Note: The YAML had 8192 while the Python config had 4096 - they're now both consistent at 1024.

3. Camera Benchmark Fix ⚠️

File: benchmark_cameras.py

The refactoring moves prim creation and path assignment to after the camera config is instantiated. The logic change:

Before: Used camera_cfg.class_type.__name__ where camera_cfg is the callable (config class)
After: Uses cfg.class_type.__name__ where cfg is the instantiated config object

And:

# Before
return camera_cfg.class_type(cfg=cfg)
# After  
return cfg.class_type(cfg=cfg)

This is correct when the config might override class_type after instantiation.

Questions/Suggestions:

1. The change prim_path=prim_path if prim_path is not None else "" - is an empty string a valid fallback here? The original code set prim_path = f"/World/{name}_.*/{name}" when None. Now the empty string is set initially, then potentially overwritten. Just want to confirm the empty string doesn't cause issues if the if prim_path is None branch isn't taken.

2. There's now a check if instantiate: duplicated twice in the same block (lines ~305 and ~311 in the new code). Consider consolidating:

   if instantiate:
       for idx in range(num_cams):
           sim_utils.create_prim(f"/World/{name}_{idx:02d}", "Xform")
       if prim_path is None:
           cfg.prim_path = f"/World/{name}_.*/{name}"
       return cfg.class_type(cfg=cfg)
   else:
       if prim_path is None:
           cfg.prim_path = f"/World/{name}_.*/{name}"
       return cfg

   This would reduce nesting and make the flow clearer.

Changelog Entries ✅

Both changelog files follow the expected .rst format and are appropriately placed in changelog.d/.

Checklist Items

The author notes:

• Tests not added - Given these are bug fixes, it would be valuable to have regression tests, especially for the shutdown fix (though testing __del__ behavior can be tricky).
• Changelog version not updated in extension.toml - May need attention depending on project versioning practices.

---

Overall: The changes are sound and address real issues. The shutdown fix uses an established Python pattern. The camera benchmark fix corrects a subtle class resolution bug. Minor code organization suggestions above are optional.

Review by isaaclab-review-bot 🤖

---

Update (commit 1437ca0): Previous concerns addressed. No new issues.

• ✅ Empty string fallback fixed: prim_path now defaults to f"/World/{name}_.*/{name}" directly in the constructor call — no more ambiguous empty string.
• ✅ Code structure simplified: Removed duplicated if instantiate: check. The function now uses a clean early-return pattern with a single if instantiate: block.
• The new _get_camera_class_name() helper cleanly resolves the camera class name from dataclass fields before instantiation — good separation of concerns.

No new issues introduced in this commit.

[greptile-apps]

Greptile Summary

This PR fixes three independent bugs: a Python interpreter-shutdown race condition in all three env __del__ methods, an incorrect env count for the ARL drone navigation task, and a wrong class_type resolution in the camera benchmark script.

• Env shutdown fix: All three env classes (DirectRLEnv, DirectMARLEnv, ManagerBasedEnv) now capture sys.is_finalizing as a default argument at class-definition time instead of performing a lazy import sys inside __del__, which could silently fail after Python's import machinery is torn down.
• Drone nav num_envs: Default env count reduced from 4096 / 8192 to 1024 in both the env config and the rl_games PPO YAML; users can override via --num_envs.
• Camera benchmark: Prim creation and prim_path assignment are now deferred until after the config object is instantiated, and cfg.class_type is used instead of camera_cfg.class_type, correctly handling configs that override the sensor class.

Confidence Score: 4/5

All three fixes address real, reproducible bugs and are safe to merge; the only question mark is whether any CameraCfg subclass validates a non-empty prim_path at construction time.

The env shutdown and drone nav changes are straightforward and low-risk. The camera benchmark reordering is the most complex change: using an empty string as a temporary prim_path before overriding it is functionally correct as long as CameraCfg (and subclasses) do not validate the field at construction time.

scripts/benchmarks/benchmark_cameras.py — specifically the empty-string placeholder for prim_path when the caller passes None.

Important Files Changed

Filename	Overview
scripts/benchmarks/benchmark_cameras.py	Moved prim creation and prim_path assignment to after config instantiation; uses cfg.class_type instead of camera_cfg.class_type; passes "" as a temporary placeholder when prim_path is None.
source/isaaclab/isaaclab/envs/direct_rl_env.py	Captures sys.is_finalizing at class-definition time as a default argument to del, eliminating the interpreter-shutdown race condition.
source/isaaclab/isaaclab/envs/direct_marl_env.py	Same del fix as direct_rl_env.py — safe sys.is_finalizing capture at definition time.
source/isaaclab/isaaclab/envs/manager_based_env.py	Same del fix as other env files.
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/navigation/config/arl_robot_1/navigation_env_cfg.py	Reduces default num_envs from 4096 to 1024 for ARL drone navigation.
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/navigation/config/arl_robot_1/agents/rl_games_rough_ppo_cfg.yaml	Reduces rl_games num_envs from 8192 to 1024, consistent with the env config change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["create_camera_base(camera_cfg, …)"] --> B{"num_cams > 0\ndata_types non-empty\nheight & width > 0?"}
    B -- No --> Z["return None"]
    B -- Yes --> C["cfg = camera_cfg(prim_path=prim_path or '', …)\nresolve cfg.class_type"]
    C --> D{"instantiate?"}
    D -- Yes --> E["create Xform prims\n/World/ClassName_00 … _N"]
    D -- No --> F{"prim_path is None?"}
    E --> F
    F -- Yes --> G["cfg.prim_path = '/World/ClassName_.*/ClassName'"]
    F -- No --> H["keep user-provided prim_path"]
    G --> I{"instantiate?"}
    H --> I
    I -- Yes --> J["return cfg.class_type(cfg=cfg)"]
    I -- No --> K["return cfg"]

Loading

_{Reviews (1): Last reviewed commit: "Fixes drone navigation, camera benchmark..." | Re-trigger Greptile}

[greptile-apps]

Empty string passed as a temporary prim_path placeholder before it is overridden. While functionally safe here (the attribute is reassigned on cfg before the sensor is instantiated), passing "" may trigger unexpected validation warnings or early-path behaviour inside CameraCfg.__init__ that assumed prim_path would always be a non-empty USD path or the MISSING sentinel. A more explicit sentinel or at minimum a short inline comment would help future readers understand why the empty string is intentional.

Suggested change

	cfg = camera_cfg(
	prim_path=prim_path,
	prim_path=prim_path if prim_path is not None else "",
	cfg = camera_cfg(
	prim_path=prim_path if prim_path is not None else "", # placeholder; overridden below when prim_path is None

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!