Bug 370 - Eigen::AlignedBox3f typedef
: Eigen::AlignedBox3f typedef
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Geometry
: 3.0
: All All
: Normal normal
Assigned To: Nobody
:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-10-31 13:40 UTC by Dennis Schridde
Modified: 2011-11-16 15:35 UTC (History)
4 users (show)



Attachments
Proposed patch (2.72 KB, patch)
2011-11-08 14:38 UTC, Dennis Schridde
no flags Details | Diff | Splinter Review
Patch to Matrix typedefs (726 bytes, patch)
2011-11-08 14:39 UTC, Dennis Schridde
no flags Details | Diff | Splinter Review
Improved patch (3.98 KB, patch)
2011-11-09 22:15 UTC, Dennis Schridde
no flags Details | Diff | Splinter Review

Description Dennis Schridde 2011-10-31 13:40:40 UTC
It would be nice if Eigen::AlignedBox would have 3f,2f,... typedefs, like
Eigen::Matrix has.
Comment 1 Benoit Jacob 2011-10-31 13:44:03 UTC
Want to make a patch? ;-) see how it's done in Matrix.h
Comment 2 Dennis Schridde 2011-11-08 14:38:51 UTC
Created attachment 226 [details] [review]
Proposed patch

Attached patch appears to work. Review it anyway, please.
Comment 3 Dennis Schridde 2011-11-08 14:39:41 UTC
Created attachment 227 [details] [review]
Patch to Matrix typedefs

While creating attachment #226 [details] [review] I came across a potential mistake in the Matrix
typedefs.
Comment 4 Jitse Niesen 2011-11-09 12:32:46 UTC
Comment on attachment 226 [details] [review]
Proposed patch

Review of attachment 226 [details] [review]:
-----------------------------------------------------------------

I do not use the Geometry module, but this patch seems very localized and thus
safe. I have some questions on details (see the comments). I would also like to
see a test; this could be as simple as changing test/geo_alignedbox to use the
new typedef in some places.

::: Eigen/src/Geometry/AlignedBox.h
@@ +379,5 @@
> +EIGEN_MAKE_TYPEDEFS_ALL_SIZES(int,                  i)
> +EIGEN_MAKE_TYPEDEFS_ALL_SIZES(float,                f)
> +EIGEN_MAKE_TYPEDEFS_ALL_SIZES(double,               d)
> +EIGEN_MAKE_TYPEDEFS_ALL_SIZES(std::complex<float>,  cf)
> +EIGEN_MAKE_TYPEDEFS_ALL_SIZES(std::complex<double>, cd)

Does AlignedBox work when Type is complex? I would think not; you need an
ordering of the numbers.

@@ +398,5 @@
> +EIGEN_USING_ALIGNEDBOX_TYPEDEFS_FOR_TYPE(i) \
> +EIGEN_USING_ALIGNEDBOX_TYPEDEFS_FOR_TYPE(f) \
> +EIGEN_USING_ALIGNEDBOX_TYPEDEFS_FOR_TYPE(d) \
> +EIGEN_USING_ALIGNEDBOX_TYPEDEFS_FOR_TYPE(cf) \
> +EIGEN_USING_ALIGNEDBOX_TYPEDEFS_FOR_TYPE(cd)

Is the EIGEN_USING_ALIGNED_TYPEDEFS macro needed? I assume the definitions were
copied from Matrix.h. In the case of Matrix.h, the macros are used in the
definition of USING_PART_OF_NAMESPACE_EIGEN in the Eigen2 era; you can still
find that macro in Eigen2Support. However, I think this idea has been abandoned
in Eigen3.

Incidentally, the EIGEN_USING_MATRIX_TYPEDEFS macro should perhaps be moved
from Matrix.h to Eigen2Support.
Comment 5 Jitse Niesen 2011-11-09 13:27:29 UTC
(In reply to comment #3)
> Created attachment 227 [details] [review] [review]
> Patch to Matrix typedefs
> 
> While creating attachment #226 [details] [review] [review] I came across a potential mistake in the Matrix
> typedefs.

I am committing this patch. Thanks.
Comment 6 Dennis Schridde 2011-11-09 22:15:25 UTC
Created attachment 228 [details] [review]
Improved patch

Improved according to comment #4.
---
[Geometry/AlignedBox] New typedefs, like for Core/Matrix

Includes 1-4 and dynamic sized boxes for int, float and double type.
Also changes the tests to use these typedefs.
---

I added 1-dim boxes because the tests used them and removed the c* types.
Comment 7 Jitse Niesen 2011-11-16 15:35:10 UTC
I committed your patch (changeset 1c6393ab51e0). 

Thanks very much for your help.

Note You need to log in before you can comment on or make changes to this bug.