-
Notifications
You must be signed in to change notification settings - Fork 277
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
Add Lens Flare System #1933
Add Lens Flare System #1933
Conversation
3693e6f
to
433e4ea
Compare
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.
nice, this works well for me! I left a few comments, mostly coding style changes.
src/systems/lens_flare/LensFlare.cc
Outdated
@@ -0,0 +1,244 @@ | |||
/* | |||
* Copyright (C) 2020 Open Source Robotics Foundation |
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.
2020
-> 2023
src/systems/lens_flare/LensFlare.hh
Outdated
@@ -0,0 +1,73 @@ | |||
/* | |||
* Copyright (C) 2020 Open Source Robotics Foundation |
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.
2020
-> 2023
@@ -0,0 +1,103 @@ | |||
/* | |||
* Copyright (C) 2022 Open Source Robotics Foundation |
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.
2022
-> 2023
src/systems/lens_flare/LensFlare.cc
Outdated
} | ||
|
||
|
||
|
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 a couple of extra lines here
<far>100</far> | ||
</clip> | ||
<lens> | ||
<type>gnomonical</type> |
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.
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
src/systems/lens_flare/LensFlare.cc
Outdated
// 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(); |
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.
you should be able to do:
this->dataPtr->postRenderConn =
this->dataPtr->eventMgr->Connect<events::PostRender>(
std::bind(&LensFlarePrivate::OnPostRender, this->dataPtr.get()));
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.
to be consistent with gazebo coding style, can you change indentation from 4 spaces to 2 spaces in this file?
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 think you missed this one. Can you update the indentation?
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.
can you add this test to CMakeLists.txt here?
can you also sign your commits? |
Signed-off-by: Jasmeet Singh <[email protected]>
Signed-off-by: Jasmeet Singh <[email protected]>
- Adds doxygen comments in LensFlare.hh Signed-off-by: Jasmeet Singh <[email protected]>
Signed-off-by: Jasmeet Singh <[email protected]>
433e4ea
to
4a6a9c9
Compare
Signed-off-by: Jasmeet Singh <[email protected]>
@iche033 I have made all the suggested changes in the latest commit and have signed all previous commits as well ✔️ |
test/integration/CMakeLists.txt
Outdated
@@ -11,6 +11,7 @@ set(tests | |||
breadcrumbs.cc | |||
buoyancy.cc | |||
buoyancy_engine.cc | |||
camera_lens_flare.cc |
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.
can you move this line to the tests_needing_display
list below?
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 think you missed this one. Can you update the indentation?
- Updated LensFlare.cc indentation to 2 spaces Signed-off-by: Jasmeet Singh <[email protected]>
@iche033 Not sure how I missed that one 😅. Anyways, I have made the required changes. |
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.
awesome, thanks for your contribution!
🎉 New feature
Closes #1909
Summary
<scale>
,<color>
, and<occlusion_steps>
tags in the SDF.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).Test it
gz-sim8
and compatible versions of other packages especiallygz-rendering8
since it contains the Lens Flare Pass.examples/worlds/camera_lens_flare.sdf
file from thegz-sim
repo using: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.🎈 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
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.