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

Add Capsule primitives #24

Closed
wants to merge 1 commit into from
Closed

Add Capsule primitives #24

wants to merge 1 commit into from

Conversation

jmirabel
Copy link

@jmirabel jmirabel commented Apr 9, 2016

Collision checking with capsules is faster than with cylinder. As FCL has this primitives, I guess it shouldn't be a problem to add them in the URDF format ?
I have made changes in urdfdom packages as well.

@jmirabel
Copy link
Author

jmirabel commented Apr 9, 2016

Parsing of capsules: ros/urdfdom#85

@scpeters
Copy link
Contributor

The code looks pretty straight-forward since capsules are so similar to cylinders.

My only concern is that adding new fields to urdf could lead to problems when trying to parse new files with an old parser, since the format doesn't have any way of declaring a version.

@jmirabel
Copy link
Author

Tinyxml can parse a version number in the xml tag.
From now on, the old parser could give a warning when version number is set and too high and the new parser could fail when version number is not what it expects. But an update of the old parser would be required...

@scpeters
Copy link
Contributor

That's the main problem, that the old parser would need to be updated.

@florent-lamiraux
Copy link

florent-lamiraux commented May 6, 2016

There are two separate issues :

  1. modifying the URDF language that is a standard,
  2. modifying the parser (urdfdom, urdfdom_headers) so that it parses new primitives.
    I understand that you do not want to do 1, since old versions of the parser will ignore capsules. This pull request however addresses 2. I would then recommend to accept the pull request and to upgrade the language when
    1. new versions of the parser check the version of the files parsed,
    2. old versions without this feature are not supported anymore.

@olivier-stasse
Copy link

Dear Joseph and Florent,
Collada accepts capsule as an analytical shape (https://www.khronos.org/files/collada_spec_1_5.pdf).
As collision accepts also link to collada files will this be acceptable ?
It would avoid any modification of the current and past parser.

@jmirabel
Copy link
Author

jmirabel commented Oct 5, 2016

This solution is really heavy in terms of development because analytical shapes are not parsed by assimp. I guess there would be some backward compatibility issues in other places in ROS.

@sbarthelemy
Copy link

I would also use capsule shapes if it was available, and - by the way - a "rounded box shape" too (or alternatively a "convex hull of spheres shape").

@scpeters could you detail the issue you foresee with old parsers?

AFAICT, when encountering a capsule, the old parser will fail to parse the file with those error logs:
"Unknown geometry type '%s'"
"Could not parse collision element for Link [%s]"

That seems quite sane to me. Is it a blocker?

@wxmerkt
Copy link

wxmerkt commented Nov 17, 2017

Hey at all -
I'd like to understand what is currently blocking adding capsules to newer versions of urdfdom as the current parser will already fail and warn if it encounters an unknown tag. Would it be possible to make a new sub-version of URDF to allow capsules? Is there any way we could address these blocking/open points?

Happy to contribute, thank you very much,
Wolfgang

@wxmerkt
Copy link

wxmerkt commented Apr 3, 2018

Hey,
I wanted to ping this thread - any input on how we may add Capsules? E.g. by having a higher version number in parser or targeting future release? What could unblock this?

Thank you very much for your input,
Wolfgang

Edit Drake also supports parsing capsule shapes

@sbarthelemy
Copy link

Hello, I'd like to see topic move forward too.

FYI: the bullet URDF parser does parse capsule shapes.

@davetcoleman
Copy link
Contributor

This seems like a great idea, and something we should add to ROS Melodic. ROS2 is an even easier place to target this.

@sbarthelemy's point about the existing error messages seems compelling enough. @scpeters what is remaining to merge this in?

@jmirabel
Copy link
Author

@davetcoleman there is a need to either:

  • make the old parser fail when it encounters a file with a capsule. Maybe, it is sufficient to well document the fact that developers should add "urdfdom >= ..." to their package.xml when using capsules.
  • drop backward compatibility of old parsers.

@traversaro
Copy link
Contributor

This seems like a great idea, and something we should add to ROS Melodic. ROS2 is an even easier place to target this.

Note that at the moment, urdfdom_headers and urdfdom for ROS1 distributions are provided as upstream Ubuntu packages, and as long as this is the case, almost any new feature needs to wait for the next Ubuntu LTS release to be used.

make the old parser fail when it encounters a file with a capsule. Maybe, it is sufficient to well document the fact that developers should add "urdfdom >= ..." to their package.xml when using capsules.

Related discussion on URDF format versioning: ros/urdfdom#123 .

@jmirabel
Copy link
Author

@traversaro thanks for the insight.

Following discussion in ros/urdfdom#123, is it practicable to define URDF v2 by changing the name of the robot tag into another tag, like robot2 (obviously, this is a silly name to get me understood) such that:

  • the new parser would be able to read tags robot and robot2 correctly
  • the old parser would fail to read tag robot2

@jmirabel
Copy link
Author

Tag robot2 could include an attribute version to allow versioning.

@jmirabel
Copy link
Author

jmirabel commented Jun 7, 2019

Dear @scpeters,
do you think it is possible to add a tag (e.g. urdf or device) that would be the successor of tag robot ?

@Levi-Armstrong
Copy link

What is left to be done to get this merged? I may have resources to address the remaining items as we are interested in adding several new types shown in PR #54 #55 #56 #57.

@davetcoleman
Copy link
Contributor

Note that at the moment, urdfdom_headers and urdfdom for ROS1 distributions are provided as upstream Ubuntu packages, and as long as this is the case, almost any new feature needs to wait for the next Ubuntu LTS release to be used.

Yes but this has been the argument for the last ~8 years, so it just remains unchanged. It would still be nice to make changes for the next LTS.

You also are hinting at the possibility of adding urdfdom back into the OSRF-hosted build farm / PPA, which I support.

@davetcoleman davetcoleman mentioned this pull request Aug 22, 2019
@jmirabel
Copy link
Author

jmirabel commented Apr 3, 2020

I didn't follow closely. From #59, it seems that versioning has been added somewhere. Any knows what should be done to update this PR to get it accepted ?

@Aetherbase
Copy link

Hello, is there a reason on why is this not merged yet?

@jmirabel
Copy link
Author

Closing this since there are very little chances this gets merge as is after 6 years.

@jmirabel jmirabel closed this Aug 17, 2022
@Levi-Armstrong
Copy link

If anyone is interested Tesseract has a urdf parser which includes additional shapes like convex mesh, cone, capsule, octomap, etc. and also supports Quaternion for origin tag and acceleration limits.

https://github.com/tesseract-robotics/tesseract/tree/master/tesseract_urdf

https://tesseract-docs.readthedocs.io/en/latest/_source/core/packages/tesseract_urdf_doc.html

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

Successfully merging this pull request may close these issues.

10 participants