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

Prim metadata editRouting #3790

Merged

Conversation

jufrantz
Copy link
Contributor

Following #3778, this proposal introduces support for a new registrable primMetadata router, designed similarly to attribute routing. A unique router will receive the USD metadata name in its context. For customData and variantSelection, the router will also receive a dictionary key path, such as the variantSet name for variantSelection.

These commands support this routing:

  • UsdUndoToggleInstanceableCommand
  • UsdUndoToggleActiveCommand
  • UsdUndoSetKindCommand
  • UsdUndoAddReferenceCommand
  • UsdUndoAddPayloadCommand
  • UsdUndoClearPayloadsCommand
  • UsdUndoClearReferencesCommand
  • SetVariantSelectionCommand
  • ClearSceneItemMetadataCommand
  • SetSceneItemMetadataCommand

I still need to add unit tests and update EditRouting.md. But before going further, I wanted to push these initial changes to ensure they fit the project.

- Designed like attribute editRouting.

- getPrimMetadataEditRouterLayer: routes metadata edits to a layer based
  on its name and the value key path for dict-valued metadata.

- PrimMetadataEditRouterContext: RAII-style class to ease scoped
  editRouting in C++ and python.
@jufrantz
Copy link
Contributor Author

jufrantz commented Jun 5, 2024

Hello @seando-adsk, @pierrebai-adsk,
Is there anything I can improve before the code review? I'm happy to adjust the design or remove certain aspects if they don't align with your vision. Of course, it's also fine to close it if the timing isn't right or if this is already planned on your end.
Julien

@seando-adsk
Copy link
Collaborator

@jufrantz We have been discussing this PR internally. I'll collect all our notes and get back to you.

Sean

@seando-adsk
Copy link
Collaborator

@jufrantz Sorry for the delay. We have discussed this internally and your changes do fit with ones we had upcoming, such as being able to edit route the custom data.

One question that came up internally was, it only support local layer routing and another user added non-local layer routing, so we might want to support it also for metadata.

And just for your information, both visibility and locking are metadata but have their own specific edit router and would not use this metadata edit router. We aren't sure if we would want to also use the metadata edit router for them, either as a replacement (but that would break existing users) or in addition to the metadata edit router.

And yes new unit tests will be required to go along with these changes.

Thank-you,
Sean

@jufrantz
Copy link
Contributor Author

Thank you @seando-adsk for the feedbacks. I'll free up some time next week to revisit this, I'll see how it may interact with non-local targets and also check other metadata routing that I may have missed.
Julien

@jufrantz
Copy link
Contributor Author

jufrantz commented Jul 17, 2024

Thank you once again @seando-adsk, I did have a deeper look at your comments,

Regarding non-local layers routing,I could not find changes related to it. However, I found #3716 that addresses non-local layer persistence in the Maya scene. 3716 and this PR should be compatible: if a non-local layer is used as editTarget before metadata editRouting (to a local layer), the non-local target will be correctly restored. From my understanding, there is currently no editRouter that can target a non-local layer, as routing callbacks respond with an SdfLayer and not a whole UsdEditTarget. Correct me if I am wrong, this might be addressed more globally in another PR.

Regarding other metadata routing, I found mayaLock this metadata only apply to properties. When set with UsdAttributeHolder, it will be routed with the attribute router. I believe it is fine that it is not routed by primMetadata since its intent was only for metadata applied to prim. While it could be more symmetric to have a more specific attributeMetadata route, IMHO it makes sense that attribute routing affects attribute metadata the same way it affects attribute values, and I don't see right now a use case for a specific attributeMetadata route. visibility is routed with attribute or visibility and should not collide neither, it looks all good to me.

Concerning backward compatibility, it appears possible to chain editRouters. eg if an attributeMetadata router is introduced in the future, the attribute router could still be executed first at least for a transition period. Client code with attribute routing but no attributeMetadata routing would not break.

If the current design of primMetadata is acceptable to you, I will adapt the PR following changes for "SessionLayer" metadata and complete with unit tests. Otherwise, I propose to change it and address only our main concern with a specific variantSelection editRouter.

Best,
Julien

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.

It would be nice to have at least one unit test to verify it all works.

@jufrantz
Copy link
Contributor Author

Yes. As mentioned in my first message, it was an initial drop. Since the PR is quite old, I wasn't sure if there was interest in this addition. I'll make some time next week to complete it.

@jufrantz
Copy link
Contributor Author

It would be nice to have at least one unit test to verify it all works.

Done.

pierrebai-adsk
pierrebai-adsk previously approved these changes Sep 23, 2024
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.

Very well done

@pierrebai-adsk
Copy link
Collaborator

Ah, you will have to reformat the files that were rejected by the linter...

@jufrantz
Copy link
Contributor Author

Oops! It should be all good now.
Thank you @pierrebai-adsk.

pierrebai-adsk
pierrebai-adsk previously approved these changes Sep 23, 2024
@jufrantz jufrantz force-pushed the issue3778/prim_metadata_edit_routing branch from 0c773ee to ca1ebb7 Compare September 24, 2024 09:58
@jufrantz
Copy link
Contributor Author

Hello,
I fixed unit-test for Maya<2025, it was not compatible with USD<23.11. The test now passes on Maya 2024: c0f8a94.
I also made a last-minute update: while testing AE, I noticed that SetKind and SetSceneItemMetadata were not enforcing edit restrictions. I’m including it in the PR, aligning with other commands that set metadata values: b317f57 ca1ebb7.
Julien

@pierrebai-adsk pierrebai-adsk assigned jufrantz and unassigned jufrantz Sep 24, 2024
@seando-adsk seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Oct 1, 2024
@seando-adsk seando-adsk merged commit 1855db8 into Autodesk:dev Oct 1, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Edit Routing ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants