[Parser] Provide API to avoid creating twice the same FCL geometry. #645

jmirabel · 2019-01-18T16:44:44Z

jcarpent

@jmirabel to ensue compatibility with older versions of hpp-fcl it would be nice to add some ifdef statement according to the versions of hpp-fcl.

jmirabel · 2019-01-19T09:54:34Z

@jmirabel to ensue compatibility with older versions of hpp-fcl it would be nice to add some ifdef statement according to the versions of hpp-fcl.

I can do it but:

I don't see any scenario where Pinocchio would be updated and not hpp-fcl,
it makes the code less readable.

bindings/python/parsers/parsers.hpp

jcarpent · 2019-01-31T10:02:43Z

bindings/python/scripts/robot_wrapper.py

@@ -18,7 +18,7 @@ def BuildFromURDF(filename, package_dirs=None, root_joint=None, verbose=False):
        robot.initFromURDF(filename, package_dirs, root_joint, verbose)
        return robot

-    def initFromURDF(self,filename, package_dirs=None, root_joint=None, verbose=False):
+    def initFromURDF(self,filename, package_dirs=None, root_joint=None, verbose=False, meshLoader=None):


You must handle the case where Pinocchio is no built with hpp-fclsupport.

What do you expect me to do here ? Raise an exception if meshLoader is provided and not built with hpp-fcl ?
If so, how do you get the information about hpp-fcl support in Python ?

One way is to check if hpp-fcl is provided, I was planning to do that by adding a global variable called maybe __PINOCCHIO_WITH_HPP_FCL__.
In the RobotWrapper, we have a similar strategy but checking

pinocchio/bindings/python/scripts/robot_wrapper.py

Line 27 in 36aeeeb

if "buildGeomFromUrdf" not in dir(pin):

if a function is included inside the namespace pinocchio.

As far as I understand, "buildGeomFromUrdf" is always part of pinocchio in Python, regardless the use of hpp-fcl. (even before this PR). So I guess this test is always false.

bindings/python/parsers/parsers.hpp

src/parsers/urdf.hpp

jcarpent · 2019-01-31T10:11:11Z

We need to first make a new release of hpp-fcl, then modifiy the minimal version requested in the CMakeLists.txtbefore merging this PR.

nim65s · 2019-01-31T10:45:45Z

the next release of hpp-fcl will be here by the end of the day

jmirabel · 2019-01-31T11:28:12Z

We need to first make a new release of hpp-fcl, then modifiy the minimal version requested in the CMakeLists.txtbefore merging this PR.

Yes, that's a requirement.

jmirabel · 2019-01-31T13:28:06Z

I pushed too quickly. Please, wait before reviewing.

…nGeometry

jmirabel · 2019-01-31T13:46:59Z

It should be fine now.

jcarpent · 2019-02-08T09:54:18Z

@nim65s What is the current status of the hpp-fcl release?

jmirabel · 2019-02-08T10:01:03Z

The release will wait for humanoid-path-planner/hpp-fcl#56.

jmirabel · 2019-02-11T13:03:50Z

There is a snake biting its own tail here:

on travis, Pinocchio uses hpp-fcl release of robotpkg so a new release of hpp-fcl is required to make it work,
to release hpp-fcl, we need to update all packages downstream, including Pinocchio.

I see three workarounds:

pushing to a branch like hpp-devel in Pinocchio, validate the whole HPP framework, release hpp-fcl, merge this and release the rest of HPP.
make Pinocchio devel branch use hpp-fcl devel branch.
merge this and all HPP waiting PRs and releasing them.

We need to decide on one of the above solutions to move on.

jmirabel · 2019-02-11T13:06:40Z

My favorite solution is 3 for its convenience. I suppose people using the devel branch of Pinocchio can also use hpp-fcl from source.

jcarpent · 2019-02-11T13:26:13Z

I think we should release first hpp-fcl and then update all the other packages. This action is not risky here.
Another solution is to introduce hpp-fcl-devel into robotpkg inside the wip branch. Easy to do and much more flexible than the three other solutions!

jmirabel · 2019-02-11T13:28:19Z

Another solution is to introduce hpp-fcl-devel into robotpkg inside the wip branch. Easy to do and much more flexible than the three other solutions!

Can you do it ?

jcarpent · 2019-02-11T13:36:36Z

I think for this tack @nim65s will be faster and much more efficient than me.

jmirabel · 2019-02-11T13:40:54Z

I will let @nim65s <https://github.com/nim65s> reply but I think his stack of packages to release is already rather big.

nim65s · 2019-02-11T14:04:09Z

No, sorry, but I will not duplicate a package in robotpkg just have it available in multiple versions, and even more maintain multiple times the same package.

Yes, it would be really fast and easy to do so, but I am pretty sure that it will bring me nightmares in the future, and that at the end we will all loose time by trying to use shortcuts.

nim65s · 2019-02-11T14:04:33Z

I see 2 solutions:

we update all robotpkg releases at once (@jmirabel 's solution n.3)
we release hpp-fcl, and I add temporary patches to all packages dependending on hpp-fcl to be sure their old versions still work with the new hpp-fcl, and then we can proceed to release every other packages and remove those patches. eg.: nim65s/robotpkg@554d0f3

I find the solution 2. a bit more elegant and less error-prone and stressful, but it require extra work. This work might be worth it or not, I can't tell yet.

jcarpent · 2019-02-11T14:30:16Z

I totally understand your reasons to not duplicate the package. But, we are very limited by the fact that all the packages are inter-dependent, and any break in the compilation procedure will affect all the users and all the robots together. I continue to think that we should adopt a fast release solution like the one that I've just suggested.

I would also prefer also the second version, which seems to be more elegant indeed.

nim65s · 2019-02-11T14:37:18Z

I totally agree with the "Release early, release often" strategy :)

nim65s · 2019-02-12T10:29:45Z

Looking at this once again, releasing everything at once will break CI/CD everywhere. Is everybody OK with this ?
If so, what are we waiting for ?
Or are you still wanting me to provide a parallel CI/CD using devel branches of some dependencies ?

jcarpent · 2019-02-12T10:41:10Z

You can start with the head of the dependency at least.

stonneau · 2019-02-12T10:42:48Z

I guess we are waiting for us, since rbprm has yet to be updated to pinocchiov2 before integrating these changes (ongoing but not so smooth)

jmirabel · 2019-02-12T10:43:21Z

I let you choose the method you prefer.

olivier-stasse · 2019-02-12T10:57:53Z

I am not ok to break everything everywhere I have at least three remote collaborations which are based on the consistency of robotpkg.

We have tools to check the consistency of candidate releases inside robotpkg.

robotpkg cannot handle all the combinaison of possible releases.
For CI on devel, a solution could be to use a custom docker.
It could be created by using robotpkg, make checkout and a list of branches validated by the responsible of the repository.

nim65s · 2019-02-12T10:58:42Z

Ok, therefore, I propose to start by merging PR in hpp-fcl and make a release of this package, but not to push it on robotpkg.
This way, no project from the team will suffer from it, and @jcarpent will be able to manually check that we can do the same with pinocchio, using the .tar.gz of hpp-fcl, as the CI won't work.
Once this will be done, @jmirabel will be able to finish the release of HPP while I finish the release the SoT in the same way (@jmirabel will probably prefer to use the tag instead of the tar.gz, but it doesn't change anythnig)
This lets time to @stonneau to finish with RBPRM (maybe it would have been better not to merge humanoid-path-planner/hpp-doc#37 )

Then, when every release is done, I will put everything at once on robotpkg, and CI will work again.
I let you choose individually if you want to put the release tag on the master branch or not before the CI works again.

nim65s · 2019-02-12T11:00:34Z

Also, @wxmerkt will be happy to have a new github release of hpp-fcl soon :)

nim65s · 2019-02-12T11:06:50Z

Oh, I forgot to sync also with @andreadelprete about TSID…

jmirabel · 2019-02-12T11:44:05Z

I propose to start by merging PR in hpp-fcl

I agree.

and make a release of this package

this could surely wait for validation, but if you want to do it, go ahead.

but not to push it on robotpkg.

I agree.

jcarpent · 2019-02-12T12:17:35Z

@nim65s @olivier-stasse All the difficulties to have some stable releases during the validation of the next releases let me think that we really need to put in the WIP of robotpkg the clone of all the existing packages with the prefix -dev or -wip in order to allow smooth maintenance and the release process.

nim65s · 2019-02-12T13:15:27Z

hpp-fcl v1.0.0 is out on github.
@jcarpent : can you merge this, update https://github.com/stack-of-tasks/pinocchio/blob/master/CMakeLists.txt#L76 and create a new github release ?

nim65s · 2019-02-12T13:23:44Z

The idea of having simultaneously multiple versions for the same package in the same repo, and that any user or any automated test could be using one or the other at any time terrifies me, this is why I do not want to do this myself, nor I want to maintain it, nor I want to help anybody who use it.
Sorry, but this is really too much work for me.

jcarpent · 2019-02-12T13:39:08Z

@nim65s We cannot simply do what you suggest.
The current version of Pinocchio is not compatible with hpp-fcl because of the namespace change.

nim65s · 2019-02-12T13:41:35Z

That's right. I will have a look at it.

jcarpent · 2019-02-12T13:46:23Z

@jmirabel @nim65s I have created another branch on the main repo to allow to use it as a sandbox, i.e. we don't care if travis is happy or not.

jmirabel · 2019-02-12T14:34:56Z

Thank for the merge.

olivier-stasse · 2019-02-13T05:44:15Z

Dear Justin,
Maybe the problem comes from the fact that all CI is done against robotpkg binaries.

Looking at the devel branch the CMakeLists.txt specifies that the current code is consistent with hpp-fck 0.5.1 where robotpkg provides 0.7.0.

If you provides a set of branches of your gepetto dependencies on which we can build the devel branch it is possible to build a docker file from the source of robotpkg (and not the binaries).

A more coherent way will be to release indenpendtly from robotpkg-binaries each package.

Wrt to wip, it already exists in robotpkg. The recurrent problem is that you cannot release package in robotpkg for binaries if they are not consistent together.

But we use robotpkg from source to make deployment test with specific release candidate.
This could be use to build the appropriate dockerfile.

We could try provide the script to build the docker file, but that would be possible only if the CMakeLists.txt is consistent.

jcarpent · 2019-02-13T08:45:30Z

A more coherent way will be to release indenpendtly from robotpkg-binaries each package.

I totally agree. Robotpkg is now becoming the limit of our approach and the docker approach seems to be a nice solution.

nim65s · 2019-02-13T09:17:28Z

Agreed too.

olivier-stasse · 2019-02-13T10:01:49Z

Just one precision, right now, if we provide the script to build the dockerfile for a specific branch, it will be the duty of the developer to:

use the provided script to build the dockerfile and
put it for the specific branch.

jcarpent · 2019-02-13T10:03:33Z

@olivier-stasse Yes, I totally agree on these two points.

jmirabel · 2019-02-18T10:12:10Z

For the update, I will merge devel into fcl-release, add a few minor changes and validate HPP (including hpp-fcl next release) in the next few days. I will then open a PR of fcl-release into devel.
Any objections ?

jcarpent · 2019-02-18T10:17:23Z

It sounds great to me. You can proceed as you've suggested.

jcarpent requested changes Jan 19, 2019

View reviewed changes

jmirabel force-pushed the devel branch from a5fbe86 to daccb1e Compare January 30, 2019 15:46

jcarpent requested changes Jan 31, 2019

View reviewed changes

[Parser] Provide API to avoid creating twice the same FCL geometry.

d82aeb2

jmirabel force-pushed the devel branch from daccb1e to 79f268f Compare January 31, 2019 13:19

jmirabel added 3 commits January 31, 2019 14:44

[Python] Bind API to parse geometries without duplication of Collisio…

f70a4bc

…nGeometry

[Python] Handle compilation with hpp-fcl.

c01ab6b

[Minor] Add missing ifdef PINOCCHIO_WITH_HPP_FCL

2563c9c

jmirabel force-pushed the devel branch from 79f268f to 2563c9c Compare January 31, 2019 13:44

jcarpent approved these changes Feb 1, 2019

View reviewed changes

jmirabel added 2 commits February 7, 2019 09:50

[Python] Fix bindings of buildGeomFromUrdf

6a78713

[FCL] Update to change of namespace

2a71ef3

nim65s mentioned this pull request Feb 11, 2019

Update to Pinocchio v2 + plugins + varying right hand side. humanoid-path-planner/hpp-core#148

Merged

nim65s mentioned this pull request Feb 12, 2019

Release & Pinocchio v2 stack-of-tasks/tsid#31

Closed

jcarpent changed the base branch from devel to topic/fcl-release February 12, 2019 13:45

jcarpent merged commit 2019cdc into stack-of-tasks:topic/fcl-release Feb 12, 2019

This was referenced Feb 13, 2019

Tests & Python3 #673

Merged

RNEA test fails on 18.04 #674

Closed

nim65s mentioned this pull request Feb 21, 2019

Update to new hpp-fcl namespace wrapping #696

Closed

[Parser] Provide API to avoid creating twice the same FCL geometry. #645

[Parser] Provide API to avoid creating twice the same FCL geometry. #645

Conversation

jmirabel commented Jan 18, 2019

jcarpent left a comment

Choose a reason for hiding this comment

jmirabel commented Jan 19, 2019 • edited Loading

jcarpent Jan 31, 2019

Choose a reason for hiding this comment

jmirabel Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

jcarpent Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

jmirabel Jan 31, 2019

Choose a reason for hiding this comment

jcarpent commented Jan 31, 2019

nim65s commented Jan 31, 2019

jmirabel commented Jan 31, 2019

jmirabel commented Jan 31, 2019

jmirabel commented Jan 31, 2019

jcarpent commented Feb 8, 2019

jmirabel commented Feb 8, 2019

jmirabel commented Feb 11, 2019

jmirabel commented Feb 11, 2019

jcarpent commented Feb 11, 2019

jmirabel commented Feb 11, 2019

jcarpent commented Feb 11, 2019

jmirabel commented Feb 11, 2019 via email

nim65s commented Feb 11, 2019

nim65s commented Feb 11, 2019 • edited Loading

jcarpent commented Feb 11, 2019

nim65s commented Feb 11, 2019

nim65s commented Feb 12, 2019

jcarpent commented Feb 12, 2019

stonneau commented Feb 12, 2019

jmirabel commented Feb 12, 2019

olivier-stasse commented Feb 12, 2019

nim65s commented Feb 12, 2019

nim65s commented Feb 12, 2019

nim65s commented Feb 12, 2019

jmirabel commented Feb 12, 2019

jcarpent commented Feb 12, 2019

nim65s commented Feb 12, 2019

nim65s commented Feb 12, 2019

jcarpent commented Feb 12, 2019

nim65s commented Feb 12, 2019

jcarpent commented Feb 12, 2019

jmirabel commented Feb 12, 2019

olivier-stasse commented Feb 13, 2019 • edited Loading

jcarpent commented Feb 13, 2019

nim65s commented Feb 13, 2019

olivier-stasse commented Feb 13, 2019

jcarpent commented Feb 13, 2019

jmirabel commented Feb 18, 2019

jcarpent commented Feb 18, 2019

jmirabel commented Jan 19, 2019 •

edited

Loading

jmirabel Jan 31, 2019 •

edited

Loading

jcarpent Jan 31, 2019 •

edited

Loading

nim65s commented Feb 11, 2019 •

edited

Loading

olivier-stasse commented Feb 13, 2019 •

edited

Loading