On Sun, Dec 20, 2009 at 11:30 PM, Manuel Yguel
<manuel.yguel@xxxxxxxxx> wrote:
Hello,
I have written some small pieces of codes to enhance AlignedBox.
The following changes I propose are as follow:
thanks a lot for your propositions and the patch but I have some concerns:
1) Define some new functions:
Scalar size( size_t k ) --> returns the size of the k-th side
Vector sizes() --> returns a vector with all the sides
Note that in the patch they are called side and sides, and I do prefer size/sizes. Also I'm thinking that size() in not needed. Indeed, if we re-implement sizes() to return an _expression_ rather than a vector, then box.sizes()[i] we'll be equivalent to size(i).
Likewise, if we should re-implement center() to return an _expression_ so that the center(int) overload is not needed.
In the same vein, minSize() is only shortcut for .sizes().minCoeff() that is not much longer and at least as explicit, so I'm not sure these two shortcuts (minSize/maxSize) are really needed. Note that though the volume() function is only a shortcut for "sizes().prod()", it really adds a semantic value, so it's fine to keep this one.
Vector corner( CornerType t ) --> defines some enum and return the
corresponding corner for 1D,2D and 3D boundingBox
why not but we should provide nice names for each dimension, I mean BottomLeft/BottomRight does not really make sens for a 1D AABB. So what about adding some aliases:
- Min Max for 1D,
- and *Floor versions for 3D.
Scalar squaredDiagonale()
Real diagonal()
diagonal() should rather return an _expression_ of the diagonal vector (=> alias for sizes(), at least for floats), and then these two functions should rather be renamed diagonalNorm() and squaredDiagonalNorm(), and then we see they are not really needed because they do not add any value compared to: diagonal().squaredNorm() diagonal().squaredNorm()
Scalar volume()
Vector sample() --> sample a point in the boundingBox volume with a
uniform distribution
2) Add the function isEmpty (I do not like very much the isNull name,
it is a matter of taste, so if it does not fit you, let me know...)
3) Semantic: I think that it is usefull to have bounding boxes with
integers: in this case the min and the max are considered inside the
bounding box and a side of the bounding box is max - min + 1 instead
of max - min.
This is a complex semantic problem, but I think it is the more natural choice.
4) Following that line, I have changed the code for the distances
functions such that non-negative integers types are safely handled and
I trust the compiler to produce as optimized code as the one that Gael
has written.
I know that such types are not natively handled by Eigen, but I think
that it is a safe change.
What do you think about those proposals?
If you agree with that lines, I plan to write an OrientedBox class
with almost same interface.
Also, I would like to launch a topic about the way random numbers are
generated because when writing sample(), I found that we can enhance
the way it is done.
Is this already an active topic ?
- best regards,
Manuel