Change transform mode from AffineCompact to Isometry

[SeanCurtis-TRI]

This increases the memory footprint of the Transform class by 33% (ie.,
3x4 to 4x4). However, the X_AB.inverse() method will now implicitly use
the transpose of the linear() component rather than computing an inverse.

Resolves #317

---

This change is 

[amcastro-tri]

, pending CI

Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved

[SeanCurtis-TRI]

@jslee02 @sherm1 This passes all of CI as we'd expect (still the pernicious box-box test). Shall we pull the trigger ?

Reviewable status: complete! all files reviewed, all discussions resolved

[wxmerkt]

Would it make sense to update the CHANGELOG?

[sherm1]

I vote yes! And I agree with @wxmerkt that a CHANGELOG update would be worthwhile.

Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved

[jslee02]

Looks good to go. Updating the changelog would be great as well.

[SeanCurtis-TRI]

What's up with the change log vs releases/tags?

We've got release tags that don't monotonically increase.
And the changelog implies that all upcoming changes are applied to version 0.6.0. Despite the fact that the 0.5.0 release dated 2016-07-19 has been followed by 0.3.3 on 2016-07-22 and 0.3.4 on 2017-07-21.

The changelog seems to have gotten quite stale. In fact, in adding this change to the changelog, it follows the comment:

Switched to Eigen for math oeprations #96, #150

It seems we need to make sure the changelog gets updated w.r.t. PRs. It also seems that we need to have a flag day to make sure the changelog is meaningful.

Thoughts and opinions, please.

Reviewable status: 1 of 2 files reviewed, all discussions resolved

[jslee02]

We've got release tags that don't monotonically increase.
And the changelog implies that all upcoming changes are applied to version 0.6.0. Despite the fact that the 0.5.0 release dated 2016-07-19 has been followed by 0.3.3 on 2016-07-22 and 0.3.4 on 2017-07-21.

They actually monotonically increases but in different branches. A version number is decided based on which branch the change is merged to. For example, changes merging into (current) master would be for 0.6.0, while changes for fcl-0.5 would be for the next patch release of 0.5 (e.g., 0.5.1 since 0.5.0 is already released).

Sometimes we need to fix a bug for a released version (e.g., 0.3.4) before the next minor or major release (e.g., 0.6.0). Then we might see 0.3.5 release before 0.6.0, which might seem not monotonic increase.

The changelog seems to have gotten quite stale.

Yeah, this is sad.

In fact, in adding this change to the changelog, it follows the comment:

Switched to Eigen for math oeprations #96, #150

I think this change may just fit to the line as:

Switched to Eigen for math operations #96, #150, #318

The section of 0.6.0 is for the diff between 0.6.0 and 0.5.x. Since the transform type of Eigen doesn't exist in 0.5.x so we don't need to add a new line for it I think.

It seems we need to make sure the changelog gets updated w.r.t. PRs. It also seems that we need to have a flag day to make sure the changelog is meaningful.

👍

I found a useful site about changelog. We don't need to exactly follow the suggestions, but I mostly agree with.

[sherm1]

The only CI failure is the usual one. Looks like this just needs a manual merge with master and then is ready to merge?

[jslee02]

I believe the CI failure is fixed by #266. So it should be gone once master is merged.

[peci1]

I guess many users will get no improvement at all as the automatic optimization hasn't still been released in Eigen - my comment from moveit/geometric_shapes#125 (comment) still holds, it hasn't even got into the recently released Eigen 3.3.8. In Moveit, we took the way of replacing all appropriate .inverse() calls to .linear().

[SeanCurtis-TRI]

@peci1 The link provided seems to discuss rotation() vs linear(). In contrast, this is about the cost of inverse() (on the full transform). It's true that the choice was made based on the documented feature and hasn't been confirmed as you did with rotation(). Have you looked at the assembly for what inverse() does on an isometry?

[peci1]

Oh, you're right, I got these two mixed up... The code for inverse() actually does examine the transform traits to improve performance for isometries: https://gitlab.com/libeigen/eigen/-/blob/3.3/Eigen/src/Geometry/Transform.h#L1211 . Sorry for the confusion...

[peci1]

And the problem with inefficient rotation() should not affect FCL, as

fcl/include/fcl/narrowphase/collision_object-inl.h

Lines 156 to 159 in 9dba579

	const Matrix3<S> CollisionObject<S>::getRotation() const
	{
	return t.linear();
	}

uses the always efficient linear() method.

[SeanCurtis-TRI]

@peci1 No apologies required. Those kinds of things can fall through the cracks all the time. Worst case scenario, it doesn't directly apply, but it's still relevant and interesting (as in this case). More typically, users keeping an eye out for things will catch unintentionally bad code. So, I appreciate you weighing in.