Iterator by Node ID #268

catevita · 2019-05-21T06:10:21Z

The node file id iterator allows traversal by accessing node id an meta. This is a pre step useful for node parallelization (using e.g. rayon and par_iter())

Test were moved into a single file to allow for less code duplication

catevita · 2019-05-21T06:10:52Z

PTAL @feuerste @nnmm

nnmm · 2019-05-21T08:49:38Z

src/octree/octree_iterator.rs

@@ -7,7 +7,7 @@ use collision::Aabb3;
 use std::collections::VecDeque;

 /// returns an Iterator over the points of the current node
-fn get_node_iterator(octree: &Octree, node_id: &NodeId) -> NodeIterator {
+pub fn get_node_iterator(octree: &Octree, node_id: &NodeId) -> NodeIterator {


Why did this become public?

here i forgot to remove some previous changes

fixed and, most importantly, pushed to origin

src/octree/octree_test.rs

nnmm · 2019-05-21T09:00:41Z

I think abstracting away traversal of the nodes into an iterator is potentially a good idea, we do that a lot. However, it's currently unused – could you comment a bit more on how this should be used?

catevita · 2019-05-21T09:11:03Z

hi nikolai,

I think abstracting away traversal of the nodes into an iterator is potentially a good idea, we do that a lot. However, it's currently unused – could you comment a bit more on how this should be used?

in another branch I am currently implementing the parallelization... PR comes soon.
Either should par_iter from rayon be directly used, or should the candidate nodes be gathered and then should the threads reading memory be parallelized.

catevita · 2019-05-22T11:27:39Z

#273 here the PR

feuerste

Can we actually also apply/use the new NodeIdIterator inside FilteredPointsIterator and AllPointsIterator?

src/octree/mod.rs

feuerste · 2019-06-05T12:12:23Z

src/octree/octree_iterator.rs

+    type Item = NodeId;
+
+    fn next(&mut self) -> Option<NodeId> {
+        loop {


What do you think about shortening this to

while let Some(current) = self.node_ids.pop_front() { if (self.filter_func)(&current, &self.octree) { for child_index in 0..8 { let child_id = current.get_child_id(ChildIndex::from_u8(child_index)); if self.octree.nodes.contains_key(&child_id) { self.node_ids.push_back(child_id); } } return Some(current); } } return None;

?

nnmm · 2019-05-21T08:56:49Z

src/octree/octree_iterator.rs

@@ -130,3 +130,42 @@ pub fn intersecting_node_ids(
    }
    node_ids
 }
+
+pub struct NodeIdIterator<'a> {


Nit: How about naming it NodeIdsIterator? The pluralization matches the other iterators.

nnmm · 2019-05-22T12:11:42Z

src/octree/octree_iterator.rs

+    type Item = NodeId;
+
+    fn next(&mut self) -> Option<NodeId> {
+        loop {


Does there need to be a loop in here? next() only gets one node, so I'd think all the state is encapsulated in the node_ids queue.

the loop is necessary to guarantee that next() returns Some(item) if the filter fuction is successful.

nonetheless can be optimized!

nnmm · 2019-06-05T12:19:51Z

src/octree/octree_iterator.rs

+
+pub struct NodeIdIterator<'a> {
+    octree: &'a Octree,
+    filter_func: Box<Fn(&NodeId, &Octree) -> bool + 'a>,


My first preference would be to integrate this directly with PointCulling instead of functions, and to use the more optimized distinction between Inside, Crossing and Outside to automatically add child nodes. If we use functions, how about making this a template parameter instead of a trait object?

As mentioned offline, the iterator is more generic than the filtering of the point culling. This distinction occurs for Frustum and a specific method returns the node ids.

Yes, but can we make this a parameter of the struct, I.e. struct NodeIdsIterator<F> and filter_func: F, and restrict it to Fn(&NodeId, &Octree) -> bool in the impl?

Actually, seems like you can directly embed the trait bound into the struct definition (where F: Fn(&NodeId, &Octree) -> bool)

catevita · 2019-06-05T13:01:40Z

Can we actually also apply/use the new NodeIdIterator inside FilteredPointsIterator and AllPointsIterator?

I removed these in #273, or better, I unified these with the implementation of the trait.
this PR is just implementing the NodeIdsIterator

feuerste

LGTM, with one nit.

feuerste · 2019-06-06T07:50:41Z

src/octree/octree_test.rs

+    }
+
+    #[test]
+    //#[ignore]


Nit: Please delete this line.

nnmm · 2019-06-06T09:09:49Z

src/octree/octree_iterator.rs

+
+impl<'a, F> Iterator for NodeIdsIterator<'a, F>
+where
+    F: Fn(&NodeId, &Octree) -> bool + 'a,


I don't think we need that here when it's already in the struct definition, do we?

Unfortunately it does not work removing that

nnmm

LGTM with one nit

catevita · 2019-06-06T10:26:00Z

@wally-the-cartographer merge

catevita · 2019-06-06T10:41:21Z

@wally-the-cartographer merge

catevita · 2019-06-06T10:44:05Z

@pifon2a Hi, sorry to bother you, Wally does not seem to be responsive, can you please restart it?

pifon2a · 2019-06-06T11:19:42Z

@catevita The Wally is not run by me anymore. I have also left the project. Merging manually...

pifon2a · 2019-06-06T11:20:28Z

@catevita Wolfgang and Holger know more about that.

catevita · 2019-06-06T13:37:00Z

Thanks, for all your recent and previuos help! next time I am asking to someone else, all the best

catevita added 7 commits May 20, 2019 09:00

added node id iterator

a9f43ae

compiling test

33d773b

test

2cc6071

compiles

3560f86

cargo clippy

81f9e16

fixed aabb with zero

6f3d7d2

added ignore to test

2cb21a0

googlebot added the cla: yes label May 21, 2019

catevita added 2 commits May 21, 2019 08:19

nits

36f5593

removed aabb code, removed full tree traversal

273d043

catevita changed the title ~~Iterator by Node ID and computing "tight" Aabb estimate (backward compatible)~~ Iterator by Node ID May 21, 2019

nnmm reviewed May 21, 2019

View reviewed changes

src/octree/octree_test.rs Outdated Show resolved Hide resolved

fixing for pr comments, cleanup

cff2a98

feuerste requested changes Jun 5, 2019

View reviewed changes

nnmm requested changes Jun 5, 2019

View reviewed changes

changes for PR review

5211654

streamlined test fuctionalities

a8f5bf9

feuerste approved these changes Jun 6, 2019

View reviewed changes

catevita added 2 commits June 6, 2019 10:42

removed commented line

6e61f3c

embed the trait bound into the struct definition

e07bce0

nnmm reviewed Jun 6, 2019

View reviewed changes

nnmm approved these changes Jun 6, 2019

View reviewed changes

pifon2a merged commit a9d0179 into master Jun 6, 2019

catevita deleted the cvitadello/nodeiditerator branch July 4, 2019 04:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iterator by Node ID #268

Iterator by Node ID #268

catevita commented May 21, 2019 •

edited

Loading

catevita commented May 21, 2019

nnmm May 21, 2019

catevita May 21, 2019

catevita May 31, 2019

nnmm commented May 21, 2019

catevita commented May 21, 2019

catevita commented May 22, 2019

feuerste left a comment

feuerste Jun 5, 2019

catevita Jun 5, 2019

nnmm May 21, 2019

catevita Jun 5, 2019

nnmm May 22, 2019

catevita Jun 5, 2019

catevita Jun 5, 2019

nnmm Jun 5, 2019

catevita Jun 5, 2019

nnmm Jun 6, 2019

nnmm Jun 6, 2019

catevita Jun 6, 2019

catevita commented Jun 5, 2019 •

edited

Loading

feuerste left a comment

feuerste Jun 6, 2019

catevita Jun 6, 2019

nnmm Jun 6, 2019

nnmm Jun 6, 2019

catevita Jun 6, 2019

nnmm left a comment

catevita commented Jun 6, 2019

catevita commented Jun 6, 2019

catevita commented Jun 6, 2019

pifon2a commented Jun 6, 2019

pifon2a commented Jun 6, 2019

catevita commented Jun 6, 2019

Iterator by Node ID #268

Iterator by Node ID #268

Conversation

catevita commented May 21, 2019 • edited Loading

catevita commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmm commented May 21, 2019

catevita commented May 21, 2019

catevita commented May 22, 2019

feuerste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catevita commented Jun 5, 2019 • edited Loading

feuerste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmm left a comment

Choose a reason for hiding this comment

catevita commented Jun 6, 2019

catevita commented Jun 6, 2019

catevita commented Jun 6, 2019

pifon2a commented Jun 6, 2019

pifon2a commented Jun 6, 2019

catevita commented Jun 6, 2019

catevita commented May 21, 2019 •

edited

Loading

catevita commented Jun 5, 2019 •

edited

Loading