-
Notifications
You must be signed in to change notification settings - Fork 871
Conversation
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.
Thank you for the helpful PR!!
We'd like to enhance test codes for the convenience of SLAM learners.
Could you check my comments and give me your thoughts?
test/openvslam/solve/pnp_solver.cc
Outdated
else | ||
return x; | ||
} | ||
double angle_of_rot(const Mat33_t& rot) { |
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.
Can be replaced with norm of converter::to_angle_axis(rot)
?
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.
It is possible. I missed that function.
test/openvslam/solve/pnp_solver.cc
Outdated
cv::KeyPoint kp; | ||
keypts.push_back(kp); |
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.
Can be replaced as follows?
keypts.emplace_back(cv::KeyPoint{});
And could you insert the comment which states the default value of octave
contained in cv::KeyPoint
is 0 ?
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.
Yes, your suggestion is more efficient.
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.
Thank you for making the revisions.
Could you remove the unused function if you don't plan on using it in the future?
test/openvslam/solve/pnp_solver.cc
Outdated
|
||
using namespace openvslam; | ||
|
||
double clamp(const double x, const double min_val, const double max_val) { |
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 function is not used.
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 function is no longer used. I removed it.
LGTM. Thanks! |
The current PnP solver takes care of coordinates or some vector as arrays.
So, I will replace them to Eigen::Vectors.
Firstly, I added a unit test to check if the solver estimates camera-pose correctly.