PointCulling Trait and Iterator cleanup #278

catevita · 2019-06-07T10:00:22Z

The iterator are splitted on
-iterator that retrieve nodes
-iterator that scan points on a single node

The PointLocation in which the Points that have to be retrieved has been streamlined,
the methods contains and intersects are part of the trait PointCulling.
This works for an Aabb3, Obb, OrientedBeam and Frustum

catevita · 2019-06-07T10:04:03Z

still quite a big PR, I tried my best to remove some other additional changes
PTAL @nnmm @feuerste

nnmm · 2019-06-07T13:51:39Z

src/math.rs

@@ -276,19 +301,43 @@ impl<S: BaseFloat> Obb<S> {
    }
 }

+impl<S> PointCulling<S> for Obb<S>


Nit: Why was this moved?

the methods have been moved to implement the trait - I hope to understand the question

nnmm · 2019-06-07T13:53:29Z

src/octree/batch_iterator.rs

-pub enum PointCulling {
-    Any(),
+#[derive(Debug, Clone)]
+pub enum GeoFence {


I think it might be clearer if we name this PointLocation, and what's currently called PointLocation can become PointQuery. GeoFence has a specific meaning: https://en.wikipedia.org/wiki/Geo-fence

good idea, I was running short on names!

nnmm · 2019-06-07T13:56:39Z

src/octree/batch_iterator.rs

-        };
-        iterator.try_for_each(|point: Point| point_stream.push_point_and_callback(point))?;
-        point_stream.callback()
+        //todo (catevita) mutable function parallelization


I'm not sure I understand and/or we need this. In the end, the user calls try_for_each_batch. Are you proposing the callbacks should happen on several threads in parallel? I suspect that's not super useful, since it requires synchronization on the user side again, and I'd prefer to (as a user) e.g. request points in non-overlapping boxes from several threads.

As told offline, this PR only takes care of implementing the trait, the parallelization follows up next

nnmm · 2019-06-07T13:58:57Z

src/octree/mod.rs

    }

-    pub fn points_in_frustum<'a>(
+    /// Returns the ids of all nodes that cut or are fully contained in 'aabb'.


Nit: Let's be consistent with two vs three slashes

this was meant to be a minimal doc comment for the method points_in_node, do we use doc comment at all?

nnmm · 2019-06-07T14:00:46Z

src/math.rs

+    S: BaseFloat + Sync + Send,
+{
+    fn contains(&self, point: &Point3<S>) -> bool;
+    fn intersects(&self, aabb: &Aabb3<S>) -> bool;


Could you add a TODO here to return Relation?

Nit: Our formatting convention for TODOs is:

// TODO(username): Do something.

Can you please adjust it everywhere in this PR?

Supernit: Still not in the format

// TODO(username): Do something.

everywhere.

My bad, I just paid attention to the caps! now it should be finally fixed!

nnmm · 2019-06-07T14:01:20Z

src/math.rs

+{
+    fn contains(&self, point: &Point3<S>) -> bool;
+    fn intersects(&self, aabb: &Aabb3<S>) -> bool;
+    fn transform(&self, isometry: &Isometry3<S>) -> Box<PointCulling<S>>;


This can probably be impl PointCulling, I'd think.

Unfortunately this does not work inside a trait, from rfcs

"impl Trait may only be written within the return type of a freestanding or inherent-impl function, not in trait definitions or any non-return type position. "

nnmm · 2019-06-07T14:05:15Z

src/octree/mod.rs

+
+#[derive(Debug, Clone)]
+pub struct Frustum<S: BaseFloat> {
+    pub matrix: Matrix4<S>,


Could you remind me why this contains the second matrix? I forgot. Also, the fields probably do not need to be public and I'd have expected this to be in math.rs.

i moved it in math, the matrix is used by the contains function

// TODO(ksavinash9) update after https://github.com/rustgd/collision-rs/issues/101 is resolved. pub fn contains<S: BaseFloat>(projection_matrix: &Matrix4<S>, point: &Point3<S>) -> bool {

the issue in collision is not resolved yet

feuerste

Can you please remove the file .DS_Store?
Did you actually check the runtime performance with all the new changes?

feuerste · 2019-06-11T07:05:23Z

src/math.rs

+    S: BaseFloat + Sync + Send,
+{
+    fn contains(&self, point: &Point3<S>) -> bool;
+    fn intersects(&self, aabb: &Aabb3<S>) -> bool;


Nit: Our formatting convention for TODOs is:

// TODO(username): Do something.

Can you please adjust it everywhere in this PR?

feuerste · 2019-06-11T07:06:59Z

point_cloud_client/src/bin/test.rs

@@ -54,8 +53,8 @@ fn main() {
    let num_points = args.num_points;
    let point_cloud_client = PointCloudClient::new(&args.locations, OctreeFactory::new())
        .expect("Couldn't create octree client.");
-    let point_location = PointLocation {
-        culling: PointCulling::Aabb(Aabb3::new(args.min, args.max)),


Any specific reason to remove the min/max restriction?
Then the whole point of having min/max arguments goes away...

i think it also was a copy-paste error

feuerste · 2019-06-11T07:12:28Z

point_viewer_grpc/src/bin/octree_benchmark.rs

+        global_from_local: None,
+    };
+    let mut batch_iterator = BatchIterator::new(&octree, &all_points, 1_000_000);
+    let mut num_batches = 0;


Nit: How about having a const variable BATCH_SIZE, removing num_batches, and just working on counter?

feuerste · 2019-06-11T07:14:24Z

point_viewer_grpc/src/service.rs

+            location: octree::PointLocation::Frustum(frustum_matrix),
+            global_from_local: None,
+        };
+        self.stream_points_back_to_sink(point_location, &req.octree_id, &ctx, resp)


It seems global_from_local is None whenever calling self.stream_points_back_to_sink. How about just passing a location and constructing the PointQuery inside stream_points_back_to_sink?

if it is ok for you i'd like to leave this pieces of code as is, as the grpc server is not intensively used, so if we have to support query with local/global transforms the PointQuery struct is already prepared to support that.

feuerste · 2019-06-11T07:20:55Z

point_viewer_grpc/src/service.rs

+                            .iter()
+                            .map(|p| {
+                                let mut v = point_viewer::proto::Color::new();
+                                v.set_red(f32::from(p.x) / 255.);


How about using Color::to_f32 to perform the conversion?

I changed it, it adds some lines but the performance seems not to be affected from that

feuerste · 2019-06-11T07:22:32Z

src/math.rs

+    fn transform(&self, isometry: &Isometry3<S>) -> Box<PointCulling<S>>;
+}
+
+// PointCulling for Aabb3


Nit: remove comment, this is clear

feuerste · 2019-06-11T07:24:41Z

src/math.rs

@@ -150,22 +199,6 @@ impl<'a, S: BaseFloat> Mul<&'a Vector3<S>> for &'a Isometry3<S> {
    }
 }

-impl<S: BaseFloat> Into<Decomposed<Vector3<S>, Quaternion<S>>> for Isometry3<S> {


Any reason why the generics were removed here and instead having an f64 implementation only?

this there is indeed some automatic or manual merging gone wrong! I am restoring them

It would be clearer to move them back exactly where they were before, so they don't show up as diff...

feuerste · 2019-06-11T07:27:02Z

src/math.rs

+        let corner_from = |x, y| {
+            isometry
+                .rotation
+                .rotate_point(Point3::new(x, y, S::from(0.0).unwrap()))


Nit: Use S::zero() instead of S:from(0.0)

feuerste · 2019-06-11T07:33:59Z

src/math.rs

+    }
+
+    fn transform(&self, isometry: &Isometry3<S>) -> Box<PointCulling<S>> {
+        let matrix = Matrix4::from(Decomposed {


Here we could nicely make use of the conversion methods between Decomposed and Isometry3, which were deleted above...

feuerste · 2019-06-11T07:36:51Z

src/octree/batch_iterator.rs

+        for node_id in node_id_iterator {
+            let point_iterator = self.octree.points_in_node(self.point_location, node_id);
+            for point in point_iterator {
+                if let Err(err) = point_stream.push_point_and_callback(point) {


Can we just do point_stream.push_point_and_callback(point)?; instead?

catevita · 2019-06-17T13:49:22Z

I did some benchmarking running time (./target/release/octree_benchmark ~/octree_marco/ --no-client) on my workstation. Results are quite aligned, typically these

new:
real 0m3.662s user 0m3.500s sys 0m0.160s

old
real 0m3.563s user 0m3.408s sys 0m0.152s

feuerste · 2019-06-18T11:38:18Z

Can you please remove the .DS_Store file?

feuerste · 2019-06-18T11:42:16Z

src/math.rs

+
+impl<S> PointCulling<S> for Aabb3<S>
+where
+    S: 'static + BaseFloat + Sync + Send,


What do we 'static need for?

feuerste · 2019-06-18T11:43:04Z

src/math.rs

+    S: BaseFloat + Sync + Send,
+{
+    fn contains(&self, point: &Point3<S>) -> bool;
+    fn intersects(&self, aabb: &Aabb3<S>) -> bool;


Supernit: Still not in the format

// TODO(username): Do something.

everywhere.

feuerste · 2019-06-18T11:44:37Z

src/math.rs

@@ -150,22 +199,6 @@ impl<'a, S: BaseFloat> Mul<&'a Vector3<S>> for &'a Isometry3<S> {
    }
 }

-impl<S: BaseFloat> Into<Decomposed<Vector3<S>, Quaternion<S>>> for Isometry3<S> {


It would be clearer to move them back exactly where they were before, so they don't show up as diff...

catevita · 2019-06-18T13:52:39Z

Can you please remove the .DS_Store file?

Indeed it shows up in the list, but it as removed file

feuerste · 2019-06-18T13:54:16Z

Can you please remove the .DS_Store file?

Indeed it shows up in the list, but it as removed file

Ah, I see, thanks, I overlooked this...

feuerste

LGTM

googlebot · 2019-06-18T15:35:13Z

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.

googlebot · 2019-06-18T15:36:46Z

☹️ Sorry, but only Googlers may change the label cla: yes.

googlebot · 2019-06-19T08:34:30Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

catevita · 2019-06-19T08:37:22Z

@wally-the-cartographer merge

catevita · 2019-06-19T09:05:07Z

@wally-the-cartographer merge

feuerste · 2019-06-19T09:09:15Z

@wally-the-cartographer merge

catevita · 2019-06-19T09:49:58Z

@gaschler sorry to bother you, Wally seems to be down again, could you help me merging this branch or pointing to the right person that mantains Wally?

catevita · 2019-06-19T12:32:19Z

thanks!

googlebot added the cla: yes label Jun 7, 2019

catevita mentioned this pull request Jun 7, 2019

PointCulling as trait #273

Closed

nnmm reviewed Jun 7, 2019

View reviewed changes

feuerste requested changes Jun 11, 2019

View reviewed changes

feuerste reviewed Jun 18, 2019

View reviewed changes

feuerste approved these changes Jun 18, 2019

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jun 18, 2019

feuerste added cla: yes and removed cla: no labels Jun 18, 2019

googlebot added cla: no and removed cla: yes labels Jun 18, 2019

all changes

897ebfd

catevita force-pushed the cvitadello/pointculling_trait branch from 6e01dbd to 897ebfd Compare June 19, 2019 08:34

googlebot added cla: yes and removed cla: no labels Jun 19, 2019

gaschler merged commit 616b957 into master Jun 19, 2019

catevita deleted the cvitadello/pointculling_trait branch July 4, 2019 04:16

PointCulling Trait and Iterator cleanup #278

PointCulling Trait and Iterator cleanup #278

Conversation

catevita commented Jun 7, 2019 • edited Loading

catevita commented Jun 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmm Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmm Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

catevita Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

feuerste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catevita commented Jun 17, 2019

feuerste commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catevita commented Jun 18, 2019

feuerste commented Jun 18, 2019

feuerste left a comment

Choose a reason for hiding this comment

googlebot commented Jun 18, 2019

googlebot commented Jun 18, 2019

googlebot commented Jun 19, 2019

catevita commented Jun 19, 2019

catevita commented Jun 19, 2019

feuerste commented Jun 19, 2019

catevita commented Jun 19, 2019

catevita commented Jun 19, 2019

catevita commented Jun 7, 2019 •

edited

Loading

nnmm Jun 7, 2019 •

edited

Loading

nnmm Jun 7, 2019 •

edited

Loading

catevita Jun 7, 2019 •

edited

Loading