Skip to content
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

Forking URDFDom? #59

Open
davetcoleman opened this issue Aug 22, 2019 · 29 comments
Open

Forking URDFDom? #59

davetcoleman opened this issue Aug 22, 2019 · 29 comments

Comments

@davetcoleman
Copy link
Contributor

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
Copy link

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:

  • changing the main tag (currently, robot) into, for instance, urdf,
  • and adding a version number to tag urdf.

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

@scpeters
Copy link
Contributor

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 (#24 (comment)). 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
Copy link

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
Copy link
Contributor

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:

  • I always intended urdfdom_headers and urdfdom (together with urdf_parser_py) as reference implementations of the URDF "specification", that I always understood it is maintained in the ROS wiki at http://wiki.ros.org/urdf/XML . Ideally a proposed change to the URDF format (such as sensor refactoring #22 or Add Capsule primitives #24) should come in the form of a modification of the specifications, together with the modification of the reference implementations. However, what is the current process for proposing modifications to the URDF specification? REPs? I think it should be clarified the process for proposing changes to the URDF specification, for example by moving it to a GitHub repo and use the usual Pull Request workflow to propose. discuss and accept modifications.
  • Once a version system is in place, should a given urdfdom version and related ROS distro provide support to load only URDF of the latest version, or also provide some kind of backword compatibility?
  • As suggested by @scpeters, a clearly documented way of specifying URDF custom extensions (as done for example for glTF or COLLADA would be a great way to permit to subset of URDF users on agreeing on some new features, without being blocked by the process of getting a change in the official spec or getting the reference implementation released.

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

@davetcoleman
Copy link
Contributor Author

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
Copy link
Member

tfoote commented Sep 11, 2019

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
Copy link

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

@clalancette
Copy link
Contributor

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
Copy link

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

@clalancette
Copy link
Contributor

clalancette commented Feb 18, 2020

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

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

I think what is left to do is:

  1. Merge ROS 2: Add in support for the version tag. (#52) urdf_parser_py#54 for ROS 2
  2. Port Add in support for a version tag. urdfdom#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
Copy link

Nice 👍

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
Copy link
Contributor

@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-legacy/gym-ignition#118 (comment)), and if the two versions are not in sync that will get even worse.

@clalancette
Copy link
Contributor

clalancette commented Feb 18, 2020

@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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link

IanTheEngineer commented May 8, 2020

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

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

@jmirabel
Copy link

jmirabel commented May 9, 2020

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
Copy link

@jmirabel: Gazebo already comes with one.

@jmirabel
Copy link

jmirabel commented May 9, 2020

@jmirabel: Gazebo already comes with one.

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

@traversaro
Copy link
Contributor

@jmirabel: Gazebo already comes with one.

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

Related issue: gazebosim/sdformat#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
Copy link

jmirabel commented May 9, 2020

Does sdformat also handles SRDF ?

@IanTheEngineer
Copy link

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 ros/urdf#34 to proposal just that.

@EricCousineau-TRI
Copy link

EricCousineau-TRI commented May 9, 2020

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 gazebosim/sdformat#274. Would love to hear more (as we on the Drake side have felt this as well), but in a separate thread.

@Levi-Armstrong
Copy link

Levi-Armstrong commented May 10, 2020

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

  • I think it would be nice to add support for Quaternion in poses.
  • ConvexMesh geometry type. I was surprised to find that this is not supported but they may be doing something at the physics engine, because most physics engines only support convex_mesh to detailed mesh, but not detailed_mesh to detailed_mesh if one of them is moving. I know this is the case for Bullet and Physx. Bullet does have GimpShapes that make this possible but does not appear to be supported anymore.
  • Also what @EricCousineau-TRI pointed out, but mainly everything that is there current srdf file.

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
Copy link
Contributor

Related discussion on "forking" URDF: tesseract-robotics/tesseract#184 (comment) .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests