gazebosim / gazebo-classic

Gazebo classic. For the latest version, see https://github.com/gazebosim/gz-sim
http://classic.gazebosim.org/
Other
1.21k stars 484 forks source link

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

Closed mogumbo closed 1 year ago

mogumbo commented 1 year ago

🎉 New feature

Closes # not applicable

Summary

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

Test it

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.

mogumbo commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. https://github.com/gazebosim/gazebo-classic/pull/3305/commits/ca6eedcf4e99d991e2db4dc8e7178c24508345b0

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

scpeters commented 1 year ago

@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 https://github.com/gazebosim/gazebo-classic/pull/3305/commits/4bf7d53ecae5aecb0bc0f1dca19bdfd922942446