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

Taking roi in cameraInfo in consideration when using camera_display plugin #1158

Merged
merged 3 commits into from
Apr 10, 2018

Conversation

Shin-nn
Copy link
Contributor

@Shin-nn Shin-nn commented Oct 5, 2017

This should fix camera_display when using roi.

@dhood
Copy link
Contributor

dhood commented Dec 18, 2017

This looks good, thanks for the patch.

Thinking about behaviour stability: this will change the behaviour for users with cameras with ROI set, but given that those users currently have the subimage stretched to the full camera width, I think this will be a welcome improvement for those users.

There's a chance this will negatively impact the following user: ROI set in the camera info, but image being published at full size. This previously would have rendered the image appropriately but now will distort the image according to the ROI. However, the camera info ought to match the camera image, so this user shouldn't exist, so I do not consider this a reason to prevent this change.

@wjwwood if you have a chance to confirm that this behavioural change is appropriate to push into kinetic that would be appreciated.

camera display with roi (selected in red) without this patch (top right, stretched)
camera_roi_without_1158

camera display with roi (selected in red) with this patch (top right, not stretched, appropriate size)
camera_roi_with_1158

The images are from testing with turtlebot in gazebo and interactive ROI selection; launch/config file: camera_roi.zip

@dhood dhood self-assigned this Dec 18, 2017
@dhood dhood requested a review from wjwwood December 19, 2017 18:41
@dhood
Copy link
Contributor

dhood commented Dec 19, 2017

@wjwwood I've requested your review, not to hurry it up, just so it's less likely to get lost over the holidays

@dhood
Copy link
Contributor

dhood commented Apr 6, 2018

@wjwwood I'm planning to push the behaviour fix into kinetic FYI

@dhood dhood added the bug label Apr 7, 2018
@dhood dhood merged commit 6577ebe into ros-visualization:kinetic-devel Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants