jlblancoc / nanoflann

nanoflann: a C++11 header-only library for Nearest Neighbor (NN) search with KD-trees
Other
2.15k stars 482 forks source link

Segmentation fault with duplicate points #93

Closed jammy4536 closed 5 years ago

jammy4536 commented 5 years ago

I recently encountered issues when trying to incorporate this library into an SPH code, and in trying to debug (The reason for the debug is also dubious: I realised I had to search the square of the radius I was interested in), I created some duplicate points. When I try to perform a radius search on one of the points, the library gives a segmentation fault. Whilst it's perhaps bad to have duplicate points in the space, it is possible in SPH simulations to have points overlap (normally when the simulation goes berserk), and I don't feel it's a robust handling of what is likely a common scenario. Surely the library should return it as any other neighbour, with a distance of 0.0. I am using the KDTreeVectorofVectorsAdaptor for a std::vector data structure, should that have any influence.

svenevs commented 5 years ago

Shot in the dark, but it sounds like maybe you aren't using a proper aligned allocator with your std::vector. If you are not, you should, and this is likely the reason for the segfault (rather than the existence of duplicate points).

The segfault probably comes from deep within eigen, where it's trying to vectorize something on unaligned memory. Depending on the platform, that usually results in either a segfault or a bus error.

Hope that is useful...

jammy4536 commented 5 years ago

I'm allocating the vector using push_back for each point. Any suggestions for aligning the allocation? Suppose it would be better to elaborate the data structure. It's actually a vector of which the structure is filled with Eigen::Vector2d vectors. typedef struct Particle { Particle(Vector2d x, Vector2d v, Vector2d f, float rho, float Rrho, bool bound) : xi(x), v(v), V(0.0,0.0), f(f), rho(rho), p(0.0), Rrho(Rrho), b(bound){} Vector2d xi, v, V, f; float rho, p, Rrho; bool b; int size() const {return(xi.size());} double operator[](int a) const { return(xi[a]); } }Particle;

svenevs commented 5 years ago

It's one of the template parameters to the declaration of the vector itself: https://eigen.tuxfamily.org/dox/group__TopicStlContainers.html

P.S. if you're using c++11 or later, use emplace_back ;)

On Sat, Aug 18, 2018, 1:39 PM Jamie MacLeod notifications@github.com wrote:

I'm allocating the vector using push_back for each point. Any suggestions for aligning the allocation?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jlblancoc/nanoflann/issues/93#issuecomment-414084975, or mute the thread https://github.com/notifications/unsubscribe-auth/AFmXZYenBKMUA5ho-U53pJARiv8KdLeuks5uSHuIgaJpZM4WCtYD .

jammy4536 commented 5 years ago

Brilliant. I think I've got the necessary adjustments to the code (I edited the previous comment to elaborate the data structure). I will test tomorrow however and report back. At any rate, this has probably made the memory much safer regardless.

jammy4536 commented 5 years ago

Yep, that seems to have solved it! A huge thanks for your help! Ended up being a one line fix. In the structure Particle, I added EIGEN_MAKE_ALIGNED_OPERATOR_NEW as suggested in the documentation.

svenevs commented 5 years ago

Excellent! If it makes you feel better, I learned about these things (aligned allocator / operator needed) from similar segfaults, but now that you know to keep an eye out for it...if you forget in the future you'll know the fix almost immediately :)