Restructure and add functionality to PCLPointCloud2 filters #3483

mvieth · 2019-11-24T09:04:55Z

See #3471
Please see commit messages for more information

taketwo · 2019-11-24T11:57:26Z

Thanks for taking the time to implement this! Would it be possible to split the last commit into smaller chunks? The bullet-point summary you put in the comment is helpful, but it's hard to trace the points to the actual code changes. Be as granular as possible, this will accelerate the review.

mvieth · 2019-11-24T12:20:27Z

Ok, will do. Can I force-push the new commits to my master branch or would that cause any problems?

SergioRAgostinho · 2019-11-24T14:18:55Z

At this stage it is totally fine to force push your renewed commit history. Once things are organized, then refrain from doing so until the review is over.

taketwo

Sorry if I appear to be stubborn, but I'd like to have these commits further simplified and possibly split into separate PRs. Let me explain my concerns.

This PR, as it is, breaks our unit tests. (And they are by no means comprehensive.) The last three commits are very code-intensive; the code is fairly convoluted and, I can imagine, contains numerous edge-cases. As a result, it's hard to say which commit introduced a breaking change. Furthermore, if you now push a small fixup commit that turns the unit tests green, I wouldn't be sure that there is no other problem that is not covered by the tests. So I feel a very careful review is needed.

As a way forward, I'd propose to limit this PR to commits 1-8 (i.e. up to and including 3d2d139). Let's see if the tests are passing with it. I'd also like to have a more thorough discussion of the relation between the two applyFilter methods. From what I see, in the templated passthrough filter the point cloud version of applyFilter is implemented through the index version of applyFilter. Also, a certain applyFilterIndices method is used as a synonym of index version of applyFilter. I wonder if we should reproduce the same structure in the PCLPointCloud2 specialization of the filter.

filters/include/pcl/filters/passthrough.h

mvieth · 2019-11-25T20:25:42Z

Sure thing, no problem!
Regarding the applyFilter functions:

From what I see, in the templated passthrough filter the point cloud version of applyFilter is implemented through the index version of applyFilter

Yes, I guess that makes sense to avoid duplicate code. That way the cloud version of applyFilter can basically be (almost?) the same in all classes.

Also, a certain applyFilterIndices method is used as a synonym of index version of applyFilter

I don't really see the point of that. Is that for legacy reasons?

I wonder if we should reproduce the same structure in the PCLPointCloud2 specialization of the filter

Concerning the applyFilter(cloud) function that calls the applyFilter(indices) function, that's worth considering IMO.

As a side question (out of curiosity): How do you see the status of the PCLPointCloud2-filters in general? It seems unnecessary to have them in addition to the templated filters.

taketwo · 2019-11-26T09:21:11Z

Yes, I guess that makes sense to avoid duplicate code. That way the cloud version of applyFilter can basically be (almost?) the same in all classes.

👍

I don't really see the point of that. Is that for legacy reasons?

I also don't see the point and I don't know if there was any good reason for this. OK, let's not have this is PCLPointCloud2 version.

As a side question (out of curiosity): How do you see the status of the PCLPointCloud2-filters in general?

On one hand, this is mostly duplicate code that (as expected) diverged over the years from the templated code. A maintainers headache. However, I'm hesitant to deprecate these filters because to my knowledge they are quite popular as building blocks in ROS perception pipelines. If we were to deprecate them, people would need to convert to and from templated point clouds, introducing unnecessary overhead in (usually) resource-limited systems.

mvieth · 2019-11-26T12:03:52Z

Ok, I have found a mistake in my changes (probably the reason why the checks were failing). I wrote indices but it should have been *indices_ which is a completely different variable. Let's see if the checks are successful now.

mvieth · 2019-11-26T17:44:47Z

The one check that fails is feature_gasd_estimation, that one also fails in the current trunk, so it does not seem to have anything to do with this PR.
Anything else you would like me to do?

taketwo · 2019-11-26T20:43:40Z

it does not seem to have anything to do with this PR.

Yes, it's a different story.

Anything else you would like me to do?

If we agree that refactoring of applyFilter() will go into a follow-up PR, then not much left to do here. Squash the last fixup commit with 3d2d139 and we are good to go.

mvieth · 2019-11-26T21:03:59Z

Maybe 328b9fa instead of 3d2d139? Makes more sense IMO

taketwo · 2019-11-26T21:40:27Z

Sorry, you are right, I did not check carefully where the issue was introduced.

…lity

…lter

… instead of Filter - add the applyFilter(indices) function, the code has been adapted from the templated filters (hpp files) - remove some functions and variables from PassThrough because FilterIndices already has them

…t class

SergioRAgostinho · 2019-12-01T16:39:22Z

Don't merge this just yet. I started looking into the implementation and I noticed I have a couple of points I have questions to ask.

SergioRAgostinho

This is not a complete review but I'll just focus on the passthrough class for now. I'm noticing a tendency for divergence between both overloads of applyFilter.

In my ideal world, I would rather have a mechanism which makes all checks and finds which indices to keep and to remove and a have a second round in which given both these sets of indices, it then generates the appropriate point clouds.

Or write a private scope method which generates both indices and output point cloud structures, but that aborts each iteration early if only the indices are required.

filters/src/passthrough.cpp

SergioRAgostinho

Overall it looks good. I just had some additional minor suggestions.

filters/src/radius_outlier_removal.cpp

mvieth · 2019-12-03T20:25:22Z

I made the requested changes.
The last commit is a suggestion for the memcpy-problem, but it doesn't compile because pcl/io/file_io.h cannot be found. I assume because it is in another module?
With the two new functions, the fields can be any datatype, but the code is still readable and compact.

SergioRAgostinho · 2019-12-03T21:34:46Z

Thank you. I believe we should leave this new reading functionality you've added in the last commit to a separate PR and merge this PR without it. We can discuss the next steps once this one is merged.

filters/src/radius_outlier_removal.cpp

SergioRAgostinho · 2019-12-05T12:53:07Z

@taketwo what your opinion on rewriting the commit history here? It feels too atomic for me in certain points parts but I suspect the effort is gonna be considerable for little benefits. I believe I would merge it as is.

taketwo

If I was the author of this PR I'd probably squash some of the clean-up commits together. But that's a matter of taste I suppose.

I can definitely say that such a level of granularity makes review a breeze. Thanks @mvieth!

Remove unnecessary semicolon

6fc47fe

taketwo added module: filters needs: code review Specify why not closed/merged yet labels Nov 24, 2019

mvieth force-pushed the master branch from 38e6e28 to bc4f1fb Compare November 24, 2019 14:19

taketwo reviewed Nov 25, 2019

View reviewed changes

filters/include/pcl/filters/passthrough.h Show resolved Hide resolved

filters/include/pcl/filters/passthrough.h Show resolved Hide resolved

mvieth force-pushed the master branch from bc4f1fb to 4b20af3 Compare November 25, 2019 19:54

mvieth force-pushed the master branch from 91dd8e5 to 89fb3f7 Compare November 26, 2019 11:35

mvieth added 9 commits November 27, 2019 18:39

Replace some for-loops with range-based for-loops to improve readability

f254af0

Rename variable translation_is_zero in CropBox to match actual meaning

c651fd4

Change for-loop in CropBox to range-based for-loop to improve readabi…

a078fe9

…lity

Rename variable in RadiusOutlierRemoval to match name in templated fi…

b3262c4

…lter

Add some debug output to RadiusOutlierRemoval

56cacf2

Remove some unnecessary whitespace

46f5d9e

Deprecate getter/setter in PassThrough that are now provided by paren…

630627b

…t class

Change now-deprecated functions to replacements

f9101f0

mvieth force-pushed the master branch from 6f74845 to f9101f0 Compare November 27, 2019 17:43

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

taketwo approved these changes Dec 1, 2019

View reviewed changes

SergioRAgostinho added the changelog: ABI break Meta-information for changelog generation label Dec 1, 2019

SergioRAgostinho self-assigned this Dec 1, 2019

SergioRAgostinho requested changes Dec 1, 2019

View reviewed changes

filters/src/passthrough.cpp Show resolved Hide resolved

filters/src/passthrough.cpp Show resolved Hide resolved

filters/src/passthrough.cpp Show resolved Hide resolved

filters/src/passthrough.cpp Outdated Show resolved Hide resolved

filters/src/passthrough.cpp Show resolved Hide resolved

mvieth added 2 commits December 2, 2019 20:53

Add checks to applyFilter in PassThrough

43cd5be

Chain conditions in PassThrough

576483c

SergioRAgostinho requested changes Dec 3, 2019

View reviewed changes

Switch to make_shared in RadiusOutlierRemoval

ffae080

SergioRAgostinho reviewed Dec 3, 2019

View reviewed changes

filters/src/radius_outlier_removal.cpp Outdated Show resolved Hide resolved

mvieth added 2 commits December 4, 2019 09:08

Simplify boolean expressions in RadiusOutlierRemoval

573c180

Replace for loops in RadiusOutlierRemoval

3bffa92

mvieth force-pushed the master branch from 1bf010a to 3bffa92 Compare December 4, 2019 08:12

SergioRAgostinho approved these changes Dec 5, 2019

View reviewed changes

taketwo approved these changes Dec 5, 2019

View reviewed changes

SergioRAgostinho added the changelog: deprecation Meta-information for changelog generation label Dec 5, 2019

SergioRAgostinho merged commit c1e4edf into PointCloudLibrary:master Dec 5, 2019

mvieth mentioned this pull request Dec 6, 2019

Restructure and add functionality to PCLPointCloud2 filters Par… #3500

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure and add functionality to PCLPointCloud2 filters #3483

Restructure and add functionality to PCLPointCloud2 filters #3483

mvieth commented Nov 24, 2019 •

edited

Loading

taketwo commented Nov 24, 2019

mvieth commented Nov 24, 2019

SergioRAgostinho commented Nov 24, 2019

taketwo left a comment

mvieth commented Nov 25, 2019

taketwo commented Nov 26, 2019

mvieth commented Nov 26, 2019

mvieth commented Nov 26, 2019

taketwo commented Nov 26, 2019 •

edited

Loading

mvieth commented Nov 26, 2019

taketwo commented Nov 26, 2019

SergioRAgostinho commented Dec 1, 2019

SergioRAgostinho left a comment

SergioRAgostinho left a comment

mvieth commented Dec 3, 2019

SergioRAgostinho commented Dec 3, 2019

SergioRAgostinho commented Dec 5, 2019

taketwo left a comment

Restructure and add functionality to PCLPointCloud2 filters #3483

Restructure and add functionality to PCLPointCloud2 filters #3483

Conversation

mvieth commented Nov 24, 2019 • edited Loading

taketwo commented Nov 24, 2019

mvieth commented Nov 24, 2019

SergioRAgostinho commented Nov 24, 2019

taketwo left a comment

Choose a reason for hiding this comment

mvieth commented Nov 25, 2019

taketwo commented Nov 26, 2019

mvieth commented Nov 26, 2019

mvieth commented Nov 26, 2019

taketwo commented Nov 26, 2019 • edited Loading

mvieth commented Nov 26, 2019

taketwo commented Nov 26, 2019

SergioRAgostinho commented Dec 1, 2019

SergioRAgostinho left a comment

Choose a reason for hiding this comment

SergioRAgostinho left a comment

Choose a reason for hiding this comment

mvieth commented Dec 3, 2019

SergioRAgostinho commented Dec 3, 2019

SergioRAgostinho commented Dec 5, 2019

taketwo left a comment

Choose a reason for hiding this comment

mvieth commented Nov 24, 2019 •

edited

Loading

taketwo commented Nov 26, 2019 •

edited

Loading