new bounding box semantics #547
Conversation
| @@ -121,7 +126,7 @@ class Shape : public virtual common::Subject | |||
| // biased mesh shape. Two Vector3ds might be better; one is for | |||
| // minimum verterx, and the other is for maximum verterx of the | |||
| // bounding box. | |||
| const Eigen::Vector3d& getBoundingBoxDim() const; | |||
| Eigen::Vector3d getBoundingBoxDim() const; | |||
There was a problem hiding this comment.
This function should be marked as DEPRECATED and the TODO should probably be made a comment that points the user to getBoundingBox.
There was a problem hiding this comment.
Okay, whats the proper way of deprecating in dart?
There was a problem hiding this comment.
There is a DEPRECATED macro defined in Deprecated.h that has #ifdefs for all of the supported compilers. The argument is the major/minor version number when the deprecation occurred.
DART 5.1.1 was just released, so I suppose it should be DEPRECATED(5.2) now?
There was a problem hiding this comment.
DEPRECATED(5.2) would be fine. In general, we might not be sure the deprecation is included in which release at the moment. So I usually increment the minor version with 1 and update the version number if needed when DART is released.
There was a problem hiding this comment.
This code would work for the deprecating.
//
// Shape.h
//
/// [Original comment]
/// \deprecated Please use getBoundingBox() instead
DEPRECATED(5.2)
const Eigen::Vector3d& getBoundingBoxDim() const;
//
// Shape.cpp
//
const Eigen::Vector3d& Shape::getBoundingBoxDim() const
{
static Eigen::Vector3d boundingBoxDim = mBoundingBox.computeFullExtents();
return boundingBoxDim;
// or by not removing mBoundingBoxDim
mBoundingBoxDim = mBoundingBox.computeFullExtents();
return mBoundingBoxDim;
}There was a problem hiding this comment.
Added this in the latest commit.
|
This pull request looks reasonable to me. The only question I would have is: Are we currently using the bounding box dimension information anywhere meaningful in the DART source code? All of the changes seem to be internal to the Shape class and its derivatives. Are we using bounding boxes in the collision detection components of DART, or are we just letting the third-party libraries deal with those? I'll check this myself, but if anyone already knows the answer, then that might save me some time. |
|
It doesn't look like we need to worry about making any other changes to the code, since the bounding boxes are only used internally by shapes right now. 👍 Edit: Although we should add the |
|
Okay, assuming the tests pass, can we merge this? |
|
Looks good to me 👍 If @jslee02 approves, I think it's ready to merge. |
|
It looks good to me as well. Thank you for your contribution! Merging now. |
Replaces old
boundingBoxDimsemantics with a newBoundingBoxclass that has a minimum and maximum. This reduces ambiguity and allows for off-center bounding boxes.