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

Pose addition and subtraction needs work [gazebo issue 216] #60

Closed
osrf-migration opened this issue Feb 2, 2017 · 11 comments
Closed

Pose addition and subtraction needs work [gazebo issue 216] #60

osrf-migration opened this issue Feb 2, 2017 · 11 comments
Labels
bug Something isn't working

Comments

@osrf-migration
Copy link

Original report (archived issue) by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).


Moving Gazebo issue 216 here now that gazebo::math has been fully deprecated. Please consider the discussion on that issue:

https://bitbucket.org/osrf/gazebo/issues/216/pose-addition-and-subtraction-needs-work

@osrf-migration
Copy link
Author

Original comment by Addisu Z. Taddese (Bitbucket: azeey, GitHub: azeey).


I'm in favor of switching the semantics of operator* and removing operator+. Should we consider doing this on the gz11 branch? I think the current semantics is very counter-intuitive.

@osrf-migration
Copy link
Author

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).


I think we can make that switch. There might be repercussions in our code that are difficult to debug. If you do make this switch, then you might want to make sure all Ignition libraries pass CI.

@osrf-migration
Copy link
Author

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).


We currently don't use operator* anywhere in code or in testing, because it's so confusing that it only creates errors. Instead people will usually convert Pose objects to Matrix4 objects and use the Matrix4::operator*.

I fully agree with removing operator+, although I think that operator does get used in some places in the code, so those uses will need to be replaced with the fixed version of operator*.

@osrf-migration
Copy link
Author

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).


+1

@osrf-migration
Copy link
Author

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


Yeah, it's really confusing that the Pose3::operator* has an opposite sign convention to Matrix4 multiplication. I've illustrated this in an added test in 3ff1a21

@osrf-migration osrf-migration added major bug Something isn't working labels Apr 15, 2020
@chapulina chapulina removed the major label May 25, 2020
@traversaro
Copy link
Contributor

traversaro commented Aug 28, 2020

@chapulina
Copy link
Contributor

I think there's still the question of whether we want to remove the addition and subtraction operators:

https://github.com/ignitionrobotics/ign-math/blob/6a3c4a32b18e6bd7956fecfe53ccfe14bfcd2907/include/ignition/math/Pose3.hh#L183-L189

@scpeters
Copy link
Member

yeah, I would support deprecating them to encourage the use of *

@traversaro
Copy link
Contributor

yeah, I would support deprecating them to encourage the use of *

I personally agree. + is often used to refer to commutative operations, and using it for a non-commutative operation is quite confusing.

@scpeters
Copy link
Member

I'll open a pull request to deprecate the + and - operators for ign-math7 and see how much trouble it causes

scpeters added a commit to scpeters/ign-math that referenced this issue Feb 25, 2022
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 <[email protected]>
scpeters added a commit to scpeters/ign-math that referenced this issue Mar 2, 2022
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 <[email protected]>
scpeters added a commit to scpeters/ign-math that referenced this issue Mar 2, 2022
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 <[email protected]>
scpeters added a commit to scpeters/ign-math that referenced this issue Mar 2, 2022
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 <[email protected]>
scpeters added a commit to scpeters/ign-math that referenced this issue Mar 23, 2022
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 <[email protected]>
scpeters added a commit to scpeters/ign-math that referenced this issue Mar 23, 2022
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 <[email protected]>
scpeters added a commit that referenced this issue Mar 28, 2022
The * operator matches the behavior of multiplying
transform matrices, so it should be preferred over
the addition operator, which is confusing.

Part of #60.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this issue Jun 1, 2022
Part of #60.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this issue Jun 6, 2022
Part of #60.

Signed-off-by: Steve Peters <[email protected]>
scpeters added a commit that referenced this issue Jun 8, 2022
@scpeters
Copy link
Member

scpeters commented Jun 8, 2022

Pose + and - operators are now deprecated in math7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants