-
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
Lens flare cleanup and colorization #2927
Lens flare cleanup and colorization #2927
Conversation
…hods to expose scale and color to derived plugins.
float f0 = 1.0/(length(uv-pos)*16.0/scale+1.0); | ||
|
||
f0 = f0+f0*(sin((ang+time/18.0 + noise(abs(ang)+n/2.0)*2.0)*12.0)*.1+dist*.1+.8); | ||
float f0 = 2.0/(length(uv-pos)*16.0/scale+1.0); |
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.
Changed 1.0 to 2.0 here. After removing the legacy animated effect (which was causing ugly visual artifacts) and cc() (which had almost no effect on color), I boosted this number to restore the LensFlare's original appearance.
In my opinion, we should not do this because it makes the LensFlare look washed out and maintains a very big hotspot over the light source. Those features would never have been there if the broken animated effect had never been included.
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.
so do you recommend 1.0
or 2.0
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.
I prefer 1.0 because the legacy animation effect made the whole lens flare look overdone. I think a more realistic lens flare effect would have excluded it. The only reason to keep 2.0 is so that appearance does not change significantly.
|
||
texture_unit RT | ||
{ | ||
tex_coord_set 0 | ||
tex_address_mode border | ||
filtering linear linear linear | ||
} | ||
texture_unit noiseRGBA | ||
{ | ||
texture noise_rgba.png |
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.
we can remove this png file as I don't think it's used else where:
https://github.com/osrf/gazebo/blob/9fba67cafb9838752c29f7a91a12c5e18e9f3dd7/media/materials/textures/noise_rgba.png
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.
done
gazebo/rendering/LensFlare.cc
Outdated
@@ -71,6 +70,13 @@ namespace gazebo | |||
this->scale = _scale; | |||
} | |||
|
|||
/// \brief Set the color of lens flare. | |||
/// \param[in] _color Color of lens flare | |||
public: void SetColor(const ignition::math::Vector3d _color) |
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.
pass by const ref here as well. For consistency with gazebo coding style, can you put &
next to variable name, i.e. &_color
?
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.
done
@@ -3,7 +3,7 @@ compositor CameraLensFlare/Default | |||
technique | |||
{ | |||
// Temporary textures | |||
texture rt0 target_width target_height PF_A8R8G8B8 | |||
texture rt0 target_width target_height PF_FLOAT32_RGB |
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.
Both issues would benefit from an sdf param because most cases can work with a smaller, integer texture. Always using a FLOAT32 texture should work for every case but often waste graphics memory.
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 working on a custom SDFormat parameter in scpeters@8e88380, just need to finish writing the test
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 ready to approve all parts of this pull request except for this line. Can we revert it and come back to it later, perhaps in the context of #2928?
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.
Agreed. Reverted to PF_A8R8G8B8
…is means there will be no compensation for the removal of the legacy animation effect and the lens flares will look less intense.
This reverts commit 1238d34.
plugins/LensFlareSensorPlugin.hh
Outdated
/// <scale> Scale of lens flare. Must be greater than 0 | ||
/// <scale> Scale of lens flare. Must be greater than 0 | ||
/// <color> Color of lens flare. | ||
/// \todo A potentially useful feature would be an option for contantly |
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.
contantly
-> constantly?
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.
fixed
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.
looks good to me.
* Added color setting to LensFlare and LensFlareSensorPlugin. Added methods to expose scale and color to derived plugins. * Removed unused animated lens flare effect. * Removed noise_rgba.png, which is no longer used. * Changed a value in the lens flare shader back to its original 1.0. This means there will be no compensation for the removal of the legacy animation effect and the lens flares will look less intense.
cherry-picking forward to gazebo11 in #2954 |
* Added color setting to LensFlare and LensFlareSensorPlugin. Added methods to expose scale and color to derived plugins. * Removed unused animated lens flare effect. * Removed noise_rgba.png, which is no longer used. * Changed a value in the lens flare shader back to its original 1.0. This means there will be no compensation for the removal of the legacy animation effect and the lens flares will look less intense.
* Added color setting to LensFlare and LensFlareSensorPlugin. Added methods to expose scale and color to derived plugins. * Removed unused animated lens flare effect. * Removed noise_rgba.png, which is no longer used. * Changed a value in the lens flare shader back to its original 1.0. This means there will be no compensation for the removal of the legacy animation effect and the lens flares will look less intense.
No description provided.