Chain::create has unintuitive behavior

[mkoval]

The Chain::create function surprisingly includes the parent joint of _start in the chain. This is incorrect because the parent joint of _start is not a member of the chain from _start to _end.

This also conflicts with the convention in other software packages. Here's an excerpt from the SRDF documentation:

Note: Based on the links in the chain, the set of joints in the chain is implicitly defined. The joints that correspond to a chain are the parent joints for the links in the chain, except the parent joint of the base_link.

And the OpenRAVE documentation:

Returns true if the dof index affects the relative transformation between the two links.

[mxgrey]

When you construct a Chain, it maintains a list of its Joints by maintaining a list of the child BodyNodes of those Joints. Vice versa, if a given BodyNode is in a Chain (or any other kind of referential skeleton) then its parent Joint is in the Chain.

The assumption being made by the Chain constructor is that you want to include the _start BodyNode in the Chain and therefore also include its parent Joint. We could either

• Change the behavior so that _start is not included in the Chain

or

• Offer a flag to let the user decide whether it's included. We can have the value of the flag default to being non-inclusive.

I'd be fine with either of these, although I think I'd prefer the second option, because I always tend to favor offering more choices.

[mkoval]

I'd prefer the first option since this definition of "chain" is pretty common. I don't feel strongly enough to argue about it, though, so I am also fine with a flag that defaults to being non-inclusive. 😄

[mxgrey]

The main reason that I want to offer both options is because I can see two use cases for the Chain class:

1. Get a group of all the DegreeOfFreedoms between two bodies
2. Get a group of all the bodies that form a chain from one body to another body

The first one would be useful for geometric planning while the second one would be useful for a controller (let's say I want to compute the center of mass of a chain of bodies).

[mkoval]

I don't see how this change would affect use case (2). We're only changing which Joints (and, thus, DegreeOfFreedoms) are in the chain. This shouldn't affect the set of BodyNodes in the Chain.

However, if you feel strongly about this, I'm fine with adding a flag that defaults to non-inclusive.

[mxgrey]

The way the indexing works, a Joint gets included in a ReferentialSkeleton by having its child BodyNode included. When you call getJoint(0) you're really saying getBodyNode(0)->getParentJoint(). This mostly has to do with the fact that the value of a Joint's pointer is liable to change at any moment, so we really need to store the BodyNode and get to the parent Joint via its child BodyNode.

So under the current scheme, there is a 1-to-1 correspondence between the Joints in a ReferentialSkeleton and the BodyNodes in that ReferentialSkeleton. There's a tiny bit more flexibility for DegreesOfFreedom (you can have a BodyNode in a ReferentialSkeleton without having all of its DegreesOfFreedom), but you only really get to take advantage of that flexibility if you use the Group class instead of the Linkage class.

It may be possible to rework the scheme, but I feel like that might add unnecessary complexity.

[mkoval]

I see. In that case it makes sense to add a flag.

[mxgrey]

It occurs to me that the default behavior we've agreed on can produce some unexpected results if the target is upstream of the start. Let's say you have:

j1 -> link1 -> j2 -> link2 -> j3 -> link3

And you run Chain::create( link3, link1 ), you'll end up with a Chain that includes { j2, j1 } in that order, when you'd probably expect { j3, j2 }.

Would you prefer if the default behavior is to check which BodyNode is upstream/downstream of the other, and then select whether to exclude the start or target according to that? If so, I assume that if neither are downstream of the other, then both should be included.

[mxgrey]

Fixed with #443