Pinocchio: Separating the functionnalities of RobotWrapper

[nmansard]

This is related to #657

RobotWrapper implements 3 very different sets of functionalities:

• it loads the 3 models (kinematics, visuals, collisions) and create the corresponding data.
• it implements nice object-oriented wrappers around pinocchio main algorithms, useful for newcommers.
• it implements the bindings with the viewer, by creating the viewer client, loading the geoms, computing their placement and forwarding the placements to the viewer.

I believe we should separate those 3 sets.

• by creating a loadModelsFromUrdf function that returns the 3 models (no need to put that in an object)
• by keeping the nice wrapping of the algorithms in a RobotWrapper object that would be initialize by an instance of Pinocchio::Model.
• by introducing a new GepettoViewerClient that would take care of the bindings with the viewer. This would be implemented as a new class, whose constructor would take a pinocchio::Model and a pinocchio::GeometryModel.

2 positive consequences of that would be:

• robot wrapper would not be systematically used, and only used for the algorithms. In particular, in TSID and Crocoddyl, the RobotWrapper is just their for functionnality 1 and 3, not for 2.
• when introducing new display like MeshCat in #662, we would not need to make RobotWrapper more complex but simply to introduce a new viewer_meshcat.py file with the corresponding clien class in it.

[jcarpent]

I would change also the syntax: not using loadModelsFromUrdf but rather loadModelsFromURDF

[jcarpent]

Concerning RobotWrapper, is already compatible with the creation from a Pinocchio Model.

[jcarpent]

* robot wrapper would not be systematically used, and only used for the algorithms. In particular, in TSID and Crocoddyl, the RobotWrapper is just their for functionnality 1 and 3, not for 2.

@nmansard Can you say in your discussion what are 1, 2 and 3?

[nim65s]

Conventions in python are RobotWrapper for class names, and load_model_from_urdf for functions / methods: https://www.python.org/dev/peps/pep-0008/#class-names / https://www.python.org/dev/peps/pep-0008/#id45

[jcarpent]

@nim65s But we need to stick to the C++ conventions I think, because we are only doing Python bindings and we want our code to interoperate between both languages.

[nim65s]

I disagree. I don't see why we could not respect c++ standards in c++ and python standards in python.

Also, this will upset users that are used to python and not c++. And this will also upset linters, if we ever come to use them.

And we agreed on using Google Coding Style, which says to follow this PEP8 for python.

And Boost Python is also using this style.

But these discussions have nothing to do with #663 :)

[gabrielebndn]

I do agree on separating the visualization from the wrappers functions, but I do not see any point on implementing a loadModelsFromURDF function. We have buildModelFromUrdf and buildGeomFromUrdf. What would a loadModelsFromURDF change? Writing one line instead of three? I think functionalities 1 and 3 can easily stay merged

[jcarpent]

When discussing this point with @nmansard , the idea was to load geometries, mechanism at the same time by just providing once the URDF file.

[gabrielebndn]

Really, the only advantage I see is that the user writes one line instead of three. The current interface is complete and intuitive enough, I think. Or maybe you want to fully rewrite the C++ function so that the URDF is only parsed once, for performance reasons? But I do not see much point in that, as URDF parsing is not time-critical in any way, and such a move would just complexify maintainance

[gabrielebndn]

What about:

• Defining a common interface for a generic viewer: each viewer client class is expected to implement methods initDisplay and display(q). They could also take care of the loading part, or maybe not
• Implement a GepettoViewerClient class for Gepetto Viewer
• Implement a MescatViewerClient class for Meshcat
• keep a viewer variable inside RobotWrapper, set by the user, which will be either None, or a GepettoViewerClient instance, or a MeshcatViewer instance, or any other future viewer type instance. Then, RobotWrapper itself will implement initDisplay and display(q), by simply calling the corresponding method on the viewer instance.

In this way, we would

• Achieve modularity
• Allow for easy extensibility
• Allow for separate use of the viewers
• Retain display capabilities of the RobotWrapper class
• Achieve some backwards compatibility (full backwards compatibility if we say Gepetto Viewer is the default)

Do you think it could be good? For this to really work in an easy and intuitive way, we would just need to make sure the various data objects are really the same between the viewer and the RobotWrapper instance. On the other hand, if the user is working without the wrapper, he/she should also manually set the data objects inside the viewer object, so the problem would still be there in a form

[nmansard]

I like this proposition.

In Python, I do think that writting 3 lines instead of 1 matters. A
little bit at least. I will not fight for this idea, but I will fight
for applying the same policy in RobotWrapper, otherwise people will
continue using RobotWrapper to spare the 2 extra lines of code while not
really using RobotWrapper.

I don't think that having the same Pinocchio::Data in both the viewer
and the wrapper is necessarily a good idea. Indeed, it is sometime
tricky to display something is the viewer and later discover that it had
a side effect on your data.

On 2/8/19 11:51 AM, gabrielebndn wrote:
>

What about:

• Defining a common interface for a generic viewer: each viewer
  client class is expected to implement methods initDisplay and
  display(q). They could also take care of the loading part, or
  maybe not
• Implement a GepettoViewerClient class for Gepetto Viewer
• Implement a MescatViewerClient class for Meshcat
• keep a viewer variable inside RobotWrapper, set by the user, which
  will be either None, or a GepettoViewerClient instance, or a
  MeshcatViewer instance, or any other future viewer type instance.
  Then, RobotWrapper itself will implement initDisplay and
  display(q), by simply calling the corresponding method on the
  viewer instance.

In this way, we would

• Achieve modularity
• Allow for easy extensibility
• Allow for separate use of the viewers
• Retain display capabilities of the RobotWrapper class
• Achieve some backwards compatibility (full backwards compatibility
  if we say Gepetto Viewer is the default)

Do you think it could be good? For this to really work in an easy and
intuitive way, we would just need to make sure the various data
objects are really the same between the viewer and the RobotWrapper
instance. On the other hand, if the user is working without the
wrapper, he/she should also manually set the data objects inside the
viewer object, so the problem would still be there in a form

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/stack-of-tasks/pinocchio/issues/663#issuecomment-461764453,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb-CBK-3qQ4pe42PkPKDDAQkdo4T6epks5vLVaZgaJpZM4aufHu.

[jmirabel]

Just a thought: Is there a reason to organize RobotWrapper such that it contains some Viewer member, rather that letting the Viewer having a RobotWrapper member ?

[matthieuvigne]

@gabrielebndn I like this idea of a generic viewer interface - it will be much more maintainable than the current (long) RobotWrapper code. As @jmirabel proposed, I think it would make sense to have a RobotWrapper instance as a member of the viewer.

[jcarpent]

@jmirabel I think if we split Viewer and Wrapper, then there is no need of having information relative to the viewer inside the RobotWrapper, and reversely.

[jmirabel]

@jcarpent I guess there has to be some shared information, hasn't it ?

[jcarpent]

Only the Model and the GeomModel for both collision and visualization, but they can be built by just copying information. But nothing else.
I think we should really split both structures for clarity and simplicity.

[gabrielebndn]

I think it's convenient to keep the functionality in the wrapper. Once the viewing part is modularized, there is no reason not to give an interface in the wrapper. Besides, completely removing the display functionalities would instantly break pretty much everybody's Python code. That is also a reason for putting the viewer inside the wrapper and not vice versa: compatibility

[gabrielebndn]

Actually, I do not see the point of putting a wrapper inside the viewer classes. If the wrapper is meant to be used internally by the viewer, then the normal Pinocchio functions might as well be used. If the wrappers methods are meant to be employed by the user, then I do not see it as a nice solution that the user should access them through an instance variable. A more elegant and python-like way would be that the viewer classes are subclasses of RobotWrapper, so that they inherit all the methods

[gabrielebndn]

The collision model is only needed by the wrapper, I think, while the visual model is only needed by the viewer, and the Model is needed by both. Corresponding data objects might or might not be shared (I favor sharing them)

[jcarpent]

@gabrielebndn you may want to plot the collision shapes too.

[gabrielebndn]

I am thinking that one of the stated purposes of separating the visualization part from the wrapper is so that "robot wrapper would not be systematically used". If we put a wrapper inside the viewer, that would basically defeat that purpose. So, I would rather keep the viewers as a completely separate entity that knows nothing about the wrapper, but then put a viewer inside the wrapper, as an additional thing which is wrapped, because that's the purpose of a wrapper, wrapping stuff. But, compared to the current situation, it would be a more flexible and easily-maintainable wrap

[jcarpent]

Done with #795.