Closed jlblancoc closed 7 years ago
Student: @raghavendersahdev
Main mentor: @famoreno Backup mentors: @feroze
Thanks @jlblancoc @famoreno and @feroze for providing me with an opportunity to work on this project, I will start doing a background study on the requisite detectors/descriptors in the coming week. I would also select and understand in detail the Place recognition, Visual Odometery and SLAM algorithm to be used for the benchmarking later in the project. Please let me know about anything else I should be doing during this initial phase period.
Hi @raghavendersahdev, how is the background study going so far?
In case you haven't had a look to it yet, I recommend you to start by getting familiar with the MRPT's CFeatureExtraction class since new detectors/descriptors will be added to it, as well as the AKAZE detector, LATCH descriptor and LSD/BLD line detector/descriptor in the OpenCV.
Hi @famoreno thanks for letting me know about this. The background study is going good so far.
Hi @raghavendersahdev !
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.
@raghavendersahdev and mentors: just mentioning that this message in another project highlights what part of that project is shared work with this one, requiring synchronization and public discussion between the two students and mentors.
Hey @raghavendersahdev !
Great to hear you're already working on the GUI skeleton :+1:
Benchmarking against KITTI is a good idea. Any particular reason for choosing SPTAM? I think it uses BRIEF descriptors.
Hi @feroze , Thanks for the reply, I choose SPTAM as it provides the source code to choose a combination of detectors / descriptors https://github.com/lrse/sptam/blob/master/src/ros/sptam_node.cpp (line 152-178). Would you suggest using a different algorithm? No other specific reason for choosing SPTAM. Also I have used it for localization on a robot before (i am roughly familiar with its code-base). Hence the choice, integrating it with our GUI app might be easier (without any ROS being involved). Other possible options might be ORB-SLAM, S-LSDSLAM, etc. I went through exiting open source Visual Odometry algorithms on http://www.cvlibs.net/datasets/kitti/eval_odometry.php .We could choose one from https://www.openslam.org/ too.
Another interesting way might be to extend this code https://github.com/avisingh599/mono-vo/tree/master/src . This provides a simple monocular feature tracking/matching based VO in subsequent frames, we could have a combination of detector / descriptors incorporated in this code. Currently this code uses FAST descriptors only. This might be a better option than SPTAM. Please let me know what you think about this.
Hi everybody, I'm back! :D Sorry for the delay but I've been in a trip in Morocco for the last days and my internet conection was REALLY limited (let's say that :P). These are my comments:
I agree with @feroze about using KITTI for benchmarking.
Regarding the visual odometry and visual SLAM techniques: There are several options to be considered here when trying to test different combinations of detectors/descriptors for SLAM and/or VO. SPTAM is OK for me but, as far as I know, it will only work for stereo datasets using keypoints (neither for monocular datasets nor for line-segment features), so I think it would interesting to also provide other systems that can deal with such kinds of data. So I will also have a look to the monocular version of ORB-SLAM (although I think it will only work with binary descriptors) and try to find some code for methods intended for line-segment features (maybe a good start would be this).
Let me know what you think about this.
Thanks @famoreno @feroze for the suggestion. I forked the mrpt code, I have made a small GUI skeleton (https://github.com/raghavendersahdev/mrpt/tree/master/apps/benchmarking-image-features ), I still need to add more contents to it in terms of evaluation metrics, detector/descriptor parameters, thresholds, more detectors/descriptors, etc. visual odometery. Could you let me know if this looks okay for now? I will update this code more in the next few days adding more content to the GUI skeleton framework. Hi @famoreno , I agree with you , SPTAM is limiting in that sense - only for stereo datasets, we should have something for monocular datasets too. Maybe ORBSLAM if it works with other descriptors too. I will go over this ( https://github.com/rubengooj/StVO-PL ) in the next few days.
correction, my code is here in the mrpt-2.0-devel branch not the master branch.
Hi @famoreno @feroze I read the documentation for the CFeatureExtraction class and went over the feature-matching app and the feature-extraction sample in mrpt. I wanted to check with you if I am on the right track, I am currently under the impression that I need to add the features(detectors/descriptors) existing in the mrpt and write classes for detectors and descriptors like AKAZE detector, LATCH descriptor and LSD/BLD line detector/descriptor from OpenCV. Do I need to write classes similar to the ones here for these unimplemented detector/descriptors in the mrpt/libs/vision folder or in my benchmarking-features-app.
I had an issue with using the mrpt::gui::CDisplayWindow class with Qt and got the following error, I will try to fix it by tomorrow. (benchmarkingImageFeatures_GUI:15061): Gtk-WARNING **: gtk_disable_setlocale() must be called before gtk_init() [CBaseGUIWindow::ctor] Timeout waiting window creation. [CDisplayWindow::setPos] Window closed!: Segmentation fault (core dumped)
I have put the study on the VO/SLAM algorithm on standby for now as it is scheduled in week 7 as per my proposal.
CDisplayWindow uses wxWidgets. wxWidgets uses gtk+ as a backend. I think you are not properly initializing wx.
Mixing Qt and wxWidgets is not wise.
Thanks @jolting for your advice :+1: I wanted to display the output image with detected detectors/descriptors in the image so was using CDisplayWindow here in my code, as my app should support CFeatureExtraction class which is coded for it to show images with CDisplayWindow am I right here? But if mixing wxWidgets with Qt is not wise, I should probably use cv::imshow to display the image results, right? I am a bit confused here as to which combination I should use
Careful with the OpenCV gui stuff as well. It also uses gtk+. As long as you are not poping up GUI windows OpenCV should be fine with Qt.
If you plan on using Qt, you should convert the image to QImage and render it in a QLabel.
Thanks jolting I will convert into a QImage and render in a QLabel for displaying.
Hi @raghavendersahdev, as far as I know, and if @jlblancoc doesn't say otherwise, new detectors/descriptors should be added to the CFeatureExtraction class as the other items in here (CFeatureExtraction_*). In most cases they are just wrappers for their corresponding OpenCV's methods, so it shouldn't be very difficult to add new ones.
That is independent of the way of showing the images. When we proposed the project, we had in mind the development of a complete GUI with the images also displayed on the GUI and not on separate windows. Besides, taking into account that some MRPT classes are being ported to QT, as @jlblancoc posted before, I think that creating the whole GUI with QT should be the way to go.
Finally, having a look to this image, I would say that a pair of QImages should be displayed on it (using @jolting tips) and I would suggest using QComboBox classes instead of radio buttons for selecting the detectors and descriptors, as it would save space to be used for images and also to place the controls to tune the parameters for the selected detector/descriptor.
Thanks @famoreno I am more clear with the approach now. I will modify the current GUI as per your suggestions and display a pair of QImages and have parameters on the same window as per selection of detectors/descriptors from the QComboBox. I will update the code with these features soon.
I should be using the CImage, CFeatureExtraction, CFeatureList, etc.. like this to extract existing mrpt detectors /descriptors and then show the detected points on the pair of QImages along with options to select other parameters as per detector/descriptor, right?
👍
I agree with all the recommendations of Paco (famoreno)!
Also, did you have more cases of this error:
Timeout waiting window creation.
It's an old known bug of the wxWidgets subsystem in mrpt... all I can say it seems to be related to a race condition because it's soooo arbitrary and random. One more reason to port CDisplayWindow to a Qt backend!
Hi jiblancoc, thanks for letting me know about the bug, I get a Timeout waiting window creation error in following cases
I will convert the images cv::Mat or CImage to a QImage and put in a QLabel to display, I need to find out a way to show the detected points/features inside the image and store them as a cv::Mat and/or CImage before converting to QImage using this and/or this
Hi @raghavendersahdev, how is it going? It's been a while since your last update and I would like to know how the development of the software is going. Please, note that the first evaluation is near so it would be desirable to have some regular updates of your work, trying to comply with the project timeline.
Btw, did you manage to show the images+features? It looks like that just drawing them on the image as a cv::Mat should be enough.
Hi @famoreno , Sorry for not providing an update recently, Yes I was able to show the images+features as a QImage in QLabel in the GUI, I used opencv's methods to draw the keypoints which stores the image with detected keypoints in a cv:Mat. I am currently writing wrappers of the respective openCV features in the CFeatureExtraction class like for LSD/BLD line detector descriptors, AKAZE detector, Latch descriptor, etc. Here is a picture of the updated GUI design. I will commit the updates to the code by Friday night (June 16) incorporating the features and the respective parameters that can be selected by the user for each feature to get your feedback. and finish the part upto week 3 as per project time-line by this weekend June 18 night. I got caught up with a job interview assignment recently so was not able to focus on this.
Good to know that your work is going on properly, @raghavendersahdev. I'll have a look at it next week after your updates are committed.
I got caught up with a job interview assignment recently
Hope everything went great!
Hi @famoreno , Thanks, the job interview assignment went nice. I have committed my code to my fork here. Currently I have the following functionality in the app:
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.
For those of you creating new MRPT apps, remember to also create a basic manpage, a must-have for Debian packages. This includes:
.pod
file in doc/man-pages/podThanks @jlblancoc for the letting me know about it, I will start committing code on a regular basis. Hi @famoreno I am currently writing code for visualizing the descriptors, its is incomplete right now, this is how I am doing it currently. For the evaluation I will be finishing the project up-to week 4 as per my project time-line and a little stuff from week 5 if time permits. would that be fine?
Hi @raghavendersahdev, I've cloned and tried to compile your code but it seems that file script_qt.cmake
is missing but it is included in your app's CMakeLists.txt
. Please, upload it as soon as possible in order to test the code and evaluate it against the project timeline. Thanks!
Hi @famoreno I have uploaded the script_qt.cmake in the cmakemodules folder. The visualize descriptor button and the window is under construction currently, I have figured out how to display the PolarImage, Spin Image and Log Polar image descriptors as QImages in a separate window. I am working on the way to visualize the graphs of the SIFT and SURF descriptors, I should upload them by today. After visualizing the descriptor, I will move on to implementing the Opencv wrappers for the unimplemented detectors in the CFeatureExtraction class and adding functionality to activate/de-activate robust stereo matching, and fixing some bugs. Please note the current version of code, you need to specify image path or the dataset folder in the TextBox, not doing so results in the app crasing, I will fix this issue by prompting a QMessageBox warning the user, a sample run looks like:
@raghavendersahdev script_qt.cmake was already added. https://github.com/MRPT/mrpt/commit/03e67f4306f67eebe4916af7abdaf9200b41a269
Looks like you haven't synced your mrpt-2.0 branch for quite a while. We need you to preserve git commit history. It's a bad idea to simply copy files out of one git tree and commit them to another. Committing a fresh new copy can cause issues when doing your final merge. It basically makes it impossible for git to know which order these commits should be in. Next time apply the patch from @LisGein first. The easiest way is to just simply merge the current head of mrpt-2.0 branch into your tree. You will assuredly get a merge conflict now that you have to manually handle.
I recommend doing a merge now, just to ensure there are no issues going forward.
I was almost the day off the computer until now... and came to write exactly what @jolting warned you already!
But watching at the list of your commits, it seems clear to me that your commit history needs a clean up. Perhaps it's just that you're not used to distributed versioning control systems, so it's fine if that's the case. From now on, this is, roughly speaking, the golden rule of when, how often and what files to include in each commit: every set of changes that provide, altogether, a relevant change without introducing inconsistencies. For example, never add in one commit the .cpp file, then the .h in another one, etc.
How to move from now on: My recommendation is:
1) Make a copy of your local working directory (the cloned dir) of your fork of MRPT.
2) Delete your GH fork.
3) Make a fork again.
4) Clone your fork in your PC in a new empty directory.
5) manually, using the file explorer or the cmd line, etc., copy just the files that you know you've changed until now from your backup copy to the new cloned dir.
6) Execute git status
to check the differences.
7) Build and test, ...
8) When satisfied, do a commit with a description like: "work for date XXX to XXX: \n Added blah blah, etc. etc.".
@jolting , @famoreno , do you agree with this plan?
Thanks @jlblancoc actually I am not used to the correct way of using distributed version control systems. Normally what I do for committing is:
git add -A
git status
git commit -m "something like added descriptor visualization code"
git push origin mrpt-2.0-devel
However to add the script_qt.cmake file I used the git GUI to add a new file and commit which might be incorrect way.
Sometimes I commit too often in one day which might be a bad idea, I guess.I would probably just rebase the commits.
Rebasing rewrites git, which is a strict "no no" when other people depend on your commit history. This means if anyone else pulls from your tree, rebasing is BAD. On the other hand sometimes it's best to just start from scratch and although the commits have already been published to a personal git account, the chances of anyone pulling from that tree is small.
Essentially, what @jlblancoc is suggesting is to reapply your changes ("in git nomenclature read patches") on top of MRPT/mrpt-2.0-devel. This is basically what rebasing does. The best way is to do it is interactively, so you have more control over how that commit history is rewritten. You can also squash multiple commits that you have made into larger commits. That should help with committing small inconsistent commits.
See not that hard: https://github.com/jolting/mrpt/tree/raghavender-fixed
I rebased to MRPT/mrpt-2.0-devel, I dropped your CMakeLists.txt commit and I squashed your two SURF bug fix commits into one.
The first line is always a summary. I'm not certain what "SURF bug fix" means, but that can be a summary. A few more words might be good, so I have some context what changed. Writing a good commit message is an art form, which I'm still trying to improve on myself.
Thanks @jolting for the advice on rebasing, I will use more informative commit messages from my next commits.
Hi all! After all these changes I'm not sure about where is the repo with the on-going work :P.
I've cloned again @raghavendersahdev https://github.com/raghavendersahdev/mrpt/tree/mrpt-2.0-devel but I couldn't compile it. The last commit (8 hours ago) included files with conflicts that should be fixed manually (visualizedialog.cpp
and visualizedialog.h
).
So I cloned the version in @jolting (https://github.com/jolting/mrpt/tree/raghavender-fixed). I had to remove the line with #include <mrpt/system/threads.h>
in visualizedialog.cpp
as it is no longer part of the MRPT (as far as I know) to compile the application. I'm testing it but not sure if this is the current state of the software or not.
Hi @famoreno my current work is in here. I think I will follow @jlblancoc advice and start from scratch after deleting my old fork and creating a new fork. Should I do that ? as @jolting had warned I would have conflicts while merging with the remote master tree here . It says I cant automatically merge.
Merging using the terminal also results in a lot of conflicts
git pull origin HEAD mrpt-2.0-devel
The current version of the app supports visualizing Polar Image, Log Polar Image and Spin Image descriptors, the visualization of the SIFT and SURF descriptor is near completion. Things I want to do before the evaluation:
A sample execution of the app requires the user to
Hi @raghavendersahdev , I've been testing the app so far and here are my comments and recommendations:
Regarding the GUI
textedit
widget to insert a number instead of two +/- buttons.In general I imagined the GUI something similar to this sketch (sorry, not too fancy but illustrative :P):
It's just my view, but obviously it can be anything else as long as it is valid for our purposes. So use it for some inspiration, if you like. I would put all buttons and selections (detectors, descriptors, on the top right part), below it I would put all the common options: decimation, non-maximal suppression, robust stereo, etc. Below it, the specific options for the selected detector and descriptor (even use two tabs if you think it is necessary for saving space). Use the bottom of the app to put the previous frame, next frame controller (two buttons, a slider ...).
In the info
part I would show information about the result of the detection: number of detected points, dispersion over the image (std of their coordinates), minimum distance between keypoints, spent time, etc.
I would use the desc_plot
part as follows:
desc_plot
to represent the current value of the descriptor (maybe using a QCustomPlot).image1
, use desc_plot
for representing the distance between its descriptor and all the descriptors in the right, highlighting the two closest keypoints (in descriptor distance) in image2
.Regarding the code
Thanks @famoreno for your comments and recommendations :-) . I agree with most of them. I have a few questions about non-maximal suppression, clicking on key-points for descriptor visualization and evaluation deadline.
- It's too big (gets out of my screen) and cannot be resized. This causes some buttons can not be clicked.
- The space in the parameters area is not wisely employed IMHO. Consider to use different widgets or resize them to save space, eg: checkboxes for true/false parameters. There is no need for a full line of text edit to enter a single number like a threshold.
I need to use an option like autoresizing layout option in QT (using SizeContraintPolicy, SizePolicy, etc for the widgets) to handle this. I will do this. I totally agree with the spacing for parameters, I was planning to change the sizes and the layout of the placements in a better manner soon
I think that for image decimation there should be a textedit widget to insert a number instead of two +/- buttons.
Keypoint representation on the image is in black and it is barely noticeable. Use a more distinctive color.
I will change this
- If I select Single Image Dataset, I can't Evaluate detector as a window pops up saying that "Please specify a valid inputs for the image / detector."
Currently you need to click the next button once begin to iterate over the images and then click on the evaluate detector button for it to work. Clicking on next starts reading from the dataset folder specified. Next and previous buttons for single image dataset and stereo image dataset should work with the detectors. Only thing is that the user needs to click on next once to start
ORB descriptor should also be an option in the descriptors list. I will add this
Some features are still not implemented: non-maximal suppression, robust stereo matching
non maximal suppression should be implemented only for features which have an option to enable / disable it in CFeatureList, right? like this here I am currently doing robust stereo matching
It seems that BRISK, LSD/BSD are not implemented. As soon as I am done with the other stuff I will come back to this.
It crashes quite often in some combinations of detectors/descriptors. Actually I couldn't reproduce your sample of execution of the app.
The detectors AKAZE, LSD/BLD and Latch are yet to be implemented, I will back to this as soon as I finish the other stuff.
- I would use the desc_plot part as follows:
I will use the QCustomPlot instead of what I am currently using. Could you tell me how do I know which keypoint the user clicks in the QImage, what I was thinking is adding a next/previous button to iterate over the descriptors one by one? like I am doing here Would this be enough or specific key-points clicking in the QImage is a 'must' for visualization
Regarding Evaluation would it be fine if I aim to finish all this stuff by June 28? I will continue to commit every day till then.
I will also change the layout of the GUI for the app something similar to the picture you shared, so no popping up of an additional window, I will put the code from the visualizedescriptor.cpp
into the mainwindow.cpp
file, Actually no popping of an additional window would make it easier to fix a few bugs responsible for app crashing too...
Hi again!
Currently you need to click the next button once begin to iterate over the images and then click on the evaluate detector button for it to work. Clicking on next starts reading from the dataset folder specified. Next and previous buttons for single image dataset and stereo image dataset should work with the detectors. Only thing is that the user needs to click on next once to start
I think it would be better if the first image of the dataset was automatically placed. Actuallly, I couldn't click on the next button since it went out of my screen.
non maximal suppression should be implemented only for features which have an option to enable / disable it in CFeatureList, right? like this here I am currently doing robust stereo matching
Well, as far as I know, some detectors already include the non-max-suppression
option in their opencv method (e.g. FAST) but not all of them. However, in MRPT other detectors implement a sort-of non-max suppression that is called when the option min_distance > 0
(see here, for example). Others like SIFT or SURF do not implement this option so non-max-suppresion
won't be available for these.
will use the QCustomPlot instead of what I am currently using.
The QCustomPlot is just a suggestion! but use the opencv visualization instead if you feel more comfortable with it (unless any of the other mentors says otherwise).
Could you tell me how do I know which keypoint the user clicks in the QImage, what I was thinking is adding a next/previous button to iterate over the descriptors one by one? like I am doing here Would this be enough or specific key-points clicking in the QImage is a 'must' for visualization
Have a look at event filters to get the position of the mouse when clicked and look for the closest keypoint in the list of keypoints.
And regarding the evaluation, I'm not the only one to decide about it. It is something we have to discuss between the mentors and orgs during these days. In any case, keep focused on meeting the project's schedule by week 4 and make sure that your code comply with what's stated in the Ideas page for the first evaluation.
RE: QCustomPlot
@LisGein pointed out some licensing issues.
See https://github.com/MRPT/mrpt/issues/107
QCustomPlot is GPL3. qwt is lgpl. Lets use qwt instead.
Ops! I didn't know about the license issue of QCustomPlot. I agree on the qwt then. Thanks for the tip @jolting.
Hi @famoreno , thanks for your recommendations, I will clean up the code before evaluation and will adhere to the requirements. Few updates:
min_distance
to be entered by the user. So that finishes non-maximal part, I believe ?Current app includes features with respect to proposed timeline:
Week 1: Skeleton GUI - finished
Week 2: Detectors/Descriptors added from MRPT with options to tune the detector/descriptor specific parameters - finished. Few more need to be added from opencv in the CFeatureExtraction Class, will try to implement some before evaluation June 28.
Week 3-4: activate/deactivate non-maximal suppression, image decimation, forward and backward step by step play-back of images, application of detector/descriptor to specific images - finished. robust stereo matching is still left, will do it before evaluation (June 28), is it simply showing a stereo disparity image?
Week 5: descriptor distances between matches is currently in moderate shape, need to find a neat way of showing it. I just checked this is actually a part of week 5, but I almost figured it partly (need to add more text/information to the graph). :-) . Other evaluations like computational cost, number of matches, distances, dispersion, repeatability is scheduled in week 5.
Week 6: Nothing scheduled: A Buffer week to finish piled up work and bug fixes.
So I think, I only need to finish robust stereo matching and OpenCV wrappers before evaluation and fix some bugs leading to app crashing. I have the reduced the number of crashes than in the previous commit*. There is a currently a bug which leads to overlaying a QLabel on top of another for showing the Descriptor matching information. This leads to non-readable information. I will fix this in the next commit before evaluation.
A sample demo of the app requires:
Robust stereo matching for me is to only consider a match between keypoint "i" in image 1 and keypoint "j" in image 2 if the descriptor distance between "i" and "j" is the minimum among all distances between "i" and all the keypoints in image 2 AND the distance between keypoint "j" and "i" is the minimum among all distances between "j" and all keypoints in image 1. It's like a double check of being a good match.
Thanks for letting me know about this, actually then robust stereo matching is currently done for one key-point in image1 to find the best match in image2. For every key-point in image1, I compute the descriptor distances of all key-points in image 2 and I need to display the shortest distance key-point on the image (I need to do this displaying of the matching key-point in image 2, I have all the info stored in variables, just need to display them), I have the plot currently. I will add some some more information on the images and beside the plot and then that should conclude robust stereo matching also. Also I only tested for SURF descriptor currently, but should be very similar for other descriptors too. Will finish this today. So most importantly I think I need to fix some bugs and opencv wrappers.
Hi @famoreno I have added all of the functionality in the GUI app :-) except writing wrappers for AKAZE, LSD Detectors and BLD, Latch Descriptors. As per the time line only these are left. Current app includes features as stated here. Few more updates:
I have a buffer week in week 6 where I have nothing scheduled (meant for finishing pending work if any), Maybe I can finish these wrappers by tomorrow June 29. I will also use this week to address any of your suggestions, recommendations, documentation and start on week 7 (Visual Odometry) stuff in week 6. Does this sound okay?
Should I make a pull request for the evaluation without these wrappers (2 detectors+2descriptors from openCV) or after implementing these? . I think I should be able to finish the wrappers and week 5 goals by week 5 end, So I can start with Visual Odometry from week 6 only instead of 7. What do you think?
Excellent progress!
I think Paco is able of testing your code from your fork, so there's not a real need for a PR at this point, unless Paco says otherwise.
Cheers
Initial description:
In this proposal we aim to build a GUI application which has the ability to benchmark various combinations of detectors/descriptors.
See attached PDF: 6307731490209792_1491218516_Raghavender_GSoC2017_proposal.pdf
Final results:
Videos: