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

Allow users to override encoding string in ROSCvMatContainer #505

Conversation

Yadunund
Copy link
Contributor

@Yadunund Yadunund commented Feb 1, 2023

Fixes #504

The implementation here relies on using an std::optional<std::string> encoding_override argument which can be supplied during instantiation of ROSCVMatContainer. If provided, the value will be used, else default to the existing behavior of inferring the encoding from cv::Mat::type().
In order to use std::optional, I also had to bump the C++ version to C++17.

Also it looks like ROSCvMatContainer::get_sensor_msgs_msg_image_copy() has duplicate code from convert_to_ros_message(). I've left a TODO to explore reusing the latter within this function. Happy to implement it if the change is acceptable.

Signed-off-by: Yadunund yadunund@openrobotics.org

Signed-off-by: Yadunund <yadunund@openrobotics.org>
@ijnek
Copy link
Member

ijnek commented Feb 2, 2023

@Yadunund thanks for raising an issue and the PR too. Could you create an issue as an alternative to a todo comment. The changes itself look good.

Signed-off-by: Yadunund <yadunund@openrobotics.org>
@Yadunund
Copy link
Contributor Author

Yadunund commented Feb 3, 2023

Thanks for the quick review. I've removed the todo comment and opened this ticket instead #506

@ijnek
Copy link
Member

ijnek commented Feb 4, 2023

Thanks for the contribution!

@ijnek ijnek merged commit b6842b0 into ros-perception:rolling Feb 4, 2023
@Yadunund Yadunund deleted the yadu/custom_encoding_for_ROSCvMatContainer branch May 9, 2023 07:22
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.

ROSCvMatContainer assumes encoding string for cv::Mat type CV_8UC3
2 participants