-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Replaces set_carb_setting and get_carb_setting with direct carb calls
#3922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Replaces wrapper functions set_carb_setting and get_carb_setting from isaacsim.core.utils.carb with direct carb API calls to remove the dependency. The changes systematically replace the generic wrapper with type-specific methods (set_bool, set_int, set_float, set_string) and direct get() calls.
Key Changes:
- Removed imports of
set_carb_settingandget_carb_settingfrom 8 files - Added explicit type checking using
isinstance()to determine which type-specific setter to use - Replaced
get_carb_setting(settings, key)withsettings.get(key) - Type checking order is correct (bool before int, since bool is a subclass of int in Python)
Issue Found:
- Two additional files still use the deprecated imports but were not updated in this PR:
source/isaaclab/isaaclab/sim/utils.py(usesget_carb_setting)scripts/tools/check_instanceable.py(usesset_carb_setting)
Confidence Score: 3/5
- Safe to merge but incomplete - two files still use the deprecated imports
- The refactoring is technically sound with proper type checking and correct implementation. However, the migration is incomplete as two files still import and use the deprecated carb utility functions that this PR aims to remove. This creates an inconsistent state where some code uses the new approach while other code still depends on the old utilities.
- Pay attention to
source/isaaclab/isaaclab/sim/utils.pyandscripts/tools/check_instanceable.pywhich still use deprecated imports
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/sim/simulation_context.py | 4/5 | Replaced carb utils with direct API calls and added type-specific setter methods with proper type checking order |
| source/isaaclab/isaaclab/app/app_launcher.py | 5/5 | Simple replacement of set_carb_setting with type-specific carb API calls for rendering and animation recording |
Sequence Diagram
sequenceDiagram
participant App as AppLauncher
participant Sim as SimulationContext
participant Carb as carb.settings
Note over App,Carb: Before: Using wrapper functions
App->>Carb: set_carb_setting(settings, key, value)
Note over Carb: Type inference in wrapper
Carb-->>App: Setting applied
Note over App,Carb: After: Direct API calls
App->>App: Check isinstance(value, bool/int/float/str)
App->>Carb: settings.set_bool/set_int/set_float/set_string(key, value)
Carb-->>App: Setting applied
Sim->>Sim: _apply_render_settings_from_cfg()
Sim->>Sim: Type check preset values
Sim->>Carb: settings.set_bool/set_int/set_float/set_string(key, value)
Carb-->>Sim: Setting applied
Note over Sim: Uses settings.get() instead of get_carb_setting()
Sim->>Carb: settings.get(key)
Carb-->>Sim: Return value
8 files reviewed, 1 comment
| import isaacsim.core.utils.stage as stage_utils | ||
| import omni.log | ||
| import omni.physx | ||
| import omni.usd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Two files still import and use the deprecated carb utils but weren't updated:
source/isaaclab/isaaclab/sim/utils.py:23imports and usesget_carb_settingon line 1075scripts/tools/check_instanceable.py:70imports and usesset_carb_settingon line 99
These should be updated to use direct carb API calls for consistency.
| ) | ||
| key = rendering_setting_name_mapping[key] | ||
| set_carb_setting(self.carb_settings, key, value) | ||
| if isinstance(value, bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In simcontext, we have a function called set_setting you can just put this logic there and later only call self.set_setting.
Description
Removes dependency on
Replaces
set_carb_setting(carb_settings, <setting-name>, <setting-value>with direct carb callcarb_settings.set_string(<setting-name>, <setting-value>),...set_int...,...set_bool...,...set_float.... And replacesget_carb_settingwithcarb_settings.get()Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there