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

List all registered Prim Types for Creation #1350

Merged
merged 4 commits into from
Apr 29, 2021

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Apr 19, 2021

This PR adds the ability to list and create all registered concrete prim types for creation in the UFE > Add Prim menu.
Concrete Prim types are listed dynamically under a Registered sub-menu, allowing for listing of new types if they're loaded by USD plugins at a later point.

I currently list them as a flat, sorted list. However I could group them by schema name if that is preferred, however I was unsure which would be more intuitive. I figure that this could be a mvp of the feature, and then could be refined as needed. I also do no filtering outside of listing only concrete types. For example, it may be desirable to filter the AL prim types, but I did not want to prematurely add filtering logic.

The motivation for this PR is that various teams we've spoken to have hit the issue of trying to populate a USD stage with a mix of vanilla and custom schema prim types. The base selection of prim types wasn't adequate in this situation, even though the dynamic AE templates provided for the USD prim types was sufficient to work with them.

@kxl-adsk kxl-adsk added the ufe-usd Related to UFE-USD plugin in Maya-Usd label Apr 21, 2021
@kxl-adsk
Copy link

@dgovil FYI. We will make a design review for this PR first before starting any code review.

@womanyoyoyo-adsk
Copy link
Collaborator

Hi @dgovil,

Thanks for this contribution. I like your idea of grouping them by schema. We already do this in the Attribute Editor. I made a quick mockup of how this can look. I categorized them by non-concrete schema in order of inheritance with Def at the top. I also took out the Registered submenu altogether. Is it possible for you to organize them this way? Going forward on our end we can add the icons, prettify the names, and maybe group the non-concrete schemas with submenus. Great MVP of the feature, I agree.

What do you think? Thanks,
Natalia

addPrimTypes

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 23, 2021

@womanyoyoyo-adsk Thanks. I'll see if it's possible to sort it that way. I had a couple questions before I start so I can set things up accordingly.

  1. I had a concern regarding performance. Since schemas can technically be loaded at any time, the items in the menu would be queried from USD every time the user opens the menu, which is why I'd put it under a submenu initially. I don't think this is particularly slow but I haven't done any timing either to check. Is that a potential concern?
    Alternatively, it could be initialized once, the first time the menu is loaded, but then any new plugins that are loaded would not be represented.

  2. For the items with existing icons, would you like to use the icons you already have? I didn't see that reflected in your image but I assume you'd still want them.

@womanyoyoyo-adsk
Copy link
Collaborator

@dgovil

  1. Yes, performance is a concern. I'm understanding why you put Registered as the submenu in the first place. I'm convinced. Can you please leave our original menu and add Registered as a submenu like you had it?
    image

  2. Sure, I'd like to see the icons we already have in the new menu.

Thanks!

@womanyoyoyo-adsk
Copy link
Collaborator

Also, can you change 'Registered' to 'All Registered'?

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 23, 2021

@womanyoyoyo-adsk Renamed the sub menu (screenshot at the bottom).
I was unclear from the last screenshot if you would be okay with it being a flat list as is at the moment, or if you'd prefer them to be grouped (while still being under this submenu).

If you did want them grouped, would you like the inheritance based grouping from your mockup further up, or would you prefer they be grouped by the Schema plugin they belong to like in the text block below?

usdGeom
- Camera
- Mesh
- Cube
- Sphere
- etc...

usdLux
- CylinderLight
- DomeLight
- etc....

Some thoughts (though of course I'd defer to your judgement)

  • The flat list is easiest and fasted as the code is already ready in this PR and has the least processing versus the other two options.
  • Grouping by inheritance like you had is good if users get familiar with the general types, but does intermix items from different schema plugins together. I think that's fine though, just thinking out loud.
  • Grouping by schema plugins (as in the text block above) has the pro of grouping objects from different schema plugins together, but of course you then get the opposite problem of different schema types being grouped together. However it does have one advantage in that if a studio adds their own schema plugin, all their types would appear grouped together.

Happy to look into implementing any of them

image

@womanyoyoyo-adsk
Copy link
Collaborator

Hey @dgovil,

I'm glad you brought up these points and questions. I mocked up some solutions to see what would be best of the user. Now that I think I have the final design I can address your points. See mockup below.

primTypes

I didn't know if I wanted grouping because I wasn't sure. Now that I've grouped them by schema plugin, the grouping makes sense and is user-friendly. I tried grouping them by inheritance but I've learned that it wasn't logical (is Cube in Geometric Prim or Imageable? Cube is in both) and not user-friendly. As for grouping by schema plugin, I'm very glad you brought it up. I think it's the right way to go.

@womanyoyoyo-adsk
Copy link
Collaborator

womanyoyoyo-adsk commented Apr 26, 2021

As for the schema plugin name, is it possible to use the more user-friendly name of the schema as seen here in my screenshot? So for USDGeom it would be 'Geometry'. I made this assumption in my design above.
schemaName

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 26, 2021

That looks good to me. I'll get started on putting this together.

I don't believe the schema plugins actually store their nice names anywhere unfortunately. Though I'll check with Pixar if there's a way to do so. I can however create a mapping in the code so it maps from usdGeom -> Geometry etc...

The upside there is that it's controllable , the downside is that any new schemas would come in under the schemas actual name till that list is updated. However I imagine, studios that add their own schemas could modify that list if need be internally to give their own schemas a nice name.

@womanyoyoyo-adsk
Copy link
Collaborator

Okay. I think nice names would benefit artists. Studios being able to add a schema nice name for their artists seems like a good addition. Thanks @dgovil!

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 27, 2021

Okay, the latest version has the following setup based on the default plugin types to match your mockup. I do not have the Arnold schemas available here so I am unware of their plugin name to create the group.

image

For code reviewers:

  • I update the dynamic list only when building the All Registered menu. If a user doesn't open that menu, there's no penalty to querying the types. I also do not re-query when showing the submenus.
  • I hid the Animal Logic schemas since I believe they aren't desirable to show. AL_USDMayaSchemasTest, AL_USDMayaSchemas. I'm not sure if anyone from Animal Logic objects to that though. I could potentially put that behind an IFDEF (not sure if one exists?) or just list it under Animal Logic if preferred.
  • The way this is set up means that it's possible (though not used currently) to put multiple schema plugins under a single sub menu group if desired (if two plugins share one nice name), or to hide them (if the user provides an empty nice name). If a nice name is not present , it'll default back to the plugins actual name.

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. Great job. Just a couple of minor things and one possible hickup with USD version.

lib/mayaUsd/ufe/UsdContextOps.h Outdated Show resolved Hide resolved
lib/mayaUsd/ufe/UsdContextOps.h Outdated Show resolved Hide resolved
lib/mayaUsd/ufe/UsdContextOps.cpp Outdated Show resolved Hide resolved
lib/mayaUsd/ufe/UsdContextOps.cpp Outdated Show resolved Hide resolved
lib/mayaUsd/ufe/UsdContextOps.cpp Show resolved Hide resolved
@dgovil
Copy link
Collaborator Author

dgovil commented Apr 27, 2021

@seando-adsk Thanks for the feedback. 555ebf0 should address all of them I hope.
Out of curiosity, is there any good tooling to check for method availability across the supported USD versions?

@seando-adsk
Copy link
Collaborator

@seando-adsk Thanks for the feedback. 555ebf0 should address all of them I hope.
Out of curiosity, is there any good tooling to check for method availability across the supported USD versions?

Thanks for fixing everything. I just have two new minor comments.
I don't know of any tooling to check for method availability. I just know that UsdSchemaRegistry has methods that are missing from older versions because I got caught by this in the past. So anytime I see a change with UsdSchemaRegistry methods being used I question it.

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 28, 2021

@seando-adsk thanks for catching those. I fixed those up. I've wrapped all my additions in the version checks so it should hopefully not cause any unused warnings now, and fixed up the namespacing.

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.

Great. Looks good.

@seando-adsk
Copy link
Collaborator

@dgovil I've started the preflight build for you.

Copy link

@kxl-adsk kxl-adsk left a comment

Choose a reason for hiding this comment

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

@dgovil Your change is failing with older versions of USD (our minimum supported version is 20.02). Here is a compilation error we are getting with USD 20.08

\maya-usd\lib\mayaUsd\ufe\UsdContextOps.cpp(473): error C2352: 'pxrInternal_v0_20__pxrReserved__::UsdSchemaRegistry::IsConcrete': illegal call of non-static member function
include\pxr/usd/usd/schemaRegistry.h(125): note: see declaration of 'pxrInternal_v0_20__pxrReserved__::UsdSchemaRegistry::IsConcrete'

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 29, 2021

@kxl-adsk Ah I see what the issue is. The USD api flip flopped between being static and not.
The API I need is actually supported all the way back to 20.02 as a static method...then went back to non static in 20.05...and finally back to static again in 21.02 (@seando-adsk just as an FYI)

I've adjusted the code to use a non static call, so it should now work with 20.08 as well.

@seando-adsk
Copy link
Collaborator

@dgovil So does that mean we can remove the if PXR_VERSION checks as our lowest supported version is v20.02 which has the method you need?

@dgovil
Copy link
Collaborator Author

dgovil commented Apr 29, 2021

@seando-adsk sorry i should have clarified that this was for IsConcrete. The GetConcreteSchemaTypeName is still 20.08 and higher only so I left the version checks in accordingly. I just wanted to highlight that some of the schema registry methods seem to change their signatures back and forth between versions.

@seando-adsk
Copy link
Collaborator

@dgovil Ah right that makes sense. Thanks. FYI, Krystian re-started the preflight.

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Apr 29, 2021
@kxl-adsk kxl-adsk merged commit 8b3af39 into Autodesk:dev Apr 29, 2021
@BigRoy
Copy link
Contributor

BigRoy commented Nov 19, 2023

I do not have the Arnold schemas available here so I am unware of their plugin name to create the group.

In 0.25.0 Arnold types still show as usdArnold:

image

I wonder if we should still add a (hardcoded?) mapping from usdArnold -> Arnold.

Also, would it make sense to maybe for those that we do not have a nice name mapping for to strip off the first three characters if they are usd. That'd make usdArnold -> Arnold already but would also solve it for usdUI, usdPhysics, usdMedia, usdRender, etc? Or are we then making an over-generalization?

For what it's worth - here's a snippet doing similar logic as this PR but via the Python API for anyone who likes testing that way.

@seando-adsk
Copy link
Collaborator

For this context menu the schema nice names are contained in a hard-coded list in the .cpp here: https://github.com/Autodesk/maya-usd/blob/dev/lib/usdUfe/ufe/UsdContextOps.cpp#L698 and https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/ufe/MayaUsdContextOps.cpp#L816

Sean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge ufe-usd Related to UFE-USD plugin in Maya-Usd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants