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

Fix camera info #13

Merged
merged 5 commits into from Aug 15, 2019
Merged

Fix camera info #13

merged 5 commits into from Aug 15, 2019

Conversation

coxep
Copy link

@coxep coxep commented Aug 13, 2019

Extensive updates to camera intrinsics. Backwards compatible with Astra mini devices.


This change is Reviewable

coxep added 4 commits August 9, 2019 11:09
method `getIRFocalLength` is not valid for astra mini cameras
IR XGA has an extra strip of pixels at bottom of image that don't correspond
to depth image. These should not impact the intrinsics.
Copy link

@dandedrick dandedrick left a comment

Choose a reason for hiding this comment

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

Have these changes been submitted upstream? I'm assuming they would generally be useful and I've actually has orbbec merge changes I've requested recently. Getting these in upstream will back rebasing on their changes easier in the future and prevent conflict where someone else might make similar changes or make changes that effect the same lines of code.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bobhenz-jabil, @c-andy-martin, @coxep, and @howardcochran)


src/astra_driver.cpp, line 879 at r1 (raw file):

sensor_msgs::CameraInfoPtr AstraDriver::getColorCameraInfo(const AstraVideoMode& video_mode) const
{
    sensor_msgs::CameraInfoPtr info;

You added 2 spaces to the indentation to this whole function. This should be made consistent with the rest of the file, which is 2 space indentation. I think remaining consistent will help produce a better diff highlighting only what actually changed.


src/astra_driver.cpp, line 962 at r1 (raw file):

sensor_msgs::CameraInfoPtr AstraDriver::getIRCameraInfo(const AstraVideoMode &video_mode) const
{

This added whitespace is unneeded and this function has also modified the indentation level. This should also been switched back to 2 space indentation for consistency.


src/astra_driver.cpp, line 1053 at r1 (raw file):

sensor_msgs::CameraInfoPtr AstraDriver::getDepthCameraInfo(const AstraVideoMode &video_mode) const
{
    sensor_msgs::CameraInfoPtr info(new sensor_msgs::CameraInfo);

This is another function that needs the indentation to be reset to match the files 2 space indent.

Copy link
Author

@coxep coxep left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @bobhenz-jabil, @c-andy-martin, @dandedrick, and @howardcochran)


src/astra_driver.cpp, line 879 at r1 (raw file):

Previously, dandedrick (Dan Dedrick) wrote…

You added 2 spaces to the indentation to this whole function. This should be made consistent with the rest of the file, which is 2 space indentation. I think remaining consistent will help produce a better diff highlighting only what actually changed.

Done


src/astra_driver.cpp, line 962 at r1 (raw file):

Previously, dandedrick (Dan Dedrick) wrote…

This added whitespace is unneeded and this function has also modified the indentation level. This should also been switched back to 2 space indentation for consistency.

Done


src/astra_driver.cpp, line 1053 at r1 (raw file):

Previously, dandedrick (Dan Dedrick) wrote…

This is another function that needs the indentation to be reset to match the files 2 space indent.

Done

Copy link
Author

@coxep coxep left a comment

Choose a reason for hiding this comment

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

Agreed on all counts. Unfortunately, we need to get these changes for stereo S pose calibration. I will try to cherry pick this commit and upstream as well in near future

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @bobhenz-jabil, @c-andy-martin, @dandedrick, and @howardcochran)

Copy link

@dandedrick dandedrick left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @bobhenz-jabil, @c-andy-martin, and @howardcochran)

@travisariggs travisariggs merged commit ac55bec into badger-develop Aug 15, 2019
@travisariggs travisariggs deleted the fix-camera-info branch August 15, 2019 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants