Enable Twist interpolator #646

ahcorde · 2024-01-26T18:24:57Z

Related with this issue #643

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde · 2024-02-09T13:59:36Z

Ey @caguero and @tfoote when you have some time, do you mind to review the PR?

I'm not really sure how to handle the doTransform API in tf2_geometry_msgs

…okupvelocity

tf2/include/tf2/convert.h

tfoote · 2024-02-09T19:11:28Z

tf2/src/buffer_core.cpp

@@ -572,6 +574,123 @@ struct TransformAccum
  tf2::Vector3 result_vec;
 };

+geometry_msgs::msg::VelocityStamped BufferCore::lookupVelocity(


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.

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.

tfoote · 2024-02-09T19:18:55Z

tf2/src/buffer_core.cpp

+  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


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.

Suggested change

// 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

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.

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

tf2_py/test/test_buffer_core.py

tfoote · 2024-02-13T00:45:23Z

tf2_ros/include/tf2_ros/buffer_interface.h

@@ -216,6 +216,21 @@ class BufferInterface
    return out;
  }

+  template<class T>
+  T & transform(


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.

transform_average sounds good too. Open to change this too

tfoote · 2024-02-13T00:46:34Z

tf2/src/buffer_core.cpp

+    averaging_interval);
+}
+
+geometry_msgs::msg::VelocityStamped BufferCore::lookupVelocity(


I think that we can simplify this down with our assumptions about points and frames of references being coincident.

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 <[email protected]>

tfoote · 2024-02-27T10:45:37Z

@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 <[email protected]>

tfoote

A few comments the tests look to be there, but not asserting. Other than that I think that we're pretty close here.

tf2_ros/include/tf2_ros/buffer_interface.h

tf2_ros/test/test_buffer.cpp

…okupvelocity

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde · 2024-04-09T12:23:08Z

Linux
Linux-aarch64
Windows

tfoote · 2024-04-09T16:20:16Z

I rebased common_interfaces with the same branch name https://github.com/ros2/common_interfaces/tree/ahcorde/rolling/lookupvelocity

Linux
Linux-aarch64
Windows

tfoote · 2024-04-09T18:20:35Z

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 <[email protected]>

ahcorde · 2024-04-09T21:33:59Z

Linux
Linux-aarch64
Windows

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde · 2024-04-09T22:19:35Z

Linux
Linux-aarch64
Windows

tfoote

Thanks for pushing this over the line.

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde · 2024-04-10T08:18:27Z

Linux
Linux-aarch64
Windows

nachovizzo · 2024-05-20T14:06:39Z

Are there any plans to backport this to Iron/jazzy?

ahcorde · 2024-05-20T15:32:43Z

Are there any plans to backport this to Iron/jazzy?

It's already on Jazzy, I can backport this to Humble and Iron, because this is not breaking ABI or API

ahcorde · 2024-05-20T15:32:53Z

https://github.com/Mergifyio backport humble iron

mergify · 2024-05-20T15:32:58Z

backport humble iron

✅ Backports have been created

#684 Enable Twist interpolator (backport #646) has been created for branch humble but encountered conflicts
#685 Enable Twist interpolator (backport #646) has been created for branch iron but encountered conflicts

Signed-off-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Tully Foote <[email protected]> (cherry picked from commit 62322b8) # Conflicts: # tf2/include/tf2/buffer_core.h # tf2_ros/test/test_buffer.cpp

nachovizzo · 2024-05-20T15:39:11Z

Are there any plans to backport this to Iron/jazzy?

It's already on Jazzy, I can backport this to Humble and Iron, because this is not breaking ABI or API

@ahcorde thanks !

* Enable Twist interpolator (#646) Signed-off-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Tully Foote <[email protected]> (cherry picked from commit 62322b8) # Conflicts: # tf2/include/tf2/buffer_core.h # tf2_ros/test/test_buffer.cpp Signed-off-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Alejandro Hernández Cordero <[email protected]>

* Enable Twist interpolator (#646) Signed-off-by: Alejandro Hernández Cordero <[email protected]> Co-authored-by: Tully Foote <[email protected]> (cherry picked from commit 62322b8)

Enable Twist interpolator

b1f807e

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde self-assigned this Jan 26, 2024

ahcorde added 3 commits February 8, 2024 23:22

added velocity tests

9cf307e

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

Fixed velocity test

da7fe84

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

Added python interfaces

0f45173

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde requested a review from tfoote February 9, 2024 13:55

ahcorde mentioned this pull request Feb 9, 2024

Enable Twist interpolator python bindings #643

Open

Merge remote-tracking branch 'origin/rolling' into ahcorde/rolling/lo…

3e6b1d9

…okupvelocity

tfoote reviewed Feb 13, 2024

View reviewed changes

add part of the feedback

789df20

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

fix compilation

a6ee0e4

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

tfoote mentioned this pull request Apr 2, 2024

Create new messages with all fields needed to define a velocity and transform it ros2/common_interfaces#240

Merged

tfoote reviewed Apr 8, 2024

View reviewed changes

tf2_ros/include/tf2_ros/buffer_interface.h Outdated Show resolved Hide resolved

tf2_ros/test/test_buffer.cpp Outdated Show resolved Hide resolved

tf2_ros/test/test_buffer.cpp Outdated Show resolved Hide resolved

tf2_ros/test/test_buffer.cpp Show resolved Hide resolved

ahcorde added 2 commits April 9, 2024 13:10

Merge remote-tracking branch 'origin/rolling' into ahcorde/rolling/lo…

098d27a

…okupvelocity

Added feedback

52cccf8

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde requested a review from tfoote April 9, 2024 12:21

tfoote marked this pull request as ready for review April 9, 2024 16:20

tfoote requested a review from clalancette as a code owner April 9, 2024 16:20

tfoote changed the title ~~[WIP] Enable Twist interpolator~~ Enable Twist interpolator Apr 9, 2024

tfoote and others added 3 commits April 9, 2024 13:57

fix line wrap

cf53a62

fix indention

005b90b

Visibility issue

10dc05c

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

make linters happy

130d29d

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

tfoote approved these changes Apr 9, 2024

View reviewed changes

Windows warnings

ee27fb8

Signed-off-by: Alejandro Hernández Cordero <[email protected]>

ahcorde merged commit 62322b8 into rolling Apr 10, 2024
1 of 2 checks passed

delete-merged-branch bot deleted the ahcorde/rolling/lookupvelocity branch April 10, 2024 12:06

ahcorde mentioned this pull request Apr 29, 2024

Added release notes for twist interpolator ros2/ros2_documentation#4328

Merged

mergify bot mentioned this pull request May 20, 2024

Enable Twist interpolator (backport #646) #684

Merged

mergify bot mentioned this pull request May 20, 2024

Enable Twist interpolator (backport #646) #685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Twist interpolator #646

Enable Twist interpolator #646

ahcorde commented Jan 26, 2024

ahcorde commented Feb 9, 2024

tfoote Feb 9, 2024

ahcorde Feb 13, 2024

tfoote Feb 9, 2024

tfoote Feb 13, 2024

ahcorde Feb 13, 2024

tfoote Feb 13, 2024

ahcorde Feb 13, 2024

tfoote Feb 13, 2024

ahcorde Feb 13, 2024

tfoote commented Feb 27, 2024 •

edited

Loading

tfoote left a comment

ahcorde commented Apr 9, 2024

tfoote commented Apr 9, 2024

tfoote commented Apr 9, 2024

ahcorde commented Apr 9, 2024 •

edited

Loading

ahcorde commented Apr 9, 2024

tfoote left a comment

ahcorde commented Apr 10, 2024 •

edited

Loading

nachovizzo commented May 20, 2024

ahcorde commented May 20, 2024

ahcorde commented May 20, 2024

mergify bot commented May 20, 2024 •

edited

Loading

nachovizzo commented May 20, 2024

	// 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

Enable Twist interpolator #646

Enable Twist interpolator #646

Conversation

ahcorde commented Jan 26, 2024

ahcorde commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfoote commented Feb 27, 2024 • edited Loading

tfoote left a comment

Choose a reason for hiding this comment

ahcorde commented Apr 9, 2024

tfoote commented Apr 9, 2024

tfoote commented Apr 9, 2024

ahcorde commented Apr 9, 2024 • edited Loading

ahcorde commented Apr 9, 2024

tfoote left a comment

Choose a reason for hiding this comment

ahcorde commented Apr 10, 2024 • edited Loading

nachovizzo commented May 20, 2024

ahcorde commented May 20, 2024

ahcorde commented May 20, 2024

mergify bot commented May 20, 2024 • edited Loading

✅ Backports have been created

nachovizzo commented May 20, 2024

tfoote commented Feb 27, 2024 •

edited

Loading

ahcorde commented Apr 9, 2024 •

edited

Loading

ahcorde commented Apr 10, 2024 •

edited

Loading

mergify bot commented May 20, 2024 •

edited

Loading