cartographer-project / point_cloud_viewer

View billions of points in your browser.
Apache License 2.0
339 stars 98 forks source link

Implement ply batch iterator. #350

Closed feuerste closed 5 years ago

feuerste commented 5 years ago

Timings on lego small for building an octree: Old: 0m34.951s New: 2m36.307s However, when additionally taking #356 into account, we get the following measurements: New: 0m39.238s So the strategy for merging would be to merge this one and #356 right after each other, so I need approvals for both of them.

nnmm commented 5 years ago

How is the octree generation going to work with batches?

feuerste commented 5 years ago

How is the octree generation going to work with batches?

As far as I saw it, the iterator is used at two places:

  1. Bounding box generation, which is pretty easy to change from points to batches.
  2. Subsampling of children: Here all points are read into memory anyway. So we need to add functionality to merge batches and then split up batches modulo 8, which should be not too difficult.
nnmm commented 5 years ago

At a high level, couldn't the buf inside PropertyReader be an AttributeData of the right type? Then we wouldn't need to cast and copy.

Maybe we don't even need all those macros then?

feuerste commented 5 years ago

I changed everything to work on AttributeData, so no vector casting is needed any more. I think the macros are still needed because of the simultaneous assignment and casting while reading, as mentioned in the comment above create_and_return_reading_fn.

nnmm commented 5 years ago

I think we should be able to get rid of the match in read_casted_property! (at which point we can probably remove that macro). In push_reader! we have $dtype, from which we can construct the LittleEndian::read_xyz function name and get the $num_bytes by using std::mem::size_of::<xyz>().

feuerste commented 5 years ago

Please be aware that $dtype is not equal the type we read in LittleEndian::read_* for x, y, and z ($dtype here is f64, the underlying type can be anything). Furthermore, there is no LittleEndian::read_u8 function.

feuerste commented 5 years ago

Thanks for your reviews!