Add warnings for unsupported shape pairs of DARTCollisionDetector

[jslee02]

This change is

[mxgrey]

Reviewed 3 of 4 files at r1.
Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

dart/collision/dart/DARTCollide.cpp, line 53 [r1] (raw file):

        << getShapeName(shape1) << "'-'"\
        << getShapeName(shape2) << "'. "\
        << "Returning false.\n";

Maybe tweak this to:

  dterr << '[DARTCollisionDetector] Attempting to check for an "\
        << "unsupported shape pair: [" << typeid(*shape1).name()\
        << "] - [" << typeid(*shape2).name() << ]. Returning false.\n";

This way it would accurately report the issue even if a non-standard shape gets used.

---

Comments from Reviewable

[jslee02]

dart/collision/dart/DARTCollide.cpp, line 53 [r1] (raw file):

Previously, mxgrey (Michael X. Grey) wrote…

Maybe tweak this to:

  dterr << '[DARTCollisionDetector] Attempting to check for an "\
        << "unsupported shape pair: [" << typeid(*shape1).name()\
        << "] - [" << typeid(*shape2).name() << ]. Returning false.\n";

This way it would accurately report the issue even if a non-standard shape gets used.

👍 Good idea! I have a little concern that typeid(~).name() sometimes return a little bit mangled name, but I would prefer it since it's more flexible and the mangled​ name s are still reasonably readable.

---

Comments from Reviewable

[mxgrey]

Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

dart/collision/dart/DARTCollide.cpp, line 53 [r1] (raw file):

Previously, jslee02 (Jeongseok Lee) wrote…

👍 Good idea! I have a little concern that typeid().name() sometimes return a little bit mangled name, but I would prefer it since it's more flexible and the mangled​ name s are still reasonably readable.

Yeah, it would be nice if a de-mangled "pretty print" version were available for typeid().name(), but we can always de-mangle the printout while debugging.

---

Comments from Reviewable

[mkoval]

Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

dart/collision/dart/DARTCollide.cpp, line 53 [r1] (raw file):

Previously, mxgrey (Michael X. Grey) wrote…

Yeah, it would be nice if a de-mangled "pretty print" version were available for typeid(~).name(), but we can always de-mangle the printout while debugging.

I would prefer to add virtual getType and a static getStaticTypefunctions, like we currently have onJoint`. That way we get the best of both worlds: (1) no RTTI, which is brittle when using dynamic linking, (2) a readable name, and (3) user-extensibility. Thoughts?

---

Comments from Reviewable

[jslee02]

dart/collision/dart/DARTCollide.cpp, line 53 [r1] (raw file):

Previously, mkoval (Michael Koval) wrote…

I would prefer to add virtual getType and a static getStaticTypefunctions, like we currently have onJoint`. That way we get the best of both worlds: (1) no RTTI, which is brittle when using dynamic linking, (2) a readable name, and (3) user-extensibility. Thoughts?

My only concern is the performance of type checking. The cost would be virtual function call + string comparing. As I known, string comparing is just pointer comparison so virtual function call is the only overhead, right?

I don't have an objection to​ using type functions.

---

Comments from Reviewable

[jslee02]

Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

dart/collision/dart/DARTCollide.cpp, line 53 [r1] (raw file):

Previously, jslee02 (Jeongseok Lee) wrote…

My only concern is the performance of type checking. The cost would be virtual function call + string comparing. As I known, string comparing is just pointer comparison so virtual function call is the only overhead, right?

I don't have an objection to​ using type functions.

I remember that there was one reason we stayed with enumerate​. The shape type wouldn't be easy to be extended with full support of related functionalities such as rendering and collision detection. Also, in general, switch statement with enumerate would be cheaper than if-else if-else statement. However, now I'm inclined to having virtual getType() because of consistency and that this is not the performance critical.

This can be handled in a separate PR. I'm merging this PR if there is no objection.

---

Comments from Reviewable