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

Implement viewport selection highlighting of point instances and point instances pick modes #1119

Conversation

mattyjams
Copy link
Contributor

Building on #1027, this PR adds support for selection highlighting of individual point instances when using the Viewport 2.0 render delegate. It also implements all of the "Prims", "Models", "Instances", and "Prototypes" pick modes that are available in the "View -> Pick mode" menu in usdview. A "Kind" pick mode unique to the render delegate represents the existing behavior and is the default. Similar to selection kind, the pick mode can be changed using the UsdPickMode optionVar.

@mattyjams mattyjams added enhancement New feature or request vp2renderdelegate Related to VP2RenderDelegate labels Jan 26, 2021
@kxl-adsk
Copy link

@mattyjams Can you provide a video to demo your new proposed additions? I would like to loop in the design here before starting the code review process.

@mattyjams
Copy link
Contributor Author

@mattyjams Can you provide a video to demo your new proposed additions? I would like to loop in the design here before starting the code review process.

@kxl-adsk: I'm not sure a video would demonstrate it all that clearly really. Like selection kind, the pick mode is only adjustable using an optionVar right now. I think it's probably most obvious to look at the PickModes test and check out the baseline image and the UFE path selected for each pick mode to see what they do. I'll try to distill that from the source files below.

The scene is a single PointInstancer prim that uses seven different prototypes. Each prototype is just a cube with a different displayColor and material. The PointInstancer then creates two instances of each prototype. The test simulates a click in the center of the viewport, hitting the lower left of the two green cubes.

Scene file:
https://github.com/Autodesk/maya-usd/blob/9a01aa58c303458c7da5e0fdf270e62b2881fbde/test/testSamples/pointInstances/PointInstancer_Grid_14.ma

The default behavior when the UsdPickMode optionVar is not set, or is set to something other than one of Prims, Models, Instances, or Prototypes, is to use what I'm calling the Kind pick mode, which is exactly the same pick behavior from before these changes. If you don't have anything set for UsdSelectionKind, then the Kind and Prototypes pick modes are exactly the same.

Pick result in Kind pick mode with no UsdSelectionKind set (same result as in Prototypes pick mode):
Pick result in Kind pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer/prototypes/GreenCube/Geom/Cube

Pick result in Prims pick mode:
Pick result in Prims pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer

Pick result in Models pick mode:
Pick result in Models pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer/prototypes/GreenCube

Pick result in Instances pick mode:
Pick result in Instances pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer/3

Again, the goal with these pick modes was to mirror their counterparts in usdview.

@ppt-adsk
Copy link
Collaborator

FWIW, my non-product design, single developer opinion is that this picking behavior seems reasonable to me. Of course, the people actually responsible for this kind of thing on our end will be weighing in with a real opinion :)

Not sure about the use of "Kind" to label the pick mode where the prototype geometry is picked, but if that's the USD terminology for it then that's fine.

@mattyjams
Copy link
Contributor Author

Not sure about the use of "Kind" to label the pick mode where the prototype geometry is picked, but if that's the USD terminology for it then that's fine.

Thanks @ppt-adsk! And I hear you about the Kind label. I just invented that because there is no such corresponding pick mode on the USD side. I needed to keep it distinct from the Prototypes pick mode, since in USD Prototypes mode will never consider kind, so I needed some term to use for the original pick resolution behavior. Definitely open to suggestions if you all think there's a better way to handle that.

@mattyjams mattyjams force-pushed the pr/vp2_render_delegate_point_instance_selection_and_highlighting branch from 9a01aa5 to b8849da Compare February 5, 2021 02:24
@mattyjams
Copy link
Contributor Author

We discussed offline how these two optionVars control pick resolution and decided to fully separate from the usdview behavior.

The optionVar that was previously UsdPickMode is now UsdPointInstancesPickMode with the possible values of PointInstancer, Instances, or Prototypes. If the object picked in the viewport is a point instance, this optionVar controls what that pick resolves to (the PointInstancer prim, a specific point instance scene item, or the prototype prim being instanced, respectively). When we're in Instances mode and we resolve to a point instance, that's the end of the pick resolution process. Otherwise if there is a UsdSelectionKind specified, we start searching up the hierarchy from whatever we resolved to for a kind match. This results in a more flexible way to use both the point instances pick mode and the selection kind in combinations that usdview doesn't support while still allowing us to express the pick modes that usdview does support.

Updating my comment from above, here is the summary based on the new changes:


The scene is a single PointInstancer prim that uses seven different prototypes. Each prototype is just a cube with a different displayColor and material. The PointInstancer then creates two instances of each prototype. The test simulates a click in the center of the viewport, hitting the lower left of the two green cubes.

Scene file:
https://github.com/Autodesk/maya-usd/blob/dev/test/testSamples/pointInstances/PointInstancer_Grid_14.ma

The default behavior when the UsdPointInstancesPickMode optionVar is not set or is invalid is to use PointInstancer. Note that this a change versus the current pick resolution which is effectively Prototypes.

Pick result in PointInstancer point instances pick mode (the default if UsdPointInstancesPickMode is unset):
Pick result in PointInstancer point instances pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer

Pick result in Instances point instances pick mode:
Pick result in Instances point instances pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer/3

Pick result in Prototypes point instances pick mode:
Pick result in Prototypes point instances pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer/prototypes/GreenCube/Geom/Cube

Pick result when configured to match usdview's Models pick mode (UsdPointInstancesPickMode=Prototypes, UsdSelectionKind=model):
usdview's Models pick mode

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid/PointInstancer/prototypes/GreenCube

Pick result with UsdPointInstancesPickMode=PointInstancer and UsdSelectionKind=model, a configuration not directly supported by usdview. In the test scene's USD, the root prim has kind component, which IsA model, whereas the PointInstancer prim has kind subcomponent, which is not model:
Non-instance models

Selected UFE path:

|UsdProxy|UsdProxyShape,/PointInstancerGrid

@mattyjams mattyjams changed the title Implement viewport selection highlighting of point instances and usdview-style pick modes Implement viewport selection highlighting of point instances and point instances pick modes Feb 5, 2021
@kxl-adsk
Copy link

kxl-adsk commented Feb 5, 2021

Thank you @mattyjams, I started the review process.

We also try to clean-up optionVars and make them use a consistent mayaUsd_ prefix and ease the discovery by locating them all in one place. Do you mind moving and renaming:
UsdPointInstancesPickMode -> mayaUsd_PointInstancesPickMode
UsdSelectionKind -> mayaUsd_SelectionKind

@huidong-chen
Copy link

@mattyjams Thank you Matt.

From first glance, USDView only has a single control on the selection behavior, its Pick mode menu has Prims, Models, Instances, and Prototypes. Maybe you've explained it in the above design doc but I missed it, could you elaborate why we need to introduce another control in addition to the current UsdSelectionKind? Is there a chance that we use only one control to support all the modes like USDView?

@mattyjams
Copy link
Contributor Author

Thank you @mattyjams, I started the review process.

We also try to clean-up optionVars and make them use a consistent mayaUsd_ prefix and ease the discovery by locating them all in one place. Do you mind moving and renaming:
UsdPointInstancesPickMode -> mayaUsd_PointInstancesPickMode
UsdSelectionKind -> mayaUsd_SelectionKind

@kxl-adsk: Sounds good. I can move and rename the optionVars. Let me take care of that in a rebase before you all get too deep into the review though, since that will cascade across a bunch of the commits here.

@huidong-chen
Copy link

@mattyjams Thank you Matt.

From first glance, USDView only has a single control on the selection behavior, its Pick mode menu has Prims, Models, Instances, and Prototypes. Maybe you've explained it in the above design doc but I missed it, could you elaborate why we need to introduce another control in addition to the current UsdSelectionKind? Is there a chance that we use only one control to support all the modes like USDView?

Please ignore this question. I just received the offline email thread which exactly answers my question.

…tionKind"

The new token is declared publicly in mayaUsd/base/tokens.h where all optionVar
name tokens should be located.
… render delegate

This ensures that individual point instances are selected in Hydra when the
UsdSceneItem represents a point instance.
…olving viewport picking of point instances

This enum represents three options of behaviors to employ when resolving
viewport picking of point instances:

PointInstancer:
    The PointInstancer prim that generated the point instance is picked. If
    multiple nested PointInstancers are involved, the top-level PointInstancer
    is the one picked. If a selection kind is specified, the traversal up the
    hierarchy looking for a kind match will begin at that PointInstancer.

Instances:
    The specific point instance is picked. In this mode, any setting for
    selection kind is ignored.

Prototypes:
    The prototype being instanced by the point instance is picked. If a
    selection kind is specified, the traversal up the hierarchy looking for a
    kind match will begin at the prototype prim.

A member variable of the new enum type was added to ProxyRenderDelegate and
will be set and used in subsequent commits, along with the implementations of
the pick modes.
… pick resolution method based on the "mayaUsd_PointInstancesPickMode" optionVar

The default UsdPointInstancesPickMode if the optionVar is unspecified or
invalid is "PointInstancer". Note that this is a departure from the current
behavior when a point instance is selected, which is effectively "Prototypes".
Copy link

@huidong-chen huidong-chen left a comment

Choose a reason for hiding this comment

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

Some general notes:

  1. Did you test the viewport selection performance for massive objects and instances? Does it have a major impact to non-point-instancer assets?

  2. A question for @kxl-adsk How do we manage image data in the repo? Do we encourage use of git lfs?

@kxl-adsk
Copy link

kxl-adsk commented Feb 6, 2021

@HdC-adsk There used to be some limitations around Git Large File Storage and the build infrastructure that we are using. As long as images are small, we should be ok without lfs.

Copy link
Collaborator

@ppt-adsk ppt-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 tests!

@mattyjams
Copy link
Contributor Author

  1. Did you test the viewport selection performance for massive objects and instances? Does it have a major impact to non-point-instancer assets?

At least in my testing so far, I haven't seen the pick mode handling incurring significant overhead and impacting performance, though I suppose it may depend on your definition of "massive". Do you have an asset handy that I should test with?

I would guess that in the case of single selections, the overhead of any particular point instances pick mode would likely be dwarfed by the rest of the selection machinery. For large marquee selections that result in many scene items getting selected, it could possibly add up though? I'm happy to take a performance optimization pass on this later if we see this becoming a significant bottleneck.

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Feb 9, 2021
@kxl-adsk kxl-adsk merged commit 5a110ae into Autodesk:dev Feb 9, 2021
@mattyjams mattyjams deleted the pr/vp2_render_delegate_point_instance_selection_and_highlighting branch February 9, 2021 17:10
mattyjams added a commit to mattyjams/maya-usd that referenced this pull request Feb 9, 2021
…USD 20.05

Point instance selection depends on getting the HdInstancerContext from Hydra
which was only exposed in USD_IMAGING_API_VERSION version 14. As a result, we
can only support point instance selection for core USD 20.08, not 20.05 as
previously thought.
mattyjams added a commit to mattyjams/maya-usd that referenced this pull request Feb 9, 2021
…legate tests

This was accidentally introduced by Autodesk#1119 since that work spanned the
transition from using VP2_RENDER_DELEGATE_PROXY to
MAYAUSD_DISABLE_VP2_RENDER_DELEGATE.
kxl-adsk pushed a commit that referenced this pull request Feb 9, 2021
…breakage_with_USD_2005

fix build breakage from PR #1119 due to missing Hydra API in USD 20.05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants