search

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

Why the loaders? #5

Open gabrielebndn opened 4 years ago

gabrielebndn commented 4 years ago

In robots_loader.py there are many utility methods to load a RobotWrapper containing one of the robots of this repository.

Is it really needed? I do not like these special methods. There are many reasons why I say this:

  1. First of all, they are based on RobotWrapper, and we should not encourage the use of RobotWrapper

  2. For most robots, they amount to little more than a call to RobotWrapper.BuildFromURDF. Pinocchio methods are already convenient enough to load robots.

  3. They make the code employing them less transparent

  4. They introduce a dependency to Pinocchio

  5. They are not general

  6. They are not flexible.

    • file path is hardcoded
    • If one wants to use a slightly modified URDF, you cannot by using these methods, unless you modify the installed files.
    • If you are using one of those methods and you find you need to modify the URDF but you do not want to do change the installed files, the only option is to scrap them and rewrite the code so that they are not used. Better not to give this option entirely! You should know were your URDF files are and what's in them
    • What if a robot is removed from this repo? Then the Python code will be removed too. This will break the code using it, and this is really unnecessary.

  7. They introduce yet-another-way of doing the same thing, i. e. loading a URDF model. Do we really need another way? This is not really desirable, as goes, for instance, the Zen of Python : There should be one-- and preferably only one --obvious way to do it. We already have

    1. native Pinocchio methods
    2. shortcuts buildModelsFromUrdf and createDatas(models)
    3. the RobotWrapper interface

  8. They make the robots contained in this repo sort of "special", because they have a special way to be loaded

  9. They are confusing to newcomers, because they hide too much detail and then they will not know how to load a generic robot or a slightly modified one (I can call some newcomers to testify)

Summarizing, I think this loader functions are a bad idea and should be removed

gabrielebndn commented 4 years ago

No answers? @nmansard? @jcarpent?

jcarpent commented 4 years ago

@gabrielebndn I think the loaders have been thought to be easy to use. Avoiding looking for a particular description of a robot (path_name, etc.). I don't see really the point of not having them.

gabrielebndn commented 4 years ago

I don't know. Design-wise, I do not like them, for all the reasons listed above

nmansard commented 4 years ago

I agree with 1. We should not use robot-wrapper in the loader, to make it easier to not use robot-wrapper in later applications. For the rest, I believe the easiness of loading these robot counterbalance the disadvantage.

cmastalli commented 4 years ago

@gabrielebndn sorry for a late reply. I agree with @nmansard.

We could remove the robot wrapper dependency, and then add a dedicate visualization (for user inspection only) in GV (and / or meshcat).

I hadn't have time to work on it. Feel free to do it if you want.

Note that we could install this package without Pinocchio dependency (or loaders), i.e.

sudo apt install robotpkg-example-robot-data

or

cmake -DBUILD_PYTHON_INTERFACE=OFF ../
sudo make install