Fixed SEGFAULTs in DartLoader

[mkoval]

DartLoader currently SEGFAULTs if it fails loading a mesh and, possibly, in a few other situations. This pull request adds error-handling logic that prints a dterr message and returns nullptr in that case.

[mxgrey]

Is there any particular reason for passing in the BodyNode::Properties with a pointer instead of by non-const reference?

[mkoval]

The Google C++ style guide (see here) advises against using non-const references for output arguments. I tend to agree: it's easy to accidentally pass an argument into an output parameter when calling a function that takes multiple arguments.

[mxgrey]

I suppose I can see the logic there. I personally prefer to use references whenever there is no ambiguity about whether the pointer is referring to a nullptr or to an actual thing, even if it's an output argument. But I also don't feel strongly enough about it to be opposed.

There's a legitimate reason for it, so I'm on board. 👍

[mkoval]

I'm fine with either convention in this case. If you're using non-const references for output parameters elsewhere in DART, then I'd prefer to change this function to be consistent.

[mxgrey]

If I've written any functions that have output parameters, they will definitely have used a non-const reference, but truthfully I'm not sure if I've done that in DART yet. We're sort of in the process of redefining the style guide for DART anyway, because the style has been largely Frankensteined over the years.

@jslee02 @karenliu Do either of you have a preference for using pointers versus references for output parameters of a function?

[mkoval]

I just pushed another commit that fixes a similar SEGFAULT in loadMesh.

[mxgrey]

After poking around through the source code, I've found numerous cases of passing an output parameter by reference instead of pointer. I think many of these uses were written by @jslee02 , so I suspect that might be his preference as well.

It's probably worth noting that many of the cases are passing an Eigen::Matrix output parameter by reference, which makes a lot of sense since vec[i] is a valid operation whether vec is an Eigen::Matrix or an Eigen::Matrix* type, but the meaning of that operation is wildly different for each case. Passing by reference at least avoids the need to say (*vec)[i].

[mkoval]

I switched to a a non-const reference output parameter to be consistent with the rest of DART.

Is this good to merge once the Travis and Appveyor builds complete?

[mxgrey]

It looks like you changed the dynamics::BodyNode* _parentNode argument in the createSkeletonRecursive function to be a non-const reference when I had been referring to the dynamics::BodyNode::Properties *node argument in the createDartNodeProperties function.

BodyNodes are typically handled as pointers, because things like assignment operations are illegal, and a nullptr BodyNode has special meaning, so there's an expectation that a BodyNode pointer may be a nullptr at any given time. On the other hand, BodyNode::Properties are completely copy-safe and a nullptr BodyNode::Properties would be nonsense, so it's preferable to pass them by value and reference.

[mkoval]

Ok, fixed for real now.

I was exhausted from battling with Assimp for the last few hours and apparently decided to change a random pointer to a reference in DartLoader.cpp. 😄

[mxgrey]

Looks good. If no one has any objections, I'll merge it in after the CI tests are finished.

[jslee02]

Could we add some regression test for this?

[jslee02]

Sorry for just throwing request but it would be good to have regression test for a fix PR to ensure that the problem is fixed.

[mkoval]

@jslee02 No problem! I've been busy trying to finish up some other code, so I haven't had a chance to write the tests yet. But I hope to get around to it soon if you leave the pull request open.

[jslee02]

Thanks! The test would be good enough if it just checks whether it returns nullptr in the error situations.

[mkoval]

I added unit tests for the error conditions that I fixed. Is there anything else I should do before we can merge this?

[mxgrey]

It might be better to dump the test into the existing testParser.cpp instead of breaking it out into its own file.

[mkoval]

All of the tests other than SDFSingleBodyWithoutJoint in testParser.cpp seem to be for the Skel parser. If anything, I'd prefer to split SDFSingleBodyWithoutJoint into a separate testSdfParser.cpp file and rename testParser.cpp to testSkelParser.cpp.

The parsers have different dependencies (urdfdom for URDF parsing) and take different input files, so I don't see any reason to put them in one file.

[mxgrey]

That's fair. Then I'd at least rename it to testURDFParsing.cpp or something similar. I don't anticipate the name DartLoader is going to stay around for too long (at least I hope not).

[mkoval]

I split the tests into three files: testDartLoader.cpp, testSkelParser.cpp, and testSdfParser.cpp. This matches the names of the respective classes. I definitely support making the names consistent, but I think that should be done as part of whatever pull request renames the actual classes.

[mxgrey]

I guess the reason I would prefer the name testURDFParsing is because it feels more accurate and descriptive to me than testDartLoader (which makes it sound like it's testing something that loads dart), but I suppose it's reasonable to have the file names match the classes that they pertain to.

👍

[jslee02]

👍 Just waiting for Travis CI to be happy