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

New approach for toMsg()/fromMsg() #427

Open
wants to merge 15 commits into
base: rolling
Choose a base branch
from

Conversation

gleichdick
Copy link
Contributor

This is a rewrite of #368. Depends (more or less) on #422, #423, #424, #425 and #425, so it should be rebased and merged once these PRs are merged. First actual commit of this PR is 68a1193.

@gleichdick gleichdick marked this pull request as draft May 26, 2021 19:49
@gleichdick gleichdick mentioned this pull request May 29, 2021
@gleichdick gleichdick force-pushed the ros2_new_toMsg_approach_v2 branch 2 times, most recently from 5c886e3 to 3951ce6 Compare May 29, 2021 14:33
@clalancette
Copy link
Contributor

@gleichdick OK, I think all of the other ones have been merged now. When you get a chance, a rebase of this one would be appreciated (or I can do it if you don't have time). Once we have that done, we can take a look at the current state of this.

@gleichdick
Copy link
Contributor Author

So here we go...
One thing came to my mind: As the messages can be used with a custom allocator, does it make sense to add support for it in the conversion methods?

@gleichdick gleichdick marked this pull request as ready for review June 1, 2021 14:20
@clalancette
Copy link
Contributor

One thing came to my mind: As the messages can be used with a custom allocator, does it make sense to add support for it in the conversion methods?

Not at the moment, no. It turns out that we need to revamp how the custom allocators are done since they don't quite work right at present. Until we know what that solution looks like, let's just stay away from the custom allocators.

@clalancette clalancette self-assigned this Jun 17, 2021
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gleichdick , do you mind to fix the conflicts?

@gleichdick
Copy link
Contributor Author

Just did a rebase

test_tf2/test/test_convert.cpp Outdated Show resolved Hide resolved
tf2/include/tf2/convert.h Outdated Show resolved Hide resolved
tf2/include/tf2/convert.h Outdated Show resolved Hide resolved
tf2/include/tf2/convert.h Outdated Show resolved Hide resolved
tf2/include/tf2/impl/convert.h Outdated Show resolved Hide resolved
tf2/include/tf2/impl/convert.h Outdated Show resolved Hide resolved
tf2/include/tf2/impl/stamped_traits.hpp Outdated Show resolved Hide resolved
tf2_bullet/include/tf2_bullet/tf2_bullet.hpp Outdated Show resolved Hide resolved
Tests for wrench conversion, Eigen <-> KDL
and the error messaging facilities from convert.h
they were disabled because of MSVC and ADL
(MSVC couldn't choose between tf2::toMsg and Eigen::toMsg)
@gleichdick
Copy link
Contributor Author

The fixups for the header includes and the doxygen comments are now squashed.

@guilyx
Copy link
Contributor

guilyx commented Dec 4, 2021

Hi there,

Any update on this ?

@gleichdick
Copy link
Contributor Author

Well, I started working on this PR almost one year ago. Indeed, it is pretty big and hard to review, but I think this redesign of geometry2 is necessary.
@clalancette @ahcorde I'm intending to close this PR after the Humble Hawksbill API freeze (April 4, 2022).

@shrijitsingh99
Copy link

Hey, is this planned to be merged?

@methylDragon
Copy link

methylDragon commented Dec 15, 2022

@gleichdick I'm assigning myself to this PR since it's gotten so stale and I'd like to get this merged...
I'm sorry this took so long to pick up again

Do you think this is this still necessary, and if so, would you be able to rebase it?
I'm iffy on downstream impacts

@methylDragon methylDragon self-requested a review December 15, 2022 18:44
@gleichdick gleichdick force-pushed the ros2_new_toMsg_approach_v2 branch 4 times, most recently from f017422 to 66ad137 Compare December 16, 2022 21:25
@gleichdick
Copy link
Contributor Author

gleichdick commented Dec 16, 2022

I'm sorry, I stopped using ROS more than a year ago. I don't have any clue on how big the impact is on downstream projects, but things will break for sure. And the ros2 branch has moved quite a bit. I don't see myself spending more time on this.

But in my mind the reasons why I wrote this PR are still valid

@methylDragon
Copy link

I'm sorry, I stopped using ROS more than a year ago. I don't have any clue on how big the impact is on downstream projects, but things will break for sure. And the ros2 branch has moved quite a bit. I don't see myself spending more time on this.

But in my mind the reasons why I wrote this PR are still valid

Alright, I understand; thanks so much for your contributions so far!

@clalancette
Copy link
Contributor

@methylDragon I'm going to reopen this, as I do think the idea here is valid. It's just that this is a large change, so needs a lot of thought.

@tfoote
Copy link
Contributor

tfoote commented Sep 22, 2023

I agree that this is something that would be great to bring in and modernize this part of the API. But it's also something that we'll need to think closely about and make sure that there's a good design document or possibly REP with a migration path clearly defined as well as appropriate deprecation process with so many people potentially using this.

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.

7 participants