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

Allow user to name a specific light that will generate lens flare. #3305

Merged
merged 4 commits into from
Mar 29, 2023

Conversation

mogumbo
Copy link
Contributor

@mogumbo mogumbo commented Mar 14, 2023

🎉 New feature

Closes # not applicable

Summary

Allow user to name a specific light that will generate lens flare.

Test it

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.

@mogumbo
Copy link
Contributor Author

mogumbo commented Mar 14, 2023

This code doesn't work yet. It tries to select the light named by the user, but sometimes that light has not been added to the scene before the first run of LensFlare::Update(). In that case the first directional light is selected and Update() is never called again: https://github.com/gazebosim/gazebo-classic/pull/3305/files#diff-49074ecb941c9d3ea68dec38fb5d75cd9fe66f3f7a0822715dd11e4845d8189dR601

I would like to be able to be notified when the named light is added to the scene and then run Update() again, but I haven't found a way to do this.

An alternative is to only stop running Update() once the named light is found, if there is one:

  // disconnect
  if (this->dataPtr->lightName.empty() ||
      this->dataPtr->lightName == this->dataPtr->lightNameCurrentlyInUse)
  {
    this->dataPtr->preRenderConnection.reset();
  }

This introduces a bit of overhead while Update() is repeatedly running. It probably wouldn't be an issue most of the time as developers would not ordinarily name a light if they didn't plan on adding it to the scene.

@scpeters
Copy link
Member

I think the LensFlare should only use the first directional light if you haven't specified the name of a light, so it would just wait for that named light to appear. I think that would be easier?

@mogumbo
Copy link
Contributor Author

mogumbo commented Mar 17, 2023

I think the LensFlare should only use the first directional light if you haven't specified the name of a light, so it would just wait for that named light to appear. I think that would be easier?

Sounds good to me. Just implemented this and it works in my sims with either a named light and a directional light. ca6eedc

@scpeters Please promote this to non-draft PR if you like the changes.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters marked this pull request as ready for review March 21, 2023 07:11
@scpeters
Copy link
Member

@scpeters Please promote this to non-draft PR if you like the changes.

I like the changes, so I've marked this ready for review. I also added an example demonstrating this feature in 4bf7d53

@@ -40,6 +40,9 @@ namespace gazebo
/// \brief Pointer to light
public: LightPtr light;

/// \brief Pointer to light
public: std::string lightName;
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 this variable is not used in the LensFlareCompositorListenerPrivate. If not we can remove it and the corresponding LensFlareCompositorListener::SetLightName method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -86,6 +89,12 @@ namespace gazebo
this->dataPtr->light = _light;
}

//////////////////////////////////////////////////
void LensFlareCompositorListener::SetLightName(std::string _name)
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 this method isn't needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

public: void SetLight(LightPtr _light);

/// \brief Set the name of light that generates lens flare
/// \param[in] _name Light that generates lens flare
public: void SetLightName(std::string _name);
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 this can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@scpeters scpeters merged commit 776b419 into gazebosim:gazebo11 Mar 29, 2023
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.

2 participants