-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#40 Performance improvements #45
Conversation
- Implement randomized vertex insertion option and make it default - Allow custom nearest point locators - Implement boost::rtree and random-nearest as locators - Implement kd-tree for nearest point location - Switch index types used in CDT (e.g., vertices, triangles) from 64-bit to 32-bit - 64-bit index types can be enabled with CDT_USE_64_BIT_INDEX_TYPE - CDT code refactoring, some APIs changed - Make necessary adjustments to visualizer code - Compatibility with c++98 standard
a38de72
to
eddbe7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, a few questions/recommendations but nothing that I can tell is a show stopper.
CDT/include/CDT.h
Outdated
@@ -180,27 +198,22 @@ class CDT_EXPORT Triangulation | |||
private: | |||
/*____ Detail __*/ | |||
void addSuperTriangle(const Box2d<T>& box); | |||
void insertVertex(const V2d<T>& pos); | |||
void addNexVertex(const V2d<T>& pos, const TriIndVec& tris); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this should be addNextVertex(...)?
void addNexVertex(const V2d<T>& pos, const TriIndVec& tris); | |
void addNextVertex(const V2d<T>& pos, const TriIndVec& tris); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed in b3143fb
It was supposed to be addNewVertex
:)
CDT/include/CDT.h
Outdated
|
||
vertices.reserve(nExistingVerts + std::distance(first, last)); | ||
for(TVertexIter it = first; it != last; ++it) | ||
addNexVertex(V2d<T>::make(getX(*it), getY(*it)), TriIndVec()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addNexVertex(V2d<T>::make(getX(*it), getY(*it)), TriIndVec()); | |
addNextVertex(V2d<T>::make(getX(*it), getY(*it)), TriIndVec()); |
CDT/include/KDTree.h
Outdated
/// @tparam MaxStackDepth size of statically allocated stack for nearest query. | ||
template < | ||
typename TCoordType, | ||
size_t NumVerticesInLeaf = 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test to see if this is a good enough leaf size. I did some testing with my original tree but it had to compete against pointer calls. It might be more efficient to use smaller leafs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some quick testing and smaller size does not improve the situation but makes it slightly worse. Bigger sized can speed-up things just a notch. I've ended up setting 32 as default for now.
CDT/include/KDTree.h
Outdated
{} | ||
}; | ||
// allocated in class (not in the 'nearest' method) for better performance | ||
mutable CDT::array<NearestTask, MaxStackDepth> m_tasksStack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't like mutable, I do understand that this is probably a good efficiency boost.
I would warn though that since we always split in half. We don't have access to all the data at the start. This tree is not balanced, if you add lets say 100 points close together and then one point really far away I believe (I haven't tried it) you will hit the MaxStackDepth pretty easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a4c7808
I switched array
to std::vector
so now we should not run out of stack depth. Performance looks to be virtually the same.
struct Node | ||
{ | ||
children_type children; ///< two children if not leaf; {0,0} if leaf | ||
point_data_vec data; ///< points' data if leaf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience allocation of deallocation of vectors is expensive. Without testing it I can't tell if this will be significant in this case. Just something to keep an eye on in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this implementation it's reserved to NumVerticesInLeaf
on construction and deallocated when the leaf is split.
So re-allocations should not be causing too many problems I hope.
Make initial stack size and stack increment when re-sizing template parameters Performance is the same
…ee, nearest-random)
Andre Fecteau contributed original KD-tree implementation: https://github.com/AndreFecteau/CDT/tree/OptimizationByUsingKDTree It was heavily modified and included into CDT source.
Removed other locators and make kd-tree the only default one. The reason is to have less code to maintain as kd-tree is clearly superior. |
Add Andre Fecteau to contributors list
bbcf5c4
to
43dd70c
Compare
After changes from pull request artem-ogre#45 initialization with a grid no longer worked and the file didn't compile at all.
After changes from pull request #45 initialization with a grid no longer worked and the file didn't compile at all.
commit 6e86109 Author: artem-ogre <artem.ogre@gmail.com> Date: Tue Oct 26 21:17:32 2021 +0200 Update CDT version in CMakeLists.txt commit a5e1339 Author: artem-ogre <artem.ogre@gmail.com> Date: Tue Oct 26 21:00:06 2021 +0200 Fix InitializeWithGrid and VerifyTopology after CDT re-factoring Add instantiations to CDT.cpp to avoid such problems in the future commit 6008441 Author: Zhivko Bogdanov <zhivkobog@gmail.com> Date: Tue Oct 26 19:51:31 2021 +0200 Fixed output from `generateGridVertices`. After changes from pull request artem-ogre#45 initialization with a grid no longer worked and the file didn't compile at all.
commit 42fdbb7 Author: artem-ogre <artem.ogre@gmail.com> Date: Thu Oct 28 14:14:21 2021 +0200 Bump version to 1.1.2 in CMakeLists.txt commit 37fe95d Author: artem-ogre <artem.ogre@gmail.com> Date: Wed Oct 27 15:14:37 2021 +0200 Update doxygen comments commit 62c6047 Author: artem-ogre <artem.ogre@gmail.com> Date: Wed Oct 27 14:18:30 2021 +0200 Remove default template arguments from KDTree class commit 9b7ad4f Author: Zhivko Bogdanov <zhivkobog@gmail.com> Date: Wed Oct 27 12:42:21 2021 +0200 Format changes using the provided .clang-format style. commit 1b356db Author: Zhivko Bogdanov <zhivkobog@gmail.com> Date: Wed Oct 27 09:29:18 2021 +0200 Renamed coordinate type template argument failing compilation. commit 6ceafa5 Author: Zhivko Bogdanov <zhivkobog@gmail.com> Date: Wed Oct 27 09:22:27 2021 +0200 Added missing additional template argument to the triangulation. After the refactor there is an additional template argument that was missing from the output parameter in `initializeWithRegularGrid` and `initializeWithIrregularGrid`. commit b8cf8f8 Author: Zhivko Bogdanov <zhivkobog@gmail.com> Date: Wed Oct 27 09:10:31 2021 +0200 Provide customization points to the k-d tree in the locator. If we want to customize any of the k-d tree template values, we have to change the instance within the locator. To support multiple locators with different configurations, it's best to have the template arguments available in the locator declaration as well. commit 86510f5 Author: Artem Amirkhanov <artem.ogre@gmail.com> Date: Wed Oct 27 10:04:23 2021 +0200 Change 'C++03' to 'C++98' in README commit 8f1b434 Author: artem-ogre <artem.ogre@gmail.com> Date: Wed Oct 27 09:51:00 2021 +0200 Move in generateGridVertices if c++11 is supported commit 6e86109 Author: artem-ogre <artem.ogre@gmail.com> Date: Tue Oct 26 21:17:32 2021 +0200 Update CDT version in CMakeLists.txt commit a5e1339 Author: artem-ogre <artem.ogre@gmail.com> Date: Tue Oct 26 21:00:06 2021 +0200 Fix InitializeWithGrid and VerifyTopology after CDT re-factoring Add instantiations to CDT.cpp to avoid such problems in the future commit 6008441 Author: Zhivko Bogdanov <zhivkobog@gmail.com> Date: Tue Oct 26 19:51:31 2021 +0200 Fixed output from `generateGridVertices`. After changes from pull request artem-ogre#45 initialization with a grid no longer worked and the file didn't compile at all.
boost::rtree
and random-nearest as locatorsCDT_USE_64_BIT_INDEX_TYPE