Gepetto / example-robot-data

Set of robot URDFs for benchmarking and developed examples.
BSD 3-Clause "New" or "Revised" License
143 stars 51 forks source link

Add continuous (SO2) variant of double_pendulum #111

Closed ManifoldFR closed 2 years ago

ManifoldFR commented 2 years ago

This PR adds a second version of the double pendulum URDF where the joints are defined as continuous (SO2) instead of revolute, and the appropriate options in the load() function.

hrp2-14 commented 2 years ago

Hi ! This project doesn't usually accept pull requests on master. If this wasn't intentionnal, you can change the base branch of this pull request to devel (No need to close it for that). Best, a bot.

ManifoldFR commented 2 years ago

Edit: rebased on Gepetto:devel branch

ManifoldFR commented 2 years ago

This looks useful, thanks for the PR !

Could you take care of those 2 comments, and add a unit test next to https://github.com/Gepetto/example-robot-data/blob/master/unittest/test_load.py#L53 ?

I took care of the comments and added a unit test (and ran the test_load.py file again)

nim65s commented 2 years ago

My question is do we really need to create an extra URDF? An alternative solution will be to modify the Pinocchio model.

Both solutions are fine for me. I mean, I agree it's not super nice to duplicate the URDF, but, on the other hand, changes to the source will be trivial to apply to the copy.

At the same time, it's not super nice to tie robot models to pinocchio, especially when we want to benchmark pinocchio vs something else on a particular model, but, on the other hand, well… We are already clearly privileging pinocchio for now, in the python utils and in the unit tests.

cmastalli commented 2 years ago

My question is do we really need to create an extra URDF? An alternative solution will be to modify the Pinocchio model.

Both solutions are fine for me. I mean, I agree it's not super nice to duplicate the URDF, but, on the other hand, changes to the source will be trivial to apply to the copy.

At the same time, it's not super nice to tie robot models to pinocchio, especially when we want to benchmark pinocchio vs something else on a particular model, but, on the other hand, well… We are already clearly privileging pinocchio for now, in the python utils and in the unit tests.

I fully agree with your different points of views. I am just concerned to maintain many hipotetical robots when the user can just make the trick from their side. Already the double pendulum is not a real robot, so having yet another version is not so great.

In any case, I am not strictly against this PR, especially if this is relevant for a number of users.

nim65s commented 2 years ago

I think it is relevant to have both constraint and unconstraint double pendulums, especially for teaching and tutorials, so this would really fit with the purpose of this repo :) This is probably not true for most other models we have.

ManifoldFR commented 2 years ago

Hi, can we merge this PR? Since it was already approved

nim65s commented 2 years ago

Sure, sorry for the delay

ManifoldFR commented 2 years ago

Thanks !