Hi Manuel,
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. I have some remarks though:
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().norm() 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...)
I don't care much, if someone else has an opinion...
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
agree.
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.
Well, as you said that depends on the semantic we give to our bounding box, and to the size (side) function. Should it return the number of elements or the length? Since it is called a *bounding* box, the min and max represent bounds, and so it make more sense to me to return the length, so min-max. For instance, an AABB of integers is useful, to represent a region in, e.g., a 3D grid. Then the min() and max() represents the indices of the respective corner, and max()-min() represents the number of cells in each dimension, and so it makes sense to make sizes() returns max()-min(). Moreover, this simplify the implementation and avoid an inconsistent behavior.
In that case, note that diagonal() == size()
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.
ok, I'll check the generated code (paranoia)
What do you think about those proposals?
If you agree with that lines, I plan to write an OrientedBox class
with almost same interface.
why not.
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 ?
We did not discussed about that yet, so feel free to start one if you have some ideas, but basically the way I see it is to add to the functions relying on random numbers a functor parameters defaulting to a functor wrapping ei_random(). For instance for the sample function you wrote we could do:
template<typename RNG> VectorType sample(const RNG& rng) const { /* ... */ };
VectorType sample() const { return sample(DefaultRNG()) };
gael
- best regards,
Manuel