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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 51 commits into from
Closed

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

wants to merge 51 commits into from

Conversation

gleichdick
Copy link
Contributor

@gleichdick gleichdick commented Jan 25, 2021 •
edited
Loading

I did a complete rework of the conversion utilities, with the goal to avoid link-time errors and to make tf2::convert more bullet-proof. In ROS1, various issues arised, mainly because of the forward declaration of toMsg() as a templated function, followed by the implementations for Eigen, KDL and friends being traditional overloaded functions (without template<>, so no specialization). This is a port of ros/geometry2#491 and addresses #242.

Now, fromMsg() and toMsg() both have a default implementation, by redirecting into impl::ImplDetails structs. SFINAE with structs allow some more flexibility than the current approach, with the bonus of compile-time errors when a specific implementation is missing.
The converison of stamped types geometry_msgs::...Stamped and tf2::Stamped<T> now uses the conversion functions of the underlying unstamped types.

To do:

  • Documentation
  • Formatting (which .clang-format to use?)
  • Some more unit tests

This was referenced Jan 26, 2021
Closed
Closed
@gleichdick gleichdick marked this pull request as ready for review January 29, 2021 16:27
@gleichdick gleichdick changed the title [WIP] New approach for toMsg()/fromMsg() New approach for toMsg()/fromMsg() Jan 29, 2021
@gleichdick gleichdick mentioned this pull request Feb 7, 2021
Merged
@gleichdick
Copy link
Contributor Author

Just rebased onto ROS2, I hope I didn't drop anything.
Friedly ping to get some comments, as the deadlines for Galactic come closer @clalancette @tfoote.

@gleichdick
Copy link
Contributor Author

Friendly ping

ahcorde
ahcorde requested changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to run the CI while I'm reviewing this PR (it's quite big)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

There is a conflict in one file, do you mind to fix it ?

test_tf2/test/test_convert.cpp Outdated Show resolved Hide resolved
test_tf2/test/test_convert.cpp Outdated Show resolved Hide resolved
ahcorde
ahcorde reviewed Mar 19, 2021
View reviewed changes
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.

CI is failing https://ci.ros2.org/job/ci_linux-aarch64/8809/console

Maybe you need to add here sensor_msgs

@ahcorde
Copy link
Contributor

ahcorde commented Apr 6, 2021

Sorry I forgot this PR. I would prefer to add the static_casts instead of disabling the warning

@gleichdick
Add static_cast to tf2_bullet
3278ca2 
bullet uses floats, which causes narrowing warnings
@gleichdick
Copy link
Contributor Author

Okay, I agree 😀

ahcorde
ahcorde requested changes Apr 8, 2021
View reviewed changes
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.

some nits and comments

tf2/include/tf2/impl/convert.h
alwaysFalse<Datatype>,
"No default message type defined, "
"please check your header file includes!");
// using type = ...;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?

tf2/include/tf2/impl/convert.h
alwaysFalse<Datatype>,
"No default transform type defined, "
"please check your header file includes!");
// using type = ...;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?

tf2/include/tf2/impl/convert.h
Comment on lines +149 to +150
// void toMsg(const Datatype&, Message&);
// void fromMsg(const Message&, Datatype&);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?

tf2/include/tf2/impl/convert.h
Comment on lines +193 to +197
// using UntampedType = ...;
// static UntampedType& accessMessage(StampedMsg &);
// static UntampedType getMessage(StampedMsg const&);
// static CovarianceType & accessCovariance(MsgWithCovarianceStamped &);
// static CovarianceType getCovariance(MsgWithCovarianceStamped const&);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?

tf2/include/tf2/impl/convert.h
Comment on lines +229 to +230
// using StampedType = ...;
// using StampedTypeWithCovariance = ...;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?
if you don't want to remove these comments at least add a comment explaining the meaning. (some for all)

tf2/include/tf2/impl/convert.h Show resolved Hide resolved
tf2/include/tf2/impl/convert.h Show resolved Hide resolved
tf2_geometry_msgs/CMakeLists.txt Outdated
@@ -30,6 +30,17 @@ ament_auto_find_build_dependencies(REQUIRED ${required_dependencies})
if(BUILD_TESTING)
find_package(ament_cmake_gtest REQUIRED)
find_package(rclcpp REQUIRED)

find_package(ament_cmake_cppcheck REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you add common ? instead of run only 4 tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cppcheck needs to be called manually (with the LANGUAGE "c++" option), because of the header file suffix being .h. Otherwise it would only recognize them as plain C headers and complain about non-C code.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to configure this before.

Some ament links:

tf2_sensor_msgs/CMakeLists.txt
find_package(ament_cmake_gtest REQUIRED)
find_package(ament_cmake_pytest REQUIRED)

#ament_add_pytest_test(test_tf2_sensor_msgs_py test/test_tf2_sensor_msgs.py)
Copy link
Contributor

Choose a reason for hiding this comment

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

add a TODO or remove

tf2_kdl/CMakeLists.txt Outdated
@@ -34,6 +34,14 @@ if(BUILD_TESTING)
find_package(rclcpp REQUIRED)
find_package(tf2_msgs REQUIRED)

find_package(ament_cmake_cppcheck REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

some here ? why only these 4 tests?

gleichdick and others added 2 commits April 8, 2021 20:17
@gleichdick @ahcorde
Fix formatting nitpicks
7795ded 
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
@gleichdick
Add TODOs and comments
d3fb3bd
ahcorde
ahcorde requested changes Apr 12, 2021
View reviewed changes
tf2_geometry_msgs/CMakeLists.txt Outdated
@@ -30,6 +30,17 @@ ament_auto_find_build_dependencies(REQUIRED ${required_dependencies})
if(BUILD_TESTING)
find_package(ament_cmake_gtest REQUIRED)
find_package(rclcpp REQUIRED)

find_package(ament_cmake_cppcheck REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to configure this before.

Some ament links:

@gleichdick
Copy link
Contributor Author

Thanks for the hint for ament_lint_auto! I hope I did everything right while defeating ament_copyright...

@gleichdick
Copy link
Contributor Author

Friendly ping for another CI shot 😁

@ahcorde
Copy link
Contributor

ahcorde commented May 12, 2021

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@gleichdick
Copy link
Contributor Author

Due to the latest changes on the ros2 branch, a lot of merge conflicts arised. As I don't see a lot of activity in this PR, I came to the conclusion that this PR won't be merged and that I can stop working on it. I guess someone else has to fix the underlying issues...

@clalancette
Copy link
Contributor

Due to the latest changes on the ros2 branch, a lot of merge conflicts arised. As I don't see a lot of activity in this PR, I came to the conclusion that this PR won't be merged and that I can stop working on it. I guess someone else has to fix the underlying issues...

I was the cause of the conflicts, so apologies for that. I can go ahead and fix them, if you'd like.

However, it would be a lot easier if this PR was cleaned up a bit. In particular, it would be much better if we split parts of this out into smaller PRs (particularly things like the tests), and also consolidated the 51 commits into a more manageable number. @gleichdick are you willing to do that?

This was referenced May 26, 2021
Merged
Merged
Closed
Merged
Merged
Open
@gleichdick
Copy link
Contributor Author

Closed in favor of #427

@gleichdick gleichdick closed this May 26, 2021
@taikitanaka3 taikitanaka3 mentioned this pull request Nov 19, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde requested changes

Assignees

@ahcorde ahcorde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gleichdick @ahcorde @clalancette