Skip to content

(Discussion) Initial refactor resampleCloudSpatially for multi-threading #67

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nigels-com
Copy link
Contributor

Note: This branch is untested. It does compile.

I have a heavily refactored version of resampleCloudSpatially that supports multi-threading.
In terms of licensing, ideally I could contribute that upstream rather than LGPL the surrounding code.
So the purpose of this PR is to open a discussion of what might be agreeable all around.

The reasoning for this change:

  • Overload with a marker/filtered count back-end version of resampleCloudSpatially that does not allocate the markers or the output point cloud.
  • In our use case we prefer the markers directly rather than converting from the ReferenceCloud
  • In our use case the ReferenceCloud isn't needed, so don't build it
  • Incrementally building the output cloud is problematic concurrently, better as a "gather" step once markers are finalised.
  • Also, it resolves the problem of reserving the appropriate storage and then shrinking it again at the end.
  • The pre-existing resampleCloudSpatially continues behaving in a compatible manner.

And to be clear this PR does not multi-thread but is more aligned with my more severe refactor.

Further thoughts:

  • Perhaps move octree build up and out to the pre-existing overload, to share it read-only across worker threads.

To broadly describe the multi-threading approach, have concurrent neighbour searching, but lock access to the markers with a mutex and check if the current point is still filtered-in before proceeding to filter-out the neighbourhood. We do see some occasional non-determinism due to the non-deterministic order of points being processed, but the speedup is worthwhile. With 12 threads we don't seem quite bottle-necked on updating markers, but that likely depends on the minimum distance and the typical neighbour count.

@nigels-com
Copy link
Contributor Author

On a related note, I'm also interested in the potential for multi-threaded octree building.
With 12 threads the resampling step is taking a similar amount of time to the octree indexing.

@dgirardeau
Copy link
Member

Hi,

From what I see in this PR (at this point) the changes to the existing code base seem reasonable.

And if you wish to contribute your parallel version upstream, no problem ;)

@dgirardeau
Copy link
Member

Hi @nigels-com, are you going to push this effort forward in the future?

@nigels-com
Copy link
Contributor Author

It has been a fair while, indeed.
I don't well recall the details of this, but will take a fresh look.

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