Skip to content

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