Re: [eigen] Patch to AlignedBox

[ Thread Index | Date Index | More Archives ]

Hi Gael,
Thanks for the response.

> what's the use case for "expand"?
In this case I am using an AlignedBox3d to keep track of the view limits for an OpenGL volume rendering application. I implemented zooming into/out of the volume by expanding/contracting the AlignedBox. AlignedBox already has a translate() method, so I thought it was odd that is was missing some way to scale it as well.

> (I think "scale" would be less
> misleading since we already have a "extend" function with a totally
> different behavior)
I completely agree.

> Regarding CornerMatrixType I think it should be a fixed size matrix
> instead of a dynamic one.
For some odd reason when I wrote it the first time I convinced myself you couldn't compute the matrix size at compile time. But obviously you can, so I've actually made this change already by defining:
typedef Matrix<Scalar,AmbientDimAtCompileTime,1 << AmbientDimAtCompileTime> CornerMatrixType;

> Finally, regarding the new names, I'd rather sort the words in X, Y, Z
> order, what do you think?
This is unfortunately a matter of personal preference. I think that X, Y, Z is the more logical ordering. However the 2D names are currently defined in Y, X order, so I went Z, Y, X for consistency. Perhaps it is also more common to say "Bottom Left" rather than "Left Bottom" in daily usage? To make it even more confusing, the 1D names are "Min" and "Max", which don't correspond to any particular axis but are more general. I would argue the most important thing here is consistency, so which ever ordering is chosen the 1D and 2D names should match.

Is there some way to mark the old names as deprecated, beyond a comment in the code and note in the documentation?

I'm happy to submit an updated patch once the above is decided on.

Best wishes,

Mail converted by MHonArc 2.6.19+