-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Simplifies import of stage and prim utils #4286
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
Conversation
Greptile SummaryThis PR simplifies the import structure by consolidating utility functions under Key Changes:
Benefits:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SimUtils as sim_utils
participant Stage as USD Stage
participant Spawner as Spawner Functions
Note over User,Spawner: Before: Multiple Import Pattern
User->>User: import prim_utils<br/>import stage_utils
User->>Spawner: prim_utils.create_prim()
Spawner->>Stage: get_prim_at_path()
Stage-->>Spawner: Prim
Spawner-->>User: Prim
Note over User,Spawner: After: Unified Import Pattern
User->>User: import sim_utils
User->>SimUtils: sim_utils.create_prim()
SimUtils->>Stage: stage.GetPrimAtPath()<br/>(Direct USD API)
Stage-->>SimUtils: Prim
SimUtils-->>User: Prim
Note over User,Spawner: Internal Refactoring
User->>SimUtils: spawn_sphere(path, cfg)
SimUtils->>SimUtils: get_current_stage()
SimUtils->>Stage: stage.DefinePrim(path, type)
Stage-->>SimUtils: Prim
SimUtils->>SimUtils: Apply attributes & transforms
SimUtils->>Stage: stage.GetPrimAtPath(path)
Stage-->>SimUtils: Final Prim
SimUtils-->>User: Prim
|
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
|
@greptile-apps want to take another look? |
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.
Looking at this PR again, I can see it's a comprehensive refactoring that simplifies the import structure for simulation utilities. The main changes are:
-
Consolidates imports: Instead of importing specific submodules like
isaaclab.sim.utils.primsandisaaclab.sim.utils.stage, users can now just importisaaclab.sim as sim_utilsand access everything directly. -
Reorganizes utility modules: The code has been restructured with new files like
legacy.py,queries.py, and improved organization of stage and prim utilities. -
Adds comprehensive testing: New test files have been added to ensure the utilities work correctly.
-
Maintains backward compatibility: Functions that are deprecated now show proper warnings and point users to the new APIs.
-
Improves documentation: New API documentation structure is added for the utils module.
The changes look well-structured and follow good software engineering practices:
- Clear separation of concerns between different utility types
- Proper deprecation warnings for old APIs
- Comprehensive test coverage
- Consistent error handling and validation
- Good documentation and examples
The refactoring makes the API much cleaner - instead of needing to remember which specific submodule has which function, everything is accessible through the main sim_utils import. This is a significant usability improvement.
Is there something specific about this PR you'd like me to focus on or any concerns you have about the changes?
|
There are only three tests failing:
Since these two go beyond the scope of this MR, I think we can move forward with this merge, as there are other MRs waiting for it. |
Signed-off-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
81a4155 to
6d02f1c
Compare
# Description This MR removes simplifies usage of prim and stage utillities. * Removed unused utility functions (some of them are in legacy sub-module) * Deleted entire nucleus.py module (functions check_server() and get_assets_root_path() were not used anywhere) * Updated all spawner functions to use USD Stage API directly via stage.GetPrimAtPath() instead of wrapper utilities * Refactored create_prim() to use stage.DefinePrim() directly with better error handling * Added stage parameter to various functions for better control * Updated files across scripts, source code, and tests to use the unified sim_utils import pattern * Enhanced logger.py with comprehensive docstrings (non-functional change) Moving forward, it is recommended to use: ```python import isaaclab.sim as sim_utils ``` instead of: ```python import isaaclab.sim.utils.prims as prim_utils import isaaclab.sim.utils.stage as stage_utils ``` ## Type of change - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) - Breaking change (existing functionality will not work without user modification) - Documentation update ## Checklist - [ ] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [ ] I have run the [`pre-commit` checks](https://pre-commit.com/) 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 --------- Signed-off-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
Description
This MR removes simplifies usage of prim and stage utillities.
Moving forward, it is recommended to use:
instead of:
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there