search

stella-cv / stella_vslam

This is a unofficial fork of OpenVSLAM (https://github.com/xdspacelab/openvslam)
https://stella-cv.rtfd.io/en/latest/
Other
920 stars 394 forks source link

Clarify the functional correspondence between the ORB_SLAM2 code and OpenVSLAM code and evaluate the similarities #239

Closed ymd-stella closed 2 years ago

ymd-stella commented 2 years ago

Continued from https://github.com/OpenVSLAM-Community/openvslam/issues/37.

Extracted similar modules between ORB_SLAM2 and OpenVSLAM.

  • data modules
    • Frame - data::frame
    • KeyFrame - data::keyframe
    • MapPoint - data::landmark
    • Map - data::map_database
    • KeyFrameDatabase - data::bow_database
  • visualization modules
    • Viewer, MapDrawer, FrameDrawer - pangolin_viewer, publish::*
  • other modules
    • System - system, io::trajectory_io
    • Initializer - solve::*, initialize::*
    • Tracking - tracking_module, module::frame_tracker, module::keyframe_inserter, module::local_map_updater, module::relocalizer, module::initializer
    • LocalMapping - mapping_module, module::local_map_cleaner, module::two_view_triangulator
    • LoopClosing - global_optimization_module, module::loop_bundle_adjuster, module::loop_detector
    • Optimizer - optimize::*
    • ORBextractor - feature::*
    • ORBmatcher - match::*
    • PnPsolver - solve::pnp_solver
    • Sim3Solver - solve::sim3_solver

Based on these correspondences, similarity should be assessed by making comparisons at a appropriately abstract level. It is important to note that their creative expression may be limited by ideas. (Please refer Merger doctrine.)

In summary, under efficiency constraints, if different expressions are considered, they should be removed and rewritten from scratch.

ymd-stella commented 2 years ago

Frame - data::frame

constructors

The natural order is from feature point extraction to stereo matching to grid assignment of feature points. The suspects are as follows

Overall, the similarities are negligible.

AssignFeaturesToGrid in Frame - assign_keypoints_to_grid in common

It is related to

Overall, the order of processing is natural, and no similarities in expression were found.

ExtractORB in Frame - extract_orb in frame

There 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 frame

It is related to

Overall, the order of processing was natural, and no similarities in expression were found.

isInFrustum in Frame - can_observe in frame

Considering the computational efficiency, the processing order looks natural. There is no similarity other than the order of processing.

ComputeBoW in Frame - compute_bow in frame

It is unnatural to return without doing anything when bowvec is not empty. However, they are similar. It needs to be removed.

UndistortKeyPoints in Frame - compute_subpixel_disparity in camera::perspective

It'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::perspective

No similarities in expression were found. (Off topic, but it is more natural to use Point2f instead of KeyPoint.)

ComputeStereoMatches in Frame - compute in matcher::stereo

matcher::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 frame

It is similar, but very simple and there is no other way to express it.

UnprojectStereo in Frame - triangulate_stereo in frame

It is similar, but very simple and there is no other way to express it.

ymd-stella commented 2 years ago

KeyFrame - data::keyframe

constructors

The similarities are negligible. It is a bit unnatural to generate a keyframe by copying the member variables of a frame. With some effort, it may be possible to make the keyframe have a reference to the frame.

ComputeBoW in KeyFrame - compute_bow in keyframe

It is unnatural to return without doing anything when bowvec and bow_featvec is not empty. However, they are similar. It needs to be removed.

SetPose in KeyFrame - set_cam_pose in keyframe

There 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_node

The 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 orderedcovisibilities and orderedweights. It is natural to use std::vector<std::pair<unsigned int, std::weak_ptr<keyframe>>>> ordered_weights_and_covisibilities_. However, there may be a rationale for execution efficiency.

landmark accessors

setters

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_node

There 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 common

The order of processing is natural, and no similarities in expression were found.

IsInImage in KeyFrame - reproject_to_image in perspective

The only similarity is in the last conditional expression, and there is no other way to express it.

UnprojectStereo in KeyFrame - triangulate_stereo in keyframe

There are similarities, but very simple and there is no other way to express it.

ComputeSceneMedianDepth in KeyFrame

There are similarities, but very simple and there is no other way to express it.

ymd-stella commented 2 years ago

MapPoint - data::landmark

constructors

The 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 observations_, but I think the current process is the simplest. There was a comment that matched EraseObservation and erase_observation. There are similarities, but very simple and there is almost no other way to express it. It is better to rewrite the comment from scratch.

Replace in MapPoint - replace in landmark

There 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 landmark

ComputeDistinctiveDescriptors 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 landmark

There are similarities, but very simple and there is no other way to express it. This naming is a bit unnatural for the description

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/data/landmark.h#L78-L79

PredictScalein MapPoint - predict_scale_level in landmark

It is unnatural to use overloading just to get log_scalefactor and num_scalelevels. It needs to be removed.

ymd-stella commented 2 years ago

Map - data::map_database

AddKeyFrame in Map - add_keyframe in map_database

The process using max_keyfrmid is unnatural, and it is better to remove the process using max_keyfrmid.

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_database

The process using max_keyfrmid is unnatural, and it is better to remove the process using max_keyfrmid.

clear in Map - clear in map_database

The process of assigning nullptr is unnatural. It needs to be removed. Other than that, it is very simple and there is no other way to describe it.

ymd-stella commented 2 years ago

KeyFrameDatabase - data::bow_database

add, erase in KeyFrameDatabase - add_keyframe, erase_keyframe in bow_database

It is similar, but very simple and there is no other way to express it.

clear in KeyFrameDatabase - add_keyframe in bow_database

The similarities are negligible.

search

There are similarities, but I think it is due to the fact that they share the same algorithm.

ymd-stella commented 2 years ago

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.

ymd-stella commented 2 years ago

System - system, io::trajectory_io

constructor

The 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 --lisence command line option is given.

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 system

Overall, 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 std::future/std::promise to wait for the process to finish.

Shutdown in System - shutdown in system

I think it is better to use std::future/std::promise to wait for the process to finish. Overall, there is unnatural similarities in the process of sending a request and waiting for it to finish. These need to be removed.

Save trajectory

The similarities are negligible.

getters

There is no similar getters. map_publisher provides landmarks, but in a completely different expression.

ymd-stella commented 2 years ago

Initializer - solve::*, initialize::*

Initialize in Initializer - initialize in initialize::perspective

There 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_solver

There are a bit of similarities, but it is due to the fact that they share the same algorithm. In OpenVSLAM, there is a link to https://www.uio.no/studier/emner/matnat/its/UNIK4690/v16/forelesninger/lecture_4_3-estimating-homographies-from-feature-correspondences.pdf.

ComputeF21 in Initializer - compute_F_21 in solve::fundamental_solver

There 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. In solve::homography_solver::decompose, there was a reference to "Motion and structure from motion in a piecewise planar environment (Faugeras et al. in IJPRAI 1988)". In solve::essential_solver::decompose, there was a link to https://en.wikipedia.org/wiki/Essential_matrix#Determining_R_and_t_from_E.

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::common

There are a bit of similarities, but it is due to the fact that they share the same algorithm.

ymd-stella commented 2 years ago

Tracking - tracking_module, module::frame_tracker, module::keyframe_inserter, module::local_map_updater, module::relocalizer, module::initializer

constructor

The 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_module

It is similar, but very simple and there is no other way to express it.

Track in Tracking - track in tracking_module

The 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. (Off topic, in OpenVSLAM, I don't think it is necessary to update_local_map after successful initialization.)

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_intracking and identifier_in_local_lmsearch in detail next(search_local_landmarks).

Search local landmarks

The following variables are temporary variables, but they are declared as member variables. https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/data/landmark.h#L114-L120 https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/data/keyframe.h#L218 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.

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_module

num_trackedlms is a member variable that is used as a temporary variable and is unnaturally similar. num_trackedlms needs to be removed.

UpdateLastFrame in Tracking - update_last_frame in tracking_module

The 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::relocalizer

There are a bit of similarities, but I think it is due to the fact that they share the similar algorithm. The main difference is that OpenVSLAM processes each candidate one by one, while ORB-SLAM2 allocates computational resources to each candidate a little at a time. (Off topic, In ORB-SLAM2, there are conditions under which outlier removal is not performed. This may be a bug. https://github.com/raulmur/ORB_SLAM2/blob/f2e6f51cdc8d067655d90a78c06261378e07e8f3/src/Tracking.cc#L1456-L1460 )

Reset in Tracking - reset in tracking_module

Overall, the processes related to request_reset have some very unnatural similarities, and the functions related to request_reset need to be removed.

ChangeCalibration - none

No similar function was found.

InformOnlyTracking in Tracking - set_mapping_module_status in tracking_module

It is similar, but very simple and there is no other way to express it.

ymd-stella commented 2 years ago

LocalMapping - mapping_module, module::local_map_cleaner, module::two_view_triangulator

setters

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_module

There 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_module

There 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_module

There 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_module

There 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_solver

There 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_module

There 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_module

There is a slightly unnatural similarity. There is no reason to clear the keyframe queue when resuming. At

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/mapping_module.cc#L82

I think we should lock mtx_keyfrmqueue 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.

ymd-stella commented 2 years ago

LoopClosing - global_optimization_module, module::loop_bundle_adjuster, module::loop_detector

setters

It is similar, but very simple and there is no other way to express it.

Run in LoopClosing - run in global_optimization_module

There 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_detector

The repetition of bow_db_->add_keyframe(cur_keyfrm_); is unnaturally similar. it is a process that is done regardless of the result of detect_loop_candidates, so it can be merged into one. It needs to be removed. No other unnatural similarities were found.

ComputeSim3 in LoopClosing - validate_candidates, select_loop_candidate_via_Sim3 in loop_detector

In 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_adjuster

The following variables are temporary variables, but they are declared as member variables.

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/data/keyframe.h#L223-L228

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/data/landmark.h#L123-L126

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.

ymd-stella commented 2 years ago

Optimizer - optimize::*

GlobalBundleAdjustemnt, BundleAdjustment in Optimizer - optimize in optimize::global_bundle_adjuster

The only unnatural similarity is the member variables used as temporary variables mentioned above.

I think x_right < 0 is correct for the following conditions. Even if you are using a stereo camera, it will be x_right < 0 in the point where the matching fails. This is a bug that exists only in OpenVSLAM.

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/optimize/global_bundle_adjuster.cc#L114

The same bug exists in local_bundle_adjuster and pose_optimizer.

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/optimize/local_bundle_adjuster.cc#L178

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/optimize/pose_optimizer.cc#L78

PoseOptimization in Optimizer - optimize in optimize::pose_optimizer

There 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_adjuster

There are similarities, but I think it is due to the fact that they share the same algorithm/library(g2o). ORB-SLAM2 uses temporary variable mnBALocalForKF, while OpenVSLAM uses unordered_set. It's a matter of preference, but if (force_stop_flag && *force_stop_flag) { is preferable to if (force_stop_flag) { if (*force_stop_flag) {. I think the similarity is slightly unnatural.

OptimizeEssentialGraph in Optimizer - optimize in optimize::graph_optimizer

There are similarities, but I think it is due to the fact that they share the same algorithm/library(g2o). The member variable loop_fusion_identifier_ and ref_keyfrm_id_in_loop_fusion_ used as temporary variables mentioned above. (Off topic, The following lines are unnecessary because it will always be false.)

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/optimize/graph_optimizer.cc#L149-L152

(Off topic, The following lines are unnecessary.)

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/optimize/graph_optimizer.cc#L212-L214

OptimizeSim3 in Optimizer - optimize in optimize::transform_optimizer

There are similarities, but I think it is due to the fact that they share the same algorithm/library(g2o).

ymd-stella commented 2 years ago

ORBextractor - feature::*

IC_Angle in ORBextractor - ic_angle in feature::orb_extractor

It 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.

constructor

Initialize 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_extractor

This 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_extractor

In ORB-SLAM2, I think this is a derivative of opencv as well as the above. The implementation in OpenVSLAM looks unique.

https://github.com/opencv/opencv/blob/82f8176b0634c5d744d1a45246244291d895b2d1/modules/features2d/src/orb.cpp#L766-L815

computeOrientation in ORBextractor - compute_orientation in feature::orb_extractor

It is similar, but very simple and there is no other way to express it.

DivideNode in ORBextractor - divide_node in feature::orb_extractor_node

There 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_extractor

There 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_extractor

In 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.

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/feature/orb_extractor.cc#L454

ymd-stella commented 2 years ago

The following points were made by AlejandroSilvestri (https://github.com/OpenVSLAM-Community/openvslam/issues/249#issuecomment-1019379866)

  • The first obvious step in the process is to extract ORB descriptors from the image. Both ORB-SLAM2 and OpenVSLAM walk the same strange path before doing so (under different names)

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.

  • the Frame class process the image in its constructor: this is a very arbitrary and odd programming decision, the same in both projects, and it's not the obvious or self-evident way to do things

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.

  • track_monocular_image() constructs frame() in one of two different ways, depending on whether they are in initialization or tracking
  • the main sequencer finite state machine is located in tracker, and is the same in both projects

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.

  • FAST keypoints are filtered by OctTree or QuadTree (same process, different name), when it is neither the most obvious nor the best way to do it

If there is a more appropriate algorithm for OpenVSLAM, I think it should be chosen. I have not been able to find one yet.

ymd-stella commented 2 years ago

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.

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/match/projection.cc#L138-L152

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::area

There 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::robust

There 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.

https://github.com/OpenVSLAM-Community/openvslam/blob/aa1f04810b0dc5ba3f974d723df4f965492e3585/src/openvslam/match/robust.cc#L109-L119

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_checker

The algorithm has been changed to be more flexible.

DescriptorDistance in ORBmatcher - compute_descriptor_distance_32 in match::base

Both 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.

ymd-stella commented 2 years ago

PnPsolver - solve::pnp_solver

I'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 SetRansacParameters

The similarities are negligible.

iterate in PnPsolver - find_via_ransac in solve::pnp_solver

There are a bit of similarities, but it is due to the fact that they share the same algorithm.

ymd-stella commented 2 years ago

Sim3Solver - solve::sim3_solver

constructor and SetRansacParameters

It 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_solver

There 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_solver

There are a bit of similarities, but it is due to the fact that they share the same algorithm.