Add: morphological filters operate on the z dimension (replaces #521)

[chambbj]

No description provided.

[taketwo]

There is a note in every function:

Assumes unique indices.

What exactly do you mean by that?

[chambbj]

@taketwo Honestly, this is a bit of a cut and paste from copyPointCloud. I can take out that note to avoid confusion.

[taketwo]

I see, but that note is relevant only for functions with const std::vector<int> &indices parameter, which we don't have here.

[chambbj]

@taketwo Same thing here. Just rebased and pushed once again.

[taketwo]

I think we need one more rebase here to get the fixes introduced by #534.

[chambbj]

@taketwo This one should be ready now, although Travis timed out on gcc.

[taketwo]

Great! Let's wait some time just in case @jspricke has anything to add. Otherwise I am fine with merging this.

[jspricke]

This is the only change compared to morphologicalDilate, I would propose to merge both functions and give them a min/max parameter.

[chambbj]

@jspricke If this looks better, I can squash, rebase with master, and push one last time.

[taketwo]

For what it's worth, the order of arguments in TEST macro is supposed be the other way round, because in its current form 5 test cases each with 1 test are created:

50: [==========] Running 5 tests from 5 test cases.
50: [----------] Global test environment set-up.
50: [----------] 1 test from Dilate
50: [ RUN      ] Dilate.Morphological
50: [       OK ] Dilate.Morphological (0 ms)
50: [----------] 1 test from Dilate (0 ms total)
50: 
50: [----------] 1 test from Erode
50: [ RUN      ] Erode.Morphological
50: [       OK ] Erode.Morphological (0 ms)
50: [----------] 1 test from Erode (0 ms total)
50: 
50: [----------] 1 test from Open
50: [ RUN      ] Open.Morphological
50: [       OK ] Open.Morphological (0 ms)
50: [----------] 1 test from Open (0 ms total)
50: 
50: [----------] 1 test from Close
50: [ RUN      ] Close.Morphological
50: [       OK ] Close.Morphological (0 ms)
50: [----------] 1 test from Close (0 ms total)
50: 
50: [----------] 1 test from Unsupported
50: [ RUN      ] Unsupported.Morphological
50: [       OK ] Unsupported.Morphological (0 ms)
50: Morphological operator is not supported!
50: [----------] 1 test from Unsupported (0 ms total)
50: 
50: [----------] Global test environment tear-down
50: [==========] 5 tests from 5 test cases ran. (0 ms total)
50: [  PASSED  ] 5 tests.

whereas logically I would expect 1 test case ("Morphological") with 5 tests ("Dilate", "Erode", etc).

[chambbj]

@taketwo Yeah, sure, I can fix this. The backwards interpretation is fairly commonplace throughout the filters tests for some reason.

• https://github.com/PointCloudLibrary/pcl/blob/master/test/filters/test_filters.cpp#L82
• https://github.com/PointCloudLibrary/pcl/blob/master/test/filters/test_sampling.cpp#L62

[taketwo]

I would admit that not only backwards interpretation in TEST macros, but also in EXPECT/ASSERT_xxx macros (where the expected value is supposed to go first) is common PCL. I guess one of the initial contributors was not aware of this, and later his mistake was replicated by others in dozens of tests... Well, this calls for a clean-up, of course.

[chambbj]

@taketwo Pretty sure the failed job was a fluke. Can you initiate a restart of the build within Travis?

[taketwo]

Can you initiate a restart of the build within Travis?

No, I have no control of that.

Pretty sure the failed job was a fluke.

Yeah, I wouldn't even bother restarting the job since the last time it was okay. Every now and then Travis dies building test executables. Some of the tests are indeed large and cover many PCL classes, so compilation turns to be a pretty memory-intensive process. Again, calls for a clean-up and splitting tests into multiple files.

[chambbj]

@taketwo @jspricke Where do we stand on accepting this pull request? I've got a couple more that depend on it that I've been holding off on.

[taketwo]

I am all pro, just waiting for @jspricke to comment.

[taketwo]

Well, I think there won't be major objections anyways, and minor things we can patch later. I will merge so that we can move on with other PRs.