Refactoring Data-tools #21

matthewspear · 2019-07-30T16:01:07Z

Working on refactoring data-tool, modernised the C++ usage, remove repeated and unused code.

- clear unused imports - adjust formatting - modernise code using auto - tidy up command-line outputs

Based on google styleguide

- usage of auto when creating new objects - usage of empty() when checking string length

- changed name to better convey what the function does - extracted into dataset_utils header

- usage of auto when creating new objects - usage of empty() when checking string length - inline checkRequirements function to allow header only usage - add decimal RegexPattern

- move parameter values into class - usage of auto when creating new objects

framework/tools/dataset-tools/TUM.cpp

- usage of auto when creating new objects - usage of empty() when checking string length - replace plain regex with RegexPattern paramters

Changing `../../dataset-tools/include/...` to just `include/...` for main DatasetReader classes

- usage of auto when creating new objects - tidy plain regex with RegexPattern parameters

- usage of auto when creating new objects - usage of empty() when checking string length - tidy plain regex with RegexPattern parameters - refactor to use checkRequirement util function

Created a generalised function which can be used to make a grey sensor with reasonable default settings. Function allows for additional changes to be made on top of the generalised sensor, reducing the general setup of the sensors.

- nest make functions in slambench::io namespace due to common use of classes within namespace - extract disparity parameters and types into header files of dataset reader classes

matthewspear · 2019-08-13T17:31:33Z

Summary

Refactored all of DatasetReaders
Usage of RegexPattern in DatasetReaders
Added checkRequirements function
Added makeSensor functions

matthewspear · 2019-08-13T17:40:21Z

One part I am not so happy with is when a CameraSensor doesn't use Distortion the make camera function:

inline CameraSensor* makeRGBSensor(const Sensor::pose_t &pose,
                                      const CameraSensor::intrinsics_t &intrinsics,
                                      const CameraSensor::distortion_coefficients_t &distortion);

The function can still be used but with {} as the distortion parameter and after creation the sensor can be defined to have no distortion:

auto rgb_sensor = makeRGBSensor(pose, intrinsics, {});
rgb_sensor->Rate = 1;
rgb_sensor->DistortionType = CameraSensor::NoDistortion;

Just think there might be a nicer way to do this without just making an additional function in dataset_utils.h and some causion might need to be taken due to undefined behaviour of how the distortion parameter is initalised with nothing even though the underlying type is float[5].

matthewspear · 2019-08-13T17:49:43Z

Another thing to discuss is the opinion to combine makeGreySensor and makeRGBSensor into a makeCameraSensor possibly with a colour parameter since the two function are pretty much the same beyond the colour components - although this might just over complicate it!

matthewspear · 2019-08-13T20:23:52Z

8d3090bb6687f5f0d6677bb062a13860  testing_master/rgbd_dataset_freiburg1_xyz.slam
c230ffe7f2d1c4f14b32b645e208738e  testing_dataset_tools/rgbd_dataset_freiburg1_xyz.slam

97ddd483afcab4b13cc39cedf68506f8  testing_master/rgbd_bonn_balloon.slam
bbcaf1513e1661a0dad2c92762b2f268  testing_dataset_tools/rgbd_bonn_balloon.slam

dfa1b40bf36f83ed9049a2436c507713  testing_master/artificial.slam
dfa1b40bf36f83ed9049a2436c507713  testing_dataset_tools/artificial.slam

cca2ec46bd20d265ac85cee75e683f58  testing_master/living_room_traj2_loop.slam
e12b4e377ae3de47df1e513eaa6523a1  testing_dataset_tools/living_room_traj2_loop.slam

43754c7401879e02aa903270e7d4936f  testing_master/MH_01_easy.slam
6553eb6a2468612614d17a863e16e2d4  testing_dataset_tools/MH_01_easy.slam

Tried checksum-ing comparision of slamfiles between master to dataset-tools but they come out different. This could be down to a number of reasons. Some of the names of the sensors has been changed across the various targets.

matthewspear · 2019-08-13T20:24:42Z

Tested each using pangolin-loader, everything seems ok except for EUROCMAV one:

Parameter input assigned value ../testing_dataset_tools/MH_01_easy.slam
*** Duplicated long option replaced: 'Grey-intrinsics-parameters'

mihaibujanca · 2019-08-13T20:54:19Z

While I agree with spaces over tabs, I feel like it's making it really hard to look through the changes, which also makes it likely that it will take longer for Bruno to review and integrate this into the main repo - assuming Bruno would be fine with the change to spaces in the first place.

I think for that reason it may be worth sticking to the current spacing method

framework/tools/dataset-tools/include/utils/dataset_utils.h

mihaibujanca · 2019-08-13T21:08:14Z

One part I am not so happy with is when a CameraSensor doesn't use Distortion the make camera function:
inline CameraSensor* makeRGBSensor(const Sensor::pose_t &pose,
                                      const CameraSensor::intrinsics_t &intrinsics,
                                      const CameraSensor::distortion_coefficients_t &distortion);
The function can still be used but with {} as the distortion parameter and after creation the sensor can be defined to have no distortion:
auto rgb_sensor = makeRGBSensor(pose, intrinsics, {});
rgb_sensor->Rate = 1;
rgb_sensor->DistortionType = CameraSensor::NoDistortion;

You could set the default distortion parameter to be empty. Then, inside your makeRGBSensor, check if the distortion parameter is still empty and set the DistortionType to NoDistortion accordingly

matthewspear · 2019-08-15T12:26:49Z

You could set the default distortion parameter to be empty. Then, inside your makeRGBSensor, check if the distortion parameter is still empty and set the DistortionType to NoDistortion accordingly

Is there any case where that causes problems, if a given set of distortion parameters were actually an empty set of 0'd values?

mihaibujanca · 2019-08-15T12:56:52Z

You could set the default distortion parameter to be empty. Then, inside your makeRGBSensor, check if the distortion parameter is still empty and set the DistortionType to NoDistortion accordingly

Is there any case where that causes problems, if a given set of distortion parameters were actually an empty set of 0'd values?

I doubt so. If a dataset provides information about lens distortion, then it has to be included. If no distortion information is available, the most sensible option seems to be assuming no distortion.

matthewspear · 2019-08-16T12:34:02Z

https://github.com/Matthewspear/slambench2/tree/data-tools-builder – Drafted the API in a seperate branch, implemented TUM as an example, feels like it reinvents the wheel (or sensor really).

It can be tidied up, was trying to make it completely generic so one of the template parameters can be removed.

- remove additional generic type - add additional grey and rgb sensor builder - update TUM class with sensor builder

framework/tools/dataset-tools/include/utils/sensor_builder.h

mihaibujanca · 2019-08-21T16:10:19Z

Looks pretty good, I haven't tried building the datasets with the latest commits yet.
I think given that many things are standard, we could also add defaults in sensor_builder, where we set the most common things to certain values (e.g in most cases, the frame rate is 30, each of the sensors has a name matching the sensor type, the frame size is (640,480) etc".

Similar to how we have
sensor->Description = description_.empty() ? "Default description" : description_;,

framework/tools/dataset-tools/include/utils/sensor_builder.h

- added defaults to SensorBuilder subclasses - made SensorBuilder constructor protected for avoid implementation

framework/tools/dataset-tools/dataset-generator.cpp

framework/tools/dataset-tools/ICL.cpp

mihaibujanca

Nice job! Had a few comments - some of them are admittedly issues not introduced in this PR, but it's not much work to change them and since we're refactoring this, it's worth fixing thm

framework/tools/dataset-tools/ICL.cpp

framework/tools/dataset-tools/ICLNUIM.cpp

framework/tools/dataset-tools/dataset-generator.cpp

framework/tools/dataset-tools/include/ICLNUIM.h

framework/tools/dataset-tools/include/utils/dataset_utils.h

framework/tools/dataset-tools/include/utils/sensor_builder.h

- extract name into separate parameters for builders - use sensor size to set index in ICLNUIM - usage of nullptr over NULL in ICLNUIM

Half complete refactor, easier to revert and complete later and have input from library author on implementation details.

Having two methods to add distortion complicates the usage of the API, so it was removed.

matthewspear added 6 commits July 26, 2019 23:26

Refactor dataset-generator

e751fd2

- clear unused imports - adjust formatting - modernise code using auto - tidy up command-line outputs

Add .clang-format file

21e7310

Based on google styleguide

Refactor Bonn DatasetReader

90afd1a

- usage of auto when creating new objects - usage of empty() when checking string length

Refactor analysing folder function

285b764

- changed name to better convey what the function does - extracted into dataset_utils header

Refactor TUM DatasetReader

a07574f

- usage of auto when creating new objects - usage of empty() when checking string length - inline checkRequirements function to allow header only usage - add decimal RegexPattern

Refactor SVO DatasetReader

4f06909

- move parameter values into class - usage of auto when creating new objects

mihaibujanca reviewed Aug 7, 2019

View reviewed changes

framework/tools/dataset-tools/TUM.cpp Outdated Show resolved Hide resolved

matthewspear added 10 commits August 12, 2019 17:27

Refactor EUROCMAV DatasetReader

d2c7957

- usage of auto when creating new objects - usage of empty() when checking string length - replace plain regex with RegexPattern paramters

Tidy up include headers

e18aa02

Changing `../../dataset-tools/include/...` to just `include/...` for main DatasetReader classes

Refactor ICLNUIM DatasetReader

1090887

- usage of auto when creating new objects - tidy plain regex with RegexPattern parameters

Refactor ICL DatasetReader

7ef6a4c

- usage of auto when creating new objects - usage of empty() when checking string length - tidy plain regex with RegexPattern parameters - refactor to use checkRequirement util function

Refactor grey sensor into utility function

0356a9a

Created a generalised function which can be used to make a grey sensor with reasonable default settings. Function allows for additional changes to be made on top of the generalised sensor, reducing the general setup of the sensors.

Refactor groundtruth sensor into utility function

9128f9c

Refactor rgb sensor into utility function

721363e

Refactor depth sensor into utility

57a5006

- nest make functions in slambench::io namespace due to common use of classes within namespace - extract disparity parameters and types into header files of dataset reader classes

Refactor accelerometer sensor into utility function

b52a0a4

Merge upstream master back into data-tools

10ad1e7

matthewspear marked this pull request as ready for review August 13, 2019 17:10

Merge upstream master back into data-tools

8f8e68c

matthewspear changed the title ~~WIP: Refactoring Data-tools~~ Refactoring Data-tools Aug 13, 2019

mihaibujanca reviewed Aug 13, 2019

View reviewed changes

framework/tools/dataset-tools/include/utils/dataset_utils.h Outdated Show resolved Hide resolved

Implement SensorBuilder classes

2b91ef2

matthewspear added 4 commits August 20, 2019 17:33

Refactor SensorBuilder class

ca39af0

- remove additional generic type - add additional grey and rgb sensor builder - update TUM class with sensor builder

Fix template return types for RGB / Grey sensors

fe9d318

Update datasets to use sensor builder

3db7ff6

Merge branch 'data-tools-builder' into data-tools

d49532f

mihaibujanca reviewed Aug 21, 2019

View reviewed changes

framework/tools/dataset-tools/include/utils/sensor_builder.h Outdated Show resolved Hide resolved

mihaibujanca reviewed Aug 21, 2019

View reviewed changes

framework/tools/dataset-tools/include/utils/sensor_builder.h Outdated Show resolved Hide resolved

mihaibujanca reviewed Aug 21, 2019

View reviewed changes

framework/tools/dataset-tools/include/utils/sensor_builder.h Outdated Show resolved Hide resolved

Add defaults for name / description in SensorBuilder

e331697

- added defaults to SensorBuilder subclasses - made SensorBuilder constructor protected for avoid implementation

matthewspear force-pushed the data-tools branch from 7835201 to e331697 Compare August 21, 2019 17:20

mihaibujanca reviewed Aug 21, 2019

View reviewed changes

framework/tools/dataset-tools/dataset-generator.cpp Outdated Show resolved Hide resolved

Remove unused imports and methods in data-tools

d2c324d

mihaibujanca reviewed Aug 22, 2019

View reviewed changes

framework/tools/dataset-tools/ICL.cpp Outdated Show resolved Hide resolved

mihaibujanca requested changes Aug 22, 2019

View reviewed changes

matthewspear added 3 commits August 22, 2019 16:56

Refactor data-tool builders

53af306

- extract name into separate parameters for builders - use sensor size to set index in ICLNUIM - usage of nullptr over NULL in ICLNUIM

Revert refactor changes to ICL

d14b3ed

Half complete refactor, easier to revert and complete later and have input from library author on implementation details.

Simplify SensorBuilder by removing duplicated parameter

57a64ac

Having two methods to add distortion complicates the usage of the API, so it was removed.

mihaibujanca approved these changes Aug 22, 2019

View reviewed changes

mihaibujanca merged commit 10006f2 into mihaibujanca:master Aug 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring Data-tools #21

Refactoring Data-tools #21

matthewspear commented Jul 30, 2019

matthewspear commented Aug 13, 2019

matthewspear commented Aug 13, 2019

matthewspear commented Aug 13, 2019

matthewspear commented Aug 13, 2019

matthewspear commented Aug 13, 2019

mihaibujanca commented Aug 13, 2019

mihaibujanca commented Aug 13, 2019

matthewspear commented Aug 15, 2019

mihaibujanca commented Aug 15, 2019

matthewspear commented Aug 16, 2019

mihaibujanca commented Aug 21, 2019

mihaibujanca left a comment

Refactoring Data-tools #21

Refactoring Data-tools #21

Conversation

matthewspear commented Jul 30, 2019

matthewspear commented Aug 13, 2019

Summary

matthewspear commented Aug 13, 2019

matthewspear commented Aug 13, 2019

matthewspear commented Aug 13, 2019

matthewspear commented Aug 13, 2019

mihaibujanca commented Aug 13, 2019

mihaibujanca commented Aug 13, 2019

matthewspear commented Aug 15, 2019

mihaibujanca commented Aug 15, 2019

matthewspear commented Aug 16, 2019

mihaibujanca commented Aug 21, 2019

mihaibujanca left a comment

Choose a reason for hiding this comment