-
Notifications
You must be signed in to change notification settings - Fork 28
Refactor robot models to use RobotModel with urdf parameter #639
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1833,7 +1833,7 @@ def self_collision_check(self): | |||||||||||||
| class RobotModel(CascadedLink): | ||||||||||||||
|
|
||||||||||||||
| def __init__(self, link_list=None, joint_list=None, | ||||||||||||||
| root_link=None): | ||||||||||||||
| root_link=None, urdf=None, include_mimic_joints=True): | ||||||||||||||
| link_list = link_list or [] | ||||||||||||||
| joint_list = joint_list or [] | ||||||||||||||
| super(RobotModel, self).__init__(link_list, joint_list) | ||||||||||||||
|
|
@@ -1853,6 +1853,39 @@ def __init__(self, link_list=None, joint_list=None, | |||||||||||||
| self._relevance_predicate_table = \ | ||||||||||||||
| self._compute_relevance_predicate_table() | ||||||||||||||
|
|
||||||||||||||
| if urdf is not None: | ||||||||||||||
| self._load_from_urdf(urdf, include_mimic_joints=include_mimic_joints) | ||||||||||||||
|
|
||||||||||||||
| 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. | ||||||||||||||
|
|
||||||||||||||
| Parameters | ||||||||||||||
| ---------- | ||||||||||||||
| urdf_input : str | ||||||||||||||
| Either the URDF model description as a string, or the path to a | ||||||||||||||
| URDF file. | ||||||||||||||
| include_mimic_joints : bool, optional | ||||||||||||||
|
||||||||||||||
| include_mimic_joints : bool, optional | |
| include_mimic_joints : bool, optional (default: True) |
Outdated
Copilot
AI
Jan 5, 2026
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.
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 |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,27 +5,27 @@ | |||||||||||||||
| from skrobot.data import fetch_urdfpath | ||||||||||||||||
| from skrobot.model import RobotModel | ||||||||||||||||
|
|
||||||||||||||||
| from .urdf import RobotModelFromURDF | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| class Fetch(RobotModelFromURDF): | ||||||||||||||||
| class Fetch(RobotModel): | ||||||||||||||||
| """Fetch Robot Model. | ||||||||||||||||
| http://docs.fetchrobotics.com/robot_hardware.html | ||||||||||||||||
| """ | ||||||||||||||||
|
|
||||||||||||||||
| def __init__(self, *args, **kwargs): | ||||||||||||||||
| super(Fetch, self).__init__(*args, **kwargs) | ||||||||||||||||
| 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() | ||||||||||||||||
|
||||||||||||||||
| 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() |
Copilot
AI
Jan 5, 2026
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.
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).
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,14 +5,18 @@ | |||||||||||||
| from skrobot.data import kuka_urdfpath | ||||||||||||||
| from skrobot.model import RobotModel | ||||||||||||||
|
|
||||||||||||||
| from .urdf import RobotModelFromURDF | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class Kuka(RobotModelFromURDF): | ||||||||||||||
| class Kuka(RobotModel): | ||||||||||||||
| """Kuka Robot Model.""" | ||||||||||||||
|
|
||||||||||||||
| def __init__(self, *args, **kwargs): | ||||||||||||||
| super(Kuka, self).__init__(*args, **kwargs) | ||||||||||||||
| 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() | ||||||||||||||
|
||||||||||||||
| 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()) | |
| ) |
Copilot
AI
Jan 5, 2026
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.
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).
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,10 +4,9 @@ | |||||||||||||||
| from ..coordinates import CascadedCoords | ||||||||||||||||
| from ..data import nextage_urdfpath | ||||||||||||||||
| from ..model import RobotModel | ||||||||||||||||
| from .urdf import RobotModelFromURDF | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| class Nextage(RobotModelFromURDF): | ||||||||||||||||
| class Nextage(RobotModel): | ||||||||||||||||
| """ | ||||||||||||||||
| - Nextage Open Official Information. | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -18,8 +17,14 @@ class Nextage(RobotModelFromURDF): | |||||||||||||||
| https://github.com/tork-a/rtmros_nextage/tree/indigo-devel/nextage_description/urdf | ||||||||||||||||
| """ | ||||||||||||||||
|
|
||||||||||||||||
| def __init__(self, *args, **kwargs): | ||||||||||||||||
| super(Nextage, self).__init__(*args, **kwargs) | ||||||||||||||||
| 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() | ||||||||||||||||
|
||||||||||||||||
| 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() |
Copilot
AI
Jan 5, 2026
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.
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).
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,18 +4,23 @@ | |||||||||||||
| from ..coordinates import CascadedCoords | ||||||||||||||
| from ..data import panda_urdfpath | ||||||||||||||
| from ..model import RobotModel | ||||||||||||||
| from .urdf import RobotModelFromURDF | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class Panda(RobotModelFromURDF): | ||||||||||||||
| class Panda(RobotModel): | ||||||||||||||
|
|
||||||||||||||
| """Panda Robot Model. | ||||||||||||||
|
|
||||||||||||||
| https://frankaemika.github.io/docs/control_parameters.html | ||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
| def __init__(self, *args, **kwargs): | ||||||||||||||
| super(Panda, self).__init__(*args, **kwargs) | ||||||||||||||
| 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() | ||||||||||||||
|
||||||||||||||
| 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()) | |
| ) |
Copilot
AI
Jan 5, 2026
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.
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).
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,17 +5,21 @@ | |||||||||||||
| from skrobot.data import pr2_urdfpath | ||||||||||||||
| from skrobot.model import RobotModel | ||||||||||||||
|
|
||||||||||||||
| from .urdf import RobotModelFromURDF | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class PR2(RobotModelFromURDF): | ||||||||||||||
| class PR2(RobotModel): | ||||||||||||||
|
|
||||||||||||||
| """PR2 Robot Model. | ||||||||||||||
|
|
||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
| def __init__(self, use_tight_joint_limit=True): | ||||||||||||||
| super(PR2, self).__init__() | ||||||||||||||
| 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() | ||||||||||||||
|
||||||||||||||
| 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()) | |
| ) |
Copilot
AI
Jan 5, 2026
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.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,19 +1,22 @@ | ||||||||||||||||||||
| import os.path as osp | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import skrobot | ||||||||||||||||||||
| from skrobot.coordinates import CascadedCoords | ||||||||||||||||||||
| from skrobot.data import r8_6_urdfpath | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| class R8_6(skrobot.model.RobotModel): | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def __init__(self, urdf_path=None, *args, **kwargs): | ||||||||||||||||||||
| super(R8_6, self).__init__(*args, **kwargs) | ||||||||||||||||||||
| urdf_path = r8_6_urdfpath() if urdf_path is None else urdf_path | ||||||||||||||||||||
| if osp.exists(urdf_path): | ||||||||||||||||||||
| self.load_urdf_file(open(urdf_path, 'r')) | ||||||||||||||||||||
| else: | ||||||||||||||||||||
| raise ValueError() | ||||||||||||||||||||
| 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() | ||||||||||||||||||||
|
||||||||||||||||||||
| 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() |
Outdated
Copilot
AI
Jan 5, 2026
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.
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.
Outdated
Copilot
AI
Jan 5, 2026
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.
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.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,21 +1,34 @@ | ||||||||
| import warnings | ||||||||
|
|
||||||||
| from ..model import RobotModel | ||||||||
|
|
||||||||
|
|
||||||||
| class RobotModelFromURDF(RobotModel): | ||||||||
| """Deprecated: Use RobotModel with urdf parameter instead. | ||||||||
| def __init__(self, urdf=None, urdf_file=None): | ||||||||
| super(RobotModelFromURDF, self).__init__() | ||||||||
| This class is kept for backward compatibility. | ||||||||
| Use ``RobotModel(urdf=urdf_input)`` or ``RobotModel.from_urdf(urdf_input)`` | ||||||||
| instead. | ||||||||
| """ | ||||||||
|
|
||||||||
| if urdf and urdf_file: | ||||||||
| 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" | ||||||||
|
||||||||
| "'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." |
Copilot
AI
Jan 5, 2026
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.
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.
Copilot
AI
Jan 5, 2026
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.
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.
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.
The
__init__method signature now includes new parametersurdfandinclude_mimic_jointsbut lacks documentation. Consider adding a docstring to document these parameters, especially since this is a public API method. The docstring should explain whaturdfaccepts (file path or URDF string), whatinclude_mimic_jointsdoes, and the defaults for all parameters.