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

Consider *all* attributes for UsdImagingCameraAdapter::TrackVariability #1798

Merged

Conversation

robpieke
Copy link
Contributor

@robpieke robpieke commented Mar 7, 2022

Description of Change(s)

Copying behaviour from lightAdapter.cpp verbatim (other than changing HdLight to HdCamera and light to camera) to allow all attributes (e.g., custom attributes) to be recognised in TrackVariability.

This may become meaningless with future versions/refactors of Hydra, but wanted to submit for completeness as we're currently using this patch.

Fixes Issue(s)

@jilliene
Copy link

jilliene commented Mar 7, 2022

Filed as internal issue #USD-7259

@spiffmon
Copy link
Member

spiffmon commented Mar 7, 2022

@robpieke , I don't know whether pragmatically we'll want to accept this change, but I wanted to point out that the two cases are not actually equivalent, but you can't tell due to unfortunate code evolution. I just did the archeology, and that lightAdapter code predates the change to make lights connectable. I believe the right thing to do there is to consider only properties in the "inputs:" namespace.

The reason we don't want to just consider all custom attributes is because we might wind up paying dearly for custom pipeline data that is irrelevant to imaging. The drum we've been beating is that any additional behaviors should be captured by Applied schemas, and new Hydra will allow you to attach behaviors for them that combine/extend the base adapters.

But since you can't do that yet, accepting this may be a short-term compromise, but I'm not sure.

@robpieke
Copy link
Contributor Author

robpieke commented Mar 8, 2022

Thanks @spiffmon ... we needed a solution to handle animated background plates, so it was either extending the hardcoded list of attributes to check, or a catch-all. Whether we're unique, or whether this is a broader community issue ... I won't venture a guess. But, either way, we wanted to be transparent and share "this is what we're currently doing".

Looking forward to new Hydra :)

@tcauchois
Copy link
Contributor

Hey Rob! I don't know if you're all set in your tree; and like Spiff mentioned, the plan in the future is to add custom behaviors like yours via API schemas which get imaging adapters. But until that lands, if you're interested in merging this I left a few notes. After those are addressed this seems like a good short-term workaround.

@robpieke
Copy link
Contributor Author

robpieke commented May 2, 2022

Hopefully I've addressed all the feedback now :)

@tcauchois
Copy link
Contributor

Yeah, thanks :).

@FlorianZ
Copy link
Contributor

FlorianZ commented May 4, 2022

Hi @robpieke. I wasn't able to find your signed CLA on our end. Did you submit one by chance? Thank you!!

@robpieke
Copy link
Contributor Author

robpieke commented May 4, 2022

Hi @robpieke. I wasn't able to find your signed CLA on our end. Did you submit one by chance? Thank you!!

Hey @FlorianZ ... I thought I'd been added to the SideFX CLA but, in case not, I've just emailed over an individual CLA. Sorry for any confusion/delay!

@FlorianZ
Copy link
Contributor

FlorianZ commented May 4, 2022

You are right @robpieke! I updated our records to show that you're on the SideFX CLA. Sorry about that.

@sunyab sunyab changed the base branch from release to dev May 9, 2022 19:24
@pixar-oss pixar-oss merged commit 7ae71f8 into PixarAnimationStudios:dev May 16, 2022
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.

7 participants