search

ros / urdfdom_headers

Headers for URDF parsers
http://ros.org/wiki/urdf
Other
27 stars 79 forks source link

Forking URDFDom? #59

Open davetcoleman opened 5 years ago

davetcoleman commented 5 years ago

There are many desired changes to the URDF spec that are needed for projects like MoveIt and Tesseract that have not been merged into this repo. This has been an issue for many years - the spec is basically unchanged while the SDF format goes through many many iterations. This is hurting the ROS project. A good discussion to get a sense of this issue is here

Based on today's MoveIt Maintainer meeting, we are considering forking this URDF project so that new features can be added. This is not an ideal outcome because we don't want to fracture the ROS community and have code duplication. As I understand it, ROS-Industrial already has forked it.

In order to prevent a fork, we believe URDFDom needs to be released within ROS, not Ubuntu Universe, so that changes can be released more frequently. We also believe we need a more aggressive policy in breaking things, especially between ROS versions, so forward progress can be made.

I may have missed some key points here as I don't follow every detail of the URDF project, but I've been following this project loosely for years and am trying to sum up my observations and the feedback I've heard from others.

How can we get the long list of PRs/improvements in this repo merged without the fear of breaking something?

@scpeters @jmirabel @rhaschke @tfoote @Levi-Armstrong

jmirabel commented 5 years ago

There are two different issues to be discussed:

  1. the fact that the source code is frozen,
  2. the fact that the binaries are released at a low frequency.

I cannot help for point 2. I can give my two cents on point 1 and summarize what has been said in #24 .

We also believe we need a more aggressive policy in breaking things, especially between ROS versions, so forward progress can be made.

I fully align with @scpeters points: An old parser should fail verbosely on a file with a too recent format version, without requiring to update the old parser.

This was the blocking point for #24. Some months (or years) after opening the PR, I proposed a solution which consists in:

I insist on this proposition because I prefer a consensus more than a fork.

scpeters commented 5 years ago

Let me first apologize for not responding to these pull requests that add features to urdfdom. Let me try to communicate my thoughts on some of these challenges:

I've spoken before about the issues with old parsers not recognizing new fields (https://github.com/ros/urdfdom_headers/pull/24#issuecomment-212528487). There is a version attribute in the urdf.xsd schema at urdfdom, but I haven't seen it used. Ideally, each parser should know which versions it supports and give a warning or error if it tries to load a version that it doesn't recognize. For me, the lack of versioning is the biggest blocker to adding features to urdfdom.

Another challenge to adding features to urdfdom_headers with the current architecture is the potential for breaking API or ABI when modifying data structures. An alternative is using a PIMPL pattern with get/set functions instead of direct member access.

For sdformat, we are discussing the use of xml namespaces for custom elements and prefixes:

rhaschke commented 5 years ago

I think we fully understand the constraints of maintaining such a key package. However, we should enter discussions how to address and overcome these concerns to allow some progress and avoid alternative forks. Hopefully, we can start this process at ROSCon.

traversaro commented 5 years ago

I personally fully agree with @davetcoleman suggestion that the URDF format should be allowed to evolve, and in particular with @jmirabel proposal.

On the top of that, I think some points that should be addressed in the discussion are the following:

There are been a few related discussions in the past that I think could be interesting for people reading this thread:

davetcoleman commented 5 years ago

Thanks for all these very quick thoughts and feedback! Lots of good actionables here, that I agree should be discussed at ROSCon

the fact that the binaries are released at a low frequency.

This seems easily fixable by providing a Melodic & future Noetic release in the ROS 1 build farm, right? Just a matter of someone making the release...

tfoote commented 5 years ago

Making the releases is relatively easy, knowing what exactly to release is the hard part. Even just jumping it into a ROS package will run into issues of colliding symbols and breaking existing implementations that rely on the system packages.

There's definitely space to accelerate the rate of development. But we do need to develop the migration plan with versioning etc that can be implemented into the specification to enable the migration path so that maintenance updates can be pushed.

There's significant value in being upstream in Ubuntu, but it does slow down the release cadence possiblities and require significant planning ahead for major changes. I'm quite hesitant to support custom extensions for various elements. That is a way that you can get to the point that they all talk URDF, but nothing is fully compatible because they all use different custom extensions.

Getting the major stakeholder together at ROSCon sounds like a good idea.

jmirabel commented 4 years ago

Was this discussed at ROSCon ? I couldn't attend.

clalancette commented 4 years ago

Was this discussed at ROSCon ? I couldn't attend.

Yes, we had a meeting about it at ROSCon. We're going to send out some additional details this week, but the short of it is that we are going to attempt not to fork URDF, and see if we can make a bit more progress with it. There is also some talk of switching to a different file format, but we still have to finish discussing that and have a follow-up meeting.

gavanderhoorn commented 4 years ago

@clalancette: what's the current status on adding the version nr to the format?

clalancette commented 4 years ago

@clalancette: what's the current status on adding the version nr to the format?

It's been done in https://github.com/ros/urdf_parser_py/pull/52 and https://github.com/ros/urdfdom/pull/133. I've also released a new version of urdfdom_py into Melodic.

I think what is left to do is:

  1. Merge https://github.com/ros/urdf_parser_py/pull/54 for ROS 2
  2. Port https://github.com/ros/urdfdom/pull/133 to https://github.com/ros2/urdfdom
  3. Release above changes for ROS 2
  4. Release urdfdom as an Ubuntu package update
  5. Announce on ROS Discourse
gavanderhoorn commented 4 years ago

Nice :+1:

Seems I need to Watch even more repositories to not miss this sort of thing ;)

Should there be a 4. Announce on ROS Discourse?

traversaro commented 4 years ago

@clalancette There is any plan of propagating this change in the fork of urdfdom used in ROS2, i.e. https://github.com/ros2/urdfdom ? The fact that ROS2 install its own urdfdom that shadows the one used by ROS1/Gazebo is already quite error prone (see https://github.com/robotology/gym-ignition/issues/118#issuecomment-575372388), and if the two versions are not in sync that will get even worse.

clalancette commented 4 years ago

@clalancette There is any plan of propagating this change in the fork of urdfdom used in ROS2, i.e. https://github.com/ros2/urdfdom ? The fact that ROS2 install its own urdfdom that shadows the one used by ROS1/Gazebo is already quite error prone (see robotology/gym-ignition#118 (comment)), and if the two versions are not in sync that will get even worse.

Ug, I didn't even realize that. OK, I'll update the list above, we definitely need to get the change into the ROS2 repository as well.

EricCousineau-TRI commented 4 years ago

Regarding this point:

[...] However, what is the current process for proposing modifications to the URDF specification? REPs? [...]

I had the same issue with SDFormat. As part of TRI-sponsored work, we (@azeey, @scpeters, and I) considered REP / PEP-style proposals, but are just keeping it simple for now: http://sdformat.org/tutorials?tut=proposal_format (I thought we included rationale of "why not REP / PEP-style indexing / format", but I guess that's lost in some Asana / BitBucket PR discussions.)

To clarify my position on URDF (at the possibly of being cast as a villain :P): I would love it if, in the future, we converge on one format, either URDF (simple parsing, but non-expansive functionality) or SDFormat (complex parsing, but more expansive functionality), and figure out a way to unfork. On my side, I wish to put my effort towards SDFormat, and would love it if this were the format to "win out" (if it's ever at all possible) and have ecosystem support.

Otherwise, if both formats continue to be co-developed, we will continue to have potentially duplicate effort between the two. (Maybe that's good? Maybe it's not?)

I know that this is a completely non-trivial undertaking that's been discussed for years, and nowhere near possible in the near future, but just want to state my position.

EricCousineau-TRI commented 4 years ago

One proposal is that, at least for the time being, we somehow make URDF and SDFormat use the same proposal processes, possibly even the same technology (e.g. same XML parser, similar conventions for *.xsd, same parsing semantics, etc.).

I know that this is an Ignition vs. ROS thing, but it would be lovely to see less forking between those technologies when possible?

gavanderhoorn commented 4 years ago

I know that this is an Ignition vs. ROS thing, but it would be lovely to see less forking between those technologies when possible?

Related discussion on ROS Discourse: Why are ROS and Ignition/Gazebo diverging (or not converging)?.

You're not the only community member wondering about the apparent divergence.

EricCousineau-TRI commented 4 years ago

Hehe, yeah. I had done a briefly survey on some modeling formats, and came across this (of many) discussions: https://discourse.ros.org/t/urdf-ng-ros2-urdf2-discussion/511/21

I'd be happy to take this (kinda crappily written) survey doc and post it on a Gist if it's at all useful. (I'm sure this is the nth survey out of n^2.)

clalancette commented 4 years ago

We also discussed it at the last ROSCon.

While there does seem to be some support for switching to another official format, its not entirely clear which format would be chosen. There are some industrial formats that have some benefits, there is COLLADA, there is SDF. There was some lukewarm support for switching to SDF at ROSCon.

I don't have strong opinions on the topic. However, I think there are two things that any proposal for a new format needs to deal with:

  1. It has to be an open format.
  2. It has to have some story for how to continue to deal with URDF in the ROS ecosystem. There is just a lot of URDF out there, and it isn't going to go away in the near (or probably even far) future.
Levi-Armstrong commented 4 years ago

I have recently been using Ignition along with SDF format and I think with some additions it would be a great improvement over the URDF format. I think if we stuck with URDF we would just be replicating a lot of what is already in SDF format. Also, with using the same format it would provide a seamless transition between ROS and Ignition Simulation. I vote for moving to the SDF format.

IanTheEngineer commented 4 years ago

@Levi-Armstrong along the lines of your comment, I proposed making SDFormat files first-class citizens in urdf::Model: https://github.com/ros/urdf/issues/34 .

Out of curiosity, what additions were you thinking of form SDFormat to make it an improvement over URDF?

jmirabel commented 4 years ago

Out of curiosity, what additions were you thinking of form SDFormat to make it an improvement over URDF?

If it does not exists and if it possible, a converter from URDF to SDF ?

gavanderhoorn commented 4 years ago

@jmirabel: Gazebo already comes with one.

jmirabel commented 4 years ago

@jmirabel: Gazebo already comes with one.

Thanks for your reply. I think it shouldn't be bound to gazebo.

traversaro commented 4 years ago

@jmirabel: Gazebo already comes with one.

Thanks for your reply. I think it shouldn't be bound to gazebo.

Related issue: https://github.com/osrf/sdformat/issues/85 . TL;DR: There is now a conversion tool directly in sdformat, but unfortunately it depends on Ruby: https://github.com/osrf/sdformat/blob/sdf9/src/cmd/cmdsdformat.rb.in .

jmirabel commented 4 years ago

Does sdformat also handles SRDF ?

IanTheEngineer commented 4 years ago

Does sdformat also handles SRDF ?

SRDF is MoveIt's "semantic" kinematic chain / collision space addition to URDF, and wholly separate from SDFormat. URDF parsers will also not recognize SRDF tags.

The term "URDF" is overloaded to mean both a file format (*.urdf XML) and a robot data representation (urdf::Model). The ROS Ecosystem has mostly standardized around the urdf::Model data representation. However the urdf_parser_plugin itself can take arbitrary file formats and populate this data representation. The plugins currently support *.urdf XML or *.DAE Collada (more than just meshes). This means the URDF Data representation used in robot_state_publisher or rviz is not limited by the file format itself.

The URDF data representation urdf::Model can support SDFormat files if there is a ROS plugin package (eg. sdformat_parser) that can populate urdfdom's API's. I created https://github.com/ros/urdf/issues/34 to proposal just that.

EricCousineau-TRI commented 4 years ago

Does sdformat also handles SRDF ?

@jmirabel In addition to Ian's answer, SDFormat 1.7 (libsdformat 9.0.0) also better supports custom tags (which would be ideal for application- or library-specific uses): http://sdformat.org/tutorials?tut=custom_elements_attributes_proposal https://github.com/osrf/sdformat/blame/3bbd303c8b94b20244d102eae095ffcbaa4c550d/Changelog.md#L143-L144 https://osrf-migration.github.io/sdformat-gh-pages/#!/osrf/sdformat/pull-requests/600

For example, some form of SRDF's information could be parsed from an sdformat file in a namespace like <srdf:disable_collision/> or <planning:disable_collision/> or whatever tag you choose. Alternatively, it remains a separate file (which may be ideal if the same robot has different planning configurations). Ideally, if the custom tags were embedded in an SDFormat file, it's done in such a way that the customizations are shared among relevant libraries.

[...] but unfortunately it depends on Ruby [...]

@traversaro I've filed https://github.com/osrf/sdformat/issues/274. Would love to hear more (as we on the Drake side have felt this as well), but in a separate thread.

Levi-Armstrong commented 4 years ago

Out of curiosity, what additions were you thinking of form SDFormat to make it an improvement over URDF?

The reason, I prefer it over the current URDF format is that it appears to be designed around optimizing the physics engine. Most of the collision checking libraries I am using are wrappers around physics engines with the exception of FCL, but they still apply. The model concept prevents duplicate collision geometries being create at the physic engine which we could take an advantage of in collision checking libraries. It also has a static flag for objects. This allows the physics engine to use an optimized data structure because it is not moving to improve performance which collision checking would also take advantage of.

traversaro commented 4 years ago

Related discussion on "forking" URDF: https://github.com/ros-industrial-consortium/tesseract/issues/184#issuecomment-646315434 .