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

PlugAlgo : Add array[1] -> scalar conversion #5817

Merged

Conversation

johnhaddon
Copy link
Member

This conversion is available in the various query nodes, and is particularly useful when querying attributes and primitive variables from certain DCCs which like to write everything as arrays, even if they make more sense as scalars.

I've only implemented this for FloatPlug, IntPlug and BoolPlug so far. If there's general support for the idea then I'll update to include other types too. Part of me dislikes the conversion and wishes everything was just typed more logically in the first place. And I worry that the existence of the conversion only for length 1 might lead to a surprise or two for things with animated array lengths. But on balance I think this is probably the most pragmatic thing to do - it lets users deal easily with a bunch of common problem scenarios without resorting to the performance hit of expressions.

@danieldresser-ie
Copy link
Contributor

Yeah, I think I'm in full agreement, both with finding this somewhat uncomfortable, and also in agreeing that this is probably the most pragmatic approach.

The code LGTM.

@murraystevenson
Copy link
Contributor

Ugh. This is quite unfortunate but I'd go for it as it will make things easier for users...

And I worry that the existence of the conversion only for length 1 might lead to a surprise or two for things with animated array lengths

I wonder if returning the first value, rather than no value for lengths > 1 would be more or less surprising in that situation?

@tomc-cinesite
Copy link
Contributor

tomc-cinesite commented Apr 24, 2024

I wonder if returning the first value, rather than no value for lengths > 1 would be more or less surprising in that situation?

+1 for 'the first' if you pick a scalar output for an array input.

@johnhaddon
Copy link
Member Author

I wonder if returning the first value, rather than no value for lengths > 1 would be more or less surprising in that situation?

I'm far less comfortable with applying this to lengths > 1. In my mind we're supporting cases where the user intended to provide a scalar but it has been incorrectly encoded in an array. If there's more than one element in the array then that clearly is not the case, and it's far more ambiguous what the intention might be.

@tomc-cinesite
Copy link
Contributor

I'm far less comfortable with applying this to lengths > 1. In my mind we're supporting cases where the user intended to provide a scalar but it has been incorrectly encoded in an array. If there's more than one element in the array then that clearly is not the case, and it's far more ambiguous what the intention might be.

Fair point.

@johnhaddon
Copy link
Member Author

I've updated the PR with the equivalent conversions for all the other plug types now.

Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

This conversion is available in the various query nodes, and is particularly useful when querying attributes and primitive variables from certain DCCs which like to write everything as arrays, even if they make more sense as scalars.
I can only assume I had them there temporarily for testing, but they certainly don't belong - GafferTest should not depend on GafferScene or GafferImage.
@johnhaddon
Copy link
Member Author

Rebased to fix merge conflict, and merging...

@johnhaddon johnhaddon merged commit 4de8e31 into GafferHQ:1.4_maintenance Apr 25, 2024
5 checks passed
@johnhaddon johnhaddon deleted the arrayScalarConversions branch April 25, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants