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.

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

Reverse octree's depth first iterator order #2332

Conversation

frozar
Copy link
Contributor

@frozar frozar commented Jun 3, 2018

Warning: this PR is based onto #2204

Fix the order in which the octree is walked through by a 'depth_first'-like iterator.

@frozar frozar force-pushed the octree_depth_first_iterator_order branch 2 times, most recently from 1a07fd5 to dc5dfde Compare June 4, 2018 19:08
@frozar frozar force-pushed the octree_depth_first_iterator_order branch 2 times, most recently from a3ce21f to 678d2c8 Compare June 19, 2018 19:12
@frozar frozar changed the title Octree depth first iterator order [OCTREE] Octree depth first iterator order Jun 20, 2018
@frozar frozar force-pushed the octree_depth_first_iterator_order branch from 678d2c8 to ec6d4fa Compare June 25, 2018 12:18
@frozar frozar force-pushed the octree_depth_first_iterator_order branch from ec6d4fa to 46acd79 Compare July 5, 2018 12:48
@frozar frozar force-pushed the octree_depth_first_iterator_order branch from 46acd79 to 35e72e0 Compare July 7, 2018 18:18
@frozar frozar force-pushed the octree_depth_first_iterator_order branch 3 times, most recently from b95b951 to 61d3490 Compare July 17, 2018 13:22
@frozar frozar force-pushed the octree_depth_first_iterator_order branch from 61d3490 to ed5b2d0 Compare August 27, 2018 07:03
@frozar frozar force-pushed the octree_depth_first_iterator_order branch from ed5b2d0 to 734f9f5 Compare September 11, 2018 08:38
@taketwo
Copy link
Member

taketwo commented Sep 11, 2018

Could you briefly describe (here) how the order is changed? Are we fixing some implementation flaw or rather changing behavior to something that makes more sense? Could this break someones code?

@taketwo taketwo added needs: code review Specify why not closed/merged yet module: octree labels Sep 11, 2018
@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Sep 12, 2018

It seems like instead of starting from index 0 is starts from index 8. It's just starting the traversal from the end, horizontally speaking.

@frozar what's the use for this? Was something broken?

Edit: I think the tests hint the problem. The octree key values which are closely tied to the depth we're looking weird before and now seem to make more sense.

@frozar
Copy link
Contributor Author

frozar commented Sep 12, 2018

Clarification

Hi there, I think this PR change the behavior of the iterator in a way it makes more sense. I will try to explain it on the simple octree example I provided in a previous PR:

Graphically, this octree appears as follows:
         root
       ' /  \ `
    '   /    \   `
 '     /      \     `
000  010      100  110
      |             |
      |             |
     020           220

If we consist the previous LeafNodeDepthFirstIterator, the leaves were visited in this order: "220", "100", "020" and "000".
With the change, the leaves are now visited visited in this order: "000", "020", "100" and "220".

The second order is the good one for me.

Side effect of this change

Usually people don't care about the order in which the iterator provide octree nodes I think. If someone use an algorithm which is sensible to the order of the octree nodes he deals with, maybe this change will fix some Issue ^^. If really it's a big issue for somebody, maybe both iterator should be provide: in one order and its reverse.
In my opinion, it's not necessary.

About implementation

Last point about the implementation, as the Depth First Iterators use a stack internally, surpringly to get the octree nodes in the correct order, we have to push them in the reverse order, and it's fine.

Also, why do I use a do while and not a for (child_idx = 8; 0 <= child_idx; --child_idx)? Because child_idx is a unsigned char. In this case, the for loop way would never end, because an unsigned char is always positive.
Also I would like to keep child_idx as a unsigned char because if I change it, I get a warning on line where a method expect a unsigned char as argument, if you see what I mean.

@SergioRAgostinho SergioRAgostinho added the changelog: behavior change Meta-information for changelog generation label Oct 5, 2018
@SergioRAgostinho SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Oct 5, 2018
frozar added 2 commits October 7, 2018 23:36
       depth first iterator go in the intuitive order over an
       octree. Say is to say, from the lowest octree key to the
       highest octree key.
@frozar frozar force-pushed the octree_depth_first_iterator_order branch from 734f9f5 to c38f4af Compare October 7, 2018 21:37
@frozar
Copy link
Contributor Author

frozar commented Oct 7, 2018

I hope everything is finally alright for this tiny PR.

@SergioRAgostinho SergioRAgostinho removed the needs: author reply Specify why not closed/merged yet label Oct 8, 2018
@SergioRAgostinho SergioRAgostinho changed the title [OCTREE] Octree depth first iterator order Reverse octree's depth first iterator order Oct 8, 2018
@SergioRAgostinho SergioRAgostinho merged commit 1490739 into PointCloudLibrary:master Oct 8, 2018
@frozar frozar deleted the octree_depth_first_iterator_order branch October 8, 2018 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: behavior change Meta-information for changelog generation module: octree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants