Skip to content

Pose3: deprecate + operator in favor of *#381

Merged
scpeters merged 3 commits intogazebosim:mainfrom
scpeters:pose_deprecate_addition
Mar 28, 2022
Merged

Pose3: deprecate + operator in favor of *#381
scpeters merged 3 commits intogazebosim:mainfrom
scpeters:pose_deprecate_addition

Conversation

@scpeters
Copy link
Copy Markdown
Member

@scpeters scpeters commented Mar 2, 2022

🎉 New feature

Part of #60.

Summary

The * operator matches the behavior of multiplying transform matrices, so it should be preferred over the addition operator, which is confusing. This will be a draft PR until the deprecation warnings have been fixed in downstream packages.

Test it

Run the UNIT_Pose_TEST, which uses the * operator instead of +.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@scpeters scpeters requested a review from azeey March 2, 2022 06:31
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Mar 2, 2022
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 2, 2022 •
edited
Loading

Codecov Report

Merging #381 (2434219) into main (b1cb6bb) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 2434219 differs from pull request most recent head 2bd60e7. Consider uploading reports for the commit 2bd60e7 to get more accurate results

@@           Coverage Diff           @@
##             main     #381   +/-   ##
=======================================
  Coverage   99.73%   99.73%           
=======================================
  Files          68       68           
  Lines        6314     6314           
=======================================
  Hits         6297     6297           
  Misses         17       17
Impacted Files Coverage Δ
include/ignition/math/Pose3.hh 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1cb6bb...2bd60e7. Read the comment docs.

azeey
azeey reviewed Mar 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM! Should we deprecate Pose3::operator- and Pose3::operator-= as well?

src/Pose_TEST.cc
math::Pose3d(5.74534, 7.01053, 8.62899, 0.675732, 0.535753, 1.01174));

pose += pose;
pose *= pose;
Copy link
Copy Markdown
Contributor

@azeey azeey Mar 2, 2022

Choose a reason for hiding this comment

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

As we found in #372, it would be good to keep tests for deprecated functions.

Copy link
Copy Markdown
Member Author

@scpeters scpeters Mar 2, 2022

Choose a reason for hiding this comment

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

coverage restored in d9a6d4a

@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Mar 2, 2022

LGTM! Should we deprecate Pose3::operator- and Pose3::operator-= as well?

I wanted to address them separately, so I'll make a follow-up pull request about - and -=

@scpeters scpeters force-pushed the pose_deprecate_addition branch from d9a6d4a to 35bda79 Compare March 23, 2022 06:31
@scpeters scpeters marked this pull request as ready for review March 23, 2022 06:31
scpeters added 2 commits March 22, 2022 23:45
@scpeters
Pose3: deprecate + operator in favor of *
8d0204e 
The * operator matches the behavior of multiplying
transform matrices, so it should be preferred over
the addition operator, which is confusing.

Part of gazebosim#60.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Add coverage for Pose += operator
06fafd9 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters force-pushed the pose_deprecate_addition branch from 35bda79 to 06fafd9 Compare March 23, 2022 06:45
@scpeters
Copy link
Copy Markdown
Member Author

scpeters commented Mar 23, 2022

I think this is ready for review as downstream packages have already been fixed to use * instead of +

@scpeters scpeters merged commit 71b6db2 into gazebosim:main Mar 28, 2022
@scpeters scpeters deleted the pose_deprecate_addition branch March 28, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azeey azeey azeey approved these changes

Assignees

No one assigned

Labels

🌱 garden Ignition Garden

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@scpeters @azeey @j-rivero @chapulina @osrf-triage