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

C++11 Templated Using #13

Open
Jmeyer1292 opened this issue May 14, 2018 · 6 comments
Open

C++11 Templated Using #13

Jmeyer1292 opened this issue May 14, 2018 · 6 comments
Assignees

Comments

@Jmeyer1292
Copy link

Would y'all be up for a revamping of this package that used c++11's templated using syntax?

template <class T>
using EigenStlVector = std::vector<T, Eigen::aligned_allocator<T>>;

// ... All of the types for backwards compatibility
// for example
using vector_Affine3d = EigenStlVector<Eigen::Affine3d>;

It's just a generic way for us to support all templated Eigen types and is a little more modern. The cost is it requires c++11, so Kinetic+.

If you're down for it, I'll do the work. Please let me know.

@sloretz
Copy link
Contributor

sloretz commented May 14, 2018

Sounds fine to me.

Target the PR at the branch master. This repo uses master branch for indigo, kinetic, lunar, and melodic, but I'll make a branch specifically for indigo if there ever needs to be another release there.

@clalancette thoughts?

@clalancette
Copy link
Contributor

I'll just ask the dumb question; what's the benefit of updating it to C++11 style? Does it make it easier to use or more efficient for downstream consumers? If the answer is yes, then I'd be for it, but if the answer is no, then it just seems like unnecessary churn. But I'm not a C++ expert, so let me know what you see as the benefits.

@Jmeyer1292
Copy link
Author

@clalancette It removes the need for all of the explicit typedefs for each type you want to support. Whereas now you have

typedef std::vector<Eigen::Vector3f, Eigen::aligned_allocator<Eigen::Vector3f> > vector_Vector3f;
typedef std::vector<Eigen::Vector3d, Eigen::aligned_allocator<Eigen::Vector3d> > vector_Vector3d;
typedef std::vector<Eigen::Vector4f, Eigen::aligned_allocator<Eigen::Vector4f> > vector_Vector4f;
typedef std::vector<Eigen::Vector4d, Eigen::aligned_allocator<Eigen::Vector4d> > vector_Vector4d;
typedef std::vector<Eigen::Affine3f, Eigen::aligned_allocator<Eigen::Affine3f> > vector_Affine3f;
typedef std::vector<Eigen::Affine3d, Eigen::aligned_allocator<Eigen::Affine3d> > vector_Affine3d;
typedef std::vector<Eigen::Isometry3f, Eigen::aligned_allocator<Eigen::Isometry3f> > vector_Isometry3f;
typedef std::vector<Eigen::Isometry3d, Eigen::aligned_allocator<Eigen::Isometry3d> > vector_Isometry3d;

Only these exact types are supported. With the c++11 style, all of these would go away in new code and you just write

template<class T>
using EigenStlVector = std::vector<T, Eigen::aligned_allocator<T>>

// ... and in user code ...

EigenStlVector<Eigen::Projective3d> camera_matrices;

I find myself using Eigen types outside of the ones defined here quite a bit: often planes, parameterized lines, projection matrices, or 2D points. I kept defining my own, so I wanted to broach the idea here.

In the near term, we gain support for arbitrary Eigen types. In the long term, we can shrink this entire package to about 4 lines of code.

But, this package changing will probably break a lot of stuff because its introducing an interface dependency on C++11 that wasn't there before.

@Jmeyer1292
Copy link
Author

@clalancette Any opinion?

@sloretz
Copy link
Contributor

sloretz commented May 18, 2018

Hi @Jmeyer1292, if you would be willing to make a PR we would be happy to look at it.

Talking with @clalancette offline, given how simple the package is (only one branch for all ROS distros) we're leaning towards keeping it that way and waiting until after indigo is EOL to merge something requiring c++11.

@Jmeyer1292
Copy link
Author

@sloretz I understand and would probably do the same in your shoes. None the less, I'd I'll leave a pull request for feedback and hopefully eventually it'll find its way in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants