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

Add displayName Metadata to Prim Spec #2055

Merged

Conversation

erslavin
Copy link
Contributor

@erslavin erslavin commented Oct 5, 2022

  • Added displayName metadata field to prims
  • Adjusted tests to test for and modify authored displayName metadata

It is important that we keep ABI compatibility on this change, so Get/Set DisplayName methods were not added to UsdPrim. This addition will be added to the PR later - all access to displayName on a prim at the moment should be via Get/Set Metadata.

  • I have submitted a signed Contributor License Agreement

- Added displayName metadata field to prims
- Adjusted tests to test for and modify authored displayName metadata
- Did not add Get/Set DisplayName methods to prims - all access should
  be via Get/Set Metadata to keep ABI compatibility
@asluk
Copy link
Collaborator

asluk commented Oct 5, 2022

This is part of the Omniverse work for UTF-8 support-- alongside @erslavin's implementation of Unicode identifiers which is not ABI-compatible, we are also offering displayName support so that stakeholders can use it for breadcrumbs back to their UTF-8 encoded source data, and view the displayNames in DCCs that support them. Thanks!

@tallytalwar
Copy link
Contributor

Filed as internal issue #USD-7681

@asluk
Copy link
Collaborator

asluk commented Oct 13, 2022

I know folks are probably busy putting the final touches on v22.11, and we don't expect to make this into there, but please let us know for now if there is any major objection to this PR in principle, as we'll need to deploy it ahead of an official USD release to unblock some key customer workflows-- thanks!

@spiffmon
Copy link
Member

Hi @asluk and @erslavin , without having done a proper review yet, we don't have any major objections but we feel it is important to include a minimum of support in usdview, something like:

  • An option in the PrimView browser (in the "Show" menu) to enable use of displayNames, which defaults to on.
  • Ensure that when this option is on:
    • Using the search bar will search display names since that's what the user sees
    • the prim tooltips will indicate the prim's True Name.
  • The path selection bar at the top of usdview would continue to show true names, even when the "Show display names" option is on, since it must be copy/pastable into relationship target paths, etc.

I know we have yet to provide a similar treatment for property displayNames in the property browser, but we feel the potential for confusion is greater at the level of scene hierarchy... but also that especially anyone who is using the displayName feature would really expect to be able to navigate their scene in usdview using them.

Thanks!

@asluk
Copy link
Collaborator

asluk commented Oct 15, 2022

Hi @asluk and @erslavin , without having done a proper review yet, we don't have any major objections but we feel it is important to include a minimum of support in usdview, something like:

  • An option in the PrimView browser (in the "Show" menu) to enable use of displayNames, which defaults to on.

  • Ensure that when this option is on:

    • Using the search bar will search display names since that's what the user sees
    • the prim tooltips will indicate the prim's True Name.
  • The path selection bar at the top of usdview would continue to show true names, even when the "Show display names" option is on, since it must be copy/pastable into relationship target paths, etc.

I know we have yet to provide a similar treatment for property displayNames in the property browser, but we feel the potential for confusion is greater at the level of scene hierarchy... but also that especially anyone who is using the displayName feature would really expect to be able to navigate their scene in usdview using them.

Thanks!

Agreed - we'll work on the usdview side over the next week or so-- thanks!

@erslavin
Copy link
Contributor Author

What if the displayName metadata is empty and the option is on - I would expect in this case the prim name would be used, what do you think?

@spiffmon
Copy link
Member

Absolutely, @erslavin - that is what we do in Presto, where displayName is a computed quantity, but if the computational inputs are empty, we fall back to prim name. I also wanted to clarify what I said about tooltip... we want it to provide useful information regardless of whether the new mode is on or off, so it should probably always tell you both the prim name and displayName (if it has one).

Thanks!

- Added context menu item to show prim display names (default = True)
- Added support for showing prim display name in the prim tree
- Added support for searching on prim display name in search box
- Modified prim tree item tooltip to show both name and display name
@erslavin
Copy link
Contributor Author

erslavin commented Oct 17, 2022

Work in progress - no automated tests yet but modifications were made in usdview to conform to the request

- Augmented automated tests in usdview to account for displayName
- Added displayName to field keys in wrapPrimSpec
- Removed unnecessary comment blocks
@erslavin
Copy link
Contributor Author

Augmented the existing automated tests for usdview to account for displayName and made some modifications to account for displayName not being authored. Let me know if the functionality is what you expect - in particular the way I implemented search was to search both the displayName (if authored) and prim identifier when the setting is on, and only the identifier when it's off.

@spiffmon
Copy link
Member

@erslavin , can you please migrate all of the "DisplayName"-related methods on UsdProperty up to UsdObject, now that it is applicable to all object types?

@spiffmon
Copy link
Member

... and we'll also want to test that in testUsdMetadata

pxr/usd/usd/testenv/testUsdMetadata.py Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/utils.h Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/utils.cpp Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/utils.cpp Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/wrapUtils.cpp Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/mainWindowUI.ui Show resolved Hide resolved
pxr/usdImaging/usdviewq/appController.py Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/appController.py Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/appController.py Show resolved Hide resolved
- Moved displayName related methods from UsdProperty to UsdObject
- Adjusted tests to better conform to user search expectations
@erslavin
Copy link
Contributor Author

erslavin commented Nov 28, 2022

@erslavin , can you please migrate all of the "DisplayName"-related methods on UsdProperty up to UsdObject, now that it is applicable to all object types?

Methods are now moved to UsdObject to apply to both prims and properties. Also adjusted tests to use both generic metadata methods and specific display name methods

- Rolled back changes to search
- Updated condition to match on display name if authored or on name
  if not
- Ensured .ui file was saved from Qt designer
Copy link
Member

@spiffmon spiffmon left a comment

Choose a reason for hiding this comment

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

Thanks, so much, @erslavin - just a couple last things for you.

- Refactored out pattern match code into separate method to
  avoid code duplication across prim and prototype search paths
- Fixed documentation issue
Copy link
Member

@spiffmon spiffmon left a comment

Choose a reason for hiding this comment

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

Just one quick performance note, @erslavin , otherwise this looks fantastic!

pxr/usdImaging/usdviewq/appController.py Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/appController.py Outdated Show resolved Hide resolved
- Adjusted usdview search matching logic to directly
  invoke GetDisplayName and compare to empty string
  to avoid overhead of calling HasAuthoredDisplayName
  followed by GetDisplayName
Copy link
Member

@spiffmon spiffmon left a comment

Choose a reason for hiding this comment

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

Corey on the USD team noted a few things that I missed - passing them on to you, @erslavin

Copy link
Member

@spiffmon spiffmon left a comment

Choose a reason for hiding this comment

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

OK, that's all - thanks, Ed!

pxr/usd/usd/object.h Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/primViewItem.py Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/utils.cpp Outdated Show resolved Hide resolved
pxr/usdImaging/usdviewq/appController.py Show resolved Hide resolved
pxr/usdImaging/usdviewq/appController.py Outdated Show resolved Hide resolved
- Modified comments appropriately for methods moved to UsdObject
- Made checks for valid displayName more pythonic
@spiffmon
Copy link
Member

We won't be doing any more dev-pushes to github this year, but just wanted to let you know this one did get merged internally in time for 23.02, @erslavin - thanks for your patience!

@asluk
Copy link
Collaborator

asluk commented Dec 18, 2022

We won't be doing any more dev-pushes to github this year, but just wanted to let you know this one did get merged internally in time for 23.02, @erslavin - thanks for your patience!

Thanks @spiffmon and @erslavin !

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.

5 participants