search

robotology / gym-ignition-models

Collection of robot models compatible with gym-ignition
https://github.com/robotology/gym-ignition
GNU Lesser General Public License v3.0
25 stars 8 forks source link

Change base iCub link to have x-forward and z-upward #12

Closed diegoferigo closed 4 years ago

diegoferigo commented 4 years ago

For historical reasons, the iCub model has the x axis of the base pointing backward. The upstream model uses a pose element to insert the model in gazebo with the right orientation (facing forward).

The position of the frame could be confusing to new users, especially when cartesian orientation are involved (e.g. floating base IK). This PR updates the model to have the base frame with x pointing forward.

icub_base_link

traversaro commented 4 years ago

Note that probably this will create problems with persons using the "normal" iCub models with gym-ignition, so if the RL team thinks that the gains are worth the possible future trouble, you can go forward, but this should be documented in the README or somewhere.

diegoferigo commented 4 years ago

In general, people that use gym-ignition should also use the models we officially support. I don't think that the icub-models repository will gain anytime soon Ignition Gazebo support, particularly due to the yarp plugins that need to be rewritten for the new simulator.

Do you have any specific problem in mind or it was a general comment? Thinking about integration work between different projects, I don't see major problem beyond the need to edit a configuration if the mismatch between world and base frame must be taken into account.

traversaro commented 4 years ago

I just have the feeling that someone in the future will have strange mismatches, and will spend two days before finding this PR, so dropping a line in the README or the model README could be useful, but it is just a feeling.

DanielePucci commented 4 years ago

CC @pattacini

pattacini commented 4 years ago

I feel myself a bit thrown in the middle of the discussion but just let me add a few general comments.

@diegoferigo I believe you're the maintainer of this repository hence you can obviously choose whatever you deem is worth doing for the goodness of your development and your community, also branching off from the mainstream.

However, considering the community at large, the official root frame is still that one specified everywhere in the software and in the documentation. Decisions to update this convention cannot take place at this level though, rather they shall stem organically, as we can all easily figure out, to trade off the implications, spread the news and possibly coordinate the effort.

This comment holds in the outlook of future integration, of course, as I mentioned the potential of gym-ignition in https://github.com/robotology/QA/issues/366.

I don't see major problems beyond the need to edit a configuration if the mismatch between world and base frame must be taken into account.

I'm afraid that it might be a bit more complex than that as manipulation targets, for example, can be specified in several different ways at the user level and a change in a configuration file is generally not sufficient.

Concluding, I also put forward @traversaro's recommendation to make this change of coordinates evident from the main README.

diegoferigo commented 4 years ago

Thank you all the the input about this discussion.

Disclaimer: what follows is what I remember from the early experiments i did some time ago

The reason behind this modification was originated from some experiments with floating-based IK in a mixed setting between Ignition Gazebo and iDynTree (but this could maybe generalized to others URDF parsers). Due to the different logic of fixed joints lumping, Gazebo will select as base the base_link, instead iDynTree the root_link. Note that, in the upstream models, these two frames do not match (base_link is pointing backwards, root_link forward). You can understand that taking data from Gazebo and feeding it to iDynTree in such setting is extremely error prone, and the user code could get overly complicated due to the extra transform that has to be taken into account. After some debugging with @traversaro, we also remembered that the upstream model has also a kind of hidden //gazebo/pose and that could be part of the problems I was experiencing.

Right now the extra frames situation of different robot descriptions is quite messy, and I hope that the new frame semantics that just landed to SDF could help improving the current state (in particular, the canonical link selection - related PR). Though, at the moment we have to live for some more while without it.

While this repository is not in the robotology organization, the models stored here are necessary for gym-ignition. In fact, gym-ignition-models is a dependency of gym-ignition. Soon I want to move this repo to the robotology organization, and I understand that keeping uniformity across our models is important. In light of my reply and your comments, I will close this PR and keep it on hold. I want to make more targeted tests and understand if the situation could be solved in other ways. I will resume the discussion after that.

If you have other comments, or if I wrote something wrong (which is possible), please let me know.