OctreeIterators special member revision by SergioRAgostinho · Pull Request #2108 · PointCloudLibrary/pcl

SergioRAgostinho · 2017-12-01T03:47:50Z

Revised the implementations of the following operators

Copy constructors
Copy assignment
Destructors
Equality
Inequality

Added unit tests to validate all changes.

#1983 is meant to be applied after this one.

Edit: Fixes #2064

taketwo

Did you try PCL.Octree_Test on your machine? Seems to segfault on Travis.

taketwo · 2017-12-01T09:03:54Z

+          : OctreeIteratorBase<OctreeT> (other)
+          , stack_ (other.stack_)
+        {
+          this->current_state_ = stack_.size()? &stack_.back() : NULL;


Style

Also, can you please explain why we don't inherit the state from other, i.e. this->current_state_ = other.current_state_?

I'm gonna redirect you to the explanation I gave @frozar when I had a first look at the copy constructor fix he did. Let me know in case you need further clarification. Here's the link #1983 (comment)

The important point to remember is that current_state_ points to an address, which is normally the back element of the stack or the front of the FIFO vector (depending on the type of iterator) and when you perform the copy, this pointer needs to be updated the equivalent element of the stack/fifo container your object has. Also notice that current_state_ is not a direct pointer to an octree node. Hence it can't be simply copied.

Thanks, makes sense.

taketwo · 2017-12-01T09:04:01Z

+          : OctreeIteratorBase<OctreeT> (other)
+          , FIFO_ (other.FIFO_)
+        {
+          this->current_state_ = FIFO_.size()? &FIFO_.front () : NULL;


SergioRAgostinho · 2017-12-01T10:54:39Z

Did you try PCL.Octree_Test on your machine? Seems to segfault on Travis.

No. It was 4 am already when I pushed this. I just decided to let Travis do it's job over the night :)

frozar · 2017-12-01T10:58:55Z

If it may help, I use this line to compile octree test on my machine (it's quick enough to get the binary):

cmake -DPCL_ONLY_CORE_POINT_TYPES=ON -DPCL_NO_PRECOMPILE=ON -DBUILD_tools=OFF -DBUILD_examples=OFF -DBUILD_apps=OFF -DBUILD_2d=OFF -DBUILD_features=OFF -DBUILD_filters=OFF -DBUILD_geometry=OFF -DBUILD_io=OFF -DBUILD_kdtree=OFF -DBUILD_keypoints=OFF -DBUILD_ml=OFF -DBUILD_octree=ON -DBUILD_outofcore=OFF -DBUILD_people=OFF -DBUILD_recognition=OFF -DBUILD_registration=OFF -DBUILD_sample_consensus=OFFF -DBUILD_search=OFF -DBUILD_segmentation=OFF -DBUILD_simulation=OFF -DBUILD_stereo=OFF -DBUILD_surface=OFF -DBUILD_tracking=OFF -DBUILD_visualization=OFF -DBUILD_global_tests=ON -DBUILD_tests_2d=OFF -DBUILD_tests_common=OFF -DBUILD_tests_features=OFF -DBUILD_tests_filters=OFF -DBUILD_tests_geometry=OFF -DBUILD_tests_io=OFF -DBUILD_tests_kdtree=OFF -DBUILD_tests_keypoints=OFF -DBUILD_tests_octree=ON -DBUILD_tests_outofcore=OFF -DBUILD_tests_people=OFF -DBUILD_tests_recognition=OFF -DBUILD_tests_registration=OFF -DBUILD_tests_sample_consensus=OFF -DBUILD_tests_search=OFF -DBUILD_tests_segmentation=OFF -DBUILD_tests_surface=OFF -DBUILD_tests_visualization=OFF -DGTEST_INCLUDE_DIR=/home/frozar/soft/googletest/googletest/include -DGTEST_SRC_DIR=/home/frozar/soft/googletest/googletest -DCMAKE_BUILD_TYPE=Debug .. && make -j8 test_octree test_octree_iterator

@taketwo is right, test_octree segfaults.

taketwo · 2017-12-01T11:23:42Z

+          : OctreeIteratorBase<OctreeT> (other)
+          , stack_ (other.stack_)
+        {
+          this->current_state_ = stack_.size()? &stack_.back() : NULL;


Thanks, makes sense.

taketwo · 2017-12-01T11:26:09Z

-                  ( max_octree_depth_ == other.max_octree_depth_) );
+          return (this == &other) ||
+                    ((octree_ == other.octree_) &&
+                     (current_state_ == other.current_state_) &&


Then here we should dereference, right? Otherwise this is never true.
(Plus protection from dereferencing null pointer).

SergioRAgostinho · 2017-12-03T02:18:10Z

The end iterators we're being constructed in a defective manner. The moment, we wrote a "proper" iterator equivalence/inequality they started failing of course.

These latest changes fix #2064 as well. The PR now lacks tests for checking the newly defined begin/end operators. To be added later.

taketwo · 2017-12-03T09:49:39Z

The PR now lacks tests for checking the newly defined begin/end operators. To be added later.

In a separate PR? Should I merge this?

SergioRAgostinho · 2017-12-03T10:19:47Z

I’ll add on this PR.

taketwo · 2017-12-08T14:51:32Z

Can I just squash, or do you want to rearrange commits in some certain way?

SergioRAgostinho · 2017-12-08T14:57:37Z

I think I will squash 60c3272 into 44ffdd6 and then I think it would leave it like that.

SergioRAgostinho · 2017-12-08T15:03:12Z

Squashed and rebased on top of master for that additional appveyor checkup.

SergioRAgostinho · 2017-12-08T15:13:00Z

Ok, scratch that. I think I will just make two commits:

Copy ctor, assignment and other methods revision, plus test
Begin/end iterator revision plus tests

* Revised the implementations of the following operators - Copy constructors - Copy assignment - Destructors - Equality - Inequality * Added unit tests

- Added full parameter constructor for the octree iterators. - Modified Octree end iterators construction - Added unit tests for octree begin/end iterators

SergioRAgostinho added the changelog: API break Meta-information for changelog generation label Dec 1, 2017

taketwo reviewed Dec 1, 2017

View reviewed changes

frozar mentioned this pull request Dec 1, 2017

Implementation of the iterator 'OctreeFixedDepthIterator'. #1983

Merged

SergioRAgostinho added needs: more work Specify why not closed/merged yet needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Dec 3, 2017

taketwo approved these changes Dec 8, 2017

View reviewed changes

taketwo removed the needs: code review Specify why not closed/merged yet label Dec 8, 2017

SergioRAgostinho force-pushed the oct-it-cpy-ctor branch 2 times, most recently from e4ec8f8 to 2e0a4e7 Compare December 8, 2017 15:02

SergioRAgostinho added 2 commits December 8, 2017 15:15

OctreeIterators special member revision

0334783

* Revised the implementations of the following operators - Copy constructors - Copy assignment - Destructors - Equality - Inequality * Added unit tests

Octree begin/end iterator revision

ce26217

- Added full parameter constructor for the octree iterators. - Modified Octree end iterators construction - Added unit tests for octree begin/end iterators

SergioRAgostinho force-pushed the oct-it-cpy-ctor branch from 2e0a4e7 to ce26217 Compare December 8, 2017 15:18

SergioRAgostinho added the status: ready to merge label Dec 9, 2017

taketwo removed the status: ready to merge label Dec 9, 2017

taketwo merged commit 8006ece into PointCloudLibrary:master Dec 9, 2017

SergioRAgostinho deleted the oct-it-cpy-ctor branch December 14, 2017 01:06

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

Uh oh!

Uh oh!

Conversation

SergioRAgostinho commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taketwo left a comment

Choose a reason for hiding this comment

Uh oh!

taketwo Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

taketwo Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

taketwo Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho commented Dec 1, 2017

Uh oh!

frozar commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taketwo Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

taketwo Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho commented Dec 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taketwo commented Dec 3, 2017

Uh oh!

SergioRAgostinho commented Dec 3, 2017 via email

Uh oh!

taketwo commented Dec 8, 2017

Uh oh!

SergioRAgostinho commented Dec 8, 2017

Uh oh!

SergioRAgostinho commented Dec 8, 2017

Uh oh!

SergioRAgostinho commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SergioRAgostinho commented Dec 1, 2017 •

edited

Loading

taketwo Dec 1, 2017 •

edited

Loading

frozar commented Dec 1, 2017 •

edited

Loading

SergioRAgostinho commented Dec 3, 2017 •

edited

Loading