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

LightFilterVisualiser : Fix Arnold light blockers #5433

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

ericmehl
Copy link
Collaborator

This fixes a regression in #5421 that prevented Arnold Light blockers from being visualized. I had originally tested to see if the lightShaderNetwork was present and skipped the visualizer if it wasn't. Arnold light blockers don't need a lightShaderNetwork, and I don't suppose would make sense for them to have one (?) so my skip criteria was too aggressive.

I think I'm happy with this solution as-is, but is it worth not returning an empty visualiser set from visualise() and throwing an exception or something else instead?

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.

@murraystevenson murraystevenson changed the base branch from main to 1.3_maintenance August 24, 2023 00:07
Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry, I should have spotted this in the initial PR.

is it worth not returning an empty visualiser set from visualise() and throwing an exception or something else instead?

I think an exception would be going too far - the Viewer would stop showing anything at all then. There might be a case for emitting a warning, but I'm not sure how much value it would have. Any thoughts @murraystevenson?

@murraystevenson
Copy link
Contributor

Any thoughts?

Think I'd be happy to keep the silent approach. There are cases where a warning could be annoying such as when using attribute inheritance to assign the same gobo to multiple spot lights in a group by assigning the gobo to the group itself.

@johnhaddon johnhaddon merged commit 0d222c4 into GafferHQ:1.3_maintenance Aug 29, 2023
4 checks passed
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.

3 participants