moved undistorter to cameras and added point processing function #34
Conversation
mfehr
left a comment
mfehr
left a comment
There was a problem hiding this comment.
did a pass through the PR, looks good, except maybe the duplication of the undistortion map might not be necessary?
| const Camera& getCamera(size_t camera_index) const; | ||
|
|
||
| /// Get the geometry object for camera i. | ||
| /// The method will assert that the camera is not in the rig! |
There was a problem hiding this comment.
that the camera IS in the rig?
| Camera& getCameraMutable(size_t camera_index); | ||
|
|
||
| /// Get the geometry object for camera i. | ||
| /// The method will assert that the camera is not in the rig! |
| << "Cast to unified projection camera failed."; | ||
| intrinsics << unified_proj_cam_ptr->xi(), output_camera_matrix(0, 0), | ||
| output_camera_matrix(1, 1), output_camera_matrix(0, 2), | ||
| output_camera_matrix(1, 1), output_camera_matrix(0, 2), |
There was a problem hiding this comment.
this seems fishy, is there a line missing here?
| return std::unique_ptr<MappedUndistorter>(new MappedUndistorter( | ||
| input_camera, output_camera, map_u, map_v, interpolation_type)); | ||
| // Convert map to non-fixed point representation for easy lookup of values. | ||
| cv::Mat map_u_float = map_u.clone(); |
There was a problem hiding this comment.
does it make sense to clone the cv::Mat and then later I guess you just overwrite everything again or even reallocate them? Why not initialize them to the right size and type here or inside the function?
| // Convert map to non-fixed point representation for easy lookup of values. | ||
| cv::Mat map_u_float = map_u.clone(); | ||
| cv::Mat map_v_float = map_v.clone(); | ||
| aslam::convertMapsLegacy(map_u, map_v, map_u_float, map_v_float, CV_32FC1); |
There was a problem hiding this comment.
why do we need to use a legacy function in this core functionality? Is this reverting the map convention to legacy?
| MappedUndistorter(aslam::Camera::Ptr input_camera, aslam::Camera::Ptr output_camera, | ||
| const cv::Mat& map_u, const cv::Mat& map_v, InterpolationMethod interpolation); | ||
| const cv::Mat& map_u, const cv::Mat& map_v, | ||
| const cv::Mat& map_u_float, const cv::Mat& map_v_float, |
There was a problem hiding this comment.
why does this now take both the fixed point maps and the float map? Shouldn't it only need one of the two?
| CHECK_LE((*point)[0], map_u_.cols); | ||
| CHECK_LE((*point)[1], map_v_.rows); | ||
| *point = Eigen::Vector2d( | ||
| map_u_float_.at<float>((*point)[1], (*point)[0]), |
There was a problem hiding this comment.
this seems to be the only reason we have the float maps now, right? Would it be that expensive to do this using the fixed point map?
For some reason
aslam_cv_pipeline/src/undistorter-mapped.cc -> aslam_cv_cameras/src/undistorter-mapped.ccis not detected