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

Fixing WideAngleCamera Lensflare issue #3276

Merged
merged 2 commits into from
Feb 9, 2023
Merged

Fixing WideAngleCamera Lensflare issue #3276

merged 2 commits into from
Feb 9, 2023

Conversation

sanjuksha
Copy link
Contributor

@sanjuksha sanjuksha commented Nov 9, 2022

Signed-off-by: Sanjuksha Nirgude sanjuksha@gmail.com

🦟 Bug fix

#3273

Summary

Porting the same change for gazebo classic from gazebosim/gz-rendering#746 and a fix for wide angle camera not showing lensflare at certain angles. Wide Angle camera used to loop through all cameras to find the occlusion scale which resulted into not rendering lensfalre at certain angles. The fix removes the looping through all cameras and uses the camera facing the light source to calculate occlusion.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Results

Screenshot from 2022-11-15 16-55-46

iche033
iche033 previously approved these changes Nov 10, 2022
gazebo/rendering/WideAngleCamera.cc Outdated Show resolved Hide resolved
@sanjuksha
Copy link
Contributor Author

Added one more change for Z check based on this comment #3273 (comment)

@sanjuksha sanjuksha changed the title Fixing Z check in WideAngleCamera::Project3d Fixing WideAngleCamera Lensflare issue Nov 16, 2022
@scpeters
Copy link
Member

cc @kjeppesen1 @mogumbo

@kjeppesen1
Copy link
Contributor

This fix works for me 👍 I moved around a pair of wideangle cameras at various angles and positions in a world with a DEM, and I no longer see the lens flare disappearing when it isn't supposed to.

gazebo/rendering/LensFlare.cc Outdated Show resolved Hide resolved
gazebo/rendering/LensFlare.cc Outdated Show resolved Hide resolved
gazebo/rendering/LensFlare.cc Outdated Show resolved Hide resolved
double occlusionScale = 1.0;
if (lightPos.z >= 0.0)
{
// loop through all env cameras
for (auto cam : ogreEnvCameras)
{
// project light world point to camera clip space.
auto viewProj = cam->getProjectionMatrix() * cam->getViewMatrix();
Copy link
Member

Choose a reason for hiding this comment

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

replace cam with _wideAngleCam two times on this line

Copy link
Contributor

@adityapande-1995 adityapande-1995 Dec 27, 2022

Choose a reason for hiding this comment

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

Hmm, _wideAngleCam doesn't have getProjectionMatrix() method here.

this->dataPtr->lightWorldPos);
break;
ignition::math::Pose3d(
Conversions::ConvertIgn(cam->getDerivedPosition()),
Copy link
Member

Choose a reason for hiding this comment

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

replace cam with _wideAngleCam on this and the following line

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, _wideAngleCam doesn't have getDerivedPosition() here

std::vector<Ogre::Camera *> ogreEnvCameras = _wideAngleCam->OgreEnvCameras();

// set dummy camera properties based on env cam
Ogre::Camera *cam = ogreEnvCameras[0];
Copy link
Member

Choose a reason for hiding this comment

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

I think the cam and ogreEnvCameras variables can be removed if all instances of cam are replaced by _wideAngleCam

Copy link
Contributor

Choose a reason for hiding this comment

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

actually just checking if cam should be replaced by _wideAngleCam in the calls below. I remember I had to use cam (one of the env cameras) because it has the right properties for doing ray casting. Can you do a quick check and print out the viewport size, FOV, and aspect ratio for cam vs _wideAngleCam? I think they could be different.

Copy link
Member

Choose a reason for hiding this comment

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

that's a good point

@iche033 iche033 dismissed their stale review November 22, 2022 20:13

more changes to review

@scpeters
Copy link
Member

I've restarted the windows build

Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

It would be nice to add an example world or a test which has the camera in various poses, showing when the lensflare is supposed to appear and when it isn't.

gazebo/rendering/WideAngleCamera.cc Outdated Show resolved Hide resolved
@scpeters
Copy link
Member

I cleaned up some of the code changes in 5740ce5 and adf7d1f and then made an adjustment to the new test in c63047c to make it easier to confirm the presence of the bug without the fix in this branch

@scpeters
Copy link
Member

scpeters commented Jan 31, 2023

Thanks to some debugging help from @iche033, I added some additional cameras in d33593d and c92f948 to illustrate some problems with the occlusion checking in the test world added in this pull request

gazebo --verbose worlds/lensflare_wideangle_cam.world

Failure to occlude in wide_angle_cameras_occluded_high camera image

There is a box directly in front of the camera, so it should occlude but doesn't

Screen Shot 2023-01-31 at 3 26 07 PM

Occluding when it shouldn't in wide_angle_cameras_occluded_higher camera image

There is a box to the right of the camera (partially visible), but it should not be occluding, but the lens flare is not there

Screen Shot 2023-01-31 at 3 26 20 PM

It seems to be checking for occlusions in the wrong direction?

sanjuksha and others added 2 commits February 9, 2023 10:39
* Fixing Z check in WideAngleCamera::Project3d

Signed-off-by: Sanjuksha Nirgude <sanjuksha@gmail.com>

Added a test case for wideangle camera lensflare

Signed-off-by: Aditya <aditya050995@gmail.com>

Add extra model in test world

The pre-existing model in the test world exhibits
the lens flare rendering bug without the fix in
this branch, so I added an extra model at a different
pose that does not exhibit the bug. It makes it
easier to confirm that the bug is fixed.

* Add extra cameras with occluded views
* Add another camera with box to its side that
  is incorrectly occluded

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
* 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.
@scpeters scpeters merged commit e1f7f1f into gazebo11 Feb 9, 2023
@scpeters scpeters deleted the ZFix branch February 9, 2023 23:22
scpeters pushed a commit that referenced this pull request Feb 9, 2023
* Fixing Z check in WideAngleCamera::Project3d

Signed-off-by: Sanjuksha Nirgude <sanjuksha@gmail.com>

Added a test case for wideangle camera lensflare

Signed-off-by: Aditya <aditya050995@gmail.com>

Add extra model in test world

The pre-existing model in the test world exhibits
the lens flare rendering bug without the fix in
this branch, so I added an extra model at a different
pose that does not exhibit the bug. It makes it
easier to confirm that the bug is fixed.

* Add extra cameras with occluded views
* Add another camera with box to its side that
  is incorrectly occluded

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
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.

7 participants