RobustFieldAutonomyLab / LeGO-LOAM

LeGO-LOAM: Lightweight and Ground-Optimized Lidar Odometry and Mapping on Variable Terrain
BSD 3-Clause "New" or "Revised" License
2.4k stars 1.12k forks source link

We need to create a unit/regression test #107

Closed facontidavide closed 5 years ago

facontidavide commented 5 years ago

Hi,

In every PR that is currently in the pipeline the question "does this affect the precision" is, understandably, asked.

I think we should implement a unit-test or regression-test. This will greatly speed up the merging of new features / refactoring.

@TixiaoShan and @YoshuaNava, any suggestion?

personally, i like what Cartographer does, that is allowing to launch directly the application in "rosbag mode".

This means that the rosbag C++ API is used instead of "rosbag play". This allows results to be more consistent, no matter how fast is your PC.

facontidavide commented 5 years ago

Can be use for example the rosbag related to this to perform the regression test?

https://github.com/RobustFieldAutonomyLab/LeGO-LOAM/blob/master/LeGO-LOAM/launch/demo.gif ?

facontidavide commented 5 years ago

And I think we should also address/discuss the fact that results are NOT deterministic, as I discussed here #53 and here #105

andre-nguyen commented 5 years ago

I believe that the non-deterministic behavior comes from having a multi-threaded application with non-blocking queues. Since the odometry and mapping are separate, the mapping might compute its solution with different amounts of odometry inputs from one run to another. To ensure deterministic and proper comparisons you would need to either bring all the code into a single thread or put everything in the same process with blocking queues between them.

This is a change you need to introduce in addition to reading your rosbag using the C++ API.

facontidavide commented 5 years ago

I can easily take care of the refactoring myself, using proper multi-threading and refactoring to improve separation of concerns.

But before doing it I would like to know if maintainers have time to review and merge PRs, since there are few already in the pipeline.

YoshuaNava commented 5 years ago

@facontidavide This is an awesome idea.

Would we aim to test full SLAM performance from the start? Should we do so with datasets, or simulated environments?

I agree that the non-deterministic behavior should be ameliorated. In his PhD thesis, Ji Zhang mentions running LOAM on KITTI at around 10% the speed, if I remember well, to obtain really good results. His method was great, but people should not need to resort to this. At least we should know how much this really influences results.

Depending on the answer from @TixiaoShan we could continue developing on this repo, or maybe start a new one? From the start I think it would be better to continue working here, but it also depends on the time that he can dedicate to review PRs.

facontidavide commented 5 years ago

In terms of moving to another repository, I do want to contribute to the original repo, since @TixiaoShan deserve all the credit for this work.

But I also want to work considerably faster and all the PR I have been sent so far has been collecting dust.

I will probably wait until middle of September, once I am back from my vacation. If things are still moving too slowly, I will create a "friendly" fork, where you @YoshuaNava is welcome to collaborate.

By "friendly" I mean that I will always give my availability to work together with the original contributors to merge my changes upstream.

YoshuaNava commented 5 years ago

I agree. And I also think that regardless of the decision taken, we should credit the authors for their work appropriately.

I'll be glad to contribute :+1:

andre-nguyen commented 5 years ago

I agree that the non-deterministic behavior should be ameliorated. In his PhD thesis, Ji Zhang mentions running LOAM on KITTI at around 10% the speed, if I remember well, to obtain really good results. His method was great, but people should not need to resort to this. At least we should know how much this really influences results.

I believe the reason is that the odometry to mapping ratio is something like 10 to 1 when running in real time. E.g. For every one scan to map solution you have time to compute 10 scan to scan (odometry) solutions. For maximum accuracy, you would run things with a 1 to 1 ratio, i.e. a single scan to scan solution to initialize the scan to map solution.

facontidavide commented 5 years ago

Aaaaand... done!

It was a lot of work, but I eventually managed to refactor (heavily) the software to solve this issue: https://github.com/facontidavide/LeGO-LOAM/tree/single_process

This new version use multiple threads in a single process and synchronization mechanism As a consequence:

1) Given the same rosbag, it will always return the same result (determinism can not be guaranteed in asynchronous code) 2) The level of parallelism is the same as the original version, i.e. number of threads used. 3) No changes which may affect the precision have been added so far. 4) We don't need to use rosbag play anymore. If the name of the rosbag is passed as an argument, the application load the rosbag and processes it at the maximum speed allowed by your CPU.

In my case.

image

You might achieve more speed up (and mostly the same result) reducing the frequency of the MapOptimization.

This also address issues #112 and #116

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.