MRPT / GSoC2017-discussions

See README
1 stars 0 forks source link

New GUI app to visualize and edit robot-made maps (LisGein) #4

Closed jlblancoc closed 6 years ago

jlblancoc commented 7 years ago

Initial description:

I would like to develop a new GUI application based on Qt 5.5 or next version that should be able to load data in simplemap format and visualize it as a set of different metric maps. Also it should allow editing robot poses to fix errors in automatic SLAM algorithms. I want integrate my application in cmake main project of mrpt.

See attached PDF: 6536204506890240_1490778297_GSoCProposaldraft.pdf

Code results:

Video: https://youtu.be/YgGcNvrxZI0

jlblancoc commented 7 years ago

Student: @LisGein

Main mentor: @jlblancoc Backup mentors: @jolting

LisGein commented 7 years ago

Hi @jlblancoc! Thanks so much for giving me this opportunity! I'm ready to start work. If you do not have special requests about my proposel, I'd like to start creating a prototype. First of all, I will add OpenGl to my Qt-project.

jlblancoc commented 7 years ago

Hi! Hope this is the beginning of a successful project! ;-)

That sounds terrific, go on for it.

Afterwards, it would be great for you to get familiar with the API of MRPT CMetricMaps to be rendered as mrpt::opengl objects, in case you are not familiar yet... Let's keep in touch.

jolting commented 7 years ago

One thought I had was to accelerate the integration of cmake. I think this was in Week 9.

I think that integrating QT with the MRPT build system a little sooner might save you a lot of time in the long run. Also it makes it easier for us to build if it's part of the MRPT build process. The application should build from mrpt/apps. Furthermore, I want to emphasize code reuse. Shared GUI classes should be part of the MRPT library, not the application. All these things really requires you to integrate with the current MRPT build system as the first step.

What do you think?

P.S. Since Coding doesn't officially begin until May 30, I applaud your desire to start early.

jlblancoc commented 7 years ago

Can't agree more with Hunter pointers!

LisGein commented 7 years ago

Thank you @jolting. I think you are absolutely right. May be can I do it now?

And thank you @jlblancoc. I think I should read examples and MRPT code about OpenGl before include it to my project.

jolting commented 7 years ago

By all means, begin now.

jlblancoc commented 7 years ago

Hi @LisGein !

This is a general note to all accepted students that will be working on a fork of MRPT/mrpt : please, use the branch https://github.com/MRPT/mrpt/tree/mrpt-2.0-devel as base for all your work. In other works: don't start working based on MRPT/master, but on MRPT/mrpt-2.0-devel. This is because it's expected that during your project, master will become a maintenance branch mrpt-1.5, while mrpt-2.0-devel eventually will be merged with master and become the main development branch.

jlblancoc commented 7 years ago

@LisGein and @raghavendersahdev: Please, note that both of your projects share a first common place: integrating Qt into MRPT CMake scripts.

So, we strongly recommend that both of you share your ideas, thoughts, discussions, etc. in these threads so, eventually, we can merge both of your proposed CMake changes. Recall some of the specifications:

jolting commented 7 years ago

Take a look at how PCL does it. The MRPT cmake scripts are a little different, but the requirements are similar. https://github.com/PointCloudLibrary/pcl/blob/master/cmake/pcl_find_qt5.cmake Note: we do not need to support QT4 like PCL.

Make sure you look at: http://doc.qt.io/qt-5/cmake-manual.html

Consider adding libmrptgui-qt5 to mrpt, so that Wx and Qt5 code doesn't get mixed in the same library.

For Ubuntu, you'll probably want to add libqt5opengl5-dev as a build dependency for packaging/debian/control.in and Travis/Shippable. That package depends on qt5base-dev, so it should pull in almost everything you need plus the opengl stuff.

LisGein commented 7 years ago

Hello! I started working with the branch(mrpt-2.0-devel) and noticed the compilation errors in the assimp and zlib. They are caused by the automatic replacement of NULL with nullptr and similar changes in the two commits: Completely remove stlplus, nullptr instead of NULL.

I've fixed it locally in files:: libs/base/src/compress/zlib/zlib.h otherlibs/assimp/code/BlenderLoader.cpp otherlibs/assimp/code/BlenderTessellator.cpp otherlibs/assimp/code/FBXParser.cpp otherlibs/assimp/code/XGLLoader.cpp

As I know, Assimp will soon be removed from repo, but we should fix zlib.

LisGein commented 7 years ago

I have forked mrpt and added some commits. First is fixing compilation of mrpt-2.0-devel. The other things are integrating qt5 in mrpt and adding new qt application. @raghavendersahdev, is this solution suitable?

jolting commented 7 years ago

You should open a pull request, so we can review it. That looks like some useful changes. Make sure you open it for the mrpt-2.0-devel branch.

jolting commented 7 years ago

Can you add something to script_show_final_summary.cmake?

jolting commented 7 years ago

Also you forgot to add script_qt.cmake to the top level CMakeLists.txt. Finally, case the filenames like this CMainWindow.cpp and CMainWindow.h instead of all lowercase. Windows probably won't care, but Linux does.

LisGein commented 7 years ago

I have made a pull request, so I have fixed the compilation for Windows (checked on win 10 msvs 2017 x64 debug) and have added installation Qt dlls.

raghavendersahdev commented 7 years ago

Thanks @jlblancoc @jolting your links helped. Hi @LisGein your solution to CMakeLists.txt looks fine to me I am also doing something very similar here . I am not using the scripts_qt.cmake currently as I am including all my dependencies in my CMakeLists, I am not sure if this is the best way, I have a CMakeLists.txt where I specify the qt, mrpt dependencies. It works fine for me.

jlblancoc commented 7 years ago

Hi @raghavendersahdev , You can now merge from the main mrpt-2.0 branch to yours, so you can use Lis's CMake scripts. Take a look at her app's CMake script. Specially, notice that find_package(), etc. are not required there, since all that part is in the other, shared, separate .cmake file. Cheers!

raghavendersahdev commented 7 years ago

thanks @jlblancoc I am using Lis's CMake scripts now. I have made a pull request after fixing my CMakeLists file. I am working on ubuntu 16 with Qt5

jlblancoc commented 7 years ago

Hi @LisGein ,

How are you doing? Any doubt on MRPT classes?

I just checked the code in your fork. Congrats, I really like your style! It's obvious you have a good background experience as a professional C++ developer :+1:

Just some random comments:

The final goal is having the first, base class, moved to mrpt-gui, as an Qt alternative to CMyGLCanvasBase which can be used in a Qt-only version of CDisplayWindow3D and obviously in other MRPT apps. Not urgent, but feels like the perfect case of code reuse... ;-)

Also: please check here the current standard in MRPT for mouse-based pan/orbit/zoom/... so we end up having the same behavior in either wxWidgets or Qt-based GUIs. Probably CMyGLCanvasBase and your new class in mrpt-gui could have a shared common base class, or CMyGLCanvasBase could be made compatible with both wx and Qt via #ifs. Just two ideas...

Cheers

LisGein commented 7 years ago

Hi! Thank you :) Yes, the last two weeks I've been working on integration Qt, mrpt and OpenGL together. I created a QtWidget that can render mrpt::opengl::COpenGLScene.

My application now supports the following features:

image image

I will submit my graduate thesis next week, and after that I will be able to spend all my time to the project.

But now, I feel that I need more files(simplemap and .simplemap.gz) for work. I'd like to add support of some 3D data, so I need maps with 3D laser scan. Some examples with a beacon and landmark will be good too. Where I can found more this files?

Thank you for advises, I believe it will be useful. I think I should further improve the navigation.

jlblancoc commented 7 years ago

I will submit my graduate thesis next week, and after that I will be able to spend all my time to the project.

Good luck with that!

On simplemaps for testing, I have collected some of them in these two Drive shared folders:

Most contain 2D LiDARs only, but there is at least one (named "...3lasers...") with 2D scanners pointing at different 3D orientations, which actually generates 3D pointcloud.

Note that there are CSensoryFrames with images, stereo images, etc. I can't remember if your proposal considered this possibility, sorry if I forgot mentioning it, but definitively those images should be visible in some (optional?) panel in the UI.

On the UI design, I understand that it's still in an early stage. I would probably recommend adding a buttons toolbar on the top with common operations, perhaps checkboxes to show/hide different layers/sensors/etc. For example, showing a little "corner" (from mrpt::opengl::stock_objects) at each sensoryframe keyframe may be one of those things one might want to show/hide.

Also, a nice feature to have is to collect all the different kinds (C++ class names) of observations in all the CSensoryFrame's of the map. Something as simple as showing this on request (via a menu or toolbar button) is enough.

To debug the visualization of 3D range scans (e.g. Kinect, RGBD cameras) I think the best is to simple take one of the RGBD datasets (in .rawlog format; make sure of reading the comment on 3D pointcloud not being embedded in the .rawlog), and write a very simple independent program (which does not have to be in github) that simple takes a few observations and create a .simplemap by assigning to them some 2D/3D poses. You can use arbitrary poses, or look at the ground-truth files provided in most of those datasets.

Lots of things to do, but so far it looks great, and I'm sure you are on track...

Let us know if you need clarifications... MRPT is quite large and there are many subtleties!

jlblancoc commented 7 years ago

BTW: Those .simplemaps were created with MRPT 0.5.x (!!!) in 2007... It will be the definitive test for my long-term maintenance of binary serialization code. If they can be still loaded after 10 years of continuous changes it will feel fantastic !!

jlblancoc commented 7 years ago

This is a kind reminder to all GSoC students and mentors

According to GSoC FAQ, students are supposed to devote 30+ hours per week to their assigned project. This amounts to 6h/day (Mon-Fri) or 4.3h/day (Mon-Sun), or any equivalent distribution given the flexibility to work on your own schedule. Clearly failing to such a commitment may be a reason to fail in the monthly evaluations, since that information was clear at the time of applying to GSoC.

It's difficult to convert hours into GitHub commits, but as a rule of dumb, there should at least a substantial daily commit during each week working day, that is, roughly 5 substantial commits per week. This is a general rule, and mentors may be flexible depending on the difficulty and nature of each project.

The first evaluation will start June 26-30.

jlblancoc commented 7 years ago

Hi @LisGein ,

Did you manage to open those old simplemap files?

jlblancoc commented 7 years ago

For those of you creating new MRPT apps, remember to also create a basic manpage, a must-have for Debian packages. This includes:

LisGein commented 7 years ago

Hi @jlblancoc, Sorry I haven't been answering for too long.

Did you mean *.simplemap.gz? If yes - I did it.

Also I added a copyright notice.
Next I will do refactoring СGlWidget and I'll adding a brief description of Doxygen.

jlblancoc commented 7 years ago

Ok, good to know the files are still functional!

Btw: I think we discussed this in detail when preparing your proposal, and I'm aware it's early for this, but just in case: the goal is having the app only Asking for .simplemap files when opening. The INI should be replaced by some GUI dialog, either with many checkboxes and fields to configure the maps, just a large edit space with the INI contents such that the user can edit, or a combination of both...

Cheers

LisGein commented 7 years ago

What do you think will happen when a user opens a .simplemap without .ini? Nothing or will there be default settings?

jlblancoc commented 7 years ago

It might end up being annoying, but... I think the best is always showing the "what kind of metric maps do you need" dialog?? Because depending on the type of sensors stored in the .simplemap, different maps could be built.

Thinking a bit more on how this ui would be, here are some ideas:

The CMemoryConfigFile class would allow you to easy read and write specific ini file entries by loading the text area text into it, editing, then saving back as a string.

What do you think?

LisGein commented 7 years ago

Perhaps it will be good if the program alternately tries to open maps (Points map, Beacon, e.t.c) until one of them opens up?

"typical setups" -- what do you mean?

jolting commented 7 years ago

Hmmm. Maybe something like the way rviz works. Each type of data that can be visualized is listed and can be added to the the visualizer. I like how there is an easy way to add each type of data with rviz. Also I like how there is a menu that lets me tweak how each visualizable element is displays. Most importantly, I can easily turn them off when I'm not trying to visualize them.

LisGein commented 7 years ago

Thanks for the advice! So, I have started working with the configuration widget.

jlblancoc commented 7 years ago

Good idea, Hunter!

As a first approach, the configuration of how each metric map is built, and rendered, is encoded in its "Initilization" section of the configuration ("INI") file; so the configuration UI for each metric map could be the same / based on / etc. the one we discussed previously, Alice, regarding the questions the user is asked when a simplemap is opened.

Search for MAP_DEFINITION_START and MAP_DEFINITION_END in all maps headers to see the list of parameters for each map.

LisGein commented 7 years ago

What do you mean the initialization section? As I can see, for example CSimplePointsMap loads from config CPointsMap::TInsertionOptions and CPointsMap::TLikelihoodOptions. Where is the common part?

LisGein commented 7 years ago

What about evaluation? Should I press the button right now?

LisGein commented 7 years ago

Hi! I did a widget for configuration. It can modify, add and remove configs for metric maps. Also it can load from *.ini file. image

I tried to save the configuration(TSetOfMetricMapInitializers::dumpToTextStream) in a file, but this function uses different format. For example: "----------- [CBeaconMap::TInsertionOptions] ------------" but needed "[MappingApplication_beaconMap_00_insertOpts]". Does MRPT contain the possibility of saving in the correct format?

Also I need to specify a range for each numeric control. Is it documented somewhere?

jlblancoc commented 7 years ago

Hi Lis!

Well, that's UI is great, thanks for the effort.... definitively, you started with one of the more complex option structures!

Your question on ranges, together with me thinking of how much work may it be to MAINTAIN your UIs in the future (imagine those parameters changing in the future!!) made me reconsider this part of your project...

I think it's time to address this in a "Proper Way (TM)" ;-)

What about adding reflection to MRPT CLoadableOptions, @LisGein , @jolting ? I thought about a way to do it that would not be really that hard: we already have all CLoadabaleOptions equipped with save and load methods. If we extend the "save" methods here such that CConfigFileBase keeps an internal map of variables to types (more discussion on "types" required....), we could just create any CLoadableOptions structure, let it be its default values, then "save" (into a dummy memory config file, for example), and we will have the list of all variables and their types. Via TEnumType we have access to values of enum types, conversion of numerical values to their corresponding strings, etc.

If we want to extend this with valid value ranges and such, we'll need to modify all existing CLoadableOption classes, but if we leave that as optional, we would have, right now, without any important code change elsewhere, the list of data members and their type... enough for dynamically generating the UIs for any CLoadableOptions on the fly. It would be great, especially thinking of maintainability :-)

Thoughts?

@LisGein : On your last question, I think I don't get it, sorry... Each map has a set of parameters for:

Let me know if this helps to your doubt...

Cheers

LisGein commented 7 years ago

Hi! I agree that a more general way to viewing and editing properties is a good idea. But the current solution is simple and works. So I think that can be done after the other main features from my proposal will be implemented.

Cheers

jlblancoc commented 7 years ago

Sure! Firstly, focus on the rendering of all kind of maps, mouse operations like selecting a pose, etc

Let me know if you have a problem with anything of this, so you don't waste time looking for mrpt documentation.

jlblancoc commented 7 years ago

Hi @LisGein !

I browsed your code and all I see is fine... I like you already addressed the re-factorization of the wx vs. Qt OpenGL widgets. Good!

Just one question: aren't these two sets of parameters duplicated?

LisGein commented 7 years ago

Hi @jlblancoc! Yes, I thought to stay only the CamaraParams structure, but for this it is necessary in the whole project to make use the functions of getters. What do you think about it?

jlblancoc commented 7 years ago

Since we are aiming at the 2.0 version of mrpt... it may be acceptable, if in that way all the camera parameters are more centralized and accessed via getter/setters instead of being plainly exposed...

LisGein commented 6 years ago

Hi! Before the pull request I will add getters and setters in the CGlCanvasBase.

I have added:

jlblancoc commented 6 years ago

Great progress Alice!

I would only recommend changing the dots of poses to another visualization that shows the heading. The simplest is mrpt::opengl::stock_objects::simplexyzcorner.

Note that pull requests should go now against master. Then we'll probably have to pass the new clang-format parser to your code, so it would save time and problems if you make your code conform to the new clang format file that you can find at the root directory in master.

Cheers

LisGein commented 6 years ago

Hello!

I changed the dots.

I have a few questions:

Cheers

jlblancoc commented 6 years ago

I think for 3D maps doesn't need minimap. Can I don't make this?

Ok, it's fine!

I wrote in my proposal: "8.2 Software should have tools for editing robot poses: 8.2.1 Adding on a map."

Adding is not necessary, ok. Changing the pose, that is useful ;-)

You said, "pull requests should go now against master". What is the right way to do this with git?

Using the GitHub web interface, it's super easy: just view your branch online and clicking on "pull request", it will automatically propose you to open the PR against master.

LisGein commented 6 years ago

When should I do pull requests?

I added several features:

Now, I am making a multi selection(with a rectangle and with a ctrl).

Can you check the code in CGlWidget::mousePressEvent? When I click a pose in the corner of the scene, I see a small error(about 10 pixels). How can I fix it?

jlblancoc commented 6 years ago

Good amount of work!

In your case, I think it makes sense to open a PR against master now, so we see the output of travis and such.

I'll take a look at the code you mention...

LisGein commented 6 years ago

Hi! Sorry for the long silence. I have done multi selection with ctrl and I added in the general settings setting for robot poses and selected robot poses(size and colour). Now, I'm preparing a PR. Today I am doing it.

Cheers :)