Skip to content

Fix get stronger layer layers not found #4176

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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

adubois-spin
Copy link

This request is about corner cases regarding allowing the edit of a layer metadata.

getStrongerLayer has corner cases for which it returns a null layer instead of one of the two passed as argument.
The idea is to allow editing in those cases.

This was our first way of solving our issue where the fact that the targetLayer could not be found was making the problem arise.
We then realized that the reason why our layer could not be found when iterating through the sub layers is because this traversal was not done with the current resolver context.

Both fix are in this branch. We would be glad if the resolver context could be at least added to the call. We think this resolver context problem might occur in other parts of maya-usd.

@seando-adsk seando-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Apr 2, 2025
@seando-adsk
Copy link
Collaborator

@adubois-spin Thanks for your interest in contributing to our maya-usd repo, before we can accept PR from a new contributor, we need you to fill, sign and email back the proper CLA (individual or corporate). See CONTRIBUTING.md for details about CLA.

@adubois-spin
Copy link
Author

@adubois-spin Thanks for your interest in contributing to our maya-usd repo, before we can accept PR from a new contributor, we need you to fill, sign and email back the proper CLA (individual or corporate). See CONTRIBUTING.md for details about CLA.

@seando-adsk Thank you for your quick answer. Okay let me see with my studio so we sign this.

@seando-adsk seando-adsk added workflows Related to in-context workflows and removed do-not-merge-yet Development is not finished, PR not ready for merge labels Apr 4, 2025
* The 2nd case appears mostly if the getStrongerLayer function fails to find which layer is stronger,
* returning an empty layer reference.
*/
bool allowed = (strongestLayer != topAuthoredLayer);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the check.It does not even use the targeted layer. Also, it is confusing because the inequality comparison to the topAuthoredLayer seems opposite of what we would want, but is done as a "hidden" check that strongestLayer is null.

The old check must stay but other checks could be added if needed.

The correct way to write your change would be something like:

bool allowed = (strongestLayer == targetLayer || !strongestLayer);

... but even that I'm not sure we want, IIRC, the reason we didn't allow editing when the strongest layer is null was that because this indicates the layer is a non-local layer and we did not want to allow modifying references. I think this could be changed but I'll confirm with designers.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Indeed bool allowed = (strongestLayer == targetLayer || !strongestLayer); is more correct than my change.
  2. As for if we want it or not, it depends. My colleagues and I were expecting more bugs as the one we had with the context so we suggested to be more lax, which is not necessarily a good thing in the long run... It could use at least a warning if we keep it this way imo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants