add support for manipulating point instances via UFE #1241

mattyjams · 2021-03-09T00:13:04Z

I've been having trouble finding the time to root out the remaining issues with point instance manipulation, so I wanted to just post what I've got so far. Feel free to review (read: ridicule) whenever you have a chance. :)

At a high level, this probably most closely aligns with the XformCommonAPI Transform3d implementation where instead of authoring translation, rotation, and scale values to xformOps dedicated to those operations, we author the values at the appropriate index in the PointInstancer prim's positions, orientations, and scales array attributes instead.

At the bottom, there is a UsdPointInstanceModifierBase class that handles most of the work of getting and setting individual element values in a PointInstancer prim's array attributes. This is then specialized into concrete subclasses for position, orientation, and scale modifiers.

Above that, there are a set of undoable commands that use the modifiers for redo'ing and undo'ing their respective operations.

There's then a UsdTransform3dPointInstance subclass of Ufe::Transform3d that creates commands of the appropriate type, and manages getting a point instance's current translation, rotation, and scale. It also supports querying the inclusive and exclusive matrices of an instance, and setting the instance's matrix (non-interactively).

One wrinkle is that for rotation, Maya/UFE speaks in terms of Euler angles while the PointInstancer schema stores the orientations attribute as quaternions (GfQuath). I have an (admittedly ham-handed) conversion for that in the orientation modifier, but it makes for some pretty jittery manipulation as you drag the handles, and I don't think it's actually working correctly when multiple manipulations are strung together.

The scale manipulator seems to work at least for the initial manipulation, but I'm seeing an odd issue when pulling on one handle then pulling on another where the second pull often scales along both axes rather than only along the second handle's axis.

The position manipulator luckily seems to be working correctly though, so at least that seems to be a win.

I'm hopeful that the things that aren't working are just small mistakes in my implementation stemming from unfamiliarity with the interface. Curious to hear what you all think!

ppt-adsk

Looks really good to me from a code structure point of view! Left a few comments / questions, and there is UFE_V2 conditional compilation work that needs a bit of clarification, but this is close to being ready, in my opinion.

I cannot tell at a glance why rotation and scaling manipulation are causing trouble. I have had trouble in the past using matrix decomposition code from Pixar, and had to change to Maya API matrix decomposition to get proper manipulation --- see

maya-usd/lib/mayaUsd/ufe/UsdTransform3dMayaXformStack.cpp

Line 175 in c568190

// Decompose new matrix to extract TRS. Neither GfMatrix4d::Factor

Possibly something to try later on; I don't feel we should delay converting this from a draft pull request to a pull request because of this issue.

lib/mayaUsd/ufe/UsdPointInstanceModifierBase.h

ppt-adsk · 2021-03-09T15:50:22Z

lib/mayaUsd/ufe/UsdTransform3dHandler.cpp

@@ -46,6 +47,10 @@ Ufe::Transform3d::Ptr UsdTransform3dHandler::transform3d(const Ufe::SceneItem::P
    assert(usdItem);
 #endif

+    if (usdItem->isPointInstance()) {


This is for the old Transform3d handling scheme, correct? I see you've adjusted the chain of responsibility for the new scheme in adsk/plugin/plugin.cpp.

Right, yeah. My main concern here was if you have multiple point instances for the same PointInstancer selected, we don't want to fall through to the "usual" Transform3d which may result in many objects that all try to treat their item as a discrete prim and attempt to create the same xformOps on the PointInstancer.

Do you think it's clearer or more safe to just not support point instances in this old handler? Maybe we're planning to get rid of it soon anyway?

lib/mayaUsd/ufe/UsdTransform3dPointInstance.cpp

ppt-adsk · 2021-03-09T16:08:41Z

lib/mayaUsd/ufe/UsdPointInstanceUndoableCommands.h

+public:
+    using Ptr = std::shared_ptr<UsdPointInstanceUndoableCommandBase>;
+
+#ifdef UFE_V2_FEATURES_AVAILABLE


If UsdTransform3dPointInstance depends on UFE_V2, does it make sense to conditionally compile in UsdPointInstanceUndoableCommands for UFE_V1?

Good catch. I admittedly have only been working with this code against PR 122, so there's an excellent chance this doesn't even build successfully against Maya 2020 as it is right now.

I think my intent was to gracefully ignore attempts to manipulate point instances for UFE v1. The way things are currently setup, it should be possible in UFE v1 to have UFE selections that include scene items representing point instances, even though point instance selection in the viewport is UFE v2 only. In that case, I was aiming for requests to manipulate point instance items in UFE v1 basically becoming no-ops. That was why I had the modifiers and the commands included in the CMakeLists.txt for both UFE v1 and UFE v2.

So I think what I actually have to do is ensure that all of the new files here are compiled for both UFE versions. Does all of this sound right/reasonable to you?

If UFE v1 is intended to be a no-op, then it seems to me that you could just return null pointers when you're asked for commands. That way, you wouldn't need to compile commands against UFE v1 --- and if the commands are the only clients of the modifiers, then the modifiers wouldn't need to be compiled against UFE v1 either. Does this align with your thinking?

It does, thanks! In the most recent changes, UsdPointInstanceUndoableCommands.h is only included for UFE v2.

I ended up leaving the modifiers in for both versions of UFE, since strictly speaking there's nothing in them that requires v2, so they could theoretically get used for non-Transform3d authoring operations.

mattyjams

Thanks for the feedback, @ppt-adsk! All great notes.

I'll see if I can "finish" this by addressing what you've noted so far, and add at least a few simple tests for the new functionality. I'll then convert it to a non-draft PR.

I'm a little concerned that the unit tests wouldn't actually cover the issues I was seeing when interactively pulling on the manipulator handles, since just doing cmds.move(), cmds.rotate(), etc. seem to work. Thanks for noting the issues you saw with the USD-side Decompose() though. I can give the Maya-side versions of that a try and see whether that makes a difference.

I'm also still a little concerned about how performance scales with having a Transform3d per point instance scene item and the the single-element modifications that each of those are doing. With the API that's currently available on both the USD side and the UFE side, I'm not sure there's much of a way around that at the moment.

lib/mayaUsd/ufe/UsdPointInstanceModifierBase.h

mattyjams · 2021-03-10T01:08:57Z

lib/mayaUsd/ufe/UsdPointInstanceUndoableCommands.h

+public:
+    using Ptr = std::shared_ptr<UsdPointInstanceUndoableCommandBase>;
+
+#ifdef UFE_V2_FEATURES_AVAILABLE


Good catch. I admittedly have only been working with this code against PR 122, so there's an excellent chance this doesn't even build successfully against Maya 2020 as it is right now.

I think my intent was to gracefully ignore attempts to manipulate point instances for UFE v1. The way things are currently setup, it should be possible in UFE v1 to have UFE selections that include scene items representing point instances, even though point instance selection in the viewport is UFE v2 only. In that case, I was aiming for requests to manipulate point instance items in UFE v1 basically becoming no-ops. That was why I had the modifiers and the commands included in the CMakeLists.txt for both UFE v1 and UFE v2.

So I think what I actually have to do is ensure that all of the new files here are compiled for both UFE versions. Does all of this sound right/reasonable to you?

mattyjams · 2021-03-10T01:13:04Z

lib/mayaUsd/ufe/UsdTransform3dHandler.cpp

@@ -46,6 +47,10 @@ Ufe::Transform3d::Ptr UsdTransform3dHandler::transform3d(const Ufe::SceneItem::P
    assert(usdItem);
 #endif

+    if (usdItem->isPointInstance()) {


Right, yeah. My main concern here was if you have multiple point instances for the same PointInstancer selected, we don't want to fall through to the "usual" Transform3d which may result in many objects that all try to treat their item as a discrete prim and attempt to create the same xformOps on the PointInstancer.

Do you think it's clearer or more safe to just not support point instances in this old handler? Maybe we're planning to get rid of it soon anyway?

lib/mayaUsd/ufe/UsdTransform3dPointInstance.cpp

…tInstancers This adds a UsdPointInstanceModifierBase class which will be specialized to create modifiers for the "positions", "orientations", and "scales" attributes of PointInstancer prims. Derived classes of this base class should implement the functions to directly get and create the USD attribute on the PointInstancer (typically with a call to UsdGeomPointInstancer::Get...Attr() and UsdGeomPointInstancer::Create...Attr(), respectively), functions to convert values between USD and UFE types, and a function to retrieve the default value in the USD type.

mattyjams · 2021-03-10T22:23:38Z

I got some great notes on this offline from @kxl-adsk and @ppt-adsk, so the newly pushed changes here seem to resolve all of the issues I was previously seeing. There were two important changes necessary:

The UFE StagesSubject needed to monitor for change notifications from USD when paths to PointInstancer schema positions, orientations, or scales attributes were modified. It was already doing something similar for xformOpOrder and specific xformOp attributes for the other transform types. That addition is represented in 15ab1de. The somewhat unsavory aspect of that though is that we can't tell which specific instances were updated, so we have to issue notifications for all of them.
The value conversion functions for the orientations modifier were rewritten to use Maya's rotation and transform handling utilities rather than USD/Gf's. The manipulation is much smoother now when dragging the handles in the viewport.

I still need to validate this against UFE v1 and then finish up the unit tests, but it looks to be in much better shape now. Should hopefully be ready to convert this to a regular pull request soon.

ppt-adsk

Notification scheme is the more troublesome part, but at least we have correctness, if not performance :) Perhaps given the fairly urgent time constraints this is sufficient, and we can move to creating a test and converting this draft to an actual pull request?

I'm still hopeful that we can provide a no-op implementation of the point instancer Transform3d for UFE v1 (i.e. Maya 2020) by returning null pointers instead of compiling actual code --- please set me straight on this if I've misunderstood.

lib/mayaUsd/ufe/StagesSubject.cpp

…Instancers

…intInstancers

…tancers

…ancer prim

…ances

…sponse to USD notices When individual point instances are manipulated, the change is represented in USD as an update to the "positions", "orientations", or "scales" array attribute(s) on the PointInstancer prim that generated the point instance. Unfortunately though, there is no way for us to know which point instance(s) were actually affected based on the USD change notification. This means that we have to assume that *all* of the PointInstancer prim's instances may have been affected. We therefore need to construct UFE paths for every instance and issue a notification for each one.

…dler Point instance manipulation using the older UsdTransform3dHandler is only supported with UFE v2. Otherwise, we disallow any manipulation for point instance scene items.

…handler chain

mattyjams · 2021-03-11T20:33:11Z

I converted this to a regular PR, as I believe what we've got now is functionally correct and the major performance concerns we can currently tackle have been addressed. Tests are still missing, but I'll append another commit for that soon.

Thanks again @ppt-adsk and @kxl-adsk for all of your help with this!

…ions, and scales

mattyjams · 2021-03-11T22:33:08Z

And now with tests in e4d1e20. Kicking off a preflight run.

Thanks again!

…to 21.02

mattyjams · 2021-03-12T00:22:58Z

Hopefully addressed the preflight build failures with bf4cbb2. I failed to notice that VtArray::AsConst() was a recent addition in USD 21.02.

The range() function returns lists in Python 2 which can be added together with the '+' operator, but that's not the case in Python 3.

mattyjams · 2021-03-12T07:05:23Z

There was one more remaining issue in the unit test when running with Python 3 which was addressed in d504519. Other than that, I think the remaining preflight failures are the spurious kind we've seen before.

kxl-adsk · 2021-03-12T11:56:05Z

I also run the remaining set of tests. All that is left is the review and we can merge it.

kxl-adsk

Looks very good. Thank you @mattyjams !

ppt-adsk

Great work Matt! Thanks for sticking with this and bringing it over the finish line.

ppt-adsk · 2021-03-12T15:46:05Z

Agree with Matt, the single failing Maya 2020 Windows python 2 test run appears to be build pipeline related, all tests pass.

mattyjams added enhancement New feature or request help wanted Extra attention is needed adsk Related to Autodesk plugin ufe Related to UFE component in Maya ufe-usd Related to UFE-USD plugin in Maya-Usd labels Mar 9, 2021

kxl-adsk requested a review from ppt-adsk March 9, 2021 00:39

ppt-adsk requested changes Mar 9, 2021

View reviewed changes

mattyjams commented Mar 10, 2021

View reviewed changes

mattyjams force-pushed the pr/ufe_point_instance_manipulation branch from 090a33e to 41db66b Compare March 10, 2021 22:08

ppt-adsk reviewed Mar 11, 2021

View reviewed changes

lib/mayaUsd/ufe/StagesSubject.cpp Show resolved Hide resolved

mattyjams added 8 commits March 11, 2021 10:55

add a specialized modifier to edit the "positions" attribute of Point…

2767421

…Instancers

add a specialized modifier to edit the "orientations" attribute of Po…

37d3a90

…intInstancers

add a specialized modifier to edit the "scales" attribute of PointIns…

c501a1c

…tancers

add undoable commands for manipulating point instances of a PointInst…

b657588

…ancer prim

add UFE Transform3d interface and handler for manipulating point inst…

fd36424

…ances

redirect transform3d handling of point instances in UsdTransform3dHan…

873c0d8

…dler Point instance manipulation using the older UsdTransform3dHandler is only supported with UFE v2. Otherwise, we disallow any manipulation for point instance scene items.

add UsdTransform3dPointInstanceHandler at the end of the Transform3d …

8c9a659

…handler chain

mattyjams force-pushed the pr/ufe_point_instance_manipulation branch from 41db66b to 8c9a659 Compare March 11, 2021 20:17

mattyjams removed the help wanted Extra attention is needed label Mar 11, 2021

mattyjams marked this pull request as ready for review March 11, 2021 20:31

add unit tests for point instance manipulation of positions, orientat…

e4d1e20

…ions, and scales

mattyjams assigned ecp-maya-devops-adsk Mar 11, 2021

fix build failure with VtArray::AsConst() missing for core USD prior …

bf4cbb2

…to 21.02

mattyjams assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Mar 12, 2021

fix for testPointInstances test when run under Python 3

d504519

The range() function returns lists in Python 2 which can be added together with the '+' operator, but that's not the case in Python 3.

mattyjams assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Mar 12, 2021

kxl-adsk approved these changes Mar 12, 2021

View reviewed changes

ppt-adsk approved these changes Mar 12, 2021

View reviewed changes

kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Mar 12, 2021

kxl-adsk merged commit 9a96421 into Autodesk:dev Mar 12, 2021

mattyjams deleted the pr/ufe_point_instance_manipulation branch March 12, 2021 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for manipulating point instances via UFE #1241

add support for manipulating point instances via UFE #1241

mattyjams commented Mar 9, 2021

ppt-adsk left a comment

ppt-adsk Mar 9, 2021

mattyjams Mar 10, 2021

ppt-adsk Mar 9, 2021

mattyjams Mar 10, 2021

ppt-adsk Mar 11, 2021

mattyjams Mar 11, 2021

mattyjams left a comment

mattyjams Mar 10, 2021

mattyjams Mar 10, 2021

mattyjams commented Mar 10, 2021

ppt-adsk left a comment

mattyjams commented Mar 11, 2021

mattyjams commented Mar 11, 2021

mattyjams commented Mar 12, 2021

mattyjams commented Mar 12, 2021

kxl-adsk commented Mar 12, 2021

kxl-adsk left a comment

ppt-adsk left a comment

ppt-adsk commented Mar 12, 2021

add support for manipulating point instances via UFE #1241

add support for manipulating point instances via UFE #1241

Conversation

mattyjams commented Mar 9, 2021

ppt-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyjams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattyjams commented Mar 10, 2021

ppt-adsk left a comment

Choose a reason for hiding this comment

mattyjams commented Mar 11, 2021

mattyjams commented Mar 11, 2021

mattyjams commented Mar 12, 2021

mattyjams commented Mar 12, 2021

kxl-adsk commented Mar 12, 2021

kxl-adsk left a comment

Choose a reason for hiding this comment

ppt-adsk left a comment

Choose a reason for hiding this comment

ppt-adsk commented Mar 12, 2021