maya-usd icon indicating copy to clipboard operation
maya-usd copied to clipboard

Prim metadata editRouting

Open jufrantz opened this issue 1 year ago • 10 comments

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.

jufrantz avatar May 28 '24 18:05 jufrantz

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

jufrantz avatar Jun 05 '24 15:06 jufrantz

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

Sean

seando-adsk avatar Jun 05 '24 17:06 seando-adsk

@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

seando-adsk avatar Jun 27 '24 18:06 seando-adsk

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 avatar Jun 28 '24 15:06 jufrantz

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

jufrantz avatar Jul 17 '24 12:07 jufrantz

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 avatar Sep 18 '24 20:09 jufrantz

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

Done.

jufrantz avatar Sep 23 '24 17:09 jufrantz

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

pierrebai-adsk avatar Sep 23 '24 18:09 pierrebai-adsk

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

jufrantz avatar Sep 23 '24 19:09 jufrantz

Hello, I fixed unit-test for Maya<2025, it was not compatible with USD<23.11. The test now passes on Maya 2024: c0f8a94c45573a3b76fb3c0d3df17705a1ef50fc. 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: b317f571a50690fd9d66f2f310aa887a111b4f87 ca1ebb730cdb0d5b4ad6baf2aa019825165e0103. Julien

jufrantz avatar Sep 24 '24 10:09 jufrantz