Closed jcarpent closed 5 years ago
Completely agree, especially on warnings, header-only and macro-naming
A CMake option to generate a library containing a default implementation would be nice as well and should not be hard to obtain. The compilation time was already an issue and keep increasing a lot. It would save time to developers and energy...
The method on how to do it and the content of the library should not be discussed here.
And a porting from 1.3 to 2 note.
I believe we should study a "rationalization" of Pinocchio headers, as @nmansard put it in the Overview of the doc. That is to say, super-headers that include smaller headers, grouped by theme, the way they do in Eigen with #include <Eigen/Core>
. This will definitely make Pinocchio easier to use in C++, but we should keep in mind that, if done incorrectly, this may non-negligibly further increase compilation time (I prepared an example). I already have a grouping in mind. If you think it is a good idea, you can open an issue on the subject
I also think that when Pinocchio 2.0.0 is officially out, a "minimal" doc should be available in the master branch. By minimal, I mean what we have now in topic/doc-v2 + clean doxygen comments
This is already in the pipe ;) But in any case, by gathering headers, it won't improve compilation times.
When I said in the pipe, I meant that I have already implemented such a structure. But this one should be added carefully to the repository, to avoid breaking of inclusion for current projects using Pinocchio.
@jcarpent of course it won't improve it, I'm saying the opposite: it risks degrading it a lot. That's why I say the grouping should be done in a clever way, so that you do not end up including too many useless headers.
I have used the inclusion naming of the current directory of src, which are pretty evident.
I was thinking of a more granular approach, say, a header which only contains kinematics-related methods, one for dynamics, one for geometry, etc.
Then I think the current organization is sufficient.
The idea of grouping by theme is also to manage in the correct order all the inclusions.
#include <iostream>
#include "pinocchio/spatial/se3.hpp"
int main()
{
se3::SE3 aMb = se3::SE3::Random();
se3::SE3 bMc = se3::SE3::Random();
se3::SE3 aMc = aMb*bMc;
std::cout << "aMb:\n" << aMb << std::endl;
std::cout << "bMc:\n" << bMc << std::endl;
std::cout << "aMc:\n" << aMc << std::endl;
}
The above code takes less than a second to compile on my system, both with gcc and clang (tested on Pinocchio 1.3). However, when I include pretty much the whole Pinocchio library, it takes 15 seconds with clang and 30 (!) with gcc. I think more granularity would not be bad. I can easily imagine situations where one is only interested in kinematics but not about dynamics, on where you do not need geometry, or where you need the parsers but you could not care less about the sample models. I think it is a point to be considered. Also, I do not think inclusion order is a problem.
Inclusion order is a problem to avoid undefined forward declaration when using the codes. This is where Eigen is very strong: they have a very high level to organize their files in order to avoid double inclusions and so on.
You can have something like:
But I think the idea of @jmirabel of generating code for double is the right way of saving compilation times, and should be the priority.
I think @jmirabel had a very good idea, but I also think that if the pre-compiled library becomes the default way of using Pinocchio it kind of defeats the purpose of making Pinocchio header-only. I don't see what the problem would be in having a simple pinocchio/Kinematics
We never said that this is the default way. The reason why I have worked so far on the version 2.0.0 is really to make Pinocchio header-only. But providing a way to use existing compiled code is ALSO an important direction that we need to consider and implement.
Then, you have to decide what is Kinematics (forward Kinematics of course, center of mass?). And the level of granularity, which is hard to define.
@gabrielebndn any suggestion?
As a general guideline, I would say anything that touches inertial data is dynamics. I can try to come up with atentative list in a separate issue later today
After merging #592, I will be done of all my tasks. @jmirabel @nim65s Do HPP and SoT are now compatible with Pinocchio 2.0.0-alpha? @aelkhour have you encountered any other troubles with Pinocchio 2.0.0-alpha?
No issues on our side so far, thanks for the fast issue resolutions !
There are still no porting notes. It seems visitors have changed.
How to write visitors now ? Is there a simple way of converting old style visitors ?
Yes, the visitors have been simplified a lot. There is no more need to add the macro inside the Algo struct.
The derived class of a JointVisitorBase
should on declare the type of arguments (ArgsType
). The rest is handled by JointVisitorBase
internally. This simplifies a lot the writing of new algorithms I think.
@jmirabel Are you convinced by this answer?
I would like to make the release of Pinocchio as soon as possible. Wandercraft, Willow and HPP are ready for this merging. @olivier-stasse, @nim65s Are you ready for the SoT?
Hi @jcarpent,
We are now ready for the SoT, so you can release v2 when you want, and I will put it on robotpkg afterwards. Sadly, this can't include CppAD / CppADCodeGen by default in 16.04, as the default Eigen version is too old (ref https://github.com/stack-of-tasks/pinocchio/issues/617#issuecomment-447939232)
Yes. I will do it. @nim65s Thank you for all the work you have done so far!
What to do next?
Before releasing the new version of Pinocchio 2.0.0, I would suggest fixing the following bullet points:
Feel free to add additional points.