stack-of-tasks / pinocchio

A fast and flexible implementation of Rigid Body Dynamics algorithms and their analytical derivatives
http://stack-of-tasks.github.io/pinocchio/
BSD 2-Clause "Simplified" License
1.82k stars 383 forks source link

Install Pinocchio URDF models #965

Closed gabrielebndn closed 4 years ago

gabrielebndn commented 4 years ago

Right now, we have lots of test URDF models (plus a Python model and a Lua one) in folder models. However, if I understand correctly, they are not installed. It would be nice to do it so that they are available to all users, even those without access to the repo.

I propose to install them at CMAKE_INSTALL_PREFIX/share/pinocchio/models.

Also, it would be nice if there were a C++ macro holding the model installation path were available to users (and exposed in Python as a constant). Is it doable? I see a macro PINOCCHIO_MODEL_DIR is currently defined for unit tests, amounting to the internal repo directory. It would be nice if a macro with the same name were made available to users, but overridden for the unit tests, so that the tests do not depend on Pinocchio being installed. This might be easy to do for C++, less easy for Python I think.

nim65s commented 4 years ago

On the other hand, all those models are available in the release tarball: it is currently 39Mo including those, and would be only 3.6Mo without.

And these models are stritcly speaking the ones from example-robot-data, which is installable by itself, including from a binary package.

So I think we should not install the models in models/others, and not even include them in the release.

This might be easy to do for C++, less easy for Python I think.

Actually, this is already done in python, eg.

from example_robot_data import loadTalos

and you'll get a RobotWrapper with Talos, without having to consider any absolute path. We can easily add the same functionality in C++.

wxmerkt commented 4 years ago

In other words, if there is a package-level dependency on example-robot-data, it shouldn't also be a submodule?

nim65s commented 4 years ago

@wxmerkt I agree, but @jcarpent may find it handy to get the models from a submodule.

Also we would have a kind of circular dependency issue, as pinocchio will depend on the models in example-robot-data, and the python scripts in py-example-robot-data already depend on pinocchio. I can easily handle this in robotpkg, but this might be harder in other distributions.

gabrielebndn commented 4 years ago

I was just looking at what @nim65s is saying... Currently, Pinocchio is a required dependency for example-robot-data. I think this is a mistake, it should be optional. Only the Python scripts depend on Pinocchio, the rest is a collection of URDF files which works perfectly well without Pinocchio

nim65s commented 4 years ago

@gabrielebndn I'm pretty sure this is already working as you describe. Otherwise, please double check that, and fill an issue in example-robot-data with a link showing what makes you say so, as this is clearly a bug :)

gabrielebndn commented 4 years ago

I've just checked it... It is a required dependency if BUILD_PYTHON_INTERFACE is ON

jcarpent commented 4 years ago

I do not think Pinocchio should depend on an extra package as a build dependency. And I do agree with @nim65s that 39 Mo is quite huge. I do not have a correct solution right now, except adding only a useful URDF model for Pinocchio.

nim65s commented 4 years ago

@jcarpent and this is the compressed size :) I should also add that the git clone --recursive in pinocchio now automatically download the 37Mo of example-robot-data's .git tree.

gabrielebndn commented 4 years ago

Ok, I see everything more clearly now. So example-robot-data is an independent package, installable and even apt-installable. If you want those models, you can just install it.

Therefore, I think there is actually no need to have all those models in Pinocchio. Also, I think that this situation of example-robot-data depending on Pinocchio and also being a submodule of Pinocchio is not clean. Especially as we have the hard-coded sample models, URDF models in Pinocchio are mostly not necessary. We just needed for very few unit tests, so that we can check that the loading of the URDF and of the meshes are fine, and maybe in a few examples, to show how URDF models are supposed to be loaded. For this, we do not need an extensive set of models, probably one or two are enough.

Indeed, today Pinocchio code is only employing simple_humanoid.urdf, romeo, and the UR5 (in examples/overview-urdf.cpp, which is not even automatically tested because at the time it was made the UR5 was not part of Pinocchio). All other models are unused and they are not part of the installed release, therefore they are useless and they just waste space.

The initial goal was to replace romeo with an alternative manipulator (we have it in simple_humanoid.urdf, although it does not have meshes), add a manipulator (we have the UR5 in the submodule), and possibly a few other representative cases, such as a quadruped.

My suggestion:

gabrielebndn commented 4 years ago

Actually, agree with @wxmerkt that making example-robot-data a dependency would be nice, but if @jcarpent does not want an extra dependency and if we have the problem of circularity, I see no other solution than the above one

jcarpent commented 4 years ago

The idea of including all the examples of robots is for examples and documentation purposes. Having access to common and used models such as UR5 or Talos is nice (Romeo is less appealing, because not used anymore). I'm not afraid about the size of the arxiv, because this one is never downloaded when using package managers such as robotpkg, conda, brew, etc. Another but useful solution would be to have each robot as a submodule of the example-robot-data repository and to upload only the one required by the applications.

jcarpent commented 4 years ago

From my side, I would like to not change the way things are working now, but rather to focus on improving unit tests, the documentation and the full integration with hpp-fcl Python bindings. I think this is the main priority now.

gabrielebndn commented 4 years ago

@jcarpent I think that for examples and documentation a couple of models is enough. If we insist on example-robot-data bing a separate folder, then the right solution is a dependency I think, if only we hadn't the problem of circularity. I find it not well made that there is a huge folder in Pinocchio containing lots of code that is not used anywhere and that cannot be employed in any way by users not having access to the source code. It is really useless to have it there. Also, if we are going to use those models in examples, I find it not well made that the examples refer to files only existing in the source package. It would be much cleaner if these examples employed installed files, either installed within Pinocchio itself or from another package, which the user should be instructed to install. I think it should be changed, but of course this is not a priority, it started out as a suggestion of installing the models, then @nim65s pointed out to the problem of the size, which I had not thought about

jcarpent commented 4 years ago

@gabrielebndn Just to let you know that most of the models will be used for benchmarks soon, to have all in one. So no precipitation is needed. Again, there are more much more important priorities at this time.

gabrielebndn commented 4 years ago

I still think a dependency would work better, but I do agree it is not a priority at all. I will close the issue