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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Lens Flare System #1933

Merged
merged 6 commits into from Mar 17, 2023
Merged

Conversation

jasmeet0915
Copy link
Contributor

馃帀 New feature

Closes #1909

Summary

  • Added Lens Flare System that adds Lens Flare Render Pass to the camera and allows users to enable lens flare in camera images. The lens flares can be configured using <scale>, <color>, and <occlusion_steps> tags in the SDF.
  • Added an example camera_lens_flare.sdf file for testing which contains 2 cameras(wide angle and normal) with different configurations for the lens flare (one default and other custom).

ezgif com-video-to-gif (1)

Test it

  • Make sure to use the gz-sim8 and compatible versions of other packages especially gz-rendering8 since it contains the Lens Flare Pass.
  • Launch the examples/worlds/camera_lens_flare.sdf file from the gz-sim repo using:
gz sim camera_lens_flare.sdf

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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

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.

馃巿 Release

Preparation for <X.Y.Z> release.

Comparison to <x.y.z>: https://github.com/gazebosim//compare/<LATEST_TAG_BRANCH>...<RELEASE_BRANCH>

Needed by <PR(s)>

Checklist

  • Asked team if this is a good time for a release
  • There are no changes to be ported from the previous major version
  • No PRs targeted at this major version are close to getting in
  • Bumped minor for new features, patch for bug fixes
  • Updated changelog
  • Updated migration guide (as needed)
  • Link to PR updating dependency versions in appropriate repository in gazebo-release (as needed):

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.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

nice, this works well for me! I left a few comments, mostly coding style changes.

examples/worlds/camera_lens_flare.sdf Outdated Show resolved Hide resolved
@@ -0,0 +1,244 @@
/*
* Copyright (C) 2020 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

2020 -> 2023

@@ -0,0 +1,73 @@
/*
* Copyright (C) 2020 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

2020 -> 2023

@@ -0,0 +1,103 @@
/*
* Copyright (C) 2022 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

2022 -> 2023

}



Copy link
Contributor

Choose a reason for hiding this comment

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

remove a couple of extra lines here

<far>100</far>
</clip>
<lens>
<type>gnomonical</type>
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm just noticed that there could an issue with the gnomonical type on certain graphics drivers, e.g. on mesa + llvmpipe the camera does not render properly but works fine on Nvidia drivers. Something to be investigated later. In the mean time, can you change this to equidistant? and maybe make the the horizontal_fov larger to make it obvious it's a wide angle camera, e.g. 3.14159

// call function that connects to post render event
// using a separate function because unique_ptr (this->dataPtr)
// is not supported as a argument of std::bind
this->dataPtr->ConnectToPostRender();
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to do:

    this->dataPtr->postRenderConn =
        this->dataPtr->eventMgr->Connect<events::PostRender>(
        std::bind(&LensFlarePrivate::OnPostRender, this->dataPtr.get()));

Copy link
Contributor

Choose a reason for hiding this comment

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

to be consistent with gazebo coding style, can you change indentation from 4 spaces to 2 spaces in this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you missed this one. Can you update the indentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add this test to CMakeLists.txt here?

@iche033
Copy link
Contributor

iche033 commented Mar 16, 2023

can you also sign your commits?

Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
 - Adds doxygen comments in LensFlare.hh

Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
@jasmeet0915
Copy link
Contributor Author

@iche033 I have made all the suggested changes in the latest commit and have signed all previous commits as well 鉁旓笍

@@ -11,6 +11,7 @@ set(tests
breadcrumbs.cc
buoyancy.cc
buoyancy_engine.cc
camera_lens_flare.cc
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move this line to the tests_needing_display list below?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you missed this one. Can you update the indentation?

 - Updated LensFlare.cc indentation to 2 spaces

Signed-off-by: Jasmeet Singh <jasmeet0915@gmail.com>
@jasmeet0915
Copy link
Contributor Author

@iche033 Not sure how I missed that one 馃槄. Anyways, I have made the required changes.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

awesome, thanks for your contribution!

Core development automation moved this from Inbox to In review Mar 17, 2023
@iche033 iche033 merged commit 309906a into gazebosim:main Mar 17, 2023
2 of 4 checks passed
Core development automation moved this from In review to Done Mar 17, 2023
@azeey azeey moved this from Done to Highlights in Core development Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃幍 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants