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

updated assembleRegion() in region_growing_rgb.hpp to speed up algorithm #538

Merged
merged 1 commit into from
Mar 4, 2014
Merged

Conversation

shengshuyang
Copy link
Contributor

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.

test_result

test_result2

@taketwo
Copy link
Member

taketwo commented Mar 1, 2014

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 std::vector::swap() which is constant time. Start two iterators, one from the beginning, another from the end. Advance the first one until find empty cluster. Advance the second one backwards until find non-empty cluster. Call swap on their indices vectors. Continue until iterators meet.

What do you think? Also, I see in line 198 of the same file there is another "clean-up" of clusters_ vector, so we may fix that as well.

@shengshuyang
Copy link
Contributor Author

Thanks @taketwo , your way is much better! I will fix it in maybe one or two days.

@taketwo
Copy link
Member

taketwo commented Mar 2, 2014

I think there are a few corner cases that the current implementation does not take care of. For example, consider the following situations:

  • there are no clusters
  • there are just two clusters, first is empty and second one is not
  • all clusters are empty
  • all clusters are non-empty

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.

@taketwo
Copy link
Member

taketwo commented Mar 2, 2014

@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 if-else stuff is required. (Well, except the very first one, but that is trivial and has to be checked before running the algorithm.)

@shengshuyang
Copy link
Contributor Author

@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.

@taketwo
Copy link
Member

taketwo commented Mar 4, 2014

Seems okay now. I don't think that any of the conditions are superfluous. Speaking about neatness, if you remove the unnecessary { }'s the code will become neater and twice as short.

@shengshuyang
Copy link
Contributor Author

done

@taketwo
Copy link
Member

taketwo commented Mar 4, 2014

Please add spaces between if and braces and also function names and braces (to follow PCL Style Guide).

@taketwo
Copy link
Member

taketwo commented Mar 4, 2014

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 git push --force and this pull request will be automatically updated). Once this is done I'll merge.

taketwo added a commit that referenced this pull request Mar 4, 2014
updated assembleRegion() in region_growing_rgb.hpp to speed up algorithm
@taketwo taketwo merged commit 4e6a0e0 into PointCloudLibrary:master Mar 4, 2014
@taketwo
Copy link
Member

taketwo commented Mar 4, 2014

Thanks for the contribution!

@shengshuyang
Copy link
Contributor Author

I'm glad I can help. This is my very first pull request, thanks a lot for guiding me through the process!

@taketwo
Copy link
Member

taketwo commented Mar 4, 2014

You are welcome. That was a nice start :)

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.

2 participants