Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Bonn Integration #18

Merged
merged 6 commits into from
Jul 26, 2019
Merged

Conversation

matthewspear
Copy link
Collaborator

Integrating Bonn RGB-D Dynamic Dataset into SLAMBench.
Added as WIP to allow better viablity.

Will submit PR after tidying up the new approach to regex!

Adds rules for downloading and processing the Bonn RGB-D Dynamic Dataset
Based off TUM implementation due to documentation mentioning "The sequences are in the same format as the TUM RGB-D Dataset, so that the same evaluation tools can be used."

Regex was adapted to handle scientific values found in some of the Bonn groundtruth.txt files.

A few Bonn depth.txt files referenced depth images that do not exist, when this happens the specific file is logged to console and that depth frame is dropped.
Provide direct access to intrinsic / distortion parameter instead of copying them.
@matthewspear
Copy link
Collaborator Author

matthewspear commented Jul 25, 2019

Need some feedback on a design approach:

I am tidying up the way regex is used across Bonn

Method A:

const std::string& t = RegexPattern::timestamp;
const std::string& w = RegexPattern::whitespace;
const std::string& n = RegexPattern::number;
const std::string& start = RegexPattern::start;
const std::string& end = RegexPattern::end;

// format: timestamp tx ty tz qx qy qz qw
std::vector<std::string> expression{start,
                                    t, w,       // timestamp
                                    n, w,       // tx
                                    n, w,       // ty
                                    n, w,       // tz
                                    n, w,       // qx
                                    n, w,       // qy
                                    n, w,       // qz
                                    n, end};    // qw

boost::regex expr = boost::regex(std::accumulate(expression.begin(), expression.end(), std::string()));

Instead of vector + accumulate these could be combined value the + operator.

Method B:

// format: timestamp tx ty tz qx qy qz qw
std::vector<std::string> expression2{RegexPattern::start,
                                        RegexPattern::timestamp, RegexPattern::whitespace, // timestamp
                                        RegexPattern::number, RegexPattern::whitespace,    // tx
                                        RegexPattern::number, RegexPattern::whitespace,    // ty
                                        RegexPattern::number, RegexPattern::whitespace,    // tz
                                        RegexPattern::number, RegexPattern::whitespace,    // qx
                                        RegexPattern::number, RegexPattern::whitespace,    // qy
                                        RegexPattern::number, RegexPattern::whitespace,    // qz
                                        RegexPattern::number, RegexPattern::end};          // qw

boost::regex expr = boost::regex(std::accumulate(expression.begin(), expression.end(), std::string()));

The main difference is having a copy for clarity and avoiding repeating RegexPattern too many times.

@mihaibujanca what do you think?

@matthewspear
Copy link
Collaborator Author

Slightly simpler:

const std::string& t = RegexPattern::timestamp;
const std::string& w = RegexPattern::whitespace;
const std::string& n = RegexPattern::number;
const std::string& start = RegexPattern::start;
const std::string& end = RegexPattern::end;

// format: timestamp tx ty tz qx qy qz qw
std::string expr = start
                  + t + w       // timestamp
                  + n + w       // tx
                  + n + w       // ty
                  + n + w       // tz
                  + n + w       // qx
                  + n + w       // qy
                  + n + w       // qz
                  + n + end;    // qw

boost::regex groundtruth = boost::regex(expr);

@mihaibujanca
Copy link
Owner

Slightly simpler:

const std::string& t = RegexPattern::timestamp;
const std::string& w = RegexPattern::whitespace;
const std::string& n = RegexPattern::number;
const std::string& start = RegexPattern::start;
const std::string& end = RegexPattern::end;

// format: timestamp tx ty tz qx qy qz qw
std::string expr = start
                  + t + w       // timestamp
                  + n + w       // tx
                  + n + w       // ty
                  + n + w       // tz
                  + n + w       // qx
                  + n + w       // qy
                  + n + w       // qz
                  + n + end;    // qw

boost::regex groundtruth = boost::regex(expr);

I quite like this. Perhaps also a midway between clarity and simplicity is using "time" or "ts" instead of "t" and "num" instead of "n"?

@matthewspear
Copy link
Collaborator Author

matthewspear commented Jul 25, 2019

Cool I'll go for that - saves the unnecessary use of vector and accumulate, and I agree about the names!!

Regex literals pulled out into a class in data-tools for reuse and cleaner code.
@matthewspear matthewspear changed the title WIP: Initial Bonn Integration Initial Bonn Integration Jul 25, 2019
@matthewspear matthewspear marked this pull request as ready for review July 25, 2019 17:15
@mihaibujanca
Copy link
Owner

Good job!

Perhaps one suggestion would be moving RegexPattern.h to something like dataset-tools/include/utils/RegexPattern.h.

It might seem a bit overkill when there's just one file in the directory but we don't know what else we might add there in the future (e.g perhaps the loadDatasetSensor kind of functions?)

@matthewspear
Copy link
Collaborator Author

Perhaps one suggestion would be moving RegexPattern.h to something like dataset-tools/include/utils/RegexPattern.h.

Yeah sure, that makes sense – I'm working on the refactoring / other utils in another branch called dataset-tools that'll do a draft PR for soon!

@mihaibujanca
Copy link
Owner

Happy for this to be merged whenever you're done with it, nicely done!

@matthewspear matthewspear merged commit dc96292 into mihaibujanca:master Jul 26, 2019
@matthewspear matthewspear deleted the bonn branch July 26, 2019 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants