Refactor robot models to use RobotModel with urdf parameter #639
Conversation
iory
commented
Jan 5, 2026
iory
commented
- Add urdf and include_mimic_joints parameters to RobotModel.init
- Add _load_from_urdf method to auto-detect file path vs string
- Simplify from_urdf to use RobotModel(urdf=...) internally
- Update all robot classes (Panda, Fetch, Kuka, PR2, Nextage, R8_6) to inherit directly from RobotModel
- Keep RobotModelFromURDF as deprecated alias for backward compatibility
- Add urdf and include_mimic_joints parameters to RobotModel.__init__ - Add _load_from_urdf method to auto-detect file path vs string - Simplify from_urdf to use RobotModel(urdf=...) internally - Update all robot classes (Panda, Fetch, Kuka, PR2, Nextage, R8_6) to inherit directly from RobotModel - Keep RobotModelFromURDF as deprecated alias for backward compatibility
There was a problem hiding this comment.
Pull request overview
This PR refactors robot model classes to use a unified URDF loading approach through the RobotModel base class, simplifying the inheritance hierarchy and improving consistency across all robot models.
Key Changes:
- Added
urdfandinclude_mimic_jointsparameters toRobotModel.__init__for direct URDF loading during instantiation - Introduced
_load_from_urdf()helper method for auto-detection of file paths vs URDF strings - Updated all robot classes (Panda, Fetch, Kuka, PR2, Nextage, R8_6) to inherit directly from
RobotModelinstead ofRobotModelFromURDF
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| skrobot/model/robot_model.py | Added urdf parameter to RobotModel constructor, implemented _load_from_urdf method, and simplified from_urdf to delegate to constructor |
| skrobot/models/urdf.py | Added deprecation warning and refactored RobotModelFromURDF to use RobotModel constructor internally |
| skrobot/models/panda.py | Changed inheritance to RobotModel, updated constructor with urdf/urdf_file parameters, removed default_urdf_path property |
| skrobot/models/fetch.py | Changed inheritance to RobotModel, updated constructor with urdf/urdf_file parameters, removed default_urdf_path property |
| skrobot/models/kuka.py | Changed inheritance to RobotModel, updated constructor with urdf/urdf_file parameters, removed default_urdf_path property |
| skrobot/models/pr2.py | Changed inheritance to RobotModel, updated constructor with urdf/urdf_file parameters and use_tight_joint_limit support |
| skrobot/models/nextage.py | Changed inheritance to RobotModel, updated constructor with urdf/urdf_file parameters, removed default_urdf_path property |
| skrobot/models/r8_6.py | Changed inheritance to RobotModel, updated constructor with urdf/urdf_file parameters, removed legacy file existence check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __init__(self, urdf=None, urdf_file=None, use_tight_joint_limit=True): | ||
| # For backward compatibility, support both urdf and urdf_file | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or pr2_urdfpath() | ||
| super(PR2, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The PR2 class still retains the default_urdf_path property (defined later in the file), while other robot models (Panda, Fetch, Kuka, Nextage) had this property removed in the refactoring. For consistency, the default_urdf_path property should be removed from PR2 as well, since the URDF path is now handled through the constructor parameters.
skrobot/models/r8_6.py
Outdated
| def __init__(self, urdf=None, urdf_file=None): | ||
| # For backward compatibility, support both urdf and urdf_file | ||
| # Also support legacy urdf_path argument via urdf_file | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or r8_6_urdfpath() |
There was a problem hiding this comment.
The comment mentions "Also support legacy urdf_path argument via urdf_file", but the function signature doesn't include a urdf_path parameter. If the intention is to support urdf_path for backward compatibility, it should be added to the signature (e.g., def __init__(self, urdf=None, urdf_file=None, urdf_path=None)) and handled appropriately, or the comment should be removed to avoid confusion.
| def __init__(self, urdf=None, urdf_file=None): | |
| # For backward compatibility, support both urdf and urdf_file | |
| # Also support legacy urdf_path argument via urdf_file | |
| if urdf is not None and urdf_file is not None: | |
| raise ValueError( | |
| "'urdf' and 'urdf_file' cannot be given at the same time" | |
| ) | |
| urdf_input = urdf or urdf_file or r8_6_urdfpath() | |
| def __init__(self, urdf=None, urdf_file=None, urdf_path=None): | |
| # For backward compatibility, support both urdf and urdf_file | |
| # Also support legacy urdf_path argument via urdf_file | |
| # Ensure that only one URDF source is specified | |
| specified_sources = [ | |
| src for src in (urdf, urdf_file, urdf_path) if src is not None | |
| ] | |
| if len(specified_sources) > 1: | |
| raise ValueError( | |
| "'urdf', 'urdf_file', and 'urdf_path' cannot be given at the same time" | |
| ) | |
| # urdf_path is treated as an alias for urdf_file for backward compatibility | |
| urdf_input = urdf or urdf_file or urdf_path or r8_6_urdfpath() |
- Remove unused default_urdf_path property from PR2 for consistency - Add urdf_path parameter to R8_6 for backward compatibility
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
skrobot/models/fetch.py
Outdated
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or fetch_urdfpath() |
There was a problem hiding this comment.
The use of or chaining to select the URDF input could cause unexpected behavior if an empty string is intentionally passed as urdf or urdf_file. The empty string is falsy in Python, so urdf or urdf_file or fetch_urdfpath() would skip over an empty string input and fall through to the default path. Consider using explicit None checks instead: urdf if urdf is not None else (urdf_file if urdf_file is not None else fetch_urdfpath()).
| urdf_input = urdf or urdf_file or fetch_urdfpath() | |
| if urdf is not None: | |
| urdf_input = urdf | |
| elif urdf_file is not None: | |
| urdf_input = urdf_file | |
| else: | |
| urdf_input = fetch_urdfpath() |
skrobot/models/kuka.py
Outdated
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or kuka_urdfpath() |
There was a problem hiding this comment.
The use of or chaining to select the URDF input could cause unexpected behavior if an empty string is intentionally passed as urdf or urdf_file. The empty string is falsy in Python, so urdf or urdf_file or kuka_urdfpath() would skip over an empty string input and fall through to the default path. Consider using explicit None checks instead: urdf if urdf is not None else (urdf_file if urdf_file is not None else kuka_urdfpath()).
| urdf_input = urdf or urdf_file or kuka_urdfpath() | |
| urdf_input = ( | |
| urdf | |
| if urdf is not None | |
| else (urdf_file if urdf_file is not None else kuka_urdfpath()) | |
| ) |
skrobot/models/r8_6.py
Outdated
| raise ValueError( | ||
| "Only one of 'urdf', 'urdf_file', or 'urdf_path' can be specified" | ||
| ) | ||
| urdf_input = urdf or urdf_file or urdf_path or r8_6_urdfpath() |
There was a problem hiding this comment.
The use of or chaining to select the URDF input could cause unexpected behavior if an empty string is intentionally passed. The empty string is falsy in Python, so this expression would skip over an empty string input and fall through to the next option or default path. Consider using explicit None checks instead: urdf if urdf is not None else (urdf_file if urdf_file is not None else (urdf_path if urdf_path is not None else r8_6_urdfpath())).
| urdf_input = urdf or urdf_file or urdf_path or r8_6_urdfpath() | |
| if urdf is not None: | |
| urdf_input = urdf | |
| elif urdf_file is not None: | |
| urdf_input = urdf_file | |
| elif urdf_path is not None: | |
| urdf_input = urdf_path | |
| else: | |
| urdf_input = r8_6_urdfpath() |
| if urdf is not None: | ||
| urdf_input = urdf | ||
| elif urdf_file is not None: | ||
| urdf_input = urdf_file | ||
| else: | ||
| self.load_urdf_file(file_obj=self.default_urdf_path) | ||
| urdf_input = self.default_urdf_path |
There was a problem hiding this comment.
The use of or chaining to select the URDF input could cause unexpected behavior if an empty string is intentionally passed. The empty string is falsy in Python, so this expression would skip over an empty string input and fall through to the next option or default path. Consider using explicit None checks with ternary operators to preserve the correct priority.
skrobot/model/robot_model.py
Outdated
| urdf_input : str | ||
| Either the URDF model description as a string, or the path to a | ||
| URDF file. | ||
| include_mimic_joints : bool, optional |
There was a problem hiding this comment.
The docstring states that include_mimic_joints is "optional" but should specify the default value explicitly in the parameter description. Based on the function signature, the default is True, but this should be documented as "bool, optional (default: True)" for clarity.
| include_mimic_joints : bool, optional | |
| include_mimic_joints : bool, optional (default: True) |
|
|
||
| def __init__(self, link_list=None, joint_list=None, | ||
| root_link=None): | ||
| root_link=None, urdf=None, include_mimic_joints=True): |
There was a problem hiding this comment.
The __init__ method signature now includes new parameters urdf and include_mimic_joints but lacks documentation. Consider adding a docstring to document these parameters, especially since this is a public API method. The docstring should explain what urdf accepts (file path or URDF string), what include_mimic_joints does, and the defaults for all parameters.
| root_link=None, urdf=None, include_mimic_joints=True): | |
| root_link=None, urdf=None, include_mimic_joints=True): | |
| """Initialize a RobotModel instance. | |
| Parameters | |
| ---------- | |
| link_list : sequence of Link, optional | |
| Initial list of links to register in the model. If ``None``, | |
| an empty list is used. | |
| joint_list : sequence of Joint, optional | |
| Initial list of joints to register in the model. If ``None``, | |
| an empty list is used. | |
| root_link : Link, optional | |
| Root link of the robot model. If ``None``, a default or | |
| inferred root link may be used depending on the implementation. | |
| urdf : str, optional | |
| URDF input used to build the robot model. This can be either a | |
| file system path to a URDF file or a string containing URDF XML. | |
| If ``None`` (the default), the model is not initialized from URDF. | |
| include_mimic_joints : bool, optional | |
| Whether to include mimic joints defined in the URDF when loading | |
| the model. Defaults to ``True``. | |
| """ |
skrobot/models/nextage.py
Outdated
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or nextage_urdfpath() |
There was a problem hiding this comment.
The use of or chaining to select the URDF input could cause unexpected behavior if an empty string is intentionally passed as urdf or urdf_file. The empty string is falsy in Python, so urdf or urdf_file or nextage_urdfpath() would skip over an empty string input and fall through to the default path. Consider using explicit None checks instead: urdf if urdf is not None else (urdf_file if urdf_file is not None else nextage_urdfpath()).
| urdf_input = urdf or urdf_file or nextage_urdfpath() | |
| if urdf is not None: | |
| urdf_input = urdf | |
| elif urdf_file is not None: | |
| urdf_input = urdf_file | |
| else: | |
| urdf_input = nextage_urdfpath() |
skrobot/models/panda.py
Outdated
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or panda_urdfpath() |
There was a problem hiding this comment.
The use of or chaining to select the URDF input could cause unexpected behavior if an empty string is intentionally passed as urdf or urdf_file. The empty string is falsy in Python, so urdf or urdf_file or panda_urdfpath() would skip over an empty string input and fall through to the default path. Consider using explicit None checks instead: urdf if urdf is not None else (urdf_file if urdf_file is not None else panda_urdfpath()).
| urdf_input = urdf or urdf_file or panda_urdfpath() | |
| urdf_input = ( | |
| urdf | |
| if urdf is not None | |
| else (urdf_file if urdf_file is not None else panda_urdfpath()) | |
| ) |
skrobot/models/pr2.py
Outdated
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or pr2_urdfpath() |
There was a problem hiding this comment.
The use of or chaining to select the URDF input could cause unexpected behavior if an empty string is intentionally passed as urdf or urdf_file. The empty string is falsy in Python, so urdf or urdf_file or pr2_urdfpath() would skip over an empty string input and fall through to the default path. Consider using explicit None checks instead: urdf if urdf is not None else (urdf_file if urdf_file is not None else pr2_urdfpath()).
| urdf_input = urdf or urdf_file or pr2_urdfpath() | |
| urdf_input = ( | |
| urdf | |
| if urdf is not None | |
| else (urdf_file if urdf_file is not None else pr2_urdfpath()) | |
| ) |
skrobot/model/robot_model.py
Outdated
| logger.error("Attempting to load as URDF string instead.") | ||
| self.load_urdf( | ||
| urdf_input, include_mimic_joints=include_mimic_joints) |
There was a problem hiding this comment.
The fallback behavior in the exception handler is problematic. If a file path is provided and the file exists but loading fails (e.g., due to malformed XML), the code attempts to interpret the file path string as URDF content. This will likely fail in a confusing way since a file path is not valid URDF XML. Consider either removing the fallback entirely and re-raising the exception, or adding logic to check if the exception is due to file I/O vs parsing errors.
| logger.error("Attempting to load as URDF string instead.") | |
| self.load_urdf( | |
| urdf_input, include_mimic_joints=include_mimic_joints) | |
| # Do not attempt to interpret a file path as URDF XML content. | |
| # Re-raise the original exception so callers see the real cause. | |
| raise |
- Add docstring to RobotModel.__init__ documenting new parameters - Add default value to include_mimic_joints docstring in _load_from_urdf - Remove problematic fallback that attempted to parse file path as URDF string
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
skrobot/models/urdf.py
Outdated
| ) | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" |
There was a problem hiding this comment.
The parameter checking logic uses urdf is not None and urdf_file is not None which correctly checks for both parameters being explicitly provided. However, the error message could be more helpful by indicating which parameters were actually provided.
| "'urdf' and 'urdf_file' cannot be given at the same time" | |
| "'urdf' and 'urdf_file' cannot be given at the same time; " | |
| "received both 'urdf' and 'urdf_file' arguments." |
skrobot/model/robot_model.py
Outdated
| URDF input used to build the robot model. This can be either a | ||
| file system path to a URDF file or a string containing URDF XML. | ||
| If None (the default), the model is not initialized from URDF. |
There was a problem hiding this comment.
The documentation for the urdf parameter should clarify the auto-detection behavior. Currently, it states "file system path to a URDF file or a string containing URDF XML" but doesn't explain that the method automatically detects which type it is using os.path.isfile(). This could be confusing if a URDF string happens to match an existing file path.
| URDF input used to build the robot model. This can be either a | |
| file system path to a URDF file or a string containing URDF XML. | |
| If None (the default), the model is not initialized from URDF. | |
| URDF input used to build the robot model. The value is interpreted | |
| automatically: if it is a string for which ``os.path.isfile`` | |
| returns ``True``, it is treated as a file system path to a URDF | |
| file; otherwise it is treated as a string containing URDF XML. | |
| Note that if you pass a URDF XML string that happens to match an | |
| existing file path, the file at that path will be loaded instead | |
| of the inline XML. If None (the default), the model is not | |
| initialized from URDF. |
| def __init__(self, urdf=None, urdf_file=None): | ||
| warnings.warn( | ||
| "RobotModelFromURDF is deprecated. " | ||
| "Use RobotModel(urdf=...) or RobotModel.from_urdf(...) instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2 | ||
| ) | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| if urdf: | ||
| self.load_urdf(urdf=urdf) | ||
| elif urdf_file: | ||
| self.load_urdf_file(file_obj=urdf_file) | ||
| if urdf is not None: | ||
| urdf_input = urdf | ||
| elif urdf_file is not None: | ||
| urdf_input = urdf_file | ||
| else: | ||
| self.load_urdf_file(file_obj=self.default_urdf_path) | ||
| urdf_input = self.default_urdf_path | ||
| super(RobotModelFromURDF, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The deprecation of RobotModelFromURDF lacks test coverage to verify that the DeprecationWarning is properly raised when the class is instantiated. Consider adding a test to ensure the deprecation warning is emitted and that the class still functions correctly for backward compatibility.
| def __init__(self, urdf=None, urdf_file=None): | ||
| # For backward compatibility, support both urdf and urdf_file | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or fetch_urdfpath() | ||
| super(Fetch, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The new urdf and urdf_file parameters added to the Fetch.__init__ method lack test coverage. Consider adding tests to verify that custom URDF inputs can be provided and that the parameter validation works correctly (e.g., testing that providing both parameters raises a ValueError).
| def __init__(self, urdf=None, urdf_file=None): | ||
| # For backward compatibility, support both urdf and urdf_file | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or panda_urdfpath() | ||
| super(Panda, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The new urdf and urdf_file parameters added to the Panda.__init__ method lack test coverage. Consider adding tests to verify that custom URDF inputs can be provided and that the parameter validation works correctly (e.g., testing that providing both parameters raises a ValueError).
skrobot/models/r8_6.py
Outdated
| def __init__(self, urdf=None, urdf_file=None, urdf_path=None): | ||
| # For backward compatibility, support urdf, urdf_file, and urdf_path | ||
| # urdf_path is treated as an alias for urdf_file for backward compatibility | ||
| specified_sources = [ | ||
| src for src in (urdf, urdf_file, urdf_path) if src is not None | ||
| ] | ||
| if len(specified_sources) > 1: | ||
| raise ValueError( | ||
| "Only one of 'urdf', 'urdf_file', or 'urdf_path' can be specified" | ||
| ) | ||
| urdf_input = urdf or urdf_file or urdf_path or r8_6_urdfpath() | ||
| super(R8_6, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The new urdf, urdf_file, and urdf_path parameters added to the R8_6.__init__ method lack test coverage. Consider adding tests to verify that custom URDF inputs can be provided, that the parameter validation works correctly (e.g., testing that providing multiple parameters raises a ValueError), and that backward compatibility with the urdf_path parameter is maintained.
| if os.path.isfile(urdf_input): | ||
| with open(urdf_input, 'r') as f: | ||
| self.load_urdf_file( | ||
| file_obj=f, include_mimic_joints=include_mimic_joints) | ||
| else: | ||
| self.load_urdf(urdf_input, | ||
| include_mimic_joints=include_mimic_joints) |
There was a problem hiding this comment.
The _load_from_urdf method uses os.path.isfile() to detect whether the input is a file path or URDF string. This approach has a potential edge case: if a URDF XML string happens to start with a valid file path that exists on the system, it will be incorrectly treated as a file. Consider checking for XML content (e.g., starts with '<?xml' or '<robot') before falling back to file path detection.
| if os.path.isfile(urdf_input): | |
| with open(urdf_input, 'r') as f: | |
| self.load_urdf_file( | |
| file_obj=f, include_mimic_joints=include_mimic_joints) | |
| else: | |
| self.load_urdf(urdf_input, | |
| include_mimic_joints=include_mimic_joints) | |
| # First, check if the input looks like URDF/XML content. | |
| if isinstance(urdf_input, six.string_types): | |
| stripped = urdf_input.lstrip() | |
| if stripped.startswith('<?xml') or stripped.startswith('<robot'): | |
| self.load_urdf( | |
| urdf_input, include_mimic_joints=include_mimic_joints | |
| ) | |
| return | |
| # If it doesn't look like XML, fall back to treating it as a file path | |
| # when possible; otherwise, pass it to load_urdf as before. | |
| if isinstance(urdf_input, six.string_types) and os.path.isfile(urdf_input): | |
| with open(urdf_input, 'r') as f: | |
| self.load_urdf_file( | |
| file_obj=f, include_mimic_joints=include_mimic_joints | |
| ) | |
| else: | |
| self.load_urdf( | |
| urdf_input, include_mimic_joints=include_mimic_joints | |
| ) |
skrobot/models/r8_6.py
Outdated
| def __init__(self, urdf=None, urdf_file=None, urdf_path=None): | ||
| # For backward compatibility, support urdf, urdf_file, and urdf_path | ||
| # urdf_path is treated as an alias for urdf_file for backward compatibility | ||
| specified_sources = [ | ||
| src for src in (urdf, urdf_file, urdf_path) if src is not None | ||
| ] | ||
| if len(specified_sources) > 1: | ||
| raise ValueError( | ||
| "Only one of 'urdf', 'urdf_file', or 'urdf_path' can be specified" | ||
| ) | ||
| urdf_input = urdf or urdf_file or urdf_path or r8_6_urdfpath() |
There was a problem hiding this comment.
The R8_6 class accepts three different parameter names (urdf, urdf_file, urdf_path) while other robot models (Panda, Fetch, Kuka, PR2, Nextage) only accept two (urdf, urdf_file). This inconsistency in the API design could confuse users. Consider whether urdf_path is necessary for backward compatibility or if it should be standardized across all models.
| def __init__(self, urdf=None, urdf_file=None): | ||
| # For backward compatibility, support both urdf and urdf_file | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or kuka_urdfpath() | ||
| super(Kuka, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The new urdf and urdf_file parameters added to the Kuka.__init__ method lack test coverage. Consider adding tests to verify that custom URDF inputs can be provided and that the parameter validation works correctly (e.g., testing that providing both parameters raises a ValueError).
| def __init__(self, urdf=None, urdf_file=None): | ||
| # For backward compatibility, support both urdf and urdf_file | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| urdf_input = urdf or urdf_file or nextage_urdfpath() | ||
| super(Nextage, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The new urdf and urdf_file parameters added to the Nextage.__init__ method lack test coverage. Consider adding tests to verify that custom URDF inputs can be provided and that the parameter validation works correctly (e.g., testing that providing both parameters raises a ValueError).
- Use explicit None checks instead of or-chaining for URDF input selection - Improve error message specificity in RobotModelFromURDF - Update docstring to explain auto-detection behavior (XML vs file path) - Add XML content detection (<?xml or <robot prefix) before file path check - Standardize R8_6 API to match other robots (remove urdf_path parameter) - Add test coverage for new URDF parameters and deprecation warning
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def __init__(self, urdf=None, urdf_file=None): | ||
| # For backward compatibility, support both urdf and urdf_file | ||
| if urdf is not None and urdf_file is not None: | ||
| raise ValueError( | ||
| "'urdf' and 'urdf_file' cannot be given at the same time" | ||
| ) | ||
| if urdf is not None: | ||
| urdf_input = urdf | ||
| elif urdf_file is not None: | ||
| urdf_input = urdf_file | ||
| else: | ||
| urdf_input = panda_urdfpath() | ||
| super(Panda, self).__init__(urdf=urdf_input) |
There was a problem hiding this comment.
The parameter handling logic for urdf and urdf_file is duplicated across all robot model classes (Panda, Fetch, Kuka, PR2, Nextage, R8_6). Consider extracting this into a helper method in the base RobotModel class or creating a decorator to reduce code duplication. This would make maintenance easier and ensure consistency across all robot models.
| def _load_from_urdf(self, urdf_input, include_mimic_joints=True): | ||
| """Load URDF from a string or a file path. | ||
| Automatically detects if the input is a URDF string or a file path. | ||
| If the input looks like XML content (starts with '<?xml' or '<robot'), | ||
| it is treated as URDF XML string. Otherwise, if the input is an | ||
| existing file path, it is loaded as a file. | ||
| Parameters | ||
| ---------- | ||
| urdf_input : str | ||
| Either the URDF model description as a string, or the path to a | ||
| URDF file. | ||
| include_mimic_joints : bool, optional (default: True) | ||
| If True, mimic joints are included in the resulting | ||
| `RobotModel`'s `joint_list`. | ||
| """ | ||
| # First, check if the input looks like URDF/XML content | ||
| if isinstance(urdf_input, str): | ||
| stripped = urdf_input.lstrip() | ||
| if stripped.startswith('<?xml') or stripped.startswith('<robot'): | ||
| self.load_urdf( | ||
| urdf_input, include_mimic_joints=include_mimic_joints) | ||
| return | ||
|
|
||
| # If it doesn't look like XML, check if it's a file path | ||
| if os.path.isfile(urdf_input): | ||
| with open(urdf_input, 'r') as f: | ||
| self.load_urdf_file( | ||
| file_obj=f, include_mimic_joints=include_mimic_joints) | ||
| else: | ||
| self.load_urdf(urdf_input, | ||
| include_mimic_joints=include_mimic_joints) |
There was a problem hiding this comment.
The _load_from_urdf method has incomplete input validation. If urdf_input is not a string (e.g., accidentally passed as a file object or other type), the code will skip the XML detection block (line 1901) and attempt to call os.path.isfile(urdf_input) on line 1909 with a non-string argument, which could cause a TypeError or unexpected behavior. Consider adding explicit type checking at the start of the method to provide a clearer error message if urdf_input is not a string.
| "'urdf' and 'urdf_file' cannot be given at the same time; " | ||
| "received both 'urdf' and 'urdf_file' arguments." |
There was a problem hiding this comment.
The error message for providing both urdf and urdf_file is inconsistent with other robot model classes. All other robot models (Panda, Fetch, Kuka, PR2, Nextage, R8_6) use the simpler message "'urdf' and 'urdf_file' cannot be given at the same time" without the additional clause. Consider using the same error message format for consistency across the codebase.
| "'urdf' and 'urdf_file' cannot be given at the same time; " | |
| "received both 'urdf' and 'urdf_file' arguments." | |
| "'urdf' and 'urdf_file' cannot be given at the same time" |
