When you create a simple cube Eigen::AlignedBox3d box( Eigen::Vector3d::Zero(), Eigen::Vector3d::Ones() ); corner vectors were as follows: std::cout << Eigen::AlignedBox3d::BottomLeftFloor << " " << box.corner(Eigen::AlignedBox3d::BottomLeftFloor) << std::endl << Eigen::AlignedBox3d::BottomRightFloor << " " << box.corner(Eigen::AlignedBox3d::BottomRightFloor) << std::endl << Eigen::AlignedBox3d::TopLeftFloor << " " << box.corner(Eigen::AlignedBox3d::TopLeftFloor) << std::endl << Eigen::AlignedBox3d::TopRightFloor << " " << box.corner(Eigen::AlignedBox3d::TopRightFloor) << std::endl << Eigen::AlignedBox3d::BottomLeftCeil << " " << box.corner(Eigen::AlignedBox3d::BottomLeftCeil) << std::endl << Eigen::AlignedBox3d::BottomRightCeil << " " << box.corner(Eigen::AlignedBox3d::BottomRightCeil) << std::endl << Eigen::AlignedBox3d::TopLeftCeil << " " << box.corner(Eigen::AlignedBox3d::TopLeftCeil) << std::endl << Eigen::AlignedBox3d::TopRightCeil << " " << box.corner(Eigen::AlignedBox3d::TopRightCeil) << std::endl; E Vector Eigen CornerType True CornerType 0 [0, 0, 0] BottomLeftFloor --> Bottom Left Floor 1 [1, 0, 0] BottomRightFloor --> Bottom Left Ceil 2 [0, 1, 0] TopLeftFloor --> Bottom Right Floor 3 [1, 1, 0] TopRightFloor --> Bottom Right Ceil 4 [0, 0, 1] BottomLeftCeil --> Top Left Floor 5 [1, 0, 1] BottomRightCeil --> Top Left Ceil 6 [0, 1, 1] TopLeftCeil --> Top Right Floor 7 [1, 1, 1] TopRightCeil --> Top Right Ceil I think the correct enum values should be as follows: BottomLeftFloor = 0, BottomRightFloor = 2, TopLeftFloor = 4, TopRightFloor = 6, BottomLeftCeil = 1, BottomRightCeil = 3, TopLeftCeil = 5, TopRightCeil = 7
I guess it depends on where you draw the axes, but I agree that the enum values are rather unfortunate. The same goes for the names; to me it seems a bit weird to have both top/bottom and floor/ceil(ing). It's probably a bit late to change this though.
Right, the main issue here is that the names are meaningless. We cannot change them, but what we can do is to add a new set of names, and mark the old ones as deprecated. For instance, Floor/Ceil could be replaced by Front/Back, and the names could be order following the X, Y, and Z axis: {Left,Right} {Bottom,Top}[Back,Front} In your example we would have: Left: x=0, Right: X=1, Bottom: y=0, Top: y=1, Back: z=0, Front: z=1
Thank you for your attention! Сan you reconsider the "isEmpty" method of this class also? inline bool isEmpty() const { return (m_min.array() > m_max.array()).any(); } Can be expanded up to complete it as follows? inline bool isEmpty() const { return (m_min.array() > m_max.array()).any() || m_min.array() == m_max.array()).all(); } Or so (most likely will run faster): inline bool isEmpty() const { return (m_min.array() > m_max.array()).any() || !(m_min.array() != m_max.array()).any()); }
well, if m_min==m_max, then it is not empty since it contains the point m_min.
Well, that's correct, thank you
Created attachment 306 [details] Patch for AlignedBox corner names. Adds more meaningful corner names and documentation (Needs DISTRIBUTE_GROUP_DOCS=YES in Doxyfile). Adds a method to obtain all the box corners in a matrix. Adds a method to scale the box in all dimensions.
Comment on attachment 306 [details] Patch for AlignedBox corner names. >+ typedef Matrix<Scalar,AmbientDimAtCompileTime,1<<AmbientDimAtCompileTime> CornerMatrixType; You should consider the case AmbientDimAtCompileTime==Dynamic (also below, when creating the matrix) >+ /** Corner names for a 1D, 2D or 3D bounding box. The old names for 1D and 3D >+ have been deprecated as they are inconsistent and meaningless for 3D. */ > enum CornerType > { I'd say these changes are ok. It would be nice to actually deprecate the old types (I don't know if there is a portable way to do that). >- inline VectorType corner(CornerType corner) const >+ inline VectorType corner(CornerType c) const I don't understand the necessity of refactoring this function. >+ inline CornerMatrixType corners() const >+ { >+ CornerMatrixType res; res needs to be constructed for dynamic dimensions (see above). Rest of the function is ok, I'd say. >+ /** Scale \c *this by the ratios found in array \a t and returns a reference to \c *this. */ >+ template<typename Derived> >+ inline AlignedBox& scale(const ArrayBase<Derived>& a_t) >+ { >+ const typename internal::nested<Derived,2>::type t(a_t.derived()); >+ Derived c = center(); >+ Derived d = diagonal(); I don't think Derived is the correct type for c and d. How about simply this (untested): VectorType c = center(); VectorType d = diagonal().array()*a_t/2.; m_min = c - d; m_max = c + d; And a final remark: Unit tests for the new functions would be nice.
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/476.