-
Notifications
You must be signed in to change notification settings - Fork 81
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
Rough support for sdformat robot descriptions #55
Conversation
Signed-off-by: Shane Loretz<sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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.
I just took a quick look here, pointed out a couple of potential issues I saw.
if robot.getElementsByTagName('sdf'): | ||
self.init_sdf(robot) | ||
elif robot.getElementsByTagName('COLLADA'): |
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.
I know you are just extending this, but this check seems too broad. That is, if I understand https://docs.python.org/3.6/library/xml.dom.html#xml.dom.Document.getElementsByTagName correctly, this will look in the entire XML tree for tags of this name. I think this has some (limited) potential to produce false positives. If someone was using COLLADA, but named one of their joints 'sdf', I think we would attempt to parse it as SDF. Is there something else we can check in the document that is more specific? The root element name, for instance?
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.
Yeah, it seems too broad to me too. I think checking the root element would be a better solution. The C++ parser is handling it a little different by scoring based on the location of the first tag text, like <COLLADA
https://github.com/ros/collada_urdf/pull/39/files#diff-f13e149a001d68baae11ac5da7f08dc5R56-R59
but that would be fooled by the text being in a comment in the xml.
<!-- this is not a <COLLADA> file -->
<sdf>
...
It would be nice if joint_state_publisher used the C++ parser so this would only have to be fixed in one place.
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.
It would be nice if joint_state_publisher used the C++ parser so this would only have to be fixed in one place.
While I somewhat agree, I also don't relish the thought of putting a CPython binding on top of the URDF ecosystem. At the very least, it means we don't have a universal wheel for Python anymore.
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.
I also don't relish the thought of putting a CPython binding on top of the URDF ecosystem. At the very least, it means we don't have a universal wheel for Python anymore.
Where were you thinking about the CPython binding living? urdf_parser_py
? I was thinking of joint_state_publisher
having a CPython extension that calls the urdf C++ api to parse the robot description and return an object with the joint info joint_state_publisher needs.
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.
Where were you thinking about the CPython binding living?
urdf_parser_py
? I was thinking ofjoint_state_publisher
having a CPython extension that calls the urdf C++ api to parse the robot description and return an object with the joint info joint_state_publisher needs.
I didn't have a specific place in mind.
But even in the case you are suggesting, its not clear to me that providing a CPython extension for joint_state_publisher is a win. It seems to me the pros and cons of switching to a CPython extension:
Pros:
- Reduce duplication in parsing the XML.
- Get support for new formats for "free" once they are in urdf_parser
Cons:
- Replace Python code most Python programmers can understand with more complex CPython binding.
- joint_state_publisher is no longer a Pure python wheel.
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.
It's been a few years. Looking back, reusing the existing URDF/SDformat parsers instead of partially implementing parsers in this package seems like a big advantage, and using another language we're already extensively using in ROS seems like a small disadvantage.
I'm not about the pure python wheel con. It looks like colcon
doesn't build packages as wheels, and if someone were building the package without using colcon I'm not sure what they would find more advantageous.
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1 |
@sloretz Just out of curiousity, is this something that you are still interested in moving forward with? |
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
* added test for sdf model * joint limits, root tag and dict * revolute joint and log output * fix joint type and catch value error Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
ros2 sdformat support
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.
I've left a few more things to fix, but this is a lot closer to being ready.
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale Now that I've merged in #94, this one has a merge conflict. If you could resolve that, I'll then do another review here. Thanks. |
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
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.
I've left two more things to fix, then I think we can get this in.
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
joint_state_publisher/joint_state_publisher/joint_state_publisher.py
Outdated
Show resolved
Hide resolved
This reverts commit 54675b5.
Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
This is a quick hack to get rough sdformat support in joint_state_publisher.
@scpeters @IanTheEngineer FYI
Needed to support ros2/urdf#13