search

PRBonn / kiss-icp

A LiDAR odometry pipeline that just works
https://www.ipb.uni-bonn.de/wp-content/papercite-data/pdf/vizzo2023ral.pdf
MIT License
1.6k stars 321 forks source link

Revise Voxel functionalities in VoxelHashMap and Preprocessing #362

Closed tizianoGuadagnino closed 4 months ago

tizianoGuadagnino commented 4 months ago

This unify the usage of Voxel and VoxelHash between Preprocessing and VoxelHashMap, so that all the using are in one and one place only.

tizianoGuadagnino commented 4 months ago

This now potentially invalidates #360, but I feel it is probably what we want.

benemer commented 4 months ago

I am a bit confused now, because we now have 3 or even 4 downsampling strategies with corresponding PRs :laughing:

How about first unifying the Voxel indexing by having one and only one PointToVoxel from the VoxelHashMap? We can still discuss together how we want to do the downsampling.

benemer commented 4 months ago

Ah, no, my confusion. The downsampled result will be the same.

This removes a lot of boilerplate code, nice!

I just don't like providing a dummy max_distance_ that we never use :thinking:

nachovizzo commented 4 months ago

For the sake of records, once we also did this. But we decided to split it to avoid inter dependencies between the modules. With this change of I want to compile the preprocessing module I first need the voxel hash map. Additionally, we also wanted to leave the door open for people who might want to borrow the solely one module and thus most of the pieces are isolated (from a build perspective).

That said, we might revise this idea now 💡 as I don't think it was a the most use case of the project.

We could do this but I would avoid this "hacky" thing, as if one reads it is not clear why you are creating a "map"

If we want to merge the implementations I see 2 options

1) extract the voxelization utility into a separate module (hashing point to voxel, etc) I think we even did this moons ago 🤔 2) create a static method in the voxel hash map that can be used without creating an unnecessary object of the hash map class

Wdyt?

nachovizzo commented 4 months ago

@tizianoGuadagnino is it too much to ask now for splitting this into 2 PR's? 90% of the changes are style changes and not the original idea we had for this PR?

I will like to have 2 commits in the history, 1 for the moving the existing utilities to a separate header, and a follow up changing style, raw for loops for for each, etc

tizianoGuadagnino commented 4 months ago

@tizianoGuadagnino is it too much to ask now for splitting this into 2 PR's? 90% of the changes are style changes and not the original idea we had for this PR?

I will like to have 2 commits in the history, 1 for the moving the existing utilities to a separate header, and a follow up changing style, raw for loops for for each, etc

@nachovizzo I can, but spiritually, these two changes are actually one, which is we revise everything that regards Voxels. This includes VoxelUtils, VoxelHash, VoxelHashMap and Voxeldownsample. Or maybe you mean we do a separate PR for for to std::for_each?. If you have a strong opinion of this, I can do it, but can I consider all the changes accepted?

In the meantime, I will change the name of the PR to be consistent.

nachovizzo commented 4 months ago

@tizianoGuadagnino I updated this PR with the only changes that should be, without any style change

Let's follow up on the style changes in https://github.com/PRBonn/kiss-icp/pull/364