Conversation
test_tf2/CMakeLists.txt
Outdated
| if(WIN32) | ||
| set(BULLET_ROOT $ENV{ChocolateyInstall}/lib/bullet) | ||
| endif() | ||
| find_package(Bullet REQUIRED) | ||
| include_directories(include ${BULLET_INCLUDE_DIRS}) |
There was a problem hiding this comment.
Do we actually need any of this? We aren't using bullet directly in this package, and hence all of this should be inherited from tf2_bullet. If it is not, then that seems like a bug in the tf2_bullet package.
There was a problem hiding this comment.
Yes, there seems to be a bug. #428 opened
| const tf2::Quaternion tq(1, 2, 3, 4); | ||
| Eigen::Quaterniond eq; |
There was a problem hiding this comment.
Add needed includes for these at the top:
#include <tf2/LinearMath/Quaternion.h>
#include <Eigen/Eigen>
Also, I think we need to declare a dependency on eigen in the package.xml and the CMakeLists.txt.
test_tf2/test/test_convert.cpp
Outdated
| { | ||
| const tf2::Quaternion tq(1, 2, 3, 4); | ||
| Eigen::Quaterniond eq; | ||
| //tf2::convert(tq, eq); |
There was a problem hiding this comment.
Remove this dead code.
| { | ||
| // Bullet | ||
| const tf2::Stamped<btVector3> b1{btVector3{1.0, 3.0, 4.0}, tf2::TimePoint(), "my_frame"}; | ||
| const geometry_msgs::msg::PointStamped msg = tf2::toMsg(b1); |
There was a problem hiding this comment.
Add the include at the top of the file:
#include <geometry_msgs/msg/point_stamped.hpp>
| { | ||
| // Eigen | ||
| const Eigen::Vector3d e1{2.0, 4.0, 5.0}; | ||
| const geometry_msgs::msg::Point msg = tf2::toMsg(e1); |
There was a problem hiding this comment.
Add the include at the top of the file.
| { | ||
| // tf2 | ||
| const tf2::Vector3 t1{2.0, 4.0, 5.0}; | ||
| const geometry_msgs::msg::Vector3 msg = tf2::toMsg(t1); |
There was a problem hiding this comment.
Add the include at the top of the file.
| * \return The Vector3 converted to a geometry_msgs message type. | ||
| */ | ||
| inline | ||
| geometry_msgs::msg::Vector3 toMsg(const tf2::Vector3& in) |
There was a problem hiding this comment.
Add the include for geometry_msgs::msg::Vector3 at the top of the file.
Also, style nit: there should be a space before and after the reference symbol (&). We aren't enforcing it in this package yet (and I don't think we should in this PR), but we may as well make one less thing we have to change later.
The same comment goes several other times below.
| * \return The Vector3 converted to a geometry_msgs message type. | ||
| */ | ||
| inline | ||
| geometry_msgs::msg::Point& toMsg(const tf2::Vector3& in, geometry_msgs::msg::Point& out) |
There was a problem hiding this comment.
Add the include for geometry_msgs::msg::Point at the top of the file.
|
I hope I addressed all of the remarks. There seems to be a blocking issue with bullet dependencies, see #428. |
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
This looks good. I'll run CI on it next.
|
CI:
|
2 participants
Vectors can be converted to a Vector message or a Point Message, some libraries behave differently. This unit test will make sure that the default return value for
toMsg()will stay the same.This was separated from #368.