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.

Sign up for GitHub

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

Jump to bottom

[WIP] Added atForDiffImTypes and atForDiffImTypesReturn #92

Closed
wants to merge 0 commits into from
Closed

[WIP] Added atForDiffImTypes and atForDiffImTypesReturn #92

wants to merge 0 commits into from

Conversation

ipa-jcl-cs
Copy link

added function in order to avoid the explicit statement of the internal type for cv::Mat.at

fmessmer
fmessmer requested changes Nov 22, 2018
View reviewed changes
Copy link
Member

@fmessmer fmessmer left a comment

Choose a reason for hiding this comment

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

what is bad about this explicit statement?
what's the improvement in this pr?
performance or does it fix a bug?
in what way does it change the current behavior?

also, there are still some indentation issues
and this pr should be squashed!

ipa-rmb
ipa-rmb requested changes Nov 23, 2018
View reviewed changes
Copy link
Contributor

@ipa-rmb ipa-rmb left a comment

Choose a reason for hiding this comment

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

We eventually also need to apply the format independent at operator to the actual image reading and writing operations. Or would you like to add this in a future pull request?

cob_image_flip/ros/include/cob_image_flip/image_flip.h Outdated
@@ -114,9 +114,15 @@ class ImageFlip

void disparityCallback(const stereo_msgs::DisparityImage::ConstPtr& disparity_image_msg);

void disparityConnectCB(const ros::SingleSubscriberPublisher& pub);
void disparityConnectCB(const ros::SingleSubscriberPublisher& pub);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the indentation as is, i.e. tabs in all these files instead of spaces.

cob_image_flip/ros/src/image_flip.cpp Outdated
@@ -192,6 +192,70 @@ bool ImageFlip::convertImageMessageToMat(const sensor_msgs::Image::ConstPtr& ima
}


template <typename T> void ImageFlip::atForDiffImTypes(cv::Mat& image, int row_index, int column_index, double value, bool replace_prev_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct indentation in this file as well, i.e. tabs instead of spaces and correct number of tabs.

cob_image_flip/ros/src/image_flip.cpp Outdated
}
}

template <typename T> double ImageFlip::atForDiffImTypesReturn(cv::Mat& image, int row_index, int column_index)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would make sense to return T instead of double because the type conversion to double or something else could be done by the outside function?

cob_image_flip/ros/src/image_flip.cpp Outdated
@@ -210,8 +274,9 @@ void ImageFlip::imageCallback(const sensor_msgs::ImageConstPtr& color_image_msg)
if (rotation_angle==0. || rotation_angle==360. || rotation_angle==-360.)
{
color_image_turned = color_image;
rot_mat.at<double>(0,0) = 1.;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave it the old way
rot_mat.at<double>(0,0) = 1.;
in this case, because the rotation matrix is always the same format CV_64FC1.

cob_image_flip/ros/src/image_flip.cpp Outdated
@@ -225,9 +290,9 @@ void ImageFlip::imageCallback(const sensor_msgs::ImageConstPtr& color_image_msg)
for (int v = 0; v < color_image_turned.rows; v++)
for (int u = 0; u < color_image_turned.cols; u++)
color_image_turned.at<cv::Vec3b>(v,u) = color_image.at<cv::Vec3b>(color_image.rows-1-u,v);
rot_mat.at<double>(0,1) = -1.;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave it the old way in this case, because the rotation matrix is always the same format CV_64FC1.

cob_image_flip/ros/src/image_flip.cpp Outdated
@@ -631,9 +698,9 @@ void ImageFlip::disparityCallback(const stereo_msgs::DisparityImage::ConstPtr& d
for (int v = 0; v < disparity_image_turned.rows; v++)
for (int u = 0; u < disparity_image_turned.cols; u++)
disparity_image_turned.at<float>(v,u) = disparity_image.at<float>(u,disparity_image.cols-1-v);
rot_mat.at<double>(0,1) = 1.;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave it the old way in this case, because the rotation matrix is always the same format CV_64FC1.

cob_image_flip/ros/src/image_flip.cpp Outdated
@@ -655,10 +722,11 @@ void ImageFlip::disparityCallback(const stereo_msgs::DisparityImage::ConstPtr& d
dst--;
}
}
rot_mat.at<double>(0,0) = -1.;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave it the old way in this case, because the rotation matrix is always the same format CV_64FC1.

cob_image_flip/ros/src/image_flip.cpp Outdated
@@ -678,8 +746,9 @@ void ImageFlip::disparityCallback(const stereo_msgs::DisparityImage::ConstPtr& d
rot_mat = cv::getRotationMatrix2D(center, -rotation_angle, 1.0);
if (switch_aspect_ratio==true)
{
rot_mat.at<double>(0,2) += 0.5*(disparity_image_turned.cols-disparity_image.cols);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave it the old way in this case, because the rotation matrix is always the same format CV_64FC1.

cob_image_flip/ros/src/image_flip.cpp Outdated
@@ -700,14 +769,14 @@ void ImageFlip::disparityCallback(const stereo_msgs::DisparityImage::ConstPtr& d
cv::Mat rot33 = cv::Mat::eye(3,3,CV_64FC1);
for (int r=0; r<2; ++r)
for (int c=0; c<3; ++c)
rot33.at<double>(r,c) = rot_mat.at<double>(r,c);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave it the old way in this case, because the rotation matrix is always the same format CV_64FC1.

cob_image_flip/ros/src/image_flip.cpp Outdated
cv::Mat rot33_inv = rot33.inv();
cob_perception_msgs::Float64ArrayStamped rot33_inv_msg;
rot33_inv_msg.header = disparity_image_msg->header;
rot33_inv_msg.data.resize(9);
for (int r=0; r<3; ++r)
for (int c=0; c<3; ++c)
rot33_inv_msg.data[r*3+c] = rot33_inv.at<double>(r,c);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leave it the old way in this case, because the rotation matrix is always the same format CV_64FC1.

@ipa-rmb ipa-rmb changed the title Added atForDiffImTypes and atForDiffImTypesReturn [WIP] Added atForDiffImTypes and atForDiffImTypesReturn Nov 23, 2018
@ipa-rmb
Copy link
Contributor

ipa-rmb commented Nov 23, 2018

@fmessmer The work here shall solve issue #85 . It is not yet finished, apparently.

@ipa-jcl-cs ipa-jcl-cs closed this Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmessmer fmessmer fmessmer requested changes

@ipa-rmb ipa-rmb ipa-rmb requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ipa-jcl-cs @ipa-rmb @fmessmer