RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.35k stars 1.27k forks source link

refactor RigidTransform IsIdentityToEpsilon => IsNearlyIdentity? #16524

Closed RussTedrake closed 2 years ago

RussTedrake commented 2 years ago

Currently RigidTransform has

I find myself often typing IsNearlyIdentity and getting reminded by the compiler. Can we pick one and stick?

mitiguy commented 2 years ago

Opened PR #16541

Proposal is to change RigidTransform::IsIdentityToEpsilon(double translation_tolerance) to RigidTransform::IsNearlyIdentity(double translation_tolerance);

Background info.
Currently, the related public methods in RigidTransform are: RigidTransform::IsExactlyIdentity(); RigidTransform::IsIdentityToEpsilon(double translation_tolerance); RigidTransform::IsNearlyEqualTo(const RigidTransform& other, double tolerance); RigidTransform::IsExactlyEqualTo(const RigidTransform& other);

Note there are related methods in RotationMatrix. Public methods: RotationMatrix::IsExactlyIdentity(); RotationMatrix::IsIdentityToInternalTolerance(); RotationMatrix::IsNearlyEqualTo(const RotationMatrix& other, double tolerance); RotationMatrix::IsExactlyEqualTo(const RotationMatrix& other); Private method: static boolean IsNearlyEqualTo(const Matrix3& R, const Matrix3& other, double tolerance);

For consistency, ONE MIGHT also consider changing RotationMatrix::IsIdentityToInternalTolerance() to
RotationMatrix::IsNearlyIdentityToInternalTolerance();

jwnimmer-tri commented 2 years ago

For the record, for some future RotationMatrix function renaming, I'd argue that the -ToInternalTolerance suffix is not helpful. The function takes no arguments, obviously the tolerance is built-in. Adding that detail to the function name just makes the call sites unreadable.

If we ever wanted to offer a the tolerance as argument in the future, we'd want to do boolean<T> IsNearlyIdentity(double tolerance = get_internal_tolerance_for_orthonormality()) const; without needing to do rename the function yet again.

mitiguy commented 2 years ago

Thanks @jwnimmer-tri
Unless I hear otherwise from @RussTedrake, I'll proceed with that in a subsequent PR.

New method: boolean RotationMatrix::IsNearlyIdentity(double tolerance = get_internal_tolerance_for_orthonormality()) const;
will replace old method: boolean RotationMatrix::IsIdentityToInternalTolerance();