Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GeoLib OctTree #714

Merged
merged 3 commits into from
Jun 16, 2015
Merged

GeoLib OctTree #714

merged 3 commits into from
Jun 16, 2015

Conversation

TomFischer
Copy link
Member

This PR contains a rework of class OctTree, which will be used in another PR fixing some issues in GeoLib::GEOObject that is already in preparation (see branch GeoLibReworkMergingGeometries). Also in this PR a tests for OctTree are added.

Maybe the OctTree can also be useful for other things like mesh node search?

@ogsbot
Copy link
Member

ogsbot commented Jun 12, 2015

Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/270/

@ogsbot
Copy link
Member

ogsbot commented Jun 12, 2015

Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/200/

@ogsbot
Copy link
Member

ogsbot commented Jun 12, 2015

Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/272/

endJunction
endJunction reviewed Jun 12, 2015
View reviewed changes
GeoLib/OctTree-impl.h
OctTree<POINT>::_max_items_per_node = max_items_per_node;
OctTree<POINT>::_eps = eps;
// compute an axis aligned cube around the points ll and ur
const double dx(ur[0] - ll[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without repeating the algorithm an AABB could be used here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see how the AABB can help at this place. Here a special AABB is computed - a cube. The input points ll and ur needs not to describe a cube.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed the cube part. Thanks.

@ogsbot
Copy link
Member

ogsbot commented Jun 12, 2015

Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/202/

endJunction
endJunction reviewed Jun 12, 2015
View reviewed changes
GeoLib/OctTree-impl.h
}

template <typename POINT>
bool OctTree<POINT>::addPoint(POINT * pnt, POINT *& ret_pnt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type could be a boost::optional if not too difficult to change.
The input could be a reference to a point.

@TomFischer
Copy link
Member Author

Summary before weekend:

  • The question for the signature of addPoint() is still open.
  • Other points are either answered or already changed.

@ogsbot
Copy link
Member

ogsbot commented Jun 12, 2015

Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/274/

@ogsbot
Copy link
Member

ogsbot commented Jun 12, 2015

Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/204/

norihiro-w
norihiro-w reviewed Jun 13, 2015
View reviewed changes
GeoLib/OctTree.h
return true;
}
private:
static double _eps;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why static? should we share the same epsilon for all Octree objects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. On the one hand I want only one epsilon (stored once) for one OctTree. On the other hand different OctTrees can and should have different epsilon. I am thinking of a good solution. Do you (or somebody else) have any suggestions how to solve this issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed from class variable to class member variable. I also changed the handling of the number of maximal points per OctTree node from a static variable to a template parameter. So it is possible to have for each OctTree it is own value.

@TomFischer
Copy link
Member Author

@endJunction wrote:

The return type could be a boost::optional if not too difficult to change.

The addPoint() method is returning a bit more information than would fit in a boost::optional. In case the point could not be inserted (return value false) the user also get the reason via the ret_pnt: If ret_pnt is a nullptr the point is outside of the OctTree cubic domain. If the ret_pnt is not equal to a nullptr the returned value is the pointer to a point within the eps-radius that was already inserted into the OctTree before.

@endJunction wrote:

The input could be a reference to a point.

If the OctTree stores references or objects instead of pointers in my opinion this would result in some performance issues. I expect that OctTree nodes are often split. As a consequence a recreation of POINT objects for the childs and destroying the POINT objects for the split node are necessary very often.

@endJunction Please let me know if the answers are okay for you.

@ogsbot
Copy link
Member

ogsbot commented Jun 15, 2015

Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/277/

@ogsbot
Copy link
Member

ogsbot commented Jun 15, 2015

Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/207/

@endJunction
Copy link
Member

Thanks Tom for clarifying the open questions.
I would though insist on use proper English "children" for the plural of a child. ✅
⏩

@norihiro-w
Copy link
Collaborator

👍

@TomFischer
Copy link
Member Author

Rebased and added changelog entry. There are not any further source code changes.

bilke
bilke reviewed Jun 16, 2015
View reviewed changes
GeoLib/OctTree.h
/// Adding a new item to an already "filled" OctTree node results in a
/// split of the OctTree node. The smaller this number is the more leafes
/// the OctTree will have, i.e. it needs more memory and more time to walk
/// through the OctTree, but the search inside a leafe is fast. In
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: leaf ✅

bilke added a commit that referenced this pull request Jun 16, 2015
@bilke
Merge pull request #714 from TomFischer/GeoLibOctTree
05f6a32 
GeoLib OctTree
@bilke bilke merged commit 05f6a32 into ufz:master Jun 16, 2015
@TomFischer TomFischer deleted the GeoLibOctTree branch June 16, 2015 08:29
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TomFischer @ogsbot @endJunction @norihiro-w @bilke