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

PointCulling as trait #273

Closed

Conversation

catevita
Copy link
Contributor

To prepare parallelization across nodes, we split into node search first and then point iteration and filtering. PointCulling has being reformatted into a trait to perform the most common operations.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@catevita catevita mentioned this pull request May 22, 2019
iterator.try_for_each(|point: Point| point_stream.push_point_and_callback(point))?;
point_stream.callback()
let mut iterator: Box<Iterator<Item = NodeId>> =
get_node_id_iterator(&self.octree, &self.culling);
Copy link
Contributor Author

@catevita catevita May 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the origin of most changes. (still wip) the node id iterator returns the id filtered by the geo query expressed by the culling and then the operation on the nodes can be parallelized.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does get_node_id_iterator come from? I don't see it in the other PR.

@catevita
Copy link
Contributor Author

finally it compiles and tests run smoothly! this is my first trait implementation so I am already grateful for all your suggestions!
PTAL @nnmm @feuerste

#[derive(Debug, Clone)]
pub struct OrientedBeam {
pub struct OrientedBeam<S> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making this its own small PR?

Copy link
Contributor Author

@catevita catevita May 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change the f64 in S to allow for Sync and Send for the trait. The other changes except for the trait are again already in master, here unfortunately yet not visible cause the PR of NodeIdIterator is still open

@nnmm
Copy link
Contributor

nnmm commented May 31, 2019

At a high level, do you think we could keep the PointCulling struct (and name the trait something else)? I think in general not having Boxed up traits is cleaner.

@catevita
Copy link
Contributor Author

catevita commented May 31, 2019

Resuming from the spontaneous pairing session with Nikolai: keeping the point culling enum struct allows to spare costly dereferencing from Arc and Box with the cost of few disambiguation lines https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b5ad139bdfae3bcc3e93d15911c8ee0a

@catevita catevita changed the title PointCulling as trait (WIP) PointCulling as trait Jun 4, 2019
@catevita
Copy link
Contributor Author

catevita commented Jun 7, 2019

this PR has been splitted up.
See #278 (still big)

@catevita catevita closed this Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants