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

UsdOutput::IsOutput should check attr validity #2877

Merged

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Dec 18, 2023

Description of Change(s)

UsdShadeOutput::IsOutput should be checking for attr validity like UsdShadeInput::IsInput does.
Thanks to Maddy Adams for catching this and suggesting the fix.

Fixes Issue(s)

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@jesschimein
Copy link
Contributor

Filed as internal issue #USD-9105

@FlorianZ
Copy link
Contributor

FlorianZ commented Jan 8, 2024

Hi @dgovil. As a pattern, we try to not do a lot of validity checking in these types of API calls. In this case, for example, we expect the client to supply an already valid UsdAttribute. If we made this change, we think there is a ton of other API that would have to be changed to now follow a similar pattern, or otherwise we run the risk of making things behave inconsistently and muddying the waters in terms of whether or not the call site needs to check for validity. Does that make sense? What are your thoughts?

@FlorianZ
Copy link
Contributor

FlorianZ commented Jan 8, 2024

Wait, I just saw your comment on #2876, and it looks like we are already being inconsistent about what I said we wanted to be consistent about ;-)

Okay, so ignore my previous comment. We'll discuss internally, but it sounds like accepting the PR would make this more consistent actually...

@dgovil
Copy link
Collaborator Author

dgovil commented Jan 8, 2024

Edit: oops missed your follow up comment.


I totally see where you're coming from but I would argue that this change would bring some symmetry and consistency to the API.

IsInput already checks for validity, which in turn means that one can follow the same pattern as prims to pass the Input object from a GetInput call to an if check.

GetOutput would be the outlier here since it doesn't follow its counterpart.

I wouldn't advocate adding the check elsewhere based on this PR, but I do think this would make validation checking on an output feel more consistent with the rest of the USD API.

@pixar-oss pixar-oss merged commit 402e517 into PixarAnimationStudios:dev Jan 16, 2024
5 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.

4 participants