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

Added 3d edge detection and fast, edge-aware normal estimation #1672

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

ipa-rmb
Copy link

@ipa-rmb ipa-rmb commented Aug 4, 2016

Added new functions for 3d surface and depth edge detection directly from organized point clouds without previous normal computation. The algorithm comes with a very fast and edge aware normal estimation function.

@SergioRAgostinho
Copy link
Member

Thanks for your contribution. As this is new feature it will takes us a while to start reviewing it.

Tutorial (optional)

Although not required, and even though you also submitted some examples from what I can tell, it would be great if you could submit also a tutorial similar to this, briefly explaining the theoretical primer and showing a simple use case. It really helps to promote your contribution as users tend to explore the tutorials, not really the repository, so examples and tests tend to be overlooked.

Thanks for contributing 👍

@SergioRAgostinho SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Aug 18, 2016
@SergioRAgostinho SergioRAgostinho added this to the pcl-1.9.0 milestone Aug 18, 2016
@UnaNancyOwen
Copy link
Member

Thank you for contribution.
You don't need to change the ChangeList (CHANGES.md) yourself.
This file will be changed at the release time.

@ipa-rmb
Copy link
Author

ipa-rmb commented Aug 22, 2016

Thanks for your comments. It is definitely on my to do list to complete the tutorial within this week.

@ipa-rmb
Copy link
Author

ipa-rmb commented Jun 7, 2017

I finally added two tutorials. Is it possible to have a (local) preview of these tutorials before merging, i.e. how they would look like at the webpage?

The failing tests at two of the Travis jobs are not connected to this contribution, I just merged in the latest trunk version of PCL.

@jspricke
Copy link
Member

jspricke commented Jun 7, 2017

Awesome, try this for the docs: https://github.com/PointCloudLibrary/pcl/blob/master/.travis.sh#L319

@ipa-rmb
Copy link
Author

ipa-rmb commented Jun 7, 2017

Thanks a lot for this hint, Jochen, my ccmake did not display the "WITH_TUTORALS" option voluntarily, that's why I did not find it. The local tutorial build helped to see that I forgot to put links to the tutorials into the index.rst file. Now they are in.

@taketwo
Copy link
Member

taketwo commented Jul 2, 2017

Hi Richard, sorry for a very long delay. Things were pretty calm in PCL over the last year, but in recent weeks we gained a good momentum. We will release new 1.8.1 version soon, and immediately after that we will be ready to start merging new features for the next major release.

Your contribution looks very interesting and it would be great to get it into PCL soon. However, we will likely need a couple of rounds of reviews to make sure that everything is implemented in the "PCL way". This PR is extremely complex, 31 files and almost 4k new lines. To make the review process manageable for all of us, I'd ask you to split it up in several Pull Requests. I think a clean and logical separation will be 1) Updates for edge detection 2) New normal estimation class 3) Tutorial. No need to create all three immediately, let's start with the first one and make our way through.

@SergioRAgostinho what is your opinion?

@SergioRAgostinho
Copy link
Member

what is your opinion?

👍 It will make things easier for sure. Thanks again @ipa-rmb.

@taketwo taketwo added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jul 2, 2017
@ipa-rmb
Copy link
Author

ipa-rmb commented Jul 11, 2017

I will consider your advice throughout this week. Thanks for your feedback.

@ipa-rmb
Copy link
Author

ipa-rmb commented Jul 18, 2017

I have opened #1942 as part one of this pull request, which only contains the edge detection addons.

@stale
Copy link

stale bot commented Dec 14, 2017

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

@stale stale bot added the status: stale label Dec 14, 2017
@SergioRAgostinho SergioRAgostinho removed this from the pcl-1.9.0 milestone Jan 25, 2018
@stale stale bot removed the status: stale label Jan 25, 2018
@stale
Copy link

stale bot commented Mar 26, 2018

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

@stale stale bot added the status: stale label Mar 26, 2018
@stale
Copy link

stale bot commented Feb 21, 2020

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: author reply Specify why not closed/merged yet status: stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants