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

tf2_geometry_msgs: Fixing covariance transformation in doTransform<PoseWithCovarianceStamped, TransformStamped> #430

Merged
merged 4 commits into from
Jun 11, 2021

Conversation

aprotyas
Copy link
Member

@aprotyas aprotyas commented Jun 7, 2021

Previously, the covariance matrix of a PoseWithCovarianceStamped message was incorrectly transformed. This PR:

  • Adds a transformCovariance method to address the covariance transformation.
  • Updates the TfGeometry/FrameWithCovariance unit test to check for the expected transformed covariance.

Fixes #372

Abrar Rahman Protyasha added 2 commits June 7, 2021 19:04
* The transform operation has been performed with the row-major form
of the covariance matrix.

* tf2 matrix types were used for easier matrix multiplication.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
* The previous test was doing a wrong covariance check,
i.e. out.covariance = in.covariance.

* The updated test checks against the correctly transformed
covariance matrix.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
@aprotyas aprotyas self-assigned this Jun 7, 2021
Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

The math LGTM (I only checked that it matches the ROS 1 code)

Previously, `transformCovariance`'s transform argument was of type
`geometry_msgs::msg::Transform`, which is not a "general" transform
type, but rather a message type. As such, that argument type has been
changed to `tf2::Transform`, so that the user can convert from whatever
transform type they have to a tf Transform.

Also:

* Moved location of a forward declaration to be closer to its use.
* Applied `ament_uncrustify` changes for maximum line length, changing
  the `transformCovariance` signature ine.
* Reflected change in `transformCovariance` function signature in
  doxygen documentation.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
@aprotyas aprotyas requested a review from sloretz June 8, 2021 21:38
@aprotyas
Copy link
Member Author

CI (build: --packages-up-to tf2_geometry_msgs test: --packages-select tf2_geometry_msgs)

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

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One minor thing to fix, then this looks good to me.

Leading tabs prevented the function parameters from lining up, both in
the Github UI and in Vim. Converting all tabs to spaces fixes said
issue.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@aprotyas
Copy link
Member Author

@Mergifyio backport galactic foxy

mergify bot pushed a commit that referenced this pull request Dec 21, 2021
…<PoseWithCovarianceStamped, TransformStamped>` (#430)

Fixes #372.

* Implement covariance transformation (cpp).

* Fixed `TfGeometry/FrameWithCovariance` test. The updated test checks against the correctly transformed
covariance matrix.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
(cherry picked from commit ae30f0b)

# Conflicts:
#	tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.hpp
mergify bot pushed a commit that referenced this pull request Dec 21, 2021
…<PoseWithCovarianceStamped, TransformStamped>` (#430)

Fixes #372.

* Implement covariance transformation (cpp).

* Fixed `TfGeometry/FrameWithCovariance` test. The updated test checks against the correctly transformed
covariance matrix.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
(cherry picked from commit ae30f0b)

# Conflicts:
#	tf2_geometry_msgs/include/tf2_geometry_msgs/tf2_geometry_msgs.hpp
@mergify
Copy link
Contributor

mergify bot commented Dec 21, 2021

aprotyas pushed a commit that referenced this pull request Dec 21, 2021
…<PoseWithCovarianceStamped, TransformStamped>` (#430)

Fixes #372.

* Implement covariance transformation (cpp).

* Fixed `TfGeometry/FrameWithCovariance` test. The updated test checks against the correctly transformed
covariance matrix.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
(cherry picked from commit ae30f0b)
aprotyas pushed a commit that referenced this pull request Dec 21, 2021
…<PoseWithCovarianceStamped, TransformStamped>` (#430)

Fixes #372.

* Implement covariance transformation (cpp).

* Fixed `TfGeometry/FrameWithCovariance` test. The updated test checks against the correctly transformed
covariance matrix.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
(cherry picked from commit ae30f0b)
aprotyas pushed a commit that referenced this pull request Dec 23, 2021
…<PoseWithCovarianceStamped, TransformStamped>` (#430) (#488)

Fixes #372.

* Implement covariance transformation (cpp).

* Fixed `TfGeometry/FrameWithCovariance` test. The updated test checks against
the correctly transformed covariance matrix.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
(cherry picked from commit ae30f0b)
aprotyas pushed a commit that referenced this pull request Dec 23, 2021
…<PoseWithCovarianceStamped, TransformStamped>` (#430) (#489)

Fixes #372.

* Implement covariance transformation (cpp).

* Fixed `TfGeometry/FrameWithCovariance` test. The updated test checks against
the correctly transformed covariance matrix.

Signed-off-by: Abrar Rahman Protyasha <abrar@openrobotics.org>
(cherry picked from commit ae30f0b)
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.

PoseWithCovarianceStamped transform doesn't transform covariance part
3 participants