mihaibujanca / slambench3

Other
46 stars 5 forks source link

Initial Bonn Integration #18

Closed matthewspear closed 5 years ago

matthewspear commented 5 years ago

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!

matthewspear commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

mihaibujanca commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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