Re: [eigen] Enhanced AlignedBox

[ Thread Index | Date Index | More Archives ]

** sorry, I sent the previous email by mistake, please ignore it. **

Hi Manuel,

On Sun, Dec 20, 2009 at 11:30 PM, Manuel Yguel <manuel.yguel@xxxxxxxxx> wrote:
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

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()) };


- best regards,


Mail converted by MHonArc 2.6.19+