New transformation type

[saarnold]

This type shall be used in the future to express transformations. I know that replacing the RigidBodyState will be a very long (and hard) process and I don't plan to force anyone to use this type. The idea is to first integrate this type in the back-end of the transformer while keeping the interfaces to the RigidBodyState and also add interfaces for the new type. At that point users could at least use the new type inside of their tasks and libraries already.

In contrast to the RigidBodyState this type has a full pose covariance matrix, no velocity members and a hopefully more intuitive frame naming.

[jmachowinski]

I agree, global and local is really confusing. parent / child is a bit better, but is more or less equal to source / target. So why not stay with that naming scheme ? Or do I miss an imported drawback of this scheme ?

[saarnold]

I'm absolutely fine with changing the frame names to something else. But I really would like to avoid source and target, since they are ambiguous. One could read it as a transformation transforming a vector v from the source to the target frame: v_target = T * v_source (Which would be the right thing to do). But one could also read it as a translation vector pointing from the source frame to the target frame (which would be the inverse). And I know that this is confusing for a lot of people.

The goal should be to find a more distinct naming scheme. What about just frame_id and child_frame_id?

[chhtz]

I think just frame_id should be avoided. And if we use it, I would rather use frame_id and parent_frame_id, since the child frame is usually the more distinctive one.

That said, I don't have too strong opinions on the naming, just let us not bikeshed this ...

[saarnold]

Since the transformation would be expressed in the frame_id the other frame should be called child_frame_id IMHO.

That said, I don't have too strong opinions on the naming, just let us not bikeshed this ...

:+1:

[doudou]

Since the transformation would be expressed in the frame_id

See, that's what I am talking about. A transformation is not expressed in any frame.

A transformation is actually an ill-defined term by itself. It is either the pose of a "frame" (i.e. the reference body of a frame) into another (which was the reason why I proposed the reference_frame/frame terminology - the pose of 'frame' expressed in 'reference_frame'), or a frame change operation that changes poses expressed in frame A into poses expressed in frame B (the source/target terminology)

[doudou]

It is either the pose of a "frame"

OK...

Why not calling this type Pose ? That would IMO remove all the ambiguity.

And the frame_ids could become object_id/frame_id (as "pose of object in frame")

[chhtz]

No objections against calling it Pose. I usually would use a Pose as a transformation, but if Pose sounds less ambiguous, I'm fine with that.

Addendum: Actually, there already is a base::Pose, which is a pure pose without frame names or covariances.

[saarnold]

I would be fine with Pose as well. But I really don't like object_id, we could call it object_frame_id.

[jhidalgocarrio]

IMO. I agree "global" and "local" frame does not improve much (just a bit) the existing "source" and "target" which confuse the user about the transformation to write. [Actually, it confuses the user no because of the naming but because the names are swapped when thinking in a geometrical meaning]

Now. My proposal is to merge the PR #86. We would like to have EnviRe in Rock (another topic) but it solves the problem of producing samples (sensory data) coming from one sensor (produced in a frame _frameid of the sensor) at a particular time (base::Time).

Then, we can have a Transformation which has the SpatialTemporal information to set the transformation origin. It set "when and where" the information was produced. Something like:

SpatioTemporal<Pose> transformation;

where Pose has only:

std::string object_frame_id;

In this way, there is not room to confuse the user. The originating frame is the _frameid because it comes from the SpatioTemporal type. The child frame is the _object_frameid. The question of naming this class Transformation or Pose does not change much in my opinion.

[doudou]

Now. My proposal is to merge the PR #86

My problem with this spatio-temporal adapter is that it embeds metadata (the frame IDs) in the data. Which makes changing the system after the fact a LOT harder: for instance when replaying data, dynamically rewiring components, or in multi-robot contexts where all these names must be globally unique.

I know that is is a sad fact of the Rock life right now because unfortunately (1) there is no support for other metadata channels and (2) RBS already has it (with the associated pain of having every single transform producer/consumer needing to know about frames even though most of them really do not care).

On top of the rest, std::string is not realtime-compatible.

A.k.a. PR #86 IMO expands the main problem of RBS, the frame names, eventually to most other types even though right now there is little need to. It already spread to Joints ... I would rather avoid a generalization.

[doudou]

chhtz: No objections against calling it Pose. I usually would use a Pose as a transformation, but if Pose sounds less ambiguous, I'm fine with that. saarnold: I would be fine with Pose as well. But I really don't like object_id, we could call it object_frame_id

So .. we were getting somewhere it seems.

Since Pose is taken, and since there is already base::PoseWithUncertainty, why not:

• base::samples::PoseWithUncertainty
• object_frame_id / frame_id

?

[saarnold]

Sorry for the delay, I had it always on my list just not with the highest priority I must admit.

I did apply the changes we discuses and adapted the documentation.

Since Pose is taken, and since there is already base::PoseWithUncertainty

I couldn't find this type. I do like PoseWithUncertainty more but the type is now named base::samples::PoseWithCovariance since we already have a TransformWithCovariance and a TwistWithCovariance.

[doudou]

Looks good to me. Anyone else ?

[jhidalgocarrio]

I couldn't find this type. I do like PoseWithUncertainty more but the type is now named base::samples::PoseWithCovariance since we already have a TransformWithCovariance and a TwistWithCovariance.

I found more meaningful PoseWithCovariance since Covariance is one way to represent Uncertainty. This is why I used TransformWithCovariance.

PoseWithCovariance is not RBS. I suggest to remove the method PoseWithCovariance::toRigidBodyState() to copy to RBS. Two reasons: 1) Already interpreting frames using the RBS convention.It forces the user to use this interpretation 2) Lineal and Angular Velocities are not part of PoseWithCovariance. RBS will be incomplete anyway.

[saarnold]

Already interpreting frames using the RBS convention.It forces the user to use this interpretation

I think that is exactly the reason why it is good to have this helper. It might be necessary to convert between those two types at every part of the code where the RBS is currently used as PoseWithCovariance. So why not having this helper method implemented once with the correct assignment of frames?

[jhidalgocarrio]

So why not having this helper method implemented once with the correct assignment of frames?

The helper method makes sense between compatible types. It makes more sense between BodyState (already existing type) and RBS, since both classes have pose and velocity information.

The helper method makes to depend on (include) RBS header. We don't have a completely independent alternative to RBS.

Having this helper method is the same as calling the frames source and target frames. Indirectly, yes, but again!!

[jhidalgocarrio]

For the rest I am good to merge this PR :+1: but removing the helper method toRigidBodyState()

[doudou]

@jhidalgocarrio I don't agree. RBS is designed to be incomplete. A RBS with only pose and orientation information is a valid RBS. You guys are going to hit yourselves in the foot in a major way later down the line if you don't realize that whenever you will want to propagate a full pose+velocity state, having two ports is going to be a major pain.

That's why RBS has all these fields. Not having a RBS-like type is going to be problematic.

[jhidalgocarrio]

You guys are going to hit yourselves in the foot in a major way later down the line if you don't realize that whenever you will want to propagate a full pose+velocity state, having two ports is going to be a major pain.

I am not saying we don't need a pose+velocity state type to send with a single port. This is a different topic and it is not PoseWithCovariance

So why not having this helper method implemented once with the correct assignment of frames?

I think is good to have PoseWithCovariance independent of RBS, but it is a minor issue.

Ok. Go for this. :+1:

[saarnold]

Are there any other concerns or can we merge this?

[doudou]

Sorry, I missed new operator *. One comment there.

I very much plan to get rid of the whole "having explicit frames" in the data types in my systems. You're obviously free to continue with that, but if at all possible I would prefer not requiring them to be set.

[Rauldg]

To summarize, what needs to be done to merge? According to the discussion I think that it is only to allow the usage of the type without the requirement of providing/using frame ids. Can't the type already be used without setting these?

[doudou]

I have nothing on my side ... @saarnold @jhidalgocarrio ?

[saarnold]

From my side this can be merged as well.

[doudou]

@jhidalgocarrio ?

[jhidalgocarrio]

I am good with it. No further comments as mentioned before here IMO we can merge.