-
Notifications
You must be signed in to change notification settings - Fork 202
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
MAYA-128407 fix edit-target loops in proxy shape compute #2955
Merged
+103
−44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The problem is recursive interactions between multiple actors. First actor: the ProxyAccessor class. This class is only vaguely documented, that is there is a mechanical description but not an explanation of why it needs to exists nor why it is written as it is. It's mechanical purpose, as I understand it from its docs, is to copy input plugs into the proxy shape to stage data and copy stage data to output plugs. For some reason which are not explained, it writes its data to the session layer. To do so, it temporarily sets the target layer to the session layer. Second actor: the ProxyShapeBase class. The class is the representation of the USD stage as a Maya node. As part if its computation, it creates the stage. It holds multiple attributes to do so: - The root layer - The session layer - The list of muted layers - The load/unload state of various payload - The current edit target layer. And, in its implementation of its computation uses the ProxyAccessor. So, computing the ProxyShapeBase use a ProxyAccessor, which switch the target layer. Third actor: the Edit Target Notifications In order for the ProxyShapeBase to set the correct edit target when computed, it needs to know when the edit target has changed. Otherwise, after the user changed the edit target, recomputing the ProxyShapeBase would set the edit target to the old target. So the ProxyShapeBase register a notification listener to be told when the edit target changes. When the edit target changes, the notification listener sets the edit-target attribute on the ProxyShapeBase so that it sets the correct target if it is recomputed. End Result So, the sequence of events is: - The ProxyShapeBase is recomputed. - The ProxyAccessor changes the target to the session layer. - This trigger the edit-target change notification - This updates the ProxyShapeBase attribute - This dirties the ProxyShapeBase - The ProxyShapeBase compute ends. - This destroys the ProxyAccessor - This reset the edit target to its previous value. - This trigger the edit-target change notification - This updates the ProxyShapeBase attribute - This dirties the ProxyShapeBase - And... now the ProxyShapeBase will get recomputed again when accessed. There are two ways to break the chain: - Detect temporary changes in the edit target and ignore them. - Not dirty the ProxyShapeBase when the edit target change. The first one would be dangerous since if a recompute is triggered at that moment, it would use the incorrect edit target. OTOH, it is unclear what is the correct edit target at that point: is it the temporary edit target set by the proxy accessor or the edit target set by the user? The second one can be done by keeping the current edit target in a member variable in the ProxyShapeBase and only use the attribute at load time to initially fill that variable, say on the first compute. I think that is what I will do to fix the problem. - Add a member variabel to ProxyShapeBase to keep track of the current edit target. - Fill it on first cmpute. - Update it when the edit target change, via the notifications. - Only save the edit target on save (this was already existing). - Refactor edit-target helper functions to support all this.
When the shared flag of the proxy shape is toggled, the session layer changes, so we need to update the edit target if the target was on the session layer.
vlasovi
approved these changes
Mar 21, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for removing the "on idle" callback.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
adsk
Related to Autodesk plugin
bug
Something isn't working
ready-for-merge
Development process is finished, PR is ready for merge
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem is recursive interactions between multiple actors.
First actor: the ProxyAccessor class.
This class is only vaguely documented, that is there is a mechanical description but not an explanation of why it needs to exists nor why it is written as it is. It's mechanical purpose, as I understand it from its docs, is to copy input plugs into the proxy shape to stage data and copy stage data to output plugs. For some reason which are not explained, it writes its data to the session layer. To do so, it temporarily sets the target layer to the session layer.
Second actor: the ProxyShapeBase class.
The class is the representation of the USD stage as a Maya node. As part if its computation, it creates the stage. It holds multiple attributes to do so:
And, in its implementation of its computation uses the ProxyAccessor. So, computing the ProxyShapeBase use a ProxyAccessor, which switch the target layer.
Third actor: the Edit Target Notifications
In order for the ProxyShapeBase to set the correct edit target when computed, it needs to know when the edit target has changed. Otherwise, after the user changed the edit target, recomputing the ProxyShapeBase would set the edit target to the old target. So the ProxyShapeBase register a notification listener to be told when the edit target changes. When the edit target changes, the notification listener sets the edit-target attribute on the ProxyShapeBase so that it sets the correct target if it is recomputed.
End Result
So, the sequence of events is:
There are two ways to break the chain:
The first one would be dangerous since if a recompute is triggered at that moment, it would use the incorrect edit target. OTOH, it is unclear what is the correct edit target at that point: is it the temporary edit target set by the proxy accessor or the edit target set by the user?
The second one can be done by keeping the current edit target in a member variable in the ProxyShapeBase and only use the attribute at load time to initially fill that variable, say on the first compute. I think that is what I will do to fix the problem.