Chain constructor to not SEGFAULT when passed a nullptr

[mkoval]

It's very convenient to write code that looks like this:

auto chain = Chain::create(skel->getBodyNode("start"), skel->getBodyNode("target"));
if (!chain)
  // error!

Unfortunately, this currently SEGFAULTs if either getBodyNode returns nullptr. The only solution is to do this, which is much more verbose:

BodyNodePtr start = skel->getBodyNode("start");
BodyNodePtr target = skel->getBodyNode("target");
ChainPtr chain;
if (start && target)
  chain = Chain::create(start, target);
else
  // error!

This pull request modifies Chain::create to return nullptr if either input BodyNode is nullptr. This enables you to write the first block of code.

API Question: Should Chain::create(~) interpret nullptr in start to mean "the world"? This would be necessary, in conjunction with my proposal in #437, to create a chain that includes a link attached to the World. This would be consistent with the usage of a nullptr parent in DART to refer to the world frame.

[mxgrey]

Should Chain::create(~) interpret nullptr in start to mean "the world"?

Yes, I completely agree with this. In fact, if you use nullptr as a target in your Linkage::Criteria, it will treat that as saying "go all the way to the root". I believe you can already do Chain::create(start, nullptr) to get a chain that goes from start down to the root. There are two reasons I didn't support nullptr as a starting point:

1. There was no reason to, because the starting point was always supposed to be inclusive.
2. It leaves some ambiguity about what Skeleton you are exploring.

(1) Is no longer valid, and (2) can be fixed by requiring that at least one target in the Linkage::Criteria must be a valid non-null pointer. If that requirement is not met, then the LinkagePtr / ChainPtr / etc that gets returned will just be a nullptr.

Does that sound reasonable?

Edit: Or instead of returning a nullptr, we could just return a pointer to a Linkage / Chain / etc that is empty. I'm not sure which would be preferable.

[mkoval]

I'm not sure that I follow. My understanding is was that _start was the "base" of the chain and _target was the "tip". I.e. _start would always be a parent (direct or indirect) of _target. If I have the chain:

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

I would expect:

1. Chain::Create(link1, link3) to contain { j2, j3 }
2. Chain::Create(link3, link1) to produce an error
3. Chain::Create(nullptr, link3) to contain { j1, j2, j3 }
4. Chain::Create(link1, nullptr) to return { j2, j3 } (and produce an error if there were branching)
5. Chain::Create(nullptr, nullptr) to produce an error

Is this correct? Do I have the roles of _start and _target flipped?

I'm not sure that I understand the purpose of Criteria. Is it intended to enforce the invariants that I listed? When would I use it instead of the Create function that takes two BodyNode pointers as arguments?

[mxgrey]

It should be noted that the construction of a Chain is a special case of the construction of a Linkage. All the same mechanics are used to construct each. A Chain::Criteria just takes a start and a single target then converts that into a Linkage::Criteria. A Linkage::Criteria can consist of multiple targets, and the resulting Linkage will spread from the start to each of the respective targets. The targets can be upstream or downstream of the start; it doesn't matter.

If you reverse the arguments that get passed into Chain::create it will work exactly the same except that the indexing will be reversed.

[mxgrey]

So all of your expectations would be correct, except that (2) would not produce an error, it would be the same as (1) but with the joint indices flipped.

Edit: Your expectations will be correct, once I make the changes to Chain that we agreed on.

[mkoval]

Also, (4) would produce { j1 }. Correct?

[mxgrey]

Ah yes, you're right. (4) would produce { j1 }.

[mxgrey]

With respect to (5), we could either print a dtwarn and return a nullptr or we could just produce an empty Chain. I'm not sure which would be preferable.

[mkoval]

Ok, that makes sense to me: I agree 100% with what you said. This will require changing the behavior of Chain::Create when one (but not both) BodyNodes are nullptr.

Or instead of returning a nullptr, we could just return a pointer to a Linkage / Chain / etc that is empty. I'm not sure which would be preferable.

I'm not sure. I think it makes sense to:

1. Return an empty Chain (and print a warning?) when the query is valid, but empty.
2. Return a nullptr (and print an error?) when the query is not valid.

The only cases I can think of for (2) are if both BodyNodes are nullptr or are owned by different Skeletons. Thoughts?

[mxgrey]

I'm actually permitting Linkages to consist of BodyNodes from multiple Skeletons, as long as there's an unbroken connection to the roots of each Skeleton. Nothing about the ReferentialSkeleton mechanics precludes referencing multiple Skeletons at once.

I'm really thinking we should just outright permit passing in two nullptrs, and simply return an empty Chain. For example, what if someone is constructing Chains programmatically to test what kind of Chain is available between arbitrary BodyNodes, and they try Chain::create( bn1->getParentBodyNode(), bn2->getParentBodyNode() ) where bn1 and bn2 happen to both be root nodes? Then they're essentially querying to see if there's a Chain that goes from the world to the world, and rather than yelling at them we should probably just return an empty Chain to them.

Users would still have a reasonable way of seeing whether their Chain creation was successful. Instead of testing if(nullptr == chain) they could just test if(chain.getNumJoints() == expected)

[mkoval]

That's fine by me.

Do you mind making this change? It sounds like this will affect Linkages other than Chain, so I'm a bit nervous that I'll break something.

[mxgrey]

Yep, I can make the changes, no problem.

[mxgrey]

This pull request is now replaced by #443