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

Porting tests from initialize_cameras_landmarks_basic to test initialize_cameras_landmarks #1630

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

OwenMcGee
Copy link
Collaborator

Currently doesn't compile due to issues in the evaluate_initializer() helper function. With the lines calling that function commented out, all tests currently run and succeed aside from subset_init, which I haven't yet altered.

The helper function helper() (to be renamed) condenses repeat code from the original initialize_cameras_landmarks_basic tests. I also adjusted the configure_algo() helper function to set additional config values that weren't needed in the original basic tests.

@OwenMcGee OwenMcGee requested a review from mleotta June 3, 2022 17:50
@OwenMcGee OwenMcGee force-pushed the dev/add-initialize-cameras-landmarks-ported-tests branch 2 times, most recently from bbcb3a6 to eadef49 Compare June 10, 2022 16:07
@OwenMcGee OwenMcGee force-pushed the dev/add-initialize-cameras-landmarks-ported-tests branch from eadef49 to 3b68ca9 Compare June 10, 2022 17:21
@OwenMcGee
Copy link
Collaborator Author

Tests currently fail in the evaluate_initialization helper function, as the est_cams variable is a nullptr. The variable should be set in the initialize function in initialize_cameras_landmarks.cxx, but isn't due to functions returning too early (these checks should probably throw exceptions, rather than just returning early)

@@ -2437,7 +2437,8 @@ ::metadata_centric_keyframe_initialization(
vector_3d pos_loc;
rotation_d R_loc;

if (constraints->get_camera_position_prior_local(fid, pos_loc) &&
if (constraints == nullptr &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because the initialize() function sets its fourth parameter constraints to have a default value of nullptr, which as far as I can tell will always result in a segmentation fault in this function . This addition (checking for nullptr) stops the segmentation fault, but I believe might cause issues later on with variables not being defined when they should be.

Unless I'm mistaken, nowhere currently calls the initialize() function using the default value for constraints (doing so would cause a seg fault, I believe), so it would seem the better option here would be to simply change the function to not have a default value for that parameter. This would also necessitate passing in real values for it in the test file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented this issue in #1639

@@ -2437,7 +2437,8 @@ ::metadata_centric_keyframe_initialization(
vector_3d pos_loc;
rotation_d R_loc;

if (constraints->get_camera_position_prior_local(fid, pos_loc) &&
if (constraints == nullptr &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the right thing to do. Though you could more compactly write if (constraints && since the value of a nullptr is zero which casts to false in Boolean.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to move this check right after cam->clear(); call before "for" loop, since if constraints is nullptr, the loop is useless. Something like this
cams->clear(); if (!constraints) { return false; }
it can be returned immediately because anway after it goes out of if-else block, cams->size() < 2 condition is being checked, which means it will always return false there, so we can leave the function immediately. Otherwise for loop needs to be in block if (constraints) { for ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants