search

jrl-umi3218 / RBDyn

RBDyn provides a set of classes and functions to model the dynamics of rigid body systems.
BSD 2-Clause "Simplified" License
163 stars 47 forks source link

URDF and YAML parsers #68

Closed BenjaminNavarro closed 4 years ago

BenjaminNavarro commented 4 years ago

It's still work in progress but I wanted a place to discus about the issues I'm facing.

My goal is to integrate the URDF (from mc_rbdyn_urdf) and YAML (my own) parsers directly into the RBDyn repository and to provide facilities to convert between formats.

For the (very short) story, I made the YAML parser because I hate reading XML and even more writing it by hand. So if I need a model for a robot that doesn't have a URDF, I can now write it in YAML instead.

The YAML parser has been extensively tested and provides the same functionalities as the URDF one.

Now, in order to convert between format, I need to be able to output URDF/YAML files from the RBDyn data structures. This leads to some issues:

  1. Not all the data is available from MultiBody, MultiBodyConfig or MultiBodyGraph. For instance, joint limits are stored separately from the Joint class. This means that either we include them in the Joint class or we need to ask users to provide them separately. Modification of the Joint class can be made API backward compatible but not ABI backward compatible since new member variables must be added. The same goes for visual, collision and collision_tf from ParserResult that could be stored within the Body class instead.
  2. The parent, child and frame components of the joints are not easy to get. So far, I couldn't find a proper way of extracting these information after the creation of the graph, but I'll keep looking.
gergondet commented 4 years ago

Now, in order to convert between format, I need to be able to output URDF/YAML files from the RBDyn data structures. This leads to some issues:

Sorry I forgot to answer about this...

Isn't the simplest solution to use the parser result to do the conversion rather than adding extra information to RBDyn classes?

gergondet commented 4 years ago

Hi @BenjaminNavarro any updates on this?

BenjaminNavarro commented 4 years ago

Sorry, not yet. I've been busy with other higher priority stuff lately. I'll try to get back to it this month

BenjaminNavarro commented 4 years ago

Just to let you know that I'm finally back on this. I just rebased my branch on master and I'll start looking into @gergondet comments.

I think this PR will only contain the parsers and then I'll make new ones for the YAML/URDF generators and for some core modifications we made in our own version of RBDyn

BenjaminNavarro commented 4 years ago

Ok so the integration of the parsers is done and both tests pass.

In addition to mc_rbdyn_urdf and the YAML parser here are a few things that were added/improved:

I need to look at the Python bindings now.

The CI fails because of the missing dependencies. Should I also take care of this? I can also make the BUILD_PARSERS CMake option default to off.

gergondet commented 4 years ago

The CI fails because of the missing dependencies. Should I also take care of this? I can also make the BUILD_PARSERS CMake option default to off.

I have pushed a patch to take care of that. Hopefully it will go well :)

Removed some UB and possible exceptions when accessing boost::variant

Out of curiosity, do you recall what were the specific changes?

Support for the following shapes in addition to the existing Mesh: Box, Cylinder, Sphere and Superellipsoid

Superllipsoid is not part of the URDF format afaik, could you link some reference on the excepted parameters of the tag? (e.g. in doxygen)

Three free functions to load models: from_urdf, from_yaml and from_file. The two first ones can either load the description from a string or load it form a file. The last one automatically checks the file extension to chose which format to parse

I think I prefer to have from_urdf(content, ...) and from_urdf_file(file, ...) rather than an overload with a tag to distinguish the two (and that will make the bindings much simpler to write as well)

It's getting late here so I'll have a closer look at it tomorrow but it looks like you haven't addressed some of the comments regarding the CMake scripts themselves. Otherwise, probably all good :)

BenjaminNavarro commented 4 years ago

Removed some UB and possible exceptions when accessing boost::variant

Out of curiosity, do you recall what were the specific changes?

basically replacing things like:

auto& mesh = boost::get<Geometry::Mesh>(v.geometry.data);
mesh.filename = meshDom->Attribute("filename");
v.geometry.data = mesh;

with:

auto mesh = Geometry::Mesh();
mesh.filename = meshDom->Attribute("filename");
v.geometry.data = mesh;

Since it assumes that a Geometry::Mesh is the current active member of the variant, which might not be the case.

Support for the following shapes in addition to the existing Mesh: Box, Cylinder, Sphere and Superellipsoid

Superllipsoid is not part of the URDF format afaik, could you link some reference on the excepted parameters of the tag? (e.g. in doxygen)

Not it's not, but since it's part of sch-core and that it can be handy we added it some time ago. Yes, I'll link the wikipedia page which has a nice visual to understand what each parameter is doing.

Three free functions to load models: from_urdf, from_yaml and from_file. The two first ones can either load the description from a string or load it form a file. The last one automatically checks the file extension to chose which format to parse

I think I prefer to have from_urdf(content, ...) and from_urdf_file(file, ...) rather than an overload with a tag to distinguish the two (and that will make the bindings much simpler to write as well)

Ok no problem, I will do that

It's getting late here so I'll have a closer look at it tomorrow but it looks like you haven't addressed some of the comments regarding the CMake scripts themselves. Otherwise, probably all good :)

I did, but I really messed up a rebase after that and lost the modifications... Sure, have a nice evening/night

BenjaminNavarro commented 4 years ago

By the way, there is this fixme laying in the code:

// FIXME! Just like visual tags, there can be several collision tags!

Should I take care of it as well? Since fixing it would break the ABI that's probably a good time to address this. Unless there's no real need for it

arntanguy commented 4 years ago

I wrote that comment when I added the parsing of the <visual> tags. Concerning collisions, I double checked why I left that FIXME, and indeed the URDF specification says:

Note: multiple instances of \<collision> tags can exist for the same link. The union of the geometry they define forms the collision representation of the link.

Thus it would be better if the parser supported this. If you don't mind taking care of it that'd be great. Especially considering that I'm planning to add support for the URDF collision primitives in mc_rtc soon :)

gergondet commented 4 years ago

Hi,

I've made some small changes/cleanup on the PR and it's good for me atm.

Changes:

I still have to make some changes for the debian packaging side.

I'm also thinking of moving all the parsers related code in to the rbd::parsers namespace, what do you think of it ? @arntanguy @BenjaminNavarro

BenjaminNavarro commented 4 years ago

That's great, thanks a lot!

I don't see any problem in moving all parsers related code to rbd::parsers.

gergondet commented 4 years ago

Ok, I've made the required changes on the packaging side and moved everything parsers related to the rbd::parsers namespace

Will merge when the CI passes and release v1.2.0 shortly after