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 382 forks source link

Is GeometryObject::ngeoms really useful ? #1587

Closed florent-lamiraux closed 2 years ago

florent-lamiraux commented 2 years ago

https://github.com/stack-of-tasks/pinocchio/blob/593d4d43fded997bb9aa2421f4e55294dbd233c4/src/multibody/geometry.hpp#L168 Is there any difference between ngeoms and geometryObjects ? If no, the first one should be deprecated and replaced by a const reference to the second one. For some robotics demo, I add and remove geometries from my model and I got confused since geometryObjects.erase(it) needs to be coupled with ngeoms--.

By the way, I wrote a function that removes a GeometryObject instance from a GeometryModel and removes the corresponding collision pairs. The input of the function is the name of the object. Do you think this function should be added to pinocchio ?

jcarpent commented 2 years ago

By the way, I wrote a function that removes a GeometryObject instance from a GeometryModel and removes the corresponding collision pairs. The input of the function is the name of the object. Do you think this function should be added to pinocchio ?

Yes, of course.

jcarpent commented 2 years ago

Is there any difference between ngeoms and geometryObjects ? If no, the first one should be deprecated and replaced by a const reference to the second one. For some robotics demo, I add and remove geometries from my model and I got confused since geometryObjects.erase(it) needs to be coupled with ngeoms--.

Why not. How do you see the solution?

florent-lamiraux commented 2 years ago

In class GeometryModel:

const Index& ngeoms;

and in the constructor:

    GeometryModel()
    : ngeoms(geometryObjects.size())
    , geometryObjects()
    , collisionPairs()
    {}

If the compiler accepts, this should do the job.

jcarpent commented 2 years ago

It will lead to some memory leaks I guess, as std::vector<T>::size follows this signature:

size_type size() const noexcept;
florent-lamiraux commented 2 years ago

It will lead to some memory leaks I guess, as std::vector<T>::size follows this signature:

I do not think so. There is no memory allocation involved.

jcarpent commented 2 years ago

Yes, but the return object is a temporary variable: it is not held by the std::vector object.

florent-lamiraux commented 2 years ago

Let see what the compiler thinks about it. I will try tomorrow.

jcarpent commented 2 years ago

Let's then bet a 🍺 dear Florent!?

florent-lamiraux commented 2 years ago

I think you are right. My solution won't work. Then I will propose a PR to add a method that removes an object. Calling this method, users will not need to handle variable ngeoms.