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

Cycles : Correct setting of background light's lightgroup #5396

Conversation

boberfly
Copy link
Collaborator

Generally describe what this PR will do, and why it is needed

  • The lightgroup name wasn't being set on background lights properly
  • Added the lightgroup option on CyclesOptions for if CyclesBackground is being used instead of a background light
  • From what Brecht has said on their roadmap, they will allow more than one background light and lightgroup name each, so we should re-visit this for when that happens
  • Added a unit-test to verify that the lightgroup is being picked up

Related issues

  • NA

Dependencies

  • NA

Breaking changes

  • NA

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@boberfly boberfly force-pushed the fixes/cyclesBackgroundLightgroup branch 3 times, most recently from 16f8630 to d5a79d3 Compare July 16, 2023 13:27
… due to 3.5.0 code changes, as well as adding a way to set a lightgroup from CyclesOptions when a CyclesBackground is being used instead.
@johnhaddon
Copy link
Member

Closing in favour of #5945. I think it's reasonable to omit the cycles:background:lightgroup option as that PR does, with the guidance being "if you need a light group, use a light". Feel free to reopen the discussion if you think there's a good case otherwise...

@johnhaddon johnhaddon closed this Jul 9, 2024
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