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

Add: morphological filters operate on the z dimension, progressive morph... #521

Conversation

chambbj
Copy link
Contributor

@chambbj chambbj commented Feb 20, 2014

...ological filter can be used to segment ground returns

@jspricke
Copy link
Member

HI chambbj, thanks for your contribution(s) :). Can you clean this one up a little? Like squashing all the fixup commits. Have a look at git rebase -i (and do a git push -f later). This would make it easier to review, bisect wouldn't break in case, and your flaws won't show up in the PCL history ;). Thanks!

…rphological filter can be used to segment ground returns
@chambbj
Copy link
Contributor Author

chambbj commented Feb 26, 2014

Didn't know your stance on rebasing and forced pushes. Done!


pcl::octree::OctreePointCloudSearch<PointT> tree (resolution);

tree.setInputCloud (cloud_in.makeShared ());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use ::Ptr instead of makeShared () (saves us a copy ;) ).

@taketwo
Copy link
Member

taketwo commented Feb 26, 2014

I think it makes sense to change function(s) signature so that the input is provided by pointer. A pointer is needed internally anyways. Would be stupid if the end-user has a shared point cloud, then he uses *cloud to call the function, and the function turns this into another shared cloud.

@jspricke
Copy link
Member

Force pushes are ok, as long as you are doing it in your own private branch ;) (and no one is basing work on it). On the other hand, this includes quite a lot of different stuff, so it would be great if you could organize this into logical independent commits (instead of historically ;) ). Maybe one for the filter, one for the segmentation, one for the octree change and for the different tools. This would make it easier to review and we wouldn't remove unrelated patches in case we have to revert it. Sorry for the trouble and thanks for the work!

@jspricke
Copy link
Member

@taketwo
Copy link
Member

taketwo commented Feb 26, 2014

@jspricke Let me restate my idea. morphologicalDilate() uses Octree internally, which requires the input to be supplied in the form of shared pointer to a cloud. morphologicalDilate() accepts input by reference to a point cloud, so it needs to turn the input cloud into a shared pointer to a cloud (which involves complete copy) before using Octree. Now imagine a situation when the user of morphologicalDilate() already has a shared pointer to a cloud. He will need to dereference it to pass to the function. And the function will create another shared pointer inside. This creates a needless copy.

@jspricke
Copy link
Member

@taketwo right (I stopped reviewing this when I saw the makeShared ;) ). So passing a ::Ptr in morphologicalDilate() would be the way to go.

@taketwo
Copy link
Member

taketwo commented Feb 26, 2014

::ConstPtr to be precise ;)

@chambbj
Copy link
Contributor Author

chambbj commented Feb 27, 2014

Thanks for the feedback @taketwo and @jspricke! I'll break this up into four separate pull requests with the suggested modifications.

@chambbj chambbj closed this Feb 27, 2014
@chambbj chambbj deleted the morphological-filters-updated branch March 3, 2014 03:28
taketwo added a commit that referenced this pull request Mar 17, 2014
Add: morphological filters operate on the z dimension (replaces #521)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants