additional constructor for collisionrequest is a bad idea

[stonneau]

6732f69

Because of the default parameter signature in the other constructor, and
because bool and size_t can be used indifferently, the constructor called is undefined
depending on the parameters given. As a matter of fact, the constructor called
in CollisionValidation is not the one you think. This leads to a bug where max_num_contacts parameter
is in fact set to 0. I am creating a pull request to fix this in hpp-core, but the ambiguity should be removed

[florent-lamiraux]

If you are true, there should be a compilation warning.

[jmirabel]

I would be in favor of using flags rather than constructors with a series of booleans.
I find CollisionRequest (Contact | DistanceLowerBound, 1) more readable than CollisionRequest (true, 1, true)...

[stonneau]

Well if you create a collision request you should see a warning stating that max_num_contacts is 0. I spotted it in the rb-rrt test of hpp-rbrm, because I was expecting collisions that did not occur. ps: depending on the compiler you don't always get a compilation warning in such cases, in particular with default constructors. See for instance a more complex case: http://www.gotw.ca/gotw/005.htm

…

________________________________ De : Joseph Mirabel <notifications@github.com> Envoyé : mardi 8 janvier 2019 13:49 À : humanoid-path-planner/hpp-fcl Cc : stonneau; Author Objet : Re: [humanoid-path-planner/hpp-fcl] additional constructor for collisionrequest is a bad idea (#39) I would be in favor of using flags rather than constructors with a series of booleans. I find CollisionRequest (Contact | DistanceLowerBound, 1) more readable than CollisionRequest (true, 1, true)... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#39 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEaCppJQ_q82coEOQe0jlIW7WnEtKu9Lks5vBJPhgaJpZM4Z1Xrk>.

[florent-lamiraux]

I meant that the constructor that is called is deprecated. The compiler should complain or call the other one.

[stonneau]

In this case my pull request should not be merged.

…

________________________________ De : Florent Lamiraux <notifications@github.com> Envoyé : mardi 8 janvier 2019 13:55 À : humanoid-path-planner/hpp-fcl Cc : stonneau; Author Objet : Re: [humanoid-path-planner/hpp-fcl] additional constructor for collisionrequest is a bad idea (#39) I meant that the constructor that is called is deprecated. The compiler should complain or call the other one. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#39 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEaCptep9LcRE3uNgZxiE5IqCMcUc1Avks5vBJU7gaJpZM4Z1Xrk>.

[florent-lamiraux]

I agree with Joseph.

[stonneau]

I'd agree as well, although this drives us further away from the original fcl api.
This is not only about an hypothetic merge, but also with respect to the rest of the api that does not follow this principle.

In any case, such modifications should be conducted on another branch than devel, until the api change is actually confirmed.

[stonneau]

Here it is:
#54