-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
updated assembleRegion() in region_growing_rgb.hpp to speed up algorithm #538
Conversation
Good point that the original code is inefficient. But as you mentioned, the proposed solution isn't too efficient either. We can avoid data copying completely if we use What do you think? Also, I see in line 198 of the same file there is another "clean-up" of |
Thanks @taketwo , your way is much better! I will fix it in maybe one or two days. |
I think there are a few corner cases that the current implementation does not take care of. For example, consider the following situations:
In some of these cases your code will lead to segfault. By the way, github does not send notifications about new commits in a pull request, so please leave a comment after pushing new stuff. |
@shengshuyang In response to your deleted comment: I would keep on using iterators and a single-pass algorithm. The corner cases that I listed could be addressed simply by adjusting the conditions of your while loops. No additional |
@taketwo How about this one? It's fully tested today, handles all corner cases and randomly generated cases. I feel like it's not the most neat solution cuz some if() conditions maybe redundant. |
Seems okay now. I don't think that any of the conditions are superfluous. Speaking about neatness, if you remove the unnecessary |
done |
Please add spaces between |
Great! One last thing will be to squash these five commits into a single one so we do not pollute the history. (After squashing locally just |
segment deletion.
updated assembleRegion() in region_growing_rgb.hpp to speed up algorithm
Thanks for the contribution! |
I'm glad I can help. This is my very first pull request, thanks a lot for guiding me through the process! |
You are welcome. That was a nice start :) |
the original code perform std::vector<>::erase() in a while loop. since std::vector<> is an implementation of array, this operation moves big chunk of data at each iteration, and thus dramatically slows down the algorithm.
Instead I copy all data into a std::list<>, perform erase() on it and then copy data back into the original std::vector<>. It's not a very neat tweak but still much faster.
Tested on region_growing_rgb_tutorial.pcd and other point clouds, outputs are identical to original code.