Implementation of the iterator 'OctreeFixedDepthIterator'. #1983

frozar · 2017-09-01T18:22:17Z

Add an octree iterator to walk over the OctreeBranch at the same depth. The code relies really on the iterator OctreeBreadthFirstIterator.

The file octree_viewer.cpp has been updated to use this iterator.

To maintain to homogenity for the different octree class, more things should be done I think. Wait for checking.

taketwo

Please add Open Perception (after Willow garage) to the copyright note of the files you edited. These changes are substantial enough.

 *  Copyright (c) 2017-, Open Perception, Inc.

The pull requests introducing new functionality need to be complemented by unit tests. Thus, it would be great if you can extend already existing test case with a couple of assertions regarding the new iterator. There is not a lot to test, really, so should not take much effort.

taketwo · 2017-09-02T15:23:27Z

octree/include/pcl/octree/octree_iterator.h

+     * \author Fabien Rozar (fabien.rozar@gmail.com)
+     */
+    template<typename OctreeT>
+      class OctreeBreadthFirstAtDepthIterator : public OctreeBreadthFirstIterator<OctreeT>


Please shift this whole class definition left by two spaces to fix the alignment.
(I know the other classes in this file are misaligned. But we have a policy that the old code stays as it is, and the new code should follow the style guide.)

taketwo · 2017-09-02T15:26:16Z

octree/include/pcl/octree/octree_iterator.h

+        explicit
+        OctreeBreadthFirstAtDepthIterator (OctreeT* octree_arg, unsigned int depth_arg = 0);
+
+        /** \brief Empty deconstructor. */


This is a pointless docstring, and also not correct, because this is called destructor, not deconstructor.
(Yes, I know you copy-pasted this, but same as the previous comment, we want the new code to be good.)

taketwo · 2017-09-02T15:26:46Z

octree/include/pcl/octree/octree_iterator.h

+        inline OctreeBreadthFirstAtDepthIterator&
+        operator = (const OctreeBreadthFirstAtDepthIterator& src)
+        {
+          OctreeBreadthFirstIterator<OctreeT>::operator=(src);


Missing space

If you're defining copy assignment, you should define as well the copy constructor, otherwise the compiler will generate one for you which probably doesn't do exactly what you want.
Edit: Also, don't you need to return *this ?

Assuming your member depth_ will be const, what happens if you're copying from an iterator with a different depth? Assuming your depth_ won't be const, you should also copy it.

As you expected, the member depth_ cannot be a const member because of the operator = (I get the compilation error: assignment of read-only member).

A copy constructor is also implemented.

taketwo · 2017-09-02T15:27:49Z

octree/include/pcl/octree/octree_iterator.h

+        inline OctreeBreadthFirstAtDepthIterator
+        operator++ (int)
+        {
+          OctreeBreadthFirstAtDepthIterator _Tmp = *this;


Local variables should be called tmp, no camel case and no underscores.

taketwo · 2017-09-02T15:28:19Z

octree/include/pcl/octree/octree_iterator.h

+        using OctreeBreadthFirstIterator<OctreeT>::FIFO_;
+
+        /** \brief Given level of the node to be iterated */
+        unsigned int depth_;


Can be const to make it explicit that depth can not be changed.

As an alternative, consider also what I wrote about the reset method and let's decide what fits best.

For the reason I gave in the operator= comment, the member depth_ cannot be const.

taketwo · 2017-09-02T15:28:32Z

tools/octree_viewer.cpp

-    pcl::octree::OctreePointCloudVoxelCentroid<pcl::PointXYZ>::Iterator tree_it;
-    pcl::octree::OctreePointCloudVoxelCentroid<pcl::PointXYZ>::Iterator tree_it_end = octree.end();
+    pcl::octree::OctreePointCloudVoxelCentroid<pcl::PointXYZ>::BreadthFirstAtDepthIterator tree_it;
+    pcl::octree::OctreePointCloudVoxelCentroid<pcl::PointXYZ>::BreadthFirstAtDepthIterator tree_it_end = octree.breadth_at_depth_end();


Missing space

taketwo · 2017-09-02T15:28:58Z

tools/octree_viewer.cpp


    pcl::PointXYZ pt;
    std::cout << "===== Extracting data at depth " << depth << "... " << std::flush;
    double start = pcl::getTime ();

-    for (tree_it = octree.begin(depth); tree_it!=tree_it_end; ++tree_it)
+    for (tree_it = octree.breadth_at_depth_begin(depth); tree_it!=tree_it_end; ++tree_it)


Missing space before braces and around operator !=.

taketwo · 2017-09-02T15:30:10Z

octree/include/pcl/octree/impl/octree_iterator.hpp

+      OctreeBreadthFirstIterator<OctreeT>::reset ();
+
+      // Set the current_state_ to the right depth
+      while( this->current_state_ && ( this->getCurrentOctreeDepth () != depth_ ) ) {


Missing space after while, extra spaces within braces. { should go to the new line

taketwo · 2017-09-02T15:31:13Z

octree/include/pcl/octree/octree_base.h

+        // Octree breadth iterators at a given depth
+        typedef OctreeBreadthFirstAtDepthIterator<OctreeT> BreadthFirstAtDepthIterator;
+        typedef const OctreeBreadthFirstAtDepthIterator<OctreeT> ConstBreadthFirstAtDepthIterator;
+        BreadthFirstAtDepthIterator breadth_at_depth_begin(unsigned int depth_arg = 0) {return BreadthFirstAtDepthIterator(this, depth_arg);};


Missing space after the function name(s). Missing spaces after { and before }.

taketwo · 2017-09-02T15:31:24Z

octree/include/pcl/octree/octree_base.h

+        typedef OctreeBreadthFirstAtDepthIterator<OctreeT> BreadthFirstAtDepthIterator;
+        typedef const OctreeBreadthFirstAtDepthIterator<OctreeT> ConstBreadthFirstAtDepthIterator;
+        BreadthFirstAtDepthIterator breadth_at_depth_begin(unsigned int depth_arg = 0) {return BreadthFirstAtDepthIterator(this, depth_arg);};
+        const BreadthFirstAtDepthIterator breadth_at_depth_end() {return BreadthFirstAtDepthIterator();};


Same as previous comment

I've been trying to figure out how exactly this iterator end() is working and it seems a little different than what one would normally expect. So semantically, end() should point to the element right after the last one, but it seems that in this case we set the iterator to an "unitialized state" with no octree attached to it. Is my assessment correct?

When I run pcl_octree_viewer with a debugger to understand (again) what's happen, on the call to the method breadth_at_depth_end(), I get the following call stack (I delete the template paramters):

#0 pcl::octree::OctreeIteratorBase::OctreeIteratorBase (this=0x7fffffffddd0, max_depth_arg=0) at /home/frozar/soft/pcl/octree/include/pcl/octree/octree_iterator.h:91 #1 pcl::octree::OctreeBreadthFirstIterator::OctreeBreadthFirstIterator (this=0x7fffffffddd0, max_depth_arg=0) at /home/frozar/soft/pcl/octree/include/pcl/octree/impl/octree_iterator.hpp:181 #2 pcl::octree::OctreeBreadthFirstAtDepthIterator::OctreeBreadthFirstAtDepthIterator (this=0x7fffffffddd0, depth_arg=0) at /home/frozar/soft/pcl/octree/include/pcl/octree/impl/octree_iterator.hpp:288 #3 pcl::octree::OctreeBase::breadth_at_depth_end (this=0x7fffffffe268) at /home/frozar/soft/pcl/octree/include/pcl/octree/octree_base.h:110

So calling breadth_at_depth_end() finally hits the constructor OctreeIteratorBase (max_depth_arg=0). So the iterator is in a "sentinal state" if I can say. In my opinion it's not an "unitialized state", but still, this doesn't mean it's the good way to do it.

Actually, for an iterator the really matter is to check that after enough call to operator++, will it be equal to the "end iterator"? In this case, the OctreeBreadthFirstAtDepthIterator iterator implementation relies almost completly on the OctreeBreadthFirstIterator iterator. The implementation I propose mimic the behavior of OctreeBreadthFirstIterator iterator for this point.

What do you think about this behavior?

SergioRAgostinho · 2017-09-03T01:31:31Z

octree/include/pcl/octree/octree_iterator.h

+    /** \brief @b Octree iterator class
+     * \note This class implements a forward iterator for traversing octrees in a breadth-first manner.
+     *       It only returns the node at a given depth in the octree, if any.
+     * \ingroup octree


Can you switch the description to "Iterator over all existing nodes at a given depth". I only understood the intent of this iterator after seeing Sergey's comment about the constness of the depth parameter. I would even suggest renaming the iterator to something like OctreeFixedDepthIterator but I leave this suggestion to your scrutiny regarding its adoption or not.

I update the description.

I keep the class name as is for the moment because I feel that it will clutter the review, but I keep it in mind as a TODO.

SergioRAgostinho · 2017-09-03T01:40:50Z

octree/include/pcl/octree/octree_iterator.h

+
+        // public typedefs
+        typedef typename OctreeBreadthFirstIterator<OctreeT>::BranchNode BranchNode;
+        typedef typename OctreeBreadthFirstIterator<OctreeT>::LeafNode LeafNode;


It's a matter of style but you can switch the typedefs to using. It's semantically more correct.

SergioRAgostinho · 2017-09-03T01:48:36Z

octree/include/pcl/octree/impl/octree_iterator.hpp

+    OctreeBreadthFirstAtDepthIterator<OctreeT>::OctreeBreadthFirstAtDepthIterator (unsigned int depth_arg) :
+        OctreeBreadthFirstIterator<OctreeT> (depth_arg), depth_(depth_arg)
+    {
+      OctreeBreadthFirstIterator<OctreeT>::reset ();


OctreeBreadthFirstAtDepthIterator<OctreeT>::reset() already calls OctreeBreadthFirstIterator<OctreeT>::reset () so this is useless.

SergioRAgostinho · 2017-09-03T01:48:59Z

octree/include/pcl/octree/impl/octree_iterator.hpp

+    OctreeBreadthFirstAtDepthIterator<OctreeT>::OctreeBreadthFirstAtDepthIterator (OctreeT* octree_arg, unsigned int depth_arg) :
+        OctreeBreadthFirstIterator<OctreeT> (octree_arg, depth_arg), depth_(depth_arg)
+    {
+      OctreeBreadthFirstIterator<OctreeT>::reset ();


same comment as above.

SergioRAgostinho

I didn't manage to complete the review but addressing the points I mentioned should be enough. I also think it would be good to have some unit tests to test the functionality. I'm sure you wrote some code to test that this was working properly :)

SergioRAgostinho · 2017-09-03T02:56:26Z

octree/include/pcl/octree/octree_iterator.h

+        /** \brief Reset the iterator to the first node at the depth given initially of the octree
+        */
+        void
+        reset ();


I think it's useful to allow specifying the intended depth on reset. If you don't specify any argument it just resets to the first node of the current set depth, but if you do it switches the iterator to the first node of the new depth value.

I make a try and I let you check it.

frozar · 2017-09-04T01:03:46Z

@taketwo : All your comments have been taken into account. I think the coding style should be good now.
@SergioRAgostinho : In the last modifications, I try to take into account all your comments and I also comment some of them. Regarding the unit test, I didn't write any code (yet, let say). In fact, I just check the expected behavior with pcl_octree_viewer. Later I will attempt to write one. Also, I don't forgive your suggestion: OctreeFixedDepthIterator.

taketwo · 2017-09-04T09:17:12Z

octree/include/pcl/octree/impl/octree_iterator.hpp

@@ -317,7 +317,8 @@ namespace pcl
      OctreeBreadthFirstIterator<OctreeT>::reset ();

      // Set the current_state_ to the right depth
-      while( this->current_state_ && ( this->getCurrentOctreeDepth () != depth_ ) ) {
+      while ( this->current_state_ && ( this->getCurrentOctreeDepth () != depth_ ) )


I'm afraid we still have style problems here :D
There shouldn't be spaces inside brackets, i.e.

while (this->current_state_ && (this->getCurrentOctreeDepth () != depth_))

taketwo · 2017-09-04T09:20:00Z

octree/include/pcl/octree/octree_iterator.h

-        // public typedefs
-        typedef typename OctreeIteratorBase<OctreeT>::BranchNode BranchNode;
-        typedef typename OctreeIteratorBase<OctreeT>::LeafNode LeafNode;
+      template<typename OctreeT>


We have a policy that we do not reformat existing code (unless we make other meaningful edits to it), the reason being that git blame won't show relevant commit anymore.

So please remove this restyling of the existing code (OctreeBreadthFirstIterator).

I understand why existing code should not be touch. In this case, I do a tiny edit: I put the keyword public: just above the typedef lines. Should I still remove my restyling? (For the git blame reason, I'll understand if you say "yes"...)

I removed the restyle of this class.

Adding public: is a needed, meaningful change, we should keep it. It does not affect git blame, because git blame reports the author on line-by-line basis. If in future we have problems with this line, git blame will tell us that you are the one to blame 😆

Now as for the style changes, for example indentation fixes, if in future we discover that a certain line does something strange and we try to understand where it comes from and who are the author, git blame will again point at you. But this won't give us anything, because your change was only about the indentation.

I rewrite the history for the git blame matter. If you do this command:

git blame octree/include/pcl/octree/octree_iterator.h

you'll see that I touch only the public: keyword line in the class OctreeBreadthFirstIterator.

Right, if you just run git blame on a file you get only your last edit. What I meant was rather the feature offered by GitHub, e.g. here. You can see the last commit for each and every line. So if you scroll down you will easily see that public: is added by you, but other lines come from Julius.

I didn't know this tool. It's pretty cool, thank you.

taketwo · 2017-09-04T09:21:56Z

octree/include/pcl/octree/octree_iterator.h

-      class OctreeBreadthFirstAtDepthIterator : public OctreeBreadthFirstIterator<OctreeT>
-      {
-      public:
+      template<typename OctreeT>


I probably was not clear enough. The template line was properly indented. But the class line and all following lines are shifted two spaces right with respect to it. So I asked to "unshift" them, i.e. remove two extra spaces.

I hope I get it right now.

taketwo · 2017-09-04T09:24:36Z

octree/include/pcl/octree/octree_iterator.h

          }

          /** \brief Reset the iterator to the first node at the depth given initially of the octree
          */
          void
          reset ();

+          /** \brief Reset the iterator to the first node at the depth given initially of the octree


Update the docstring to match the reality.

taketwo · 2017-09-04T15:05:55Z

Thanks for the changes. Besides a missing unit test this looks fine for me.

frozar · 2017-09-22T12:53:20Z

After my rebase, this branch seems to be broken (not the correct display, segfault if one tries to look at the level 0 of the octree).

taketwo · 2017-09-26T07:36:28Z

Any idea why?

frozar · 2017-09-26T08:00:51Z

Currently I don't know why because I don't have the time right now to check. Until the 6th october, my schedule is full. Also, I have an idea to build a non regression test for this feature, but I'm busy. I'll fix it later.

SergioRAgostinho · 2017-11-22T01:06:09Z

Hey @frozar. Do you mind rebasing this with the current master? We cleaned a number of warnings and I want to make sure we're not adding any with this PR.

Edit: Just noticed we're still missing that unit test.

frozar · 2018-02-26T14:45:08Z

In short: you cannot set max_octree_depth_ = 1 and try calling operator++.

When you said:

as long as max_octree_depth_ is set correctly

OK, but what does it mean for the OctreeFixedDepthIterator?

Previously I said :

As in this iterator, the purpose is to reach a given depth of an octree, it's important to have the correct value of max_octree_depth_ before calling operator++ (). The other iterator don't need to call their operator++ () to get the good behavior.

This concerns the OctreeFixedDepthIterator::reset () method. The OctreeFixedDepthIterator is different from any other iterator because at initialisation, it needs to walk through the octree (call operator++). The operator ++() has a good behavior if the current max_octree_depth_ <= getCurrentOctreeDepth (). Schematically:

fixed_depth_ always remember what was given by the user,
max_octree_depth_ is adjusted accordingly to the real depth of the current octree.

Let imagine that you never fill the octree (the octree contains only the root node) given as input to this iterator, and you supply a fixed_depth_arg = 1, you expect to obtain an iterator in its end state, right?

In the test where you fill the octree after initialisation and then you call the reset method of the iterator, you expect to reach the first node of the depth=1 of the octree, right?

This is the behavior of the current OctreeFixedDepthIterator.
Challenge: Try to find (even suggestions are accepted) a situation where OctreeFixedDepthIterator misbehave?!?

Personally, I find the current implement good, because it is semantically correct and it well reuses the parent methods such that OctreeBreadthFirstIterator<OctreeT>::operator++ (), so I'm fine with it, and I will not look back anger (when I'll check it again).

SergioRAgostinho · 2018-02-27T10:22:16Z

This concerns the OctreeFixedDepthIterator::reset () method. The OctreeFixedDepthIterator is different from any other iterator because at initialisation, it needs to walk through the octree (call operator++). The operator ++() has a good behavior if the current max_octree_depth_ <= getCurrentOctreeDepth (). Schematically:

fixed_depth_ always remember what was given by the user,

max_octree_depth_ is adjusted accordingly to the real depth of the current octree.

Semantically this makes sense for me.

Let imagine that you never fill the octree (the octree contains only the root node) given as input to this iterator, and you supply a fixed_depth_arg = 1, you expect to obtain an iterator in its end state, right?

Yes. But I could also cap the "fixed_depth_" at 0 and warn the user "You can't set that depth level because your tree has only depth zero".

I'm ok in merging this but I still believe it could be pulled off without fixed_depth_. I'll give it tomorrow morning as post here the results for us to evaluate differences.

SergioRAgostinho · 2018-02-28T11:05:11Z

Here's my working proof of concept SergioRAgostinho@a17d63e

Test results:

 cases.
[----------] Global test environment set-up.
[----------] 2 tests from OctreeIteratorBaseTest
[ RUN      ] OctreeIteratorBaseTest.CopyConstructor
[       OK ] OctreeIteratorBaseTest.CopyConstructor (0 ms)
[ RUN      ] OctreeIteratorBaseTest.CopyAssignment
[       OK ] OctreeIteratorBaseTest.CopyAssignment (0 ms)
[----------] 2 tests from OctreeIteratorBaseTest (0 ms total)

[----------] 2 tests from OctreeIteratorTest/0, where TypeParam = pcl::octree::OctreeDepthFirstIterator<pcl::octree::OctreeBase<int, pcl::octree::OctreeContainerEmpty> >
[ RUN      ] OctreeIteratorTest/0.CopyConstructor
[       OK ] OctreeIteratorTest/0.CopyConstructor (0 ms)
[ RUN      ] OctreeIteratorTest/0.CopyAssignment
[       OK ] OctreeIteratorTest/0.CopyAssignment (0 ms)
[----------] 2 tests from OctreeIteratorTest/0 (0 ms total)

[----------] 2 tests from OctreeIteratorTest/1, where TypeParam = pcl::octree::OctreeBreadthFirstIterator<pcl::octree::OctreeBase<int, pcl::octree::OctreeContainerEmpty> >
[ RUN      ] OctreeIteratorTest/1.CopyConstructor
[       OK ] OctreeIteratorTest/1.CopyConstructor (0 ms)
[ RUN      ] OctreeIteratorTest/1.CopyAssignment
[       OK ] OctreeIteratorTest/1.CopyAssignment (0 ms)
[----------] 2 tests from OctreeIteratorTest/1 (0 ms total)

[----------] 2 tests from OctreeIteratorTest/2, where TypeParam = pcl::octree::OctreeLeafNodeIterator<pcl::octree::OctreeBase<int, pcl::octree::OctreeContainerEmpty> >
[ RUN      ] OctreeIteratorTest/2.CopyConstructor
[       OK ] OctreeIteratorTest/2.CopyConstructor (0 ms)
[ RUN      ] OctreeIteratorTest/2.CopyAssignment
[       OK ] OctreeIteratorTest/2.CopyAssignment (0 ms)
[----------] 2 tests from OctreeIteratorTest/2 (0 ms total)

[----------] 2 tests from OctreeIteratorTest/3, where TypeParam = pcl::octree::OctreeFixedDepthIterator<pcl::octree::OctreeBase<int, pcl::octree::OctreeContainerEmpty> >
[ RUN      ] OctreeIteratorTest/3.CopyConstructor
[       OK ] OctreeIteratorTest/3.CopyConstructor (0 ms)
[ RUN      ] OctreeIteratorTest/3.CopyAssignment
[       OK ] OctreeIteratorTest/3.CopyAssignment (0 ms)
[----------] 2 tests from OctreeIteratorTest/3 (0 ms total)

[----------] 8 tests from OctreeBaseBeginEndIteratorsTest
[ RUN      ] OctreeBaseBeginEndIteratorsTest.Begin
[       OK ] OctreeBaseBeginEndIteratorsTest.Begin (1 ms)
[ RUN      ] OctreeBaseBeginEndIteratorsTest.End
[       OK ] OctreeBaseBeginEndIteratorsTest.End (0 ms)
[ RUN      ] OctreeBaseBeginEndIteratorsTest.LeafBegin
[       OK ] OctreeBaseBeginEndIteratorsTest.LeafBegin (0 ms)
[ RUN      ] OctreeBaseBeginEndIteratorsTest.LeafEnd
[       OK ] OctreeBaseBeginEndIteratorsTest.LeafEnd (0 ms)
[ RUN      ] OctreeBaseBeginEndIteratorsTest.DepthBegin
[       OK ] OctreeBaseBeginEndIteratorsTest.DepthBegin (0 ms)
[ RUN      ] OctreeBaseBeginEndIteratorsTest.DepthEnd
[       OK ] OctreeBaseBeginEndIteratorsTest.DepthEnd (0 ms)
[ RUN      ] OctreeBaseBeginEndIteratorsTest.BreadthBegin
[       OK ] OctreeBaseBeginEndIteratorsTest.BreadthBegin (0 ms)
[ RUN      ] OctreeBaseBeginEndIteratorsTest.BreadthEnd
[       OK ] OctreeBaseBeginEndIteratorsTest.BreadthEnd (0 ms)
[----------] 8 tests from OctreeBaseBeginEndIteratorsTest (1 ms total)

[----------] 5 tests from OctreeBaseIteratorsForLoopTest
[ RUN      ] OctreeBaseIteratorsForLoopTest.DefaultIterator
[       OK ] OctreeBaseIteratorsForLoopTest.DefaultIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsForLoopTest.LeafNodeIterator
[       OK ] OctreeBaseIteratorsForLoopTest.LeafNodeIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsForLoopTest.DepthFirstIterator
[       OK ] OctreeBaseIteratorsForLoopTest.DepthFirstIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsForLoopTest.BreadthFirstIterator
[       OK ] OctreeBaseIteratorsForLoopTest.BreadthFirstIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsForLoopTest.FixedDepthIterator
[       OK ] OctreeBaseIteratorsForLoopTest.FixedDepthIterator (0 ms)
[----------] 5 tests from OctreeBaseIteratorsForLoopTest (0 ms total)

[----------] 5 tests from OctreeBaseIteratorsPrePostTest
[ RUN      ] OctreeBaseIteratorsPrePostTest.DefaultIterator
[       OK ] OctreeBaseIteratorsPrePostTest.DefaultIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsPrePostTest.LeafNodeIterator
[       OK ] OctreeBaseIteratorsPrePostTest.LeafNodeIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsPrePostTest.DepthFirstIterator
[       OK ] OctreeBaseIteratorsPrePostTest.DepthFirstIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsPrePostTest.BreadthFirstIterator
[       OK ] OctreeBaseIteratorsPrePostTest.BreadthFirstIterator (0 ms)
[ RUN      ] OctreeBaseIteratorsPrePostTest.FixedDepthIterator
[       OK ] OctreeBaseIteratorsPrePostTest.FixedDepthIterator (1 ms)
[----------] 5 tests from OctreeBaseIteratorsPrePostTest (1 ms total)

[----------] 8 tests from OctreePointCloudAdjacencyBeginEndIteratorsTest
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.LeafBegin
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.LeafBegin (0 ms)
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.LeafEnd
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.LeafEnd (1 ms)
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.DepthBegin
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.DepthBegin (0 ms)
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.DepthEnd
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.DepthEnd (1 ms)
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.BreadthBegin
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.BreadthBegin (1 ms)
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.BreadthEnd
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.BreadthEnd (0 ms)
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.FixedDepthBegin
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.FixedDepthBegin (1 ms)
[ RUN      ] OctreePointCloudAdjacencyBeginEndIteratorsTest.FixedDepthEnd
[       OK ] OctreePointCloudAdjacencyBeginEndIteratorsTest.FixedDepthEnd (1 ms)
[----------] 8 tests from OctreePointCloudAdjacencyBeginEndIteratorsTest (5 ms total)

[----------] 5 tests from OctreePointCloudSierpinskiTest
[ RUN      ] OctreePointCloudSierpinskiTest.DefaultIterator
[       OK ] OctreePointCloudSierpinskiTest.DefaultIterator (69 ms)
[ RUN      ] OctreePointCloudSierpinskiTest.LeafNodeIterator
[       OK ] OctreePointCloudSierpinskiTest.LeafNodeIterator (67 ms)
[ RUN      ] OctreePointCloudSierpinskiTest.DepthFirstIterator
[       OK ] OctreePointCloudSierpinskiTest.DepthFirstIterator (67 ms)
[ RUN      ] OctreePointCloudSierpinskiTest.BreadthFirstIterator
[       OK ] OctreePointCloudSierpinskiTest.BreadthFirstIterator (68 ms)
[ RUN      ] OctreePointCloudSierpinskiTest.FixedDepthIterator
[       OK ] OctreePointCloudSierpinskiTest.FixedDepthIterator (69 ms)
[----------] 5 tests from OctreePointCloudSierpinskiTest (342 ms total)

[----------] Global test environment tear-down
[==========] 41 tests from 10 test cases ran. (349 ms total)
[  PASSED  ] 41 tests.

frozar · 2018-03-02T10:38:59Z

@SergioRAgostinho It seems good. I look at the diff and I see that you have to modify the behavior of OctreeIteratorBase. In this way you can achieve it, I agree :)

If you want to update the code in this way, I would say, why not.

SergioRAgostinho · 2018-03-02T17:35:03Z

@SergioRAgostinho It seems good. I look at the diff and I see that you have to modify the behavior of OctreeIteratorBase. In this way you can achieve it, I agree :)

I assumed from the beginning I was not getting something in your explanation, I just didn't know exactly what. It became clear now.

If you want to update the code in this way, I would say, why not.

It still needs cleanup but let's gather opinions @PointCloudLibrary/maintainers .

taketwo · 2018-03-03T11:58:20Z

No opinion, really.

frozar · 2018-03-03T12:21:09Z

How many people are in @PointCloudLibrary/maintainers?

taketwo · 2018-03-03T12:30:11Z

Hover the link with the mouse, it will show 4 people.

frozar · 2018-03-03T12:51:42Z

You are a secret organisation, let's check the capture:

SergioRAgostinho · 2018-03-03T13:10:17Z

Nothing suspicious here (whistles) . AFAIK there are two settings that influence one's ability to visualize members of an organization:

The team needs to be set to public, otherwise it's only visible to members
Each member needs to set its own visibility to public.

If you access https://github.com/orgs/PointCloudLibrary/people without credentials you won't see for instance Sergey, although he's definitely in it.

SergioRAgostinho · 2018-03-03T13:13:48Z

I forgot to add the most important comment: I have a weak preference of not having fixed_depth_ but if it's not a thing someone else thinks we should go for, I'm totally fine in leaving it as it is.

taketwo · 2018-03-03T15:38:02Z

I was not aware of this visibility setting. From now on the world will know about my affiliation with PCL :)

NPatch · 2018-09-13T17:05:54Z

Hi all,
Just to let you know, the FixedDepthIterator has found its way in the latest stable release when it comes to the octree_viewer,which throws an error that the iterator doesn't exist.

SergioRAgostinho · 2018-09-13T18:37:11Z

@NPatch can you open a new issue and post a self contained code snippet/procedure exemplifying what is failing?

NPatch · 2018-09-13T19:19:57Z

I will. But it's nothing custom. PCL binary installer and then copy paste the octree_viewer code to test it out.
UPDATE: created it.

taketwo reviewed Sep 2, 2017

View reviewed changes

taketwo added the needs: author reply Specify why not closed/merged yet label Sep 2, 2017

taketwo added this to the pcl-1.9.0 milestone Sep 2, 2017

SergioRAgostinho reviewed Sep 3, 2017

View reviewed changes

SergioRAgostinho requested changes Sep 3, 2017

View reviewed changes

SergioRAgostinho reviewed Sep 3, 2017

View reviewed changes

taketwo reviewed Sep 4, 2017

View reviewed changes

frozar force-pushed the octree_iterator branch from 0625c20 to 5b3cf3f Compare September 4, 2017 13:38

frozar force-pushed the octree_iterator branch from 5b3cf3f to 3140148 Compare September 22, 2017 12:50

taketwo added needs: more work Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Oct 23, 2017

frozar force-pushed the octree_iterator branch 4 times, most recently from 1d9da0a to 61d4c53 Compare November 14, 2017 16:23

SergioRAgostinho removed the status: needs decision label Feb 26, 2018

frozar added 6 commits February 26, 2018 16:34

[OCTREE] Fix the public scope in class OctreeBreadthFirstIterator.

ba61cf5

[OCTREE] Implementation of the iterator 'OctreeFixedDepthIterator'.

8353b0c

[TEST] Update tests regarding every iterator in test_octree_iterator.cpp

76914e9

[OCTREEKEY] Add the operator!= for the OctreeKey.

2996db6

[OCTREE_VIEWER] Use 'FixedDepthIterator' in octree_viewer tool.

b165f31

[CMAKE] Fix compilation for AppVeyor CI

15cf4b9

frozar force-pushed the octree_iterator branch from 81dd85f to 15cf4b9 Compare February 26, 2018 15:35

SergioRAgostinho merged commit 9dae1ea into PointCloudLibrary:master Feb 27, 2018

taketwo removed the needs: author reply Specify why not closed/merged yet label Feb 27, 2018

frozar deleted the octree_iterator branch February 27, 2018 23:49

This was referenced Aug 25, 2018

Change Log automation #2385

Closed

Change Log generation tool. Automates change log generation. #2396

Merged

SergioRAgostinho added the module: octree label Aug 25, 2018

SergioRAgostinho changed the title ~~[OCTREE] Implementation of the iterator 'OctreeFixedDepthIterator'.~~ Implementation of the iterator 'OctreeFixedDepthIterator'. Aug 25, 2018

Implementation of the iterator 'OctreeFixedDepthIterator'. #1983

Implementation of the iterator 'OctreeFixedDepthIterator'. #1983

Conversation

frozar commented Sep 1, 2017

taketwo left a comment • edited Loading

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

SergioRAgostinho Sep 3, 2017 • edited Loading

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

Choose a reason for hiding this comment

frozar Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

SergioRAgostinho Sep 3, 2017 • edited Loading

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

SergioRAgostinho left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frozar commented Sep 4, 2017

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

taketwo commented Sep 4, 2017

frozar commented Sep 22, 2017

taketwo commented Sep 26, 2017

frozar commented Sep 26, 2017

SergioRAgostinho commented Nov 22, 2017 • edited Loading

frozar commented Feb 26, 2018

SergioRAgostinho commented Feb 27, 2018

SergioRAgostinho commented Feb 28, 2018

frozar commented Mar 2, 2018

SergioRAgostinho commented Mar 2, 2018

taketwo commented Mar 3, 2018

frozar commented Mar 3, 2018

taketwo commented Mar 3, 2018

frozar commented Mar 3, 2018

SergioRAgostinho commented Mar 3, 2018

SergioRAgostinho commented Mar 3, 2018

taketwo commented Mar 3, 2018

NPatch commented Sep 13, 2018

SergioRAgostinho commented Sep 13, 2018

NPatch commented Sep 13, 2018 • edited Loading

taketwo left a comment •

edited

Loading

SergioRAgostinho Sep 3, 2017 •

edited

Loading

frozar Sep 4, 2017 •

edited

Loading

SergioRAgostinho Sep 3, 2017 •

edited

Loading

SergioRAgostinho left a comment •

edited

Loading

SergioRAgostinho commented Nov 22, 2017 •

edited

Loading

NPatch commented Sep 13, 2018 •

edited

Loading