Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TSID compatible with Pinocchio 2 #25

Closed

Conversation

huaijiangzhu
Copy link

This PR resolves the issue #23 by replacing the namespace se3 with pinocchio.

@jcarpent
Copy link
Collaborator

@huaijiangzhu Do you know why your system has changed the right on each file of TSID? Can you revert it?

@huaijiangzhu
Copy link
Author

@huaijiangzhu Do you know why your system has changed the right on each file of TSID? Can you revert it?

What do you mean by "right"?

@jviereck
Copy link
Contributor

What do you mean by "right"?

@huaijiangzhu, if you look at the file changes on GitHub, you see many files have no content change but an ownership/permission change from 100644 → 100755.

@huaijiangzhu
Copy link
Author

What do you mean by "right"?

@huaijiangzhu, if you look at the file changes on GitHub, you see many files have no content change but an ownership/permission change from 100644 → 100755.

I see. The new commits should fix it!

@jviereck
Copy link
Contributor

Builds with pinocchio v2 and make test passes for me. Well done @huaijiangzhu 👍 .

@wxmerkt
Copy link
Member

wxmerkt commented Jan 27, 2019

Awesome! Can this be merged before the MEMMO winter school such that people have an easier time installing it?

@andreadelprete
Copy link
Collaborator

I could merge it right now, but I am afraid we are right now in a limbo where people who install pinocchio from source would benefit from this PR, while people who install pinocchio from binaries would not, so I don't know if it's worth it. @jcarpent maybe has a clearer view of the status of Pinocchio's binaries?

@wxmerkt
Copy link
Member

wxmerkt commented Jan 27, 2019

That's a good point! We've also noticed that the Romeo example does not work when run from source.

@andreadelprete
Copy link
Collaborator

That's a good point! We've also noticed that the Romeo example does not work when run from source.

Maybe you could open an issue describing the problem(s) you've found? So I can try to help and hopefully everybody can benefit from this.

@andreadelprete
Copy link
Collaborator

I've checked and I can now confirm that Pinocchio's binaries are still on version 1.3, so merging this PR on master would make TSID incompatible with Pinocchio's binaries. For this reason I think it's better to create a new branch of TSID that is compatible with Pinocchio 2, so that people can choose which one to use depending on their version of Pinocchio.

@jcarpent
Copy link
Collaborator

@andreadelprete If I were you, I would let this PR open but set it in a waiting mode.
Indeed, @nim65s is about to release Pinocchio 2.0.0 on robotpkg this week. @nim65s Can you confirm this schedule?

@nim65s
Copy link
Contributor

nim65s commented Jan 28, 2019

I can confirm this is the expected schedule :)

@andreadelprete
Copy link
Collaborator

Too late, I've already merged it in the new branch pinocchio_v2

@wxmerkt
Copy link
Member

wxmerkt commented Jan 28, 2019

Is there a way to do it with a compile time define guard? [Version guard I mean]

@nim65s
Copy link
Contributor

nim65s commented Jan 28, 2019

We can put ADD_REQUIRED_DEPENDENCY("pinocchio < 2.0.0") in master and ADD_REQUIRED_DEPENDENCY("pinocchio >= 2.0.0") in the pinocchio_v2 branch

@wxmerkt
Copy link
Member

wxmerkt commented Jan 28, 2019

I was more thinking of a way to check for the pinocchio version during compile time and have defines for the API bits that changed. E.g.:

#ifdef PINOCCHIO_VERSION_2
using namespace se3 = pinocchio;
#endif

or similar - so it can be a single branch instead of two to be maintained.

@jcarpent
Copy link
Collaborator

Yes, there is such can of mechanism in Pinocchio itself. You just need to add the compilation flag -DPINOCCHIO_ENABLE_COMPATIBILITY_WITH_VERSION_1. See #23 for related discussions.

@nim65s nim65s mentioned this pull request Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants