Clarify the functional correspondence between the ORB_SLAM2 code and OpenVSLAM code and evaluate the similarities #239
Comments
ymd-stella
changed the title
Frame - data::frameconstructorsThe natural order is from feature point extraction to stereo matching to grid assignment of feature points.
Overall, the similarities are negligible. AssignFeaturesToGrid in Frame - assign_keypoints_to_grid in commonIt is related to
Overall, the order of processing is natural, and no similarities in expression were found. ExtractORB in Frame - extract_orb in frameThere is a very unnatural similarity between the two. It is natural to simply implement extract_orb_left and extract_orb_right or use lambda expression. It needs to be removed. SetPose in Frame - set_cam_pose in frameIt is related to
Overall, the order of processing was natural, and no similarities in expression were found. isInFrustum in Frame - can_observe in frameConsidering the computational efficiency, the processing order looks natural. There is no similarity other than the order of processing. ComputeBoW in Frame - compute_bow in frameIt is unnatural to return without doing anything when bow_vec_ is not empty. However, they are similar. It needs to be removed. UndistortKeyPoints in Frame - compute_subpixel_disparity in camera::perspectiveIt's similar order of processing. However, the order of processing is natural. There are ways to avoid overwriting the result in mat, but this is the natural implementation from the perspective of memory efficiency. ComputeImageBounds in Frame - compute_image_bounds in camera::perspectiveNo similarities in expression were found. (Off topic, but it is more natural to use Point2f instead of KeyPoint.) ComputeStereoMatches in Frame - compute in matcher::stereomatcher::stereo::compute calls
The same stereo matching algorithm is used. There are similarities, but I think it is due to the fact that they share the same algorithm. ComputeStereoFromRGBD in Frame - compute_stereo_from_depth in frameIt is similar, but very simple and there is no other way to express it. UnprojectStereo in Frame - triangulate_stereo in frameIt is similar, but very simple and there is no other way to express it. |
KeyFrame - data::keyframeconstructorsThe similarities are negligible. ComputeBoW in KeyFrame - compute_bow in keyframeIt is unnatural to return without doing anything when bow_vec_ and bow_feat_vec_ is not empty. However, they are similar. It needs to be removed. SetPose in KeyFrame - set_cam_pose in keyframeThere are similarities, but very simple and there is no other way to express it. Cw (Stereo middel point) exists only in ORB_SLAM2. pose accessors
There are similarities, but very simple and there is no other way to express it. AddConnection, UpdateBestCovisibles in KeyFrame - add_connection, update_covisibility_orders in graph_nodeThe suspects are as follows
Overall, the similarities are negligible. However, I think it is natural to keep the mutex lock on during add_connection. keyframe getters and related data structures
I'm curious about the similarity of ordered_covisibilities_ and ordered_weights_. It is natural to use landmark accessorssetters
There are similarities, but very simple and there is no other way to express it. (Off topic, in ORB_SLAM2, EraseMapPointMatch and ReplaceMapPointMatch are not mutex locked. Is this a bug?) getters
There are similarities, but very simple and there is no other way to express it. UpdateConnections in KeyFrame - update_connections in graph_nodeThere are similarities, but I think it is due to the fact that they share the same algorithm. accessors for graph
There are similarities, but very simple and there is no other way to express it. Life Span Management
The similarities are negligible. GetFeaturesInArea in KeyFrame - get_keypoints_in_cell in commonThe order of processing is natural, and no similarities in expression were found. IsInImage in KeyFrame - reproject_to_image in perspectiveThe only similarity is in the last conditional expression, and there is no other way to express it. UnprojectStereo in KeyFrame - triangulate_stereo in keyframeThere are similarities, but very simple and there is no other way to express it. ComputeSceneMedianDepth in KeyFrameThere are similarities, but very simple and there is no other way to express it. |
MapPoint - data::landmarkconstructorsThe similarities are negligible. accessors
In SetWorldPos, mGlobalMutex is used. This is locked during pose optimization, which is not present in OpenVSLAM. GetMinDistanceInvariance and get_min_valid_distance have different magic numbers, but the basis for both is unknown. The same is true for GetMaxDistanceInvariance and get_max_valid_distance. There are similarities, but very simple and there is no other way to express it. Life Span Management
The similarities are negligible. add, erase observation
There is a process to update the reference keyframe with the first observed keyframe. We could use any other keyframe in Replace in MapPoint - replace in landmarkThere are similarities, but very simple and there is no other way to express it. The order of processing is natural. ComputeDistinctiveDescriptors in MapPoint - compute_descriptor in landmarkComputeDistinctiveDescriptors returns when vDescriptors is empty, while OpenVSLAM does not. The order of processing is natural. There are similarities, but I think it is due to the fact that they share the same algorithm. UpdateNormalAndDepth in MapPoint - update_normal_and_depth in landmarkThere are similarities, but very simple and there is no other way to express it. This naming is a bit unnatural for the description PredictScalein MapPoint - predict_scale_level in landmarkIt is unnatural to use overloading just to get log_scale_factor_ and num_scale_levels_. It needs to be removed. |
Map - data::map_databaseAddKeyFrame in Map - add_keyframe in map_databaseThe process using max_keyfrm_id_ is unnatural, and it is better to remove the process using max_keyfrm_id_. add/erase keyframe/landmark
It is similar, but very simple and there is no other way to express it. InformNewBigChange, GetLastBigChangeIdx in Map - none?No similar function was found. accessors
It is similar, but very simple and there is no other way to express it. GetMaxKFid in Map - get_max_keyframe_id in map_databaseThe process using max_keyfrm_id_ is unnatural, and it is better to remove the process using max_keyfrm_id_. clear in Map - clear in map_databaseThe process of assigning nullptr is unnatural. It needs to be removed. |
KeyFrameDatabase - data::bow_databaseadd, erase in KeyFrameDatabase - add_keyframe, erase_keyframe in bow_databaseIt is similar, but very simple and there is no other way to express it. clear in KeyFrameDatabase - add_keyframe in bow_databaseThe similarities are negligible. search
There are similarities, but I think it is due to the fact that they share the same algorithm. |
Viewer, MapDrawer, FrameDrawer - pangolin_viewer, publish::*Overall, the similarities in code are negligible. However, I am concerned about the similarity in screen design. I am not sure if that is covered by GPLv3. The colors are just one of the customizable setting values, so I don't think there is a problem. It might be better to determine the default setting values specific to OpenVSLAM. The content displayed at the bottom of the frame is not very similar. As for the drawing of the overlay, it is too similar and would be better to redesign it. For OpenVSLAM, it is natural to separate the statistics and status from frame_publisher and create another publisher. It is needed to change the default color scheme and remove the overlay text. |
System - system, io::trajectory_ioconstructorThe similarities are negligible. The necessity of displaying the copyright is unclear. It may be enough to provide a function like show_copyright_notice. For example, it will show the notice when the Feed an image
The similarities are negligible, except for the handling of resets. Reset is explained in detail in the next section. Reset in System - request_reset in systemOverall, the process for reset has some very unnatural similarities, and the functions related to request_reset need to be removed. accessors for modes/state
I think it is better to use Shutdown in System - shutdown in systemI think it is better to use Save trajectory
The similarities are negligible. getters
There is no similar getters. map_publisher provides landmarks, but in a completely different expression. |
Initializer - solve::*, initialize::*Initialize in Initializer - initialize in initialize::perspectiveThere are similarities, but it is due to the fact that they share the same algorithm. RANSAC
There are similarities, but it is due to the fact that they share the same algorithm. ComputeH21 in Initializer - compute_H_21 in solve::homography_solverThere are a bit of similarities, but it is due to the fact that they share the same algorithm. ComputeF21 in Initializer - compute_F_21 in solve::fundamental_solverThere are a bit of similarities, but it is due to the fact that they share the same algorithm. Check inlisers
There are a bit of similarities, but it is due to the fact that they share the same algorithm. Decompose
There are a bit of similarities, but it is due to the fact that they share the same algorithm. Check pose
In OpenVSLAM, bearings are used instead of keypoints. There are a bit of similarities, but it is due to the fact that they share the same algorithm. Normalize in Initializer - normalize in solve::commonThere are a bit of similarities, but it is due to the fact that they share the same algorithm. |
Tracking - tracking_module, module::frame_tracker, module::keyframe_inserter, module::local_map_updater, module::relocalizer, module::initializerconstructorThe similarities are negligible. setters
It is similar, but very simple and there is no other way to express it. Feed an image
Related functions are as follows:
There is a slight similarity. The frame is initialized in a different way for each camera configuration and passed to the common process. The similarity is due to the efficiency of the implementation. In monocular, the use of a feature point extractor for initialization is also similar, due to the fact that it is better to use more feature points to get better/stable results. Overall, no problematic similarities were found. Initialization
There are similarities, but I think it is due to the fact that they share the same algorithm. CheckReplacedInLastFrame in Tracking - apply_landmark_replace in tracking_moduleIt is similar, but very simple and there is no other way to express it. Track in Tracking - track in tracking_moduleThe first few lines are similar, but the similarity after that is negligible. For example, only ORB-SLAM2 performs VO. It also has a conditional branch depending on whether mapping is enabled or disabled, and the fallback from TrackWithMotionModel to TrackReferenceKeyFrame is different. On the other hand, OpenVSLAM does almost the same thing regardless of whether mapping is enabled or disabled. Tracking by reference keyframe or previous motion
There are similarities, but very simple and there is no other way to express it. We will consider is_observable_in_tracking_ and identifier_in_local_lm_search_ in detail next(search_local_landmarks). Search local landmarks
The following variables are temporary variables, but they are declared as member variables. Update local map
There are similarities, but I think it is due to the fact that they share the same algorithm. We need to remove the member variables used as temporary variables described above. TrackLocalMap in Tracking - optimize_current_frame_with_local_map in tracking_modulenum_tracked_lms_ is a member variable that is used as a temporary variable and is unnaturally similar. num_tracked_lms_ needs to be removed. UpdateLastFrame in Tracking - update_last_frame in tracking_moduleThe similarities are negligible. Insert new keyframe
There are similarities, but I think it is due to the fact that they share the same algorithm. Relocalization in Tracking - relocalize in module::relocalizerThere are a bit of similarities, but I think it is due to the fact that they share the similar algorithm. Reset in Tracking - reset in tracking_moduleOverall, the processes related to request_reset have some very unnatural similarities, and the functions related to request_reset need to be removed. ChangeCalibration - noneNo similar function was found. InformOnlyTracking in Tracking - set_mapping_module_status in tracking_moduleIt is similar, but very simple and there is no other way to express it. |
LocalMapping - mapping_module, module::local_map_cleaner, module::two_view_triangulatorsetters
It is similar, but very simple and there is no other way to express it. Run in LocalMapping - run, mapping_with_new_keyframe in mapping_moduleThere are a bit of similarities, but I think it is due to the fact that they share the same algorithm. In OpenVSLAM, when this module receives a pause request, it processes all the keyframes of the queue. ProcessNewKeyFrame in LocalMapping - store_new_keyframe in mapping_moduleThere are similarities, but very simple and there is no other way to express it. CreateNewMapPoints in LocalMapping - create_new_landmarks, triangulate_with_two_keyframes in mapping_moduleThere are similarities, but I think it is due to the fact that they share the same algorithm. In OpenVSLAM, it is parallelized and extended for Equirectangular image (in two_view_triangulator::check_depth_is_positive). SearchInNeighbors in LocalMapping - update_new_keyframe in mapping_moduleThere are similarities, but I think it is due to the fact that they share the same algorithm. ORB-SLAM2 uses temporary variables as described in "Search local landmarks", while OpenVSLAM uses unordered_set. Keyframe queue
It is similar, but very simple and there is no other way to express it. Remove reduncant landmarks/keyframes
There are a bit of similarities, but I think it is due to the fact that they share the same algorithm. ComputeF12 in LocalMapping - create_F_21 in solve::fundamental_solverThere are a bit of similarities, but very simple and there is no other way to express it. keyframe acceptability
There are a bit of similarities, but very simple and there is no other way to express it. OpenVSLAM uses atomic, while ORB-SLAM2 uses mutex. InterruptBA in LocalMapping - abort_local_BA in mapping_moduleThere are a bit of similarities, but very simple and there is no other way to express it. I'm a bit concerned that it doesn't have an exclusive lock. Reset
Overall, the processes related to request_reset have some very unnatural similarities, and the functions related to request_reset need to be removed. Pause
There are similarities, but very simple and there is no other way to express it. Release in LocalMapping - resume in mapping_moduleThere is a slightly unnatural similarity. There is no reason to clear the keyframe queue when resuming. At I think we should lock mtx_keyfrm_queue_ and then pause and clear the queue. During pause, keyframe insertion is stopped, so no additional keyframes will be added to the queue. See https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/module/keyframe_inserter.cc#L29-L31 The line needs to be removed. Then make the above changes in mapping_module::run. Finish
It is similar, but very simple and there is no other way to express it. |
LoopClosing - global_optimization_module, module::loop_bundle_adjuster, module::loop_detectorsetters
It is similar, but very simple and there is no other way to express it. Run in LoopClosing - run in global_optimization_moduleThere are a bit of similarities, but very simple and there is no other way to express it. Keyframe queue
It is similar, but very simple and there is no other way to express it. However, extra parentheses should be removed. DetectLoop in LoopClosing - detect_loop_candidates, compute_min_score_in_covisibilities, find_continuously_detected_keyframe_sets in loop_detectorThe repetition of ComputeSim3 in LoopClosing - validate_candidates, select_loop_candidate_via_Sim3 in loop_detectorIn select_loop_candidate_via_Sim3, As with "Relocalization in Tracking - relocalize in module::relocalizer", the use of computational resources in RANSAC is different. In validate_candidates, the process of allowing keyframes to be erased is repeated, but it can be merged into one. This is unnaturally similar. It needs to be removed. No other unnatural similarities were found. CorrectLoop, RunGlobalBundleAdjustment, SearchAndFuse in LoopClosing - correct_loop, replace_duplicated_landmarks in global_optimization_module, optimize in loop_bundle_adjusterThe following variables are temporary variables, but they are declared as member variables. These are advantageous in terms of execution speed, but disadvantageous in terms of readability and memory efficiency. In view of this, it is better to use unordered_set. In the design philosophy of OpenVSLAM, it is unnatural to prioritize execution efficiency over readability. Therefore, we need to remove these member variables. No other unnatural similarities were found. Reset
Overall, the processes related to request_reset have some very unnatural similarities, and the functions related to request_reset need to be removed. Finish
It is similar, but very simple and there is no other way to express it. |
Optimizer - optimize::*GlobalBundleAdjustemnt, BundleAdjustment in Optimizer - optimize in optimize::global_bundle_adjusterThe only unnatural similarity is the member variables used as temporary variables mentioned above. I think The same bug exists in local_bundle_adjuster and pose_optimizer. PoseOptimization in Optimizer - optimize in optimize::pose_optimizerThere are similarities, but I think it is due to the fact that they share the same algorithm/library(g2o). LocalBundleAdjustment in Optimizer - optimize in optimize::local_bundle_adjusterThere are similarities, but I think it is due to the fact that they share the same algorithm/library(g2o). ORB-SLAM2 uses temporary variable OptimizeEssentialGraph in Optimizer - optimize in optimize::graph_optimizerThere are similarities, but I think it is due to the fact that they share the same algorithm/library(g2o). (Off topic, The following lines are unnecessary.) OptimizeSim3 in Optimizer - optimize in optimize::transform_optimizerThere are similarities, but I think it is due to the fact that they share the same algorithm/library(g2o). |
ORBextractor - feature::*IC_Angle in ORBextractor - ic_angle in feature::orb_extractorIt is derived from opencv; it is unclear which opencv ORB-SLAM2 and OpenVSLAM are based on for their implementations, but I think it is incorrect that there is no indication of the BSD license, since the expression is almost unchanged. The license notation needs to be added. constructorInitialize the variables used by IC_Angle. I think this is a derivative of opencv as well as the above. The license notation needs to be added. No other unnatural similarities were found. operator() in ORBextractor - extract, correct_keypoint_scale in feature::orb_extractorThis is based in part on the opencv process. Especially in ORB-SLAM2, some parts are copied almost without modification.
Compute descriptor
In ORB-SLAM2, I think this is a derivative of opencv as well as the above. The implementation in OpenVSLAM looks unique. ComputePyramid in ORBextractor - compute_image_pyramid in feature::orb_extractorIn ORB-SLAM2, I think this is a derivative of opencv as well as the above. The implementation in OpenVSLAM looks unique. computeOrientation in ORBextractor - compute_orientation in feature::orb_extractorIt is similar, but very simple and there is no other way to express it. DivideNode in ORBextractor - divide_node in feature::orb_extractor_nodeThere are a bit of similarities, but it is due to the fact that they share the same algorithm. ComputeKeyPointsOctTree in ORBextractor - compute_fast_keypoints in feature::orb_extractorThere are similarities, but it is due to the fact that they share the same algorithm. In OpenVSLAM, masks can be set and are parallelized. DistributeOctTree in ORBextractor - distribute_keypoints_via_tree, initialize_nodes, assign_child_nodes, find_keypoints_with_max_response in feature::orb_extractorIn OpenVSLAM, the process for portrait orientation images is different. There are a bit of similarities, but it is due to the fact that they share the same algorithm. In distribute_keypoints_via_tree, the sort result is unstable when the number of keypoints is the same. |
|
The following points were made by AlejandroSilvestri (#249 (comment))
I think this is considered a strange similarity because, for example, an image can also be converted in the system class and input to the tracking_module. We can't prove that the current structure is optimal in terms of readability or efficiency; we need to input an image or a preprocessed frame into tracking_module, but we need to determine when to do so based on our own policy. One policy to solve this is to minimize the number of unnecessary functions for readability: if we generate frames in system, we can reduce the number of functions.
It is helpful to see how ProSLAM is implemented. https://github.com/AhmedShaban94/ProSLAM/tree/master/src/framepoint_generation Currently, the cohesion is too low. It is unnatural to adopt such a design as is in OpenVSLAM, which is intended to be readable.
I'll have to think about it in detail, but I think it's possible to have tracking_module return the result of initialization or tracking to system, and system manage the state and call different functions of tracking_module. From a readability point of view, we should choose a better way with OpenVSLAM.
If there is a more appropriate algorithm for OpenVSLAM, I think it should be chosen. I have not been able to find one yet. |
ORBmatcher - match::*projection
There are similarities, but I think it is due to the fact that they share the same algorithm. However, I am concerned that there is duplicate code in both. For example, the following code. It should fix duplicated sections of code. BoW
There are similarities, but I think it is due to the fact that they share the similar algorithm. match_keyframes in OpenVSLAM skips the comparison with matched keypoints. SearchForInitialization in ORBmatcher - match_in_consistent_area in match::areaThere are similarities, but I think it is due to the fact that they share the same algorithm. SearchForTriangulation, CheckDistEpipolarLine in ORBmatcher - match_for_triangulation, check_epipolar_constraint in match::robustThere are similarities, but I think it is due to the fact that they share the similar algorithm. The similarity between CheckDistEpipolarLine and check_epipolar_constraint is negligible. The following statements are unique to OpenVSLAM. fuse
There are similarities, but I think it is due to the fact that they share the similar algorithm. ComputeThreeMaxima in ORBmatcher - get_valid_matches in angle_checkerThe algorithm has been changed to be more flexible. DescriptorDistance in ORBmatcher - compute_descriptor_distance_32 in match::baseBoth refer to the same link. It is similar, but very simple and there is no other way to express it. There is a 64-bit version in OpenVSLAM. |
PnPsolver - solve::pnp_solverI'll skip the EPnP part; I compared it to https://github.com/cvlab-epfl/EPnP. It looked like it was created from scratch. constructor and SetRansacParametersThe similarities are negligible. iterate in PnPsolver - find_via_ransac in solve::pnp_solverThere are a bit of similarities, but it is due to the fact that they share the same algorithm. |
Sim3Solver - solve::sim3_solverconstructor and SetRansacParametersIt calls the following.
There are similarities, but it is due to the fact that they share the same algorithm. iterate in Sim3Solver - find_via_ransac in solve::sim3_solverThere are a bit of similarities, but it is due to the fact that they share the same algorithm. ComputeSim3, ComputeCentroid, CheckInliers, Project in Sim3Solver - compute_Sim3, count_inliers, reproject_to_other_image in solve::sim3_solverThere are a bit of similarities, but it is due to the fact that they share the same algorithm. |
Continued from #37.
In summary, under efficiency constraints, if different expressions are considered, they should be removed and rewritten from scratch.
The text was updated successfully, but these errors were encountered: