[RSDK-1035] Camera Properties call fails #1802
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bazile-clyde
commented
Jan 23, 2023
Comment on lines
+169
to
+177
| if intrinsics := resp.IntrinsicParameters; intrinsics != nil { | ||
| result.IntrinsicParams = &transform.PinholeCameraIntrinsics{ | ||
| Width: int(intrinsics.WidthPx), | ||
| Height: int(intrinsics.HeightPx), | ||
| Fx: intrinsics.FocalXPx, | ||
| Fy: intrinsics.FocalYPx, | ||
| Ppx: intrinsics.CenterXPx, | ||
| Ppy: intrinsics.CenterYPx, | ||
| } |
There was a problem hiding this comment.
This was the bug fix for the original issue. Here, if resp.IntrinsicParameters == nil we would panic with the error
invalid memory address or nil pointer dereference
on line 171.
Checking if the resp.IntrinsicParameters == nil before attempting to access its methods fixes the issue.
bazile-clyde
commented
Jan 23, 2023
| @@ -237,6 +237,104 @@ func TestClient(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestClientProperties(t *testing.T) { | |||
There was a problem hiding this comment.
This just tests we've fixed the bug and checks for similar issues.
bazile-clyde
commented
Jan 23, 2023
| @@ -204,3 +206,17 @@ func TestGetCameraMatrix(t *testing.T) { | |||
| test.That(t, intrinsicsK.At(1, 2), test.ShouldEqual, intrinsics.Ppy) | |||
| test.That(t, intrinsicsK.At(2, 2), test.ShouldEqual, 1) | |||
| } | |||
|
|
|||
| func TestNilIntrinsics(t *testing.T) { | |||
There was a problem hiding this comment.
If a user gets back nil intrinsic parameters (which is now possible thanks to my fix 😞) and attempts to call any of the methods below we'd panic. For example
props, err := cam.Properties(context.Background())
if err != nil {
logger.Fatal("attempting to get cam properties: %v", err)
return
}
props.GetCameraMatrix() // panic!
This is easily fixable by checking if the parameters are nil in these methods. It's also idiomatic to do this in Go AFAIK.
I noticed for the distortion parameters we have a struct transform.NoDistortion that solves the same problem but I'm not sure what the advantage would be if we adopted that solution for intrinsic parameters too. Seems like they're both valid but I could be missing something. @bhaney it looks like you created that struct so if you have an opinion on this let me know.
There was a problem hiding this comment.
Yea, I think using nil as the "no intrinsics" and "no distortion" scenario is much more intuitive than creating a specific struct. Because then you have to remember to always use the "no such thing" struct, and nil is still there potentially causing problems!
edaniels
approved these changes
Jan 24, 2023
viambot
added
the
safe to test
bazile-clyde
force-pushed
the
RSDK-1035
branch
from
3893928 to
37536a9
Compare
viambot
added
safe to test
|
@bhaney removed NoDistortion |
bhaney
approved these changes
Jan 24, 2023
Comment on lines
-9
to
-10
| // NoneDistortionType applies no distortion to an input image. Essentially an identity transform. | ||
| NoneDistortionType = DistortionType("no_distortion") |
There was a problem hiding this comment.
Thanks for getting rid of this, making "no distortion" just be nil is much simpler
| @@ -204,3 +206,17 @@ func TestGetCameraMatrix(t *testing.T) { | |||
| test.That(t, intrinsicsK.At(1, 2), test.ShouldEqual, intrinsics.Ppy) | |||
| test.That(t, intrinsicsK.At(2, 2), test.ShouldEqual, 1) | |||
| } | |||
|
|
|||
| func TestNilIntrinsics(t *testing.T) { | |||
There was a problem hiding this comment.
Yea, I think using nil as the "no intrinsics" and "no distortion" scenario is much more intuitive than creating a specific struct. Because then you have to remember to always use the "no such thing" struct, and nil is still there potentially causing problems!
Comment on lines
215
to
221
| nilIntrinsics.CheckValid() | ||
| nilIntrinsics.GetCameraMatrix() | ||
| nilIntrinsics.PixelToPoint(0.0, 0.0, 0.0) | ||
| nilIntrinsics.PointToPixel(0.0, 0.0, 0.0) | ||
| nilIntrinsics.ImagePointTo3DPoint(image.Point{}, rimage.Depth(0)) | ||
| nilIntrinsics.RGBDToPointCloud(&rimage.Image{}, &rimage.DepthMap{}) | ||
| nilIntrinsics.PointCloudToRGBD(pointcloud.PointCloud(nil)) |
There was a problem hiding this comment.
You can use test.ShouldNotPanic so that it doesn't actually panic and cause the testing program to crash
You need to wrap them each in a function i.e.
test.That(t, func() { nilIntrinsics.CheckValid() }, test.ShouldNotPanic)
There was a problem hiding this comment.
Oh, fancy! Done.
viambot
added
safe to test
bazile-clyde
force-pushed
the
RSDK-1035
branch
from
5172a18 to
6f973a3
Compare
viambot
added
safe to test
gvaradarajan
approved these changes
Jan 25, 2023
|
Note: merge base coverage results not available, comparing against closest aa1914d~2=(9e2f63d) instead
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug fix: See script in RSDK-1035 for steps to reproduce.