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

Use separate UsdStageCache's for stages that loadAll or loadNone. #519

Merged
merged 4 commits into from
May 26, 2020

Conversation

williamkrick
Copy link
Contributor

@williamkrick williamkrick commented May 21, 2020

Rename forcePopulate to loadAll to match the enum name. Remove the default parameter value for loadAll so future callers have to make a choice, they will be more likely to choose correctly.

The stage cache does know about different session layers and so it won't hit the cache if the session layers don't match. The interesting code is at https://github.com/PixarAnimationStudios/USD/blob/master/pxr/usd/usd/stage.cpp#L975.

There you can see the call to the cache to FindOneMatching takes all the arguments except for the InitialLoadSet.

…ame forcePopulate to loadAll to match the enum name. Remove the default parameter value for loadAll so future callers have to make a choice, they will be more likely to choose correctly.
@williamkrick williamkrick added proxy Related to base proxy shape workflows Related to in-context workflows labels May 21, 2020
@kxl-adsk kxl-adsk requested a review from mattyjams May 21, 2020 20:24
Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

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

Just some minor cosmetic comments, but otherwise looks good to me!

Worth noting though that any maya-usd client code with parameter-less usages of mayaUsd.lib.StageCache.Get() in Python will need to be updated following this change.

@williamkrick
Copy link
Contributor Author

williamkrick commented May 22, 2020

Just some minor cosmetic comments, but otherwise looks good to me!

Worth noting though that any maya-usd client code with parameter-less usages of mayaUsd.lib.StageCache.Get() in Python will need to be updated following this change.

I've updated the only test which is calling mayaUsd.lib.StageCache.Get(), I don't see any more uses in maya-usd.

Removing that default value is a breaking change from any users point of view, @kxl-adsk do we have a process to notify customers about changes that may break their existing scripts that are outside of maya-usd? How can I make this as painless as possible for users?

At the end of the day I don't really need to remove the default parameter, so if we want to avoid breaking changes we can do it. We will need to break the "python API" at some point, if we don't have a process in place should we start thinking about it now?

Flagged this one do-not-merge-yet until we come to a decision on this issue.

…mous layer. Improve documentation for UsdMayaStageCache.
@williamkrick williamkrick added the do-not-merge-yet Development is not finished, PR not ready for merge label May 22, 2020
@mattyjams
Copy link
Contributor

Removing that default value is a breaking change from any users point of view, @kxl-adsk do we have a process to notify customers about changes that may break their existing scripts that are outside of maya-usd? How can I make this as painless as possible for users?

At the end of the day I don't really need to remove the default parameter, so if we want to avoid breaking changes we can do it. We will need to break the "python API" at some point, if we don't have a process in place should we start thinking about it now?

Flagged this one do-not-merge-yet until we come to a decision on this issue.

Thanks @williamkrick! Yeah, to be sure, I have no objection at all to the API change, and it's obviously easy for calling code to absorb. Just wanted raise the question as to the process, since I'm sure this won't be the first such case of this. It doesn't need to gate this PR, but definitely worth thinking about as we move forward.

@williamkrick
Copy link
Contributor Author

ecg-mayausd-branch-preflight-master-public run 93

@kxl-adsk
Copy link

We don't currently have any good place to announce such changes. All existing channels won't be able to reach everybody. Something for TSC to discuss - note taken.

For this particular one, I doubt many people will be affected other than Pixar. So if @mattyjams you are cool with it, I wouldn't change plans.

@williamkrick
Copy link
Contributor Author

We don't currently have any good place to announce such changes. All existing channels won't be able to reach everybody. Something for TSC to discuss - note taken.

For this particular one, I doubt many people will be affected other than Pixar. So if @mattyjams you are cool with it, I wouldn't change plans.

If y'all are happy that this change is minor enough that we don't need to wait on the notification channel being ready I'm fine with that. We just need to finalize the discussion in #519 (comment) before I'm ready to merge.

@williamkrick williamkrick removed the do-not-merge-yet Development is not finished, PR not ready for merge label May 25, 2020
@williamkrick
Copy link
Contributor Author

If y'all are happy that this change is minor enough that we don't need to wait on the notification channel being ready I'm fine with that. We just need to finalize the discussion in #519 (comment) before I'm ready to merge.

I'm not totally happy with the state of stage caching right now, but I think there is a bigger discussion on stage caching that we need to have before we can continue, and most of the issues are not a high priority for us right now. This change makes things incrementally better, but not perfect, so I feel it can be merged now.

@williamkrick
Copy link
Contributor Author

ecg-mayausd-branch-preflight-master-public number 105

@kxl-adsk kxl-adsk merged commit 324e1ca into dev May 26, 2020
@kxl-adsk kxl-adsk deleted the krickw/MAYA-103693/load_payloads branch May 26, 2020 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proxy Related to base proxy shape workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants