Eigen Alignment issue

[alecjacobson]

https://eigen.tuxfamily.org/dox/group__TopicStructHavingEigenMembers.html

A memory alignment assertion fired once for the .data_list in the viewer. I'm not sure if we also need the special stl vector allocator for the data list of if this is enough to fix it. It's hard to get this assertion to fire in a reproducible way.

Check all that apply (change to [x])

• All changes meet libigl style-guidelines.
• Adds new .cpp file.
• Adds corresponding unit test.
• Adds corresponding python binding.
• This is a minor change.

[jdumas]

The problem is probably the member Eigen::Vector4f line_color which is stack-allocated. Other members seems to be heap-allocated matrices so I don't imagine they pose any problem.

[alecjacobson]

That's what I thought. Do you know if this is enough or if we also need that stl-vector allocator for data_list?

[jdumas]

Honestly I would rather disable alignment for this specific case Eigen::Matrix<float,4,1,Eigen::DontAlign> v, since I don't imagine it's performance-critical...

[alecjacobson]

I'm worried later on somebody will add another Vector4f field and forget this...

[jdumas]

Fair enough, but I'm not versed enough in Eigen's arcane to know if we need to mess also with the std::vector allocator. If we need to specialize it then it's also gonna be easy for the users to forget when they write their own std::vector<ViewerData> in their apps.

[alecjacobson]

We can be really conservative and do everything. We should get an official answer from Eigen whether an stl container of classes containing fixed-size vectorizable Eigen types needs this special allocator... I'll post something

[alecjacobson]

Actually from the first sentence of https://eigen.tuxfamily.org/dox/group__TopicStlContainers.html it seems pretty clear that we do need the special allocator for std::vector<igl::ViewerData , ...

[jdumas]

Yeah then I vote for really disabling alignment. I may use std::vector<igl::ViewerData> in my code (of stuff unrelated to the data_list of the viewer), and it would be extremely annoying to have to pay attention to alignment.

[alecjacobson]

OK. But let's put a big warning at the top and bottom of the class that any new members should not be aligned(-able).

…

On Fri, Nov 23, 2018 at 4:02 PM Jérémie Dumas ***@***.***> wrote: Yeah then I vote for really disabling alignment. I may use std::vector<igl::ViewerData> in my code (of stuff unrelated to the data_list of the viewer), and it would be extremely annoying to have to pay attention to alignment. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1029 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACI0me59tb0Zm5wsbQjODK6D_50tJ8hWks5uyGJ5gaJpZM4YxFf2> .

[jdumas]

It's a warning that should apply on all classes, not only this one right? And in any case, I don't see that new members will be added very often to this class, but a warning wouldn't hurt.

[alecjacobson]

The eigen issue applies to all classes, but we're choosing a particular solution (to not use alignment, rather than to do the macros/std allocators). For other classes, it might make more sense to use those options. So the warning here is specific to this class and the decision we're making.

…

[jdumas]

Fair enough. I find the solution of disabling alignment less intrusive in this case, since it doesn't propagate to the user code, so I suggest we do that with a warning in the comments.

[bubnikv]

Speaking of memory alingment issues: We at Prusa Research started to use libigl for bits and pieces on the Slic3r Prusa Edition project (slicer for FDM / SLA fabrication), thank you!

We stumbled over memory alignment issues reported by the Visual Studio 2013, 32bit builds, for example:
igl\AABB.h(73) : error C2719: 'other': formal parameter with __declspec(align('16')) won't be aligned

This is what Microsoft has to say to it:
https://msdn.microsoft.com/en-us/library/373ak2y1.aspx?f=255&MSPPError=-2147217396

I interpret the error as violating the Eigen recommendation to never pass memory aligned structures by value to a function.
https://eigen.tuxfamily.org/dox/group__TopicPassingByValue.html

I understand that the libigl authors do not guarantee 32bit compatibility, but here (likely by a lucky accident) the Visual Studio 2013 32bit compiler gives us a hint where we may have alignment issues.

For our code to work, I had to fix the copy constructor of AABB to accept a refernce

•  AABB& operator=(AABB other)
  

•  AABB& operator=(const AABB &other)
   {
  

•    swap(*this,other);
  

•    this->deinit();
  

•    m_left  = other.m_left ? new AABB(*other.m_left) : NULL;
  

•    m_right = other.m_right ? new AABB(*other.m_right) : NULL;
  

•    m_box   = other.m_box;
  

•    m_primitive = other.m_primitive;
     return *this;
   }
   AABB(AABB&& other):
  

and ClosestBaryPtPointTriangle lambda in igl::point_simplex_squared_distance() to accept references as well:
const auto & ClosestBaryPtPointTriangle =
[&Dot](const Point &p, const Point &a, const Point &b, const Point &c, BaryPoint& bary_out )->Point

[jdumas]

Hmm, I don't think the current implementation of AABB trees in libigl is very efficient, since it seems to be using pointers. You might want to check out geogram which has a very efficient implementation of AABB trees.

That being said if you have fixes you want to contribute to the current class, we'd be happy to evaluate PRs. I don't remember exactly what kind of issue there was with 32bit compilation on Windows, but it was probably alignment-related as well. If 64bits work for you (and passed valgrind/addressSanitizer checks), then you might as well do nothing.