Add multirotor/thruster actuator, multirotor asset and manager-based LMF2 drone task #3760
Conversation
… thrusters, new thruster actor class
…sation not visible
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review. The developer has made two targeted simplifications to the thruster actuator implementation: (1) removed the use_rps boolean configuration field from ThrusterCfg that previously allowed switching between RPM-based and force-based integration modes, and (2) removed all force-domain integration logic from the Thruster class itself. The change consolidates the thruster model to exclusively use RPM-based physics (thrust = k_f * ω²), eliminating the conditional branching and separate integration methods (compute_thrust_with_force_time_constant and compute_thrust_with_force_time_constant_rk4). This refactoring fits into the broader multirotor actuator system by providing a single, physically consistent thrust computation model that all multirotor configurations will use, reducing code complexity and maintenance burden while enforcing a standard integration approach across the codebase.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_cfg.py | 4/5 | Removed use_rps boolean field and simplified thrust_const_range docstring to reflect single integration mode |
| source/isaaclab/isaaclab/actuators/thruster.py | 3/5 | Eliminated force-domain integration methods and conditional logic, but introduced inconsistent reset initialization that may break thrust/RPM relationship |
Confidence score: 3/5
- This PR requires careful review due to a potential logic bug in the reset method that could cause physical inconsistencies
- Score lowered because the
reset_idxmethod now initializescurr_thrustwith random values instead of computing it fromthrust_const * rps², breaking the physical relationship established in__init__and potentially causing thrust/speed discontinuities when environments reset; also, a duplicate copyright header remains in thruster.py that should be removed - Pay close attention to source/isaaclab/isaaclab/actuators/thruster.py lines 184-193 where the reset logic diverges from the initialization pattern used in
__init__
2 files reviewed, 2 comments
| # Copyright (c) 2023, Autonomous Robots Lab, Norwegian University of Science and Technology. | ||
| # All rights reserved. | ||
| # | ||
| # SPDX-License-Identifier: BSD-3-Clause |
There was a problem hiding this comment.
syntax: duplicate copyright header - already present at lines 1-4
| desired_rpm = torch.sqrt(des_thrust / self.thrust_const) | ||
| rpm_error = desired_rpm - current_rpm | ||
| current_rpm += self.rk4_integration(rpm_error, mixing_factor) | ||
| return self.thrust_const * current_rpm**2 No newline at end of file |
There was a problem hiding this comment.
style: missing newline at end of file
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest change removes duplicate copyright headers from the thruster.py file. The file previously contained two copyright notices (lines 1-4 and lines 6-9), with the second being specific to the Norwegian University of Science and Technology. This change consolidates to a single Isaac Lab Project BSD-3-Clause license header, maintaining proper attribution while following the repository's standard licensing format. This is a purely administrative cleanup that doesn't affect the Thruster actuator class functionality or its integration with the multirotor asset system.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/thruster.py | 4/5 | Removed duplicate copyright header (lines 6-9) to consolidate licensing attribution |
Confidence score: 4/5
- This change is safe to merge with minimal risk as it only removes duplicate copyright headers
- Score reflects that this is a non-functional administrative change with no impact on code behavior, though the thruster.py file still contains previously-identified logic issues (division-by-zero risks in RPM calculations, slice handling in reset_idx) that remain unaddressed
- No files require special attention for this specific change, though the broader PR should still address the previously-flagged issues in thruster.py before final merge
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the latest change made to this PR. The developer has updated a single type annotation in the observation function generated_drone_commands from RigidObject to Multirotor (line 55 in source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/mdp/observations.py). This change aligns with the broader PR effort to introduce the new Multirotor articulation asset class, which extends Articulation with drone-specific features like thruster management. The type annotation update improves type safety and clarifies that this observation function expects a multirotor drone asset rather than a generic rigid object. This is consistent with previous changes in the file where other functions were similarly updated to use Multirotor.
However, one observation function (base_roll_pitch on line 35) still uses the Articulation type annotation, creating an inconsistency. While Multirotor extends Articulation and the code will function correctly, this inconsistency could confuse future maintainers about which asset type should be used for drone-specific observation functions in this file.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/mdp/observations.py | 4/5 | Updated type annotation from RigidObject to Multirotor in generated_drone_commands function for improved type safety |
Confidence score: 4/5
- This PR change is safe to merge with minimal risk as it only updates a type annotation without modifying any logic or behavior
- Score reflects that this is a simple, non-breaking change that improves type correctness, though the file still has an inconsistency with one remaining function using
Articulationinstead ofMultirotor - The
base_roll_pitchfunction on line 35 should also be updated to useMultirotorfor consistency with the rest of the drone-specific observation functions in this file
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The most recent change fixes a critical initialization order bug in the Thruster class constructor. Previously, the code attempted to compute self.curr_thrust using self.thrust_const before thrust_const was initialized, which would cause an AttributeError at runtime. The fix reorders the initialization so that thrust_const_r and thrust_const are created (lines 80-83) before being used to compute curr_thrust (lines 85-89). The reset_idx method was also updated (lines 183-185) to use the same physics-based initialization formula (thrust_const × rps²) ensuring consistency between reset and initial state. This change maintains the physical relationship between thrust, thrust constant, and rotor speed that was introduced in previous iterations, while fixing the broken constructor.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/thruster.py | 3/5 | Fixed initialization order bug by moving thrust_const initialization before curr_thrust computation, but division-by-zero risks remain in compute methods |
Confidence score: 3/5
- This change fixes a critical initialization bug but does not address remaining numerical stability issues (division by zero when
thrust_constcontains zeros) - Score reflects that the initialization order fix resolves the immediate runtime error, but deducted points for: (1) unaddressed division-by-zero vulnerabilities in lines 217-221 where sqrt(thrust/thrust_const) can produce NaN/inf, and (2) lack of validation that
thrust_constvalues are always positive - Focus attention on lines 217-221 and 246-250 in thruster.py where sqrt operations and divisions by
thrust_constneed epsilon protection or validation to prevent numerical errors during simulation
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest change updates the LMF2_model submodule pointer from commit 5f4ca0fd to 55e50b51. This submodule update references external drone model assets (URDF files, meshes, or other model definition files) maintained in a separate repository at ntnu-arl/LMF2_model. The change is part of the broader PR that introduces multirotor/thruster actuator support and a manager-based LMF2 drone task. Updating the submodule pointer is a standard Git operation to ensure the codebase references a specific version of external assets that are compatible with the new thruster actuator model, Multirotor articulation class, and manager-based task configurations introduced in this PR.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/LMF2_model |
4/5 | Submodule reference updated from commit 5f4ca0fd to 55e50b51 to point to a newer version of the LMF2 drone model assets |
Confidence score: 4/5
- This change is relatively safe to merge as it only updates a submodule pointer reference
- Score reflects that while the submodule update itself is low-risk, the actual changes within the submodule (between the two commits) are not visible in this diff and could potentially introduce issues if the new commit contains incompatible model changes or untested modifications to the LMF2 drone URDF/assets
- The
LMF2_modelsubmodule file requires attention to ensure the new commit (55e50b51) contains tested and compatible drone model changes that work correctly with the new thruster actuator and multirotor asset class introduced in this PR
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the submodule update change, not the entire PR. The change updates the Git submodule pointer for LMF2_model from commit 5e51b514 to e4947a55. This submodule contains the LMF2 drone model assets (URDF, meshes, configuration files) that are referenced by the new manager-based drone task introduced in the broader PR. The submodule is hosted in an external repository (ntnu-arl/LMF2_model.git) and is integrated into IsaacLab's task structure to provide the physical model definition for the multirotor asset class and thruster actuators. The submodule pattern allows the model to be maintained independently while being consumed by the task configurations, commands, observations, and rewards defined in the drone_ntnu task module.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/LMF2_model | 2/5 | Updates Git submodule pointer for LMF2 drone model assets from commit 5e51b514 to e4947a55 |
Confidence score: 2/5
- This submodule update has moderate risk because the commit changes in the external repository are not visible for review, making it impossible to verify compatibility with the task code
- Score reflects uncertainty about what changed between submodule commits and lack of visibility into breaking changes that could affect model structure, thruster naming conventions, link definitions, or inertial properties that the multirotor class depends on
- The LMF2_model submodule file requires special attention since the external repository commit history should be reviewed to understand what changed, and integration testing should verify the new model works correctly with ThrusterCfg, MultirotorCfg, and all task configurations before merging
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. This update refactors the drone task configuration to support an additional robot platform called ARL_ROBOT_1 alongside the existing LMF2 drone. The changes consist of renaming the LMF2 configuration directory and files to ARL_ROBOT_1, with corresponding updates to class names, experiment names, environment registration IDs, and asset references. All task logic, MDP configurations, RL hyperparameters, and agent architectures remain identical between the two platforms - only naming and path identifiers changed. The file structure follows the established pattern of robot-specific configuration directories (.../config/ARL_ROBOT_1/) with separate agent configs for each RL framework (rl_games, rsl_rl, skrl). This extends the multirotor abstraction introduced in the PR to support multiple drone platforms with platform-specific configurations while reusing the core task implementation.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/__init__.py |
4/5 | Registers gym environments for ARL_ROBOT_1 platform with environment IDs using underscore convention instead of dash-separated |
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/state_based_control_env_cfg.py |
2/5 | Renamed from LMF2 config; retains incorrect type annotation (MultirotorCfg instead of Articulation) and placeholder command ranges (all 0.0) |
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/empty_env_cfg.py |
5/5 | Straightforward robot asset swap from LMF2_CFG to ARL_ROBOT_1_CFG with updated class names |
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/agents/rsl_rl_ppo_cfg.py |
5/5 | Updated experiment name from "lmf2_state_based_control" to "arl_robot_1_state_based_control" |
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/agents/rl_games_rough_ppo_cfg.yaml |
5/5 | Updated experiment name on line 51 to match new robot platform |
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/agents/skrl_rough_ppo_cfg.yaml |
5/5 | Updated experiment directory name to "arl_robot_1_state_based_control" |
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/agents/__init__.py |
4/5 | Empty package marker file with only copyright header |
source/isaaclab_assets/isaaclab_assets/robots/arl_robot_1.py |
3/5 | Renamed all LMF2 identifiers to ARL_ROBOT_1; USD path references old LMF2_model directory causing path inconsistency |
Confidence score: 3/5
- This refactoring duplicates the LMF2 configuration but propagates existing issues (incorrect type annotations, placeholder command ranges) to the new ARL_ROBOT_1 platform without addressing them.
- Score reflects successful pattern duplication for multi-platform support, but points deducted for: (1) the type annotation bug on line 36 of
state_based_control_env_cfg.py(should beArticulationnotMultirotorCfg), (2) zero command ranges that make the task non-functional without further configuration, and (3) USD path inconsistency inarl_robot_1.pyreferencing old directory structure. - Pay close attention to
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/state_based_control/config/ARL_ROBOT_1/state_based_control_env_cfg.py(line 36 type annotation, lines 62-69 command ranges) andsource/isaaclab_assets/isaaclab_assets/robots/arl_robot_1.py(line 38 USD path) to ensure the ARL_ROBOT_1 platform is properly configured before use.
8 files reviewed, 5 comments
|
|
||
|
|
||
| @configclass | ||
| class ARL_ROBOT_1_EmptyEnvCfg(StateBasedControlEmptyEnvCfg): |
There was a problem hiding this comment.
style: class name follows inconsistent naming pattern - uses underscores (ARL_ROBOT_1_EmptyEnvCfg) instead of PascalCase (ArlRobot1EmptyEnvCfg). Check if this matches the pattern used elsewhere in the codebase for robot-specific configs. Is the ALL_CAPS naming (ARL_ROBOT_1) a required convention for robot configs in this codebase, or should it follow standard PascalCase?
| """Configuration for the terrain scene with a flying robot.""" | ||
|
|
||
| # robots | ||
| robot: MultirotorCfg = MISSING |
There was a problem hiding this comment.
syntax: type annotation should be Articulation not MultirotorCfg - MultirotorCfg is the configuration class, but the scene attribute should hold the asset instance type
| ## | ||
|
|
||
| gym.register( | ||
| id="Isaac-StateBasedControl-ARL_ROBOT_1-v0", |
There was a problem hiding this comment.
style: Environment ID contains inconsistent naming - uses underscores (ARL_ROBOT_1) while other Isaac Lab environment IDs use dashes (e.g., Isaac-Velocity-Flat-Anymal-D-v0). Consider using Isaac-StateBasedControl-ARL-ROBOT-1-v0 for consistency with the broader Isaac Lab ecosystem.
|
|
||
| The following configuration parameters are available: | ||
|
|
||
| * :obj:`ARL_ROBOT_1_CFG`: The ARL_Robot_1 with (TODO add motor propeller combination) |
There was a problem hiding this comment.
style: incomplete TODO left in docstring - should specify motor/propeller combination for ARL_ROBOT_1 or remove
| ARL_ROBOT_1_CFG = MultirotorCfg( | ||
| spawn=sim_utils.UsdFileCfg( | ||
| usd_path=( | ||
| f"{ISAACLAB_EXT_DIR}/../isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/LMF2_model/arl_drone_1/arl_drone_1.usd" |
There was a problem hiding this comment.
logic: USD path references old lmf2 directory but should point to arl_drone_1 directory to match the renamed asset. The path says arl_drone_1.usd but the directory is still LMF2_model which may cause confusion. Should the parent directory also be renamed from LMF2_model to something like drone_models or arl_model? Was renaming the parent directory from LMF2_model to match the new asset name intentional, or should it be updated as well?
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes include two specific updates: (1) renaming the asset path in arl_robot_1.py from arl_drone_1/arl_drone_1.usd to arl_robot_1/arl_robot_1.usd to align the filesystem structure with Python naming conventions, and (2) updating the Git submodule pointer for LMF2_model from commit e4947a5 to 963153f. These changes are part of asset organization cleanup to ensure consistency between file paths, Python identifiers, and the underlying asset repository. The submodule update brings in newer versions of the LMF2 drone model assets (likely URDF files and meshes) required for the multirotor/thruster functionality introduced in this PR. Both changes are straightforward refactoring operations with minimal risk - the path rename is a simple string update maintaining the same relative structure, and the submodule update follows standard Git workflow for external dependencies.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab_assets/isaaclab_assets/robots/arl_robot_1.py | 5/5 | Updated USD asset path from arl_drone_1/arl_drone_1.usd to arl_robot_1/arl_robot_1.usd for naming consistency |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/LMF2_model | 5/5 | Updated Git submodule reference from commit e4947a5 to 963153f to include updated drone model assets |
Confidence score: 5/5
- This PR is safe to merge with minimal risk - the changes are isolated refactoring operations
- Score reflects simple, well-scoped changes: a single-line path rename and a standard submodule update with no logic modifications or potential side effects
- No files require special attention - both changes are straightforward and unlikely to introduce bugs
2 files reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. This update modifies the LMF2_model submodule reference, advancing the commit pointer from 963153f to 7492ed6. The submodule update ensures the drone task implementation references the latest version of the LMF2 model assets (URDF, meshes, collision geometry, visual assets) that are compatible with the new multirotor/thruster actuator implementation introduced in this PR. This is a standard Git submodule version bump that synchronizes the external model repository with the task code, ensuring the asset definitions match the Isaac Lab configuration for the manager-based LMF2 drone task.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/LMF2_model | 5/5 | Submodule commit pointer updated from 963153f to 7492ed6 to incorporate latest LMF2 drone model assets |
Confidence score: 5/5
- This submodule update is safe to merge with minimal risk
- Score reflects that this is a straightforward submodule version bump with no code changes, typical for asset synchronization
- No files require special attention; this is a standard Git submodule pointer update
1 file reviewed, no comments
|
@AntoineRichard @ooctipus would be great if you could give the PR another pass through and give your okay |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest change updates the Git submodule configuration, renaming the drone task directory structure from drone_ntnu/LMF2_model to drone_arl/robot_model. This represents a significant organizational shift - from location-based naming (NTNU = Norwegian University of Science and Technology) to organization-based naming (ARL = Autonomous Robotics Lab), and from a model-specific repository (LMF2_model) to a more generic one (robot_model). The restructuring prepares the codebase to support multiple robot models beyond just LMF2, making it more scalable and less tightly coupled to a single drone platform. This change integrates with the broader multirotor/thruster infrastructure introduced earlier in this PR, providing a more maintainable foundation for future drone models and control improvements.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| .gitmodules | 1/5 | Renames drone task submodule from drone_ntnu/LMF2_model to drone_arl/robot_model - requires verification that all imports and asset paths throughout the codebase are updated to match this directory restructuring |
Confidence score: 1/5
- This change is high-risk and requires careful verification before merging, as submodule path changes can break imports, asset loading, and task references across the entire codebase
- Score reflects the critical nature of directory restructuring: previous reviews identified numerous references to
drone_ntnupaths in Python imports, URDF asset paths, configuration files, and documentation that must all be consistently updated todrone_arl- any missed reference will cause runtime failures - The .gitmodules file requires immediate attention to verify consistency with all import statements in task files (especially in
source/isaaclab_tasks/isaaclab_tasks/manager_based/), asset path references in USD/URDF files, and documentation links
1 file reviewed, 1 comment
| [submodule "source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/robot_model"] | ||
| path = source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/robot_model | ||
| url = https://github.com/ntnu-arl/robot_model.git |
There was a problem hiding this comment.
logic: submodule path changed from drone_ntnu to drone_arl and from LMF2_model to robot_model, but other files in this PR still reference drone_ntnu directory structure (e.g., previous comments mention isaaclab_tasks/manager_based/drone_ntnu/). This mismatch will break imports and asset loading unless all references are updated consistently throughout the codebase. Have all Python imports and asset path references been updated to use drone_arl/robot_model instead of drone_ntnu/LMF2_model? Are there any remaining references to the old drone_ntnu/LMF2_model paths in Python files, configs, or documentation that weren't updated in this PR?
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, focusing on the renaming refactoring from drone_ntnu to drone_arl throughout the manager-based drone task infrastructure. The changes systematically rename directories, update import paths, standardize naming conventions (from uppercase ARL_ROBOT_1 to lowercase arl_robot_1), and update environment IDs to use kebab-case. The refactoring also includes renaming agent configuration files (removing "rough" qualifiers), updating Git submodule references, and consolidating the robot model structure. These changes integrate with the broader PR's introduction of multirotor/thruster support by ensuring the task infrastructure uses consistent naming that aligns with the new asset organization. The rename affects MDP components (commands, observations, rewards), state-based control configurations, agent configs (PPO variants for rl_games, rsl_rl, skrl), and asset path references.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| .gitmodules | 5/5 | Reformatted with consistent indentation (spaces vs tabs), no functional changes |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/commands/commands_cfg.py | 5/5 | Renamed from drone_ntnu, defines DroneUniformPoseCommandCfg with no code changes |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/commands/init.py | 5/5 | Renamed from drone_ntnu, imports command classes with no logic changes |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/init.py | 5/5 | Minimal package marker file renamed from drone_ntnu, contains only boilerplate |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/agents/init.py | 5/5 | Empty module reduced to package marker, agent configs now standalone files |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/state_based_control_env_cfg.py | 5/5 | Updated import path from drone_ntnu.mdp to drone_arl.mdp, no logic changes |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/LMF2_model | 2/5 | Removed Git submodule entry, requires verification that all references updated |
| source/isaaclab_assets/isaaclab_assets/robots/arl_robot_1.py | 4/5 | Updated USD asset path and module docstring from LMF2/NTNU to ARL naming |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/empty_env_cfg.py | 5/5 | Renamed classes from robot-prefixed to generic names, updated imports |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/agents/rl_games_ppo_cfg.yaml | 3/5 | Renamed from rl_games_rough_ppo_cfg.yaml, num_envs (8192) conflicts with num_actors (1024) |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/init.py | 5/5 | Updated module docstring from NTNU to ARL, pure documentation change |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/init.py | 5/5 | Renamed from drone_ntnu, updated docstring to reflect new directory name |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/init.py | 5/5 | Updated docstring from NTNU to ARL, re-exports unchanged |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/agents/skrl_ppo_cfg.yaml | 4/5 | Renamed from skrl_rough_ppo_cfg.yaml, low timesteps (36000) may be insufficient |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/init.py | 2/5 | Updated environment IDs to kebab-case, simplified class names, rl_games config filename changed |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/observations.py | 3/5 | Renamed from drone_ntnu, inconsistent type annotations (Articulation vs Multirotor), potential AttributeError with root_link_quat_w |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/agents/rsl_rl_ppo_cfg.py | 4/5 | Renamed from ARL_ROBOT_1 to arl_robot_1, defines PPO config with short rollout horizon (24 steps) |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/commands/drone_pose_command.py | 2/5 | Renamed from drone_ntnu, frame mixing in pose error calculation (world + base frames), potential AttributeError with root_link_quat_w |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/rewards.py | 4/5 | Renamed from drone_ntnu, incorrect type annotations (RigidObject should be Articulation), hardcoded zero target yaw |
Confidence score: 3/5
- This refactoring is moderately safe but requires careful verification that all cross-references have been updated consistently throughout the codebase to avoid import failures and asset loading errors.
- Score reduced from 5/5 to 3/5 due to: (1) critical frame mixing issue in drone_pose_command.py line 46 that will produce incorrect pose error calculations, (2) unverified attribute access (
root_link_quat_w,body_link_pose_w) that may cause AttributeErrors at runtime, (3) potential import failures if any references to olddrone_ntnupaths remain in untouched files, (4) num_envs/num_actors mismatch in rl_games config (8192 vs 1024) that could cause training issues, and (5) removed Git submodule without confirmation that all dependent code was updated to reference the new location. - Pay close attention to source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/commands/drone_pose_command.py (frame mixing bug), source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/init.py (verify rl_games_ppo_cfg.yaml exists and num_envs/num_actors are intentionally mismatched), and source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_ntnu/LMF2_model (verify all references to old submodule path have been updated).
19 files reviewed, 8 comments
| ARL_ROBOT_1_CFG = MultirotorCfg( | ||
| spawn=sim_utils.UsdFileCfg( | ||
| usd_path=( | ||
| f"{ISAACLAB_EXT_DIR}/../isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/robot_model/arl_robot_1/arl_robot_1.usd" |
There was a problem hiding this comment.
style: path uses relative traversal ../ which assumes specific directory layout between isaaclab core and isaaclab_tasks. If these packages are installed separately or directory structure changes, this will break. Consider using a package-relative path resolver or environment variable
| entry_point="isaaclab.envs:ManagerBasedRLEnv", | ||
| disable_env_checker=True, | ||
| kwargs={ | ||
| "env_cfg_entry_point": f"{__name__}.empty_env_cfg:EmptyEnvCfg", |
There was a problem hiding this comment.
logic: env_cfg_entry_point references EmptyEnvCfg which doesn't match the previous class name pattern. Based on previous comments on other files, this should be ArlRobot1EmptyEnvCfg (or similar) to maintain consistency with robot-specific naming. Does empty_env_cfg.py export EmptyEnvCfg or was this renamed from the robot-specific class name? Does empty_env_cfg.py actually export a class named EmptyEnvCfg, or should this reference a robot-specific class name like ArlRobot1EmptyEnvCfg?
| entry_point="isaaclab.envs:ManagerBasedRLEnv", | ||
| disable_env_checker=True, | ||
| kwargs={ | ||
| "env_cfg_entry_point": f"{__name__}.empty_env_cfg:EmptyEnvCfg_PLAY", |
There was a problem hiding this comment.
logic: same issue - EmptyEnvCfg_PLAY should match the actual class name in empty_env_cfg.py. Based on previous comments, the Play variant should follow PascalCase (e.g., EmptyEnvCfgPlay or ArlRobot1EmptyEnvCfgPlay) rather than using underscore suffix
| disable_env_checker=True, | ||
| kwargs={ | ||
| "env_cfg_entry_point": f"{__name__}.empty_env_cfg:EmptyEnvCfg", | ||
| "rl_games_cfg_entry_point": f"{agents.__name__}:rl_games_ppo_cfg.yaml", |
There was a problem hiding this comment.
logic: filename changed from rl_games_rough_ppo_cfg.yaml to rl_games_ppo_cfg.yaml - verify this file exists in the agents directory. If the file was renamed to remove 'rough', ensure the file actually exists with the new name. Was rl_games_rough_ppo_cfg.yaml renamed to rl_games_ppo_cfg.yaml, or should this still reference the 'rough' variant?
| """ | ||
|
|
||
|
|
||
| def base_roll_pitch(env: ManagerBasedEnv, asset_cfg: SceneEntityCfg = SceneEntityCfg("robot")) -> torch.Tensor: |
There was a problem hiding this comment.
syntax: type annotation should be Multirotor not Articulation - this is a drone observation function that operates on multirotor assets (see line 55 which correctly uses Multirotor type)
|
|
||
| @configclass | ||
| class StateBasedControlEnvPPORunnerCfg(RslRlOnPolicyRunnerCfg): | ||
| num_steps_per_env = 24 |
There was a problem hiding this comment.
style: With only 24 steps per rollout, 4 mini_batches, and 4 learning epochs, you get 16 gradient updates per collection cycle (24*num_envs / minibatch_size * 4 epochs). For continuous control tasks like drone pose tracking, consider increasing to 32-48steps or more mini_epochs to improve sample efficiency. Was the 24-step rollout horizon empirically validated for the ARL_ROBOT_1 drone dynamics, or inherited from another config? Drone control tasks often benefit from longer horizons to learn trajectory tracking.
| self.pose_command_b[:, :3] + self._env.scene.env_origins, self.pose_command_b[:, 3:] | ||
| ) | ||
| # -- current body pose | ||
| body_link_pose_w = self.robot.data.body_link_pose_w[:, self.body_idx] |
There was a problem hiding this comment.
syntax: body_link_pose_w attribute may not exist. Standard Isaac Lab articulations use body_pose_w. Verify this attribute exists in the Multirotor asset or change to body_pose_w. Does the Multirotor asset define a custom body_link_pose_w attribute, or should this use the standard body_pose_w?
| """ | ||
| Drone-navigation rewards. | ||
| """ |
There was a problem hiding this comment.
style: docstring says 'Drone-navigation rewards' but this file contains general drone control rewards (distance, velocity, orientation). Consider renaming to 'Drone control rewards' or 'Drone-navigation and control rewards' for accuracy
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest update restructures the drone asset organization by converting from the drone_ntnu/LMF2_model structure to a Git submodule at drone_arl/robot_model. Specifically, the .gitmodules file was updated to correct indentation (spaces to tabs) and define the submodule configuration pointing to https://github.com/ntnu-arl/robot_model.git. The submodule itself was added at commit d5a25c68. This change aligns with the broader multirotor infrastructure being introduced in the PR - moving robot model assets (URDF, meshes, USD files) to a separate repository for better maintainability and sharing across projects. The renaming from drone_ntnu to drone_arl likely reflects organizational ownership changes or a generalization beyond the specific NTNU LMF2 platform.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
.gitmodules |
5/5 | Corrected indentation from spaces to tabs and formalized submodule configuration for drone assets |
source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/robot_model |
5/5 | Added Git submodule reference to external robot model repository at commit d5a25c68 |
Confidence score: 3/5
- This review focuses on a structural reorganization that introduces a Git submodule for drone assets, which is generally low-risk from a code execution perspective
- Score lowered due to critical path inconsistency issues: previous reviews identified multiple references to the old
drone_ntnu/LMF2_modelpath structure throughout Python imports, configs, and asset loading code that may not have been updated to match the newdrone_arl/robot_modelstructure, potentially causing runtime import failures and broken asset references - Pay close attention to verifying that ALL Python imports, configuration files, asset path references, and documentation have been updated from the old
drone_ntnunaming todrone_arlthroughout the entire codebase - grep the repository for any remainingdrone_ntnuorLMF2_modelreferences that would break with this submodule restructuring
2 files reviewed, no comments
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes consist primarily of minor formatting and documentation improvements across four files. The most significant change is an import statement simplification in empty_env_cfg.py (removing unnecessary parentheses). The thruster.py file shows a confidence score of 0, indicating it may have been added or significantly modified but details are not available in this review snapshot. Documentation was enhanced in rewards.py with comprehensive docstrings for four reward functions (distance_to_goal_exp, ang_vel_xyz_exp, lin_vel_xyz_exp, yaw_aligned), explaining exponential-kernel reward formulations, parameter meanings, and expected tensor shapes. The arl_robot_1.py file underwent purely cosmetic formatting, consolidating a multi-line f-string path assignment into a single line. These changes integrate with the broader PR goal of providing a reusable multirotor abstraction by improving code readability and API documentation for the manager-based drone task framework.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/empty_env_cfg.py | 5/5 | Import statement simplified by removing unnecessary parentheses - purely stylistic cleanup |
| source/isaaclab/isaaclab/actuators/thruster.py | 0/5 | File details unavailable - requires manual review |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/mdp/rewards.py | 5/5 | Added comprehensive docstrings to four reward functions without logic changes |
| source/isaaclab_assets/isaaclab_assets/robots/arl_robot_1.py | 5/5 | Consolidated multi-line f-string path assignment into single line - formatting only |
Confidence score: 4/5
- This review subset is mostly safe to merge with minimal risk, though one file (thruster.py) could not be evaluated
- Score reflects that three of four files contain only documentation and formatting improvements with no logic changes, but thruster.py shows confidence 0/5 and requires manual inspection before merging
- Pay close attention to source/isaaclab/isaaclab/actuators/thruster.py which could not be evaluated in this review and may contain substantive changes that need verification
4 files reviewed, 1 comment
|
|
||
| ARL_ROBOT_1_CFG = MultirotorCfg( | ||
| spawn=sim_utils.UsdFileCfg( | ||
| usd_path=f"{ISAACLAB_EXT_DIR}/../isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/robot_model/arl_robot_1/arl_robot_1.usd", |
There was a problem hiding this comment.
style: formatting change reduces readability - the multi-line f-string was more readable than the single long line (98 chars). Consider keeping the wrapped format for paths >80 chars
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes address the renaming from LMF2-specific naming to generic ARL robot naming throughout the drone task implementation. Specifically, the submodule reference has been updated from drone_ntnu/LMF2_model to drone_arl/robot_model, reflecting the transition to a more generic naming convention for ARL robots. This submodule update advances the commit pointer to include the renamed asset files (arl_robot_1 instead of lmf2), ensuring the manager-based drone task references the correct URDF/USD assets. Additionally, the skrl agent configuration entry point was updated from skrl_rough_ppo_cfg.yaml to skrl_ppo_cfg.yaml, removing the "rough" qualifier and standardizing the configuration file naming across agent configs. These changes integrate with the broader refactoring introduced in this PR, which adds multirotor/thruster actuator support and a manager-based ARL robot task with new asset classes, thrust actions, and MDP configurations.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/robot_model | 5/5 | Git submodule pointer updated to newer commit (d5a25c6→f53d8e8) for renamed robot_model assets |
| source/isaaclab_tasks/isaaclab_tasks/manager_based/drone_arl/state_based_control/config/arl_robot_1/__init__.py | 4/5 | Updated skrl config entry point from skrl_rough_ppo_cfg.yaml to skrl_ppo_cfg.yaml for standardization |
Confidence score: 4/5
- This review covers minor refactoring changes (submodule update and config file renaming) that are low-risk and align with the broader transition to generic ARL robot naming.
- Score reflects that these are straightforward naming/reference updates, but deducted one point because the submodule commit hash cannot be independently verified without access to the external repository, and there's a dependency on the external repository containing the correctly renamed assets.
- Pay attention to verifying that the external
robot_modelsubmodule at commit f53d8e8 actually contains the renamedarl_robot_1assets and that theskrl_ppo_cfg.yamlfile exists in the agents directory.
2 files reviewed, no comments
6 participants
Description
This PR introduces multirotor and thruster support and adds a manager-based example/task for the LMF2 drone. The change contains a new low-level thruster actuator model, a new
Multirotorarticulation asset class + configs, new thrust actions, and a manager-based drone task (LMF2) with MDP configs and RL agent configs.Motivation and context
Type of change
Files changed (high-level summary)
Dependencies
.zip) — ensure Git LFS is used if you want large assets tracked by LFS.Checklist (status)
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there