-
Notifications
You must be signed in to change notification settings - Fork 872
SuperPoint #1603
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
base: master
Are you sure you want to change the base?
SuperPoint #1603
Conversation
…ally by the superpoint model
…ints is lower then the k value (scripting fails in this case)
| kFeatureSurfDaisy=14, //new 0.20.6 | ||
| kFeaturePyDetector=15}; //new 0.20.8 | ||
| kFeaturePyDetector=15, //new 0.20.8 | ||
| kFeatureSuperPointRpautrat=16}; |
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.
Bump the patch version here
Line 23 in a307639
| SET(RTABMAP_PATCH_VERSION 2) |
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.
Bumped the patch and added a comment, (0.23.3)
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.
These changes seem unrelated to the new approach
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.
I found these changes are necessary to get the old SuperPoint to work. They are unrelated to the focus of this MR so I can remove them if preferred.
| # | ||
| # Drop this file in the root folder of SuperPoint git: https://github.com/rpautrat/SuperPoint | ||
| # To use with rtabmap: | ||
| # --Vis/FeatureType 15 --Kp/DetectorStrategy 15 --PyDetector/Path "~/SuperPoint/rtabmap_superpoint_rpautrat.py" |
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.
| # --Vis/FeatureType 15 --Kp/DetectorStrategy 15 --PyDetector/Path "~/SuperPoint/rtabmap_superpoint_rpautrat.py" | |
| # --Vis/FeatureType 16 --Kp/DetectorStrategy 16 --PyDetector/Path "~/SuperPoint/rtabmap_superpoint_rpautrat.py" |
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.
This is the python impl which relies on the detector strategy to be 15 (Pydetector), so I believe that this is correct.
| image = np.asarray(imageBuffer) | ||
| image = (image.astype('float32') / 255.) | ||
|
|
||
| # image_tensor = torch.from_numpy(image[None, None]).float().to(device) |
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.
cleanup comments
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.
Whoops, removed the comment.
| const bool & cuda) | ||
| { | ||
| // Resolve paths | ||
| const std::string srcDir = UDirectory::getDir(__FILE__); // folder of this .cpp at build time |
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.
That won't work when rtabmap is installed and/or deployed. I think the best would be to include superpoint_to_torchscript.py as a "resource" in rtabmap library. That can be done by appending superpoint_to_torchscript.py to RESOURCES here
rtabmap/corelib/src/CMakeLists.txt
Line 829 in a307639
| SET(RESOURCES |
The way it works afterwards is that you can include the resource file and decode it into a string:
#include "superpoint_to_torchscript_py.h"
[...]
std::string file_content = uHex2Str(SUPERPOINT_TO_TORSCRIPT_PY);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.
Updated to load the file as a resource. I'm generating the loaded .py file in the rtabmap working dir, but it is cleaned up as soon as the model file is generated.
| << cuda_flag; | ||
|
|
||
| UINFO("Executing: %s", cmd.str().c_str()); | ||
| const int code = std::system(cmd.str().c_str()); |
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.
Not sure if the windows CI will pass with this, and I would try to stay away from system calls for security reason. Could pybind be used to call the conversion script?
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.
I'm now executing using pybind11 through runpy.
auto runpy = pybind11::module_::import("runpy");
runpy.attr("run_path")(dstScript, pybind11::arg("run_name") = "__main__");
corelib/src/CMakeLists.txt
Outdated
| SET(SRC_FILES | ||
| ${SRC_FILES} | ||
| superpoint_torch/SuperPoint.cc | ||
| superpoint_rpautrat/SuperpointRpautrat.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.
SuperpointRpautrat may be added also only if we build with python too.
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.
Now only add superpoint rpautrat if we have python and torch.
| RTABMAP_PARAM(SuperPoint, NMSRadius, int, 4, uFormat("[%s=true] Minimum distance (pixels) between keypoints.", kSuperPointNMS().c_str())); | ||
| RTABMAP_PARAM(SuperPoint, Cuda, bool, true, "Use Cuda device for Torch, otherwise CPU device is used by default."); | ||
|
|
||
| RTABMAP_PARAM_STR(SuperPointRpautrat, Dir, "", "[Required] SuperPoint root directory containing scripts and weights."); |
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.
I would suggest to use a parameter for the pth file directly: SuperPointRpautrat/modelPath, instead of a directory. See my other comment about which directory to use to output the converted weights.
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.
I've changed it so that we expect the path to the .pth weights file instead of the SuperPoint repo. I originally expected the SuperPoint dir because we need to set the PYTHONPATH there in order for the superpoint_pytorch.py python module import to resolve.
Currently, the code still makes assumptions about the structure of the SuperPoint directory in order to set the PYTHONPATH correctly.
// Add both the weights directory and its parent (repo root) to sys.path
// This assumes a certain direcotry structure: /SuperPoint/weights/superpoint_v6_from_tf.pth
std::string weightsDir = UDirectory::getDir(weightsPath);
std::string repoRoot = UDirectory::getDir(weightsDir);
auto sys = pybind11::module_::import("sys");
auto sysPath = sys.attr("path").cast<pybind11::list>();
sysPath.append(repoRoot);
sysPath.append(weightsDir);The most robust solution would be to expect a path to the SuperPoint model class AND the weights file as params. I can implement this if requested.
corelib/src/Features2d.cpp
Outdated
| bool previousCuda = cuda_; | ||
| #endif | ||
|
|
||
| Parameters::parse(parameters, Parameters::kSuperPointRpautratDir(), superpointDir_); |
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.
The output file could be generated in the RTABMap's working directory: available by parsing Rtabmap/WorkingDirectory parameter.
superpointDir_ = Parameters::parse(parameters, Parameters::kRtabmapWorkingDirectory(), "");As suggested above, you may have also to add:
Parameters::parse(parameters, Parameters::kSuperPointRpautratModelPath(), superpointModelPath_);
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.
I'm now forwarding the kRtabmapWorkingDirectory parameter to Features2D and generating the model file there.
corelib/src/Features2d.cpp
Outdated
| else | ||
| { | ||
| superPoint_->setSuperpointDir(superpointDir_); | ||
| superPoint_->setThreshold(threshold_); | ||
| superPoint_->setNMS(nms_); | ||
| superPoint_->setMinDistance(minDistance_); | ||
| } |
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 may remove these setters because they are not really supported. The model is initialized on the first frame, so any changes here won't matter, the whole model should be re-generated again to use of the new parameters.
It seems that if any parameter changes, we need to delete the previous detector to force it re-initialize on the next frame.
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.
I've removed the setters, we will now regenerate the detector when any parameter changes.
guilib/src/ui/preferencesDialog.ui
Outdated
| p, li { white-space: pre-wrap; } | ||
| </style></head><body style=" font-family:'Ubuntu'; font-size:11pt; font-weight:400; font-style:normal;"> | ||
| <p style=" margin-top:12px; margin-bottom:12px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;">SuperPoint C++ implementation from the <a href="https://github.com/rpautrat/SuperPoint"><span style=" text-decoration: underline; color:#0000ff;">rpautrat/SuperPoint</span></a> project.</p> | ||
| <p style=" margin-top:12px; margin-bottom:12px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;">A superpoint_v6_cpp.pt model file will be generated automatically by using the SuperPointRpautrat detector as long as RTAB-Map is built with python support.</p> |
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.
| <p style=" margin-top:12px; margin-bottom:12px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;">A superpoint_v6_cpp.pt model file will be generated automatically by using the SuperPointRpautrat detector as long as RTAB-Map is built with python support.</p> | |
| <p style=" margin-top:12px; margin-bottom:12px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;">A "pt" model file with same name than the "pth" file will be generated automatically in RTAB-Map's working directory so that the model can be called by c++ libtorch directly.</p> |
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.
Also: "RTAB-Map should be built with libtorch and python to use this feature."
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.
I updated the info text in the GUI. The MR comment above is updated as well.
guilib/src/ui/preferencesDialog.ui
Outdated
| </style></head><body style=" font-family:'Ubuntu'; font-size:11pt; font-weight:400; font-style:normal;"> | ||
| <p style=" margin-top:12px; margin-bottom:12px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;">SuperPoint C++ implementation from the <a href="https://github.com/rpautrat/SuperPoint"><span style=" text-decoration: underline; color:#0000ff;">rpautrat/SuperPoint</span></a> project.</p> | ||
| <p style=" margin-top:12px; margin-bottom:12px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;">A superpoint_v6_cpp.pt model file will be generated automatically by using the SuperPointRpautrat detector as long as RTAB-Map is built with python support.</p> | ||
| <p style=" margin-top:12px; margin-bottom:12px; margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px;">To generate the TorchScript model file manually, clone the repository at <a href="https://github.com/rpautrat/SuperPoint"><span style=" text-decoration: underline; color:#0000ff;">https://github.com/rpautrat/SuperPoint.</span></a> Copy the script from <span style=" font-family:'monospace'; background-color:#ffff99;">corelib/src/superpoint_rpautrat/superpoint_to_torchscript.py</span> into the SuperPoint repo. Then run the script with the desired parameters, e.g.<br /><span style=" font-weight:600;">python superpoint_to_torchscript.py --width WIDTH --height HEIGHT --weights weights/superpoint_v6_from_tf.pth --output weights/superpoint_v6_cpp.pt</span><br /></p></body></html></string> |
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.
I would remove this. We won't support the manual way.
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.
Removed!
| _ui->comboBox_detector_strategy->setItemData(16, 0, Qt::UserRole - 1); | ||
| _ui->vis_feature_detector->setItemData(16, 0, Qt::UserRole - 1); |
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.
#if !defined(RTABMAP_TORCH) || !defined(RTABMAP_PYTHON)
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.
Now checking if RTAB-Map is built with Python and Torch everywhere for SuperPoint rpautrat to be used.
|
In the top CMakeLists.txt around here Lines 1455 to 1461 in a307639
Can you add a section for SuperPoint Rpautrat? Maybe like: IF(TORCH_FOUND AND WITH_PYTHON AND Python3_FOUND)
MESSAGE(STATUS " With SuperPoint Rpautrat = YES (License: MIT)")
ELSEIF(NOT WITH_TORCH)
MESSAGE(STATUS " With SuperPoint Rpautrat = NO (WITH_TORCH=OFF)")
ELSEIF(NOT WITH_PYTHON)
MESSAGE(STATUS " With SuperPoint Rpautrat = NO (WITH_PYTHON=OFF)")
ELSE()
MESSAGE(STATUS " With SuperPoint Rpautrat = NO (libtorch and/or python3 not found)")
ENDIF() |
|
Similarly to previous comment: the about dialog could also be updated to show if rtabmap is built with superpoint rpautrat For Parameters.cpp, we can also add the status to know if rtabmap is built with tht feature: rtabmap/corelib/src/Parameters.cpp Lines 659 to 664 in a307639
str = "With SuperPoint Rpautrat:";
#if defined(RTABMAP_TORCH) and defined(RTABMAP_PYTHON)
std::cout << str << std::setw(spacing - str.size()) << "true" << std::endl;
#else
std::cout << str << std::setw(spacing - str.size()) << "false" << std::endl;
#endif
|
…cannot support changing params after the model is constructed
|
@matlabbe I cannot add comments on the last two threads for some reason, but I addressed both of them. This MR should be ready for another round of review when you have time. |

Implements the pytorch based Superpoint implementation for keypoint and descriptor extraction:
https://github.com/rpautrat/SuperPoint.
I've also updated the standalone application GUI to include a superpoint rpautrat configuration window with instructions on how to generate the model weights manually.
