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

updated flip realsense #27

Merged
merged 8 commits into from
Apr 14, 2022
Merged

Conversation

ste93
Copy link
Contributor

@ste93 ste93 commented Apr 5, 2022

Added option rotateImage180 in order to rotate the image when the realsense is flipped upside down

@ste93 ste93 requested a review from Nicogene as a code owner April 5, 2022 14:04
@ste93
Copy link
Contributor Author

ste93 commented Apr 5, 2022

@traversaro

@traversaro
Copy link
Member

If this new parameter is suggested (not specifying it results in a warning) can you update the README to cover it, and add a line to the changelog about it? Thanks!

@ste93
Copy link
Contributor Author

ste93 commented Apr 5, 2022

Thanks for the review @traversaro, I think that the best way is to remove the warning. Under the suggestion of @elandini84 I've put a warning when the parameter is enabled.

Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

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

If the nested for loops are not affecting too much the performance, lgtm
I think you already looked if there is something in the configuration of the device or native functions for flipping in the sdk

@ste93
Copy link
Contributor Author

ste93 commented Apr 6, 2022

If the nested for loops are not affecting too much the performance, lgtm I think you already looked if there is something in the configuration of the device or native functions for flipping in the sdk

On the sdk they're saying to use opencv, so I think this is the best way. Also if this option is not enabled the behavior is the same as before

@Nicogene
Copy link
Member

Nicogene commented Apr 6, 2022

If the nested for loops are not affecting too much the performance, lgtm I think you already looked if there is something in the configuration of the device or native functions for flipping in the sdk

On the sdk they're saying to use opencv, so I think this is the best way. Also if this option is not enabled the behavior is the same as before

I would give a try in order to see if the improvement in performance with OpenCV is worthy adding the dependency. But if you already tested it and you have solid fps we can merge it

CHANGELOG.md Outdated Show resolved Hide resolved
@ste93
Copy link
Contributor Author

ste93 commented Apr 6, 2022

If the nested for loops are not affecting too much the performance, lgtm I think you already looked if there is something in the configuration of the device or native functions for flipping in the sdk

On the sdk they're saying to use opencv, so I think this is the best way. Also if this option is not enabled the behavior is the same as before

I would give a try in order to see if the improvement in performance with OpenCV is worthy adding the dependency. But if you already tested it and you have solid fps we can merge it

I've not tested it with big fps, since on the robot we're using low fps. But I can open an issue to track it and check it

README.md Outdated Show resolved Hide resolved
@randaz81
Copy link
Member

If the nested for loops are not affecting too much the performance, lgtm I think you already looked if there is something in the configuration of the device or native functions for flipping in the sdk

On the sdk they're saying to use opencv, so I think this is the best way. Also if this option is not enabled the behavior is the same as before

I would give a try in order to see if the improvement in performance with OpenCV is worthy adding the dependency. But if you already tested it and you have solid fps we can merge it

I think it does not worth adding opencv dependency for such a simple processing...

ste93 and others added 2 commits April 11, 2022 15:44
Co-authored-by: Silvio Traversaro <silvio@traversaro.it>
Co-authored-by: Silvio Traversaro <silvio@traversaro.it>
@ste93
Copy link
Contributor Author

ste93 commented Apr 14, 2022

@traversaro do you want me to squash the commits? otherwise for me it can be merged

@traversaro
Copy link
Member

Thanks for the remainder and for the PR, I simply forgot.

@traversaro traversaro merged commit 7ee8604 into robotology:master Apr 14, 2022
@ste93
Copy link
Contributor Author

ste93 commented Apr 14, 2022

no problem, thank you

@ste93 ste93 deleted the flip_realsense branch June 7, 2022 16:05
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.

4 participants