-
Notifications
You must be signed in to change notification settings - Fork 206
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
Porting GUI to Qt-5 #13
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall very valuable but impossible to review properly on GitHub as you mixed reformatting with code improvements. I'll let you fix my comments.
@@ -0,0 +1,7 @@ | |||
include_directories(../include/gmapping) | |||
add_library(configfile configfile.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this library an INI parser ? Why not used the standard boost property tree? http://www.boost.org/doc/libs/1_55_0/doc/html/boost_propertytree/parsers.html#boost_propertytree.parsers.ini_parser
endif() | ||
|
||
include_directories(../include/gmapping ../ ../include/gmapping/log/) | ||
include_directories(.. ../include/gmapping ../include/gmapping/log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should first check for find_package(Qt5Widgets) and return if not found: if we add this GUI for gmapping, it will be made into it's own package. For now, we allow to build it in here.
else() | ||
target_link_libraries(gfs_simplegui gridfastslam ${QT_LIBRARIES}) | ||
find_package(Qt4 REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the whole code also compile with Qt4?
@@ -15,244 +15,249 @@ using namespace std; | |||
using namespace GMapping; | |||
using namespace GMapping::GFSReader; | |||
|
|||
inline double min(double a, double b){ | |||
return (a<b)?a:b; | |||
inline double min(double a, double b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never reformat code and modify code. Always have two commits to make it easier to review. Too late ...
} | ||
void computeBoundingBox(double& xmin, double& ymin, double& xmax, double& ymax, const RecordList& rl, double maxrange) | ||
{ | ||
xmin = ymin = DBL_MAX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should use http://en.cppreference.com/w/cpp/types/numeric_limits/max
@@ -1,133 +0,0 @@ | |||
std::vector<unsigned int> sistematicResampler<State,Numeric>::resample(const vector<Particle>& particles) const{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened to that file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bowang could you create new PR against melodic-devel
branch? we have switched from master
to melodic-branch
when we changed licence to BSD
These few commits ported gmapping GUI tools (gfs_simplegui, gfs_nogui, gfs_logplayer, gfs2img) to Qt-5 and re-enabled log tools (log_plot, log_test, rdk2carmen, scanstudio2carmen).
It has been tested on Ubuntu 14.04 with Qt-5.2.1