-
Notifications
You must be signed in to change notification settings - Fork 486
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
Improvements to WideAngleCamera LensFlare bug fix #3296
Conversation
…leCamera LensFlare results. Fixed small related bugs.
…ng of lens flare.
{ | ||
// The ogreEnvCamera and wideAngleDummyCamera used here both | ||
// transform from gazebo z-up coords to Ogre y-up coords. If the |
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.
In retrospect, describing this as "transform from gazebo z-up coords to Ogre y-up coords" might not be accurate. Any insight into what this transform is really doing would be appreciated. It can be found here https://github.com/gazebosim/gazebo-classic/blob/gazebo11/gazebo/rendering/Camera.cc#L1585-L1586 and here https://github.com/gazebosim/gazebo-classic/blob/gazebo11/gazebo/rendering/WideAngleCamera.cc#L590-L591
I looked at a few ways to reverse one of the transforms seen in the above links. I would have preferred to override one of those methods such that one of the transforms is never made in the first place, but I see didn't an easy way to do this. My current solution probably uses much less code than such an approach.
@@ -279,19 +283,41 @@ namespace gazebo | |||
pos.y /= pos.w; | |||
// check if light is visible | |||
if (std::fabs(pos.x) <= 1 && | |||
std::fabs(pos.y) <= 1 && pos.z > abs(pos.w)) | |||
std::fabs(pos.y) <= 1 && pos.z > -abs(pos.w)) |
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 know this check doesn't match the one done in WideAngleCamera.cc, but I think that was a bug noticed by Matias. I think @iche033 can confirm
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 don't see any difference in behavior in practice. If we set this back to positive, can someone please explain why? If it is inconsistent with the code in WideAngleCamera.cc it should have a comment explaining why.
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.
actually, looking at comparable code in gz-rendering, it seems like maybe it should be as Terry has changed it here z > -abs(w)
?
and should it be fabs(w)
?
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'll merge this into the ZFix
branch and then follow up with further changes there. Thanks for fixing it!
* Reverse extraneous z-up to y-up transform that was corrupting WideAngleCamera LensFlare results. Fixed small related bugs. * Use fisheye cameras in lensflare_wideangle_cam.world to improve testing of lens flare.
* Reverse extraneous z-up to y-up transform that was corrupting WideAngleCamera LensFlare results. Fixed small related bugs. * Use fisheye cameras in lensflare_wideangle_cam.world to improve testing of lens flare.
🦟 Bug fix
Fixes #3273
Summary
This PR improves an existing bug fix branch. Test with and without this PR by running
gazebo --verbose worlds/lensflare_wideangle_cam.world
and look at the various camera images in Topic Visualization.Additionally, I tried posing the cameras and obstacles in that world file many different ways. Lens flares appear to behave as they should.
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.