-
Notifications
You must be signed in to change notification settings - Fork 58
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
Added projection values from camera when are not defined in the SDF #289
Conversation
Signed-off-by: Aditya <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
nvm, I see it targeted at that branch |
cameraSdf->SetLensProjectionFy(intrinsicMatrix(1, 1)); | ||
cameraSdf->SetLensProjectionCx(intrinsicMatrix(0, 2)); | ||
cameraSdf->SetLensProjectionCy(intrinsicMatrix(1, 2)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this duplicate block? (same code as lines above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code above is setting the intrinsic parameters this one is setting the projection parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh I see now 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also confused about this. Is the conversion from projection to intrinsic matrix necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method get these values https://github.com/gazebosim/gz-rendering/blob/c70882b96fc126e13fa82bc60ead17bf803bec08/include/gz/rendering/base/BaseCamera.hh#L505-L522
Maybe there is another way. @iche033 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but note that there wouldn't be any way for users to know if the projection values were set by the user to override the values in <intrinsic>
or if they were computed based on what's in <intrinsic>
. If that's not important, then I'm good with this change. Since this is all very confusing, I would ask that you would add a detailed comment explaining why we're doing this and why the projectionToCameraIntrinsic
is being used (i.e how Ogre2 projection matrix is not the same as the projection matrix from <projection>
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I would suggest adding "Note that the matrix from Ogre via camera->ProjectionMatrix()
has a different format than the projection matrix used in SDFormat. This is why they are converted using projectionToCameraIntrinsic
. The resulting matrix is the intrinsic matrix, but since the user has not overridden the values, this is also equal to the projection matrix"
That's my understanding at least. If that's not correct, feel free to modify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks
Thank you for kicking this off! We just spent a few days debugging because of this, and when I came here to make an issue I encountered this PR 👍🏻 |
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
I believe this still needs an |
preparing for prerelease in gazebosim/sdformat#1212 |
windows and homebrew builds look fine with the new sdformat 13.3.0-pre1 releease. I think ubuntu builds no longer pick up pre-releases? |
Hello, we spent a day working on that too .. ;) |
I opened #304 from the tip of this branch using a special branch name along with gazebo-tooling/gzdev@b7e954c to test with prerelease packages on Ubuntu in our CI |
reminder that we have a plan to simplify this process in gazebo-tooling/gzdev#65 |
Codecov Report
@@ Coverage Diff @@
## gz-sensors7 #289 +/- ##
===============================================
+ Coverage 69.54% 69.61% +0.07%
===============================================
Files 35 35
Lines 3805 3814 +9
===============================================
+ Hits 2646 2655 +9
Misses 1159 1159
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
this branch was updated, so I merged those changes into #304 to update CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs an SDFormat release.
builds look good with the new sdformat 13.3.0 release, merging. |
Signed-off-by: Alejandro Hernández Cordero [email protected]
🦟 Bug fix
Summary
This PR solves the camera projection matrix values not being reflected properly in
/camera_info
topic when the tag is not provided. The PR uses theprojectionToCameraIntrinsic()
fromgz-rendering
to get the camera intrinsics and update the sdf DOM object and/camera_info
topic.Depends on gazebosim/sdformat#1203
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.