Skip to content
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

Changed condition to set pKFm #44

Closed
wants to merge 1 commit into from

Conversation

ethanseq
Copy link
Collaborator

@ethanseq ethanseq commented Jul 4, 2022

The solution proposed for #13 (comment) involved switching bDifferentKFs to be initially set to true and would then be switched to false if vpKeyFrameMatchedMp was empty. However, this technically isn't correct as an empty vpKeyFrameMatchedMp insinuates that it is a different key frame. Thus,

if(!bDifferentKFs)
was changed to only set pKFm if it isn't a different frame. Otherwise, the default pKFm value of pKF2 is used instead.

@ethanseq ethanseq linked an issue Jul 4, 2022 that may be closed by this pull request
@Soldann Soldann added the needs validation Further testing required label Jul 5, 2022
@ethanseq
Copy link
Collaborator Author

ethanseq commented Jul 11, 2022

Additional Clarification:

bool bDifferentKFs = false;
if(vpKeyFrameMatchedMP.empty())
{
bDifferentKFs = true;
vpKeyFrameMatchedMP = vector<KeyFrame*>(vpMatched12.size(), pKF2);
If vpKeyFrameMatchedMP is empty, we determine that we have different keyframes and set bDifferentKFs to true while also setting vpKeyFrameMatchedMP to pKF2 which is the keyframe with the most BoW matches.
KeyFrame* pKFm = pKF2; //Default variable
The default for pKFm is set to pKF2. However, we can see in
if(!bDifferentKFs)
pKFm = vpKeyFrameMatchedMP[i1];
that pKFm is only set if bDifferentKFs is true, which is reduntant since it is already set the pKF2 as the default. Therefore, the condition needs to be changed in order to make sure that pKFm when bDifferentKFs is false.

@Ryan-Red Ryan-Red added the backlog Stuff we will deal with later, low priority label Jul 13, 2022
@Ryan-Red
Copy link
Collaborator

Tried and did not produce any noticeable results

@Ryan-Red Ryan-Red closed this Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Stuff we will deal with later, low priority needs validation Further testing required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential bug in Sim3Solver.cc
3 participants