Enable Twist interpolator #646
Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
| @@ -572,6 +574,123 @@ struct TransformAccum | |||
| tf2::Vector3 result_vec; | |||
| }; | |||
|
|
|||
| geometry_msgs::msg::VelocityStamped BufferCore::lookupVelocity( | |||
There was a problem hiding this comment.
I'm not sure that we want to bother with having this API version. The user can construct the last value inline and it will be much clearer to them what's happening.
There was a problem hiding this comment.
uhmm, when you just want the velocity of the object without any reference point it might be usefull, fill the rest of parameters is error prone.
| const TimePoint & time, const tf2::Duration & averaging_interval) const | ||
| { | ||
| tf2::TimePoint latest_time; | ||
| // TODO(anyone): This is incorrect, but better than nothing. Really we want the latest time for |
There was a problem hiding this comment.
Note the todo's are not about who will do them, They're for who wrote them in case you want to ask more questions/get more context. And if it goes through a refactor chasing it down is harder.
| // TODO(anyone): This is incorrect, but better than nothing. Really we want the latest time for | |
| // TODO(ahcorde): This is incorrect, but better than nothing. Really we want the latest time for |
There was a problem hiding this comment.
Secondly, could you explain what you mean by any of the frames? You mean all 3? If so you could do the minimum of the common time across two pairs.
There was a problem hiding this comment.
This comment is from the original implementation. Let me dig in to this more
tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.hpp
Outdated
Show resolved
Hide resolved
| @@ -216,6 +216,21 @@ class BufferInterface | |||
| return out; | |||
| } | |||
|
|
|||
| template<class T> | |||
| T & transform( | |||
There was a problem hiding this comment.
I wonder for this one if the overload here makes sense. It's slightly more understandable to have transform_averaged() or something like that which explicitly lets you know that it's doing averaging. But maybe that implementation detail is obvious from the extra parameter required. Mostly thinking out loud here. And making sure that we're not boxing ourselves into the API, but I don't think that there's something else on the horizion with which this will collide.
There was a problem hiding this comment.
transform_average sounds good too. Open to change this too
| averaging_interval); | ||
| } | ||
|
|
||
| geometry_msgs::msg::VelocityStamped BufferCore::lookupVelocity( |
There was a problem hiding this comment.
I think that we can simplify this down with our assumptions about points and frames of references being coincident.
There was a problem hiding this comment.
the method above is a simplified version with some assumptions, can you clarify more how we can simplify this?
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
@ahcorde I took a stab at adding some notation here to get us consistent. https://github.com/ros2/ros2_documentation/tree/tf_velocity Based on this I think that the header.frame_id should be the observation frame. And the other two datatypes should be capturing the semantic meaning. Where the naive transform method is only doing the reprojection of the observation. And then we can have a separate API for adding/subtracting transforms across moving frames which will be more complicated. |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
|
I rebased common_interfaces with the same branch name https://github.com/ros2/common_interfaces/tree/ahcorde/rolling/lookupvelocity
|
|
It looks like we have some linter errors and a symbol visibility on Windows issue in the CI |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
|
Related with this issue #643