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

EMSUSD-2084 - adding or removing of items in collections should respect the edit restrictions as other attributes #4143

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

Conversation

haberlu
Copy link
Collaborator

@haberlu haberlu commented Mar 5, 2025

Adding isRelationshipEditAllowed() to UsdUfe.

This function can be used to determine if a list of paths could be added to or removed from a collection within the current edit target.

@haberlu haberlu requested a review from seando-adsk March 5, 2025 14:54
@haberlu haberlu self-assigned this Mar 5, 2025
@haberlu haberlu assigned haberlu and unassigned haberlu Mar 5, 2025
@haberlu haberlu assigned haberlu and unassigned haberlu Mar 5, 2025
@haberlu haberlu force-pushed the haberlu/EMSUSD-2084_edit_restrictions_in_relationships branch from d421a20 to 83d1799 Compare March 5, 2025 18:50
@haberlu haberlu assigned haberlu and unassigned haberlu Mar 5, 2025
@haberlu haberlu assigned haberlu and unassigned haberlu Mar 6, 2025
@haberlu haberlu requested a review from brossee-adsk March 10, 2025 09:08
seando-adsk
seando-adsk previously approved these changes Mar 10, 2025
Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the unit test.

@seando-adsk
Copy link
Collaborator

Added Pierre as reviewer as he knows a lot more about edit restrictions that I do.

Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk left a comment

Choose a reason for hiding this comment

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

I think the wrapping code list update is error-prone and could be made simpler, but otherwise this looks fine.

//! \param errMsg The optional error message to be filled in case of any target
//! not being able to be added/removed.
//! \return True, if one ore more of the targets given are allowed to be
//! added/removed to the relationship in the stage's local Layer Stack.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we should require that all desired changes be allowed? Was that specified in the story that we allow if a single one is correct and all the other incorrect?

result = UsdUfe::isRelationshipEditAllowed(
relationship, &targetsToAddVec, &targetsToRemoveVec, &errMsg);

// pass the removal of elements to the python lists
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it not be much simpler to copy the vector in full instead of these complicated iterations when the length changed? Popping multiple times will probably be less efficient anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if we can re-assign the list as the list object is a python object living in the calling context and getting passed from python into this function. In my humble and still tiny understanding of the python/c++ inter-op, I thought this needs to be adapted in place to have any affects on the outside scope, hence the popping.

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.

3 participants