Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why the loaders? #5

Open
gabrielebndn opened this issue Nov 28, 2019 · 5 comments
Open

Why the loaders? #5

gabrielebndn opened this issue Nov 28, 2019 · 5 comments

Comments

@gabrielebndn
Copy link

gabrielebndn commented Nov 28, 2019

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
Copy link
Author

No answers? @nmansard? @jcarpent?

@jcarpent
Copy link
Member

jcarpent commented Dec 5, 2019

@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
Copy link
Author

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

@nmansard
Copy link
Contributor

nmansard commented Dec 6, 2019

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
Copy link
Collaborator

cmastalli commented Apr 3, 2020

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants