MAYA-112039 - As a user, I'd like to have an unshareable stage #1455

alsoussi · 2021-06-03T13:04:40Z

ProxyShape Changes
- Adding New unsharable workflow
  - Main idea is we create a new (unshared) root and put the incoming source as a sublayer, thereby having the ability to sandbox your changes
  - Exposed new attribute for the unshared composition (needed for layer management)
  - There's an unshared SDF layer on the proxy (private member) which stores the data so that the user can toggle on and off without losing their data
  - Since we have the ability to create new layers under this new unshared root, we need to keep track of the "unshared layers" and the source layers. That data (layer list) is stored as metadata on the unshared composition root.
  - Saving on disk follows the same mechanism that it normally would, but as you will see in the save portion we have added some special handling for unshared comp layers (saved in maya file)
  - Also added a bunch of helper functions to Util save/load layer metadata and have a shared token
- Adding Ability to know if the Stage or Layer is incoming
  - By incoming I mean the stage is coming in from a connection, for example the instage data or the cache Id
  - This is needed since we want to have special handling for these stages. For example incoming stages should NOT be saved and show up differently in the layer edtior
  - Also added a bunch of helper functions to Util to be able to get all the layers recursively so we know if a layer is incoming.
Save Changes
- Moving some of the shared code between saveToUSD and SaveToMayaFile into separate local functions
- Refactor the getStagesToSave to Actually be getProxiesToSave, other than the naming, Tim had already started this, but what I did here is instead of walking the stages to determine which ones are dirty, I walk the proxies and determine which ones need saving, also I don't walk the proxies again when we do the savetousd or savetomaya, which should be a performance improvement (insignificant probably)
- We skip saving incoming Layers, here you will see I added the concept of Stub, so I save empty layer data and this is needed so that I can keep track of where the user moves these "incoming layers" in the hierarchy.
- Unshared comp layers are saved to maya file ALWAYS. Similar to the session layer the unshared comp layers always get saved into the maya file, so I put a special case to not prompt the user for changes when the changes are only in a layer that gets saved to maya file. NOTE: I did NOT fix this for the session layer.
Layer Editor Changes
- Adding the concept of ReadOnly Layers
- For now the read only layers are the incoming layers in the stage
- For UI I'm using the same UI as the mute
- These Readonly layers cannot be moved unless you are moving the parent readonly layer in the tree that contains both readonly and non-readonly layers
- Exposing new functions on the command hook to ask the proxy if the stage is coming from an incoming source and if its also using the unshared composition workflow

1) ProxyShape Changes - Adding New unsharable workflow - Main idea is we create a new (unshared) root and put the incoming source as a sublayer, thereby having the ability to sandbox your changes - Exposed new attribute for the unshared composition (needed for layer management) - There's an unshared SDF layer on the proxy (private member) which stores the data so that the user can toggle on and off without losing their data - Since we have the ability to create new layers under this new unshared root, we need to keep track of the "unshared layers" and the source layers. That data (layer list) is stored as metadata on the unshared composition root. - Saving on disk follows the same mechanism that it normally would, but as you will see in the save portion we have added some special handling for unshared comp layers (saved in maya file) - Also added a bunch of helper functions to Util save/load layer metadata and have a shared token - Adding Ability to know if the Stage or Layer is incoming - By incoming I mean the stage is coming in from a connection, for example the instage data or the cache Id - This is needed since we want to have special handling for these stages. For example incoming stages should NOT be saved and show up differently in the layer edtior - Also added a bunch of helper functions to Util to be able to get all the layers recursively so we know if a layer is incoming. 2) Save Changes - Moving some of the shared code between saveToUSD and SaveToMayaFile into separate local functions - Refactor the getStagesToSave to Actually be getProxiesToSave, other than the naming, Tim had already started this, but what I did here is instead of walking the stages to determine which ones are dirty, I walk the proxies and determine which ones need saving, also I don't walk the proxies again when we do the savetousd or savetomaya, which should be a performance improvement (insignificant probably) - We skip saving incoming Layers, here you will see I added the concept of Stub, so I save empty layer data and this is needed so that I can keep track of where the user moves these "incoming layers" in the hierarchy. - Unshared comp layers are saved to maya file ALWAYS. Similar to the session layer the unshared comp layers always get saved into the maya file, so I put a special case to not prompt the user for changes when the changes are only in a layer that gets saved to maya file. NOTE: I did NOT fix this for the session layer. 3) Layer Editor Changes - Adding the concept of ReadOnly Layers - For now the read only layers are the incoming layers in the stage - For UI I'm using the same UI as the mute - These Readonly layers cannot be moved unless you are moving the parent readonly layer in the tree that contains both readonly and non-readonly layers - Exposing new functions on the command hook to ask the proxy if the stage is coming from an incoming source and if its also using the unshared composition workflow

ppt-adsk

Not quite as familiar as I'd like to be with the workflow and how it connects to the data model and implementation, so my review doesn't have enough depth to stand on its own. Otherwise looks good to me.

lib/mayaUsd/commands/Readme.md

lib/mayaUsd/commands/abstractLayerEditorWindow.h

lib/mayaUsd/nodes/layerManager.cpp

lib/mayaUsd/nodes/proxyShapeBase.cpp

seando-adsk

Overall very good job. Just a few comments and suggestions.

lib/mayaUsd/nodes/layerManager.cpp

seando-adsk · 2021-06-17T13:06:30Z

lib/mayaUsd/nodes/layerManager.cpp

+        }
+    }
+
+    serializedHandle.setString(UsdMayaUtil::convert(temp));


Should we be setting this when status == MS:kFailure (from line 497). Won't temp be bad?

Yes, but this is the old behavior, I didn't want to change it.. But I agree with you

Let me know if I should change it

lib/mayaUsd/nodes/layerManager.cpp

lib/mayaUsd/nodes/proxyShapeBase.cpp

lib/usd/ui/layerEditor/mayaCommandHook.cpp

lib/usd/ui/layerEditor/mayaCommandHook.h

lib/usd/ui/layerEditor/stringResources.h

plugin/adsk/scripts/mayaUsdMenu.mel

kxl-adsk

Still have ProxyShape changes to review, but posting everything else that I have reviewed already.

lib/mayaUsd/nodes/layerManager.cpp

lib/usd/ui/layerEditor/layerEditorWidget.h

lib/usd/ui/layerEditor/mayaCommandHook.cpp

kxl-adsk · 2021-06-17T16:37:47Z

test/lib/mayaUsd/nodes/testProxyShapeBase.py

@@ -179,5 +179,93 @@ def testDuplicateProxyStageFileBacked(self):
        self.assertEqual(0, len(ufe.Hierarchy.hierarchy(duplProxyShapeItem).children()))
        self.assertEqual(0, len(ufe.Hierarchy.hierarchy(treebaseItem).children()))

+    def testShareStage(self):


Few more tests to add in here:

serialization with complex layer stack on the downstream proxy

upstream stage src changes, e.g. change file paths on the src proxy and make sure edits are preserved AND layer stack is preserved as well

Added more tests!

lib/mayaUsd/utils/util.cpp

lib/mayaUsd/nodes/layerManager.cpp

kxl-adsk · 2021-06-17T16:58:37Z

lib/mayaUsd/nodes/layerManager.cpp

+    return status;
+}
+
+MStatus setValueForAttr(const MObject& node, const MObject& attribute, const std::string& value)


Wouldn't https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/utils/util.h#L479 work?

No, its using the MObject not the attribute name. But nice one! Would have used it!

Co-authored-by: Sean Donnelly <[email protected]>

Co-authored-by: Krystian Ligenza <[email protected]>

alsoussi · 2021-06-30T17:06:22Z

@ppt-adsk @seando-adsk @kxl-adsk I updated with regards to your comments, apologies for resolving them on my side, it was my way of keeping track of them but moving forward I will let the person who puts the comment resolve it.

Let me know if you have any questions.

kxl-adsk · 2021-07-05T19:41:10Z

lib/mayaUsd/nodes/proxyShapeBase.cpp

+
+    // Share the stage
+    if (sharableStage) {
+        if (isIncomingStage) {


We are only introducing a new mode and shouldn't change the behavior of the sharable mode. Please remove this change of edit target.

kxl-adsk · 2021-07-05T19:43:24Z

lib/mayaUsd/nodes/proxyShapeBase.cpp

+            for (const auto& identifier : subLayerIds) {
+                SdfLayerRefPtr sublayer = SdfLayer::Find(identifier);
+                if (sublayer) {
+                    _unsharedStageRootSublayers.push_back(sublayer);


Why these layers don't get into UsdMayaStageCache?

Its simpler to keep pointers to them, putting them in the cache I will have to keep track of the id and remove them once i'm done with them or we leak (grow the cache).

kxl-adsk · 2021-07-05T19:46:27Z

lib/mayaUsd/nodes/proxyShapeBase.h

@@ -126,6 +129,8 @@ class MayaUsdProxyShapeBase
    static MObject outStageDataAttr;
    MAYAUSD_CORE_PUBLIC
    static MObject outStageCacheIdAttr;
+    MAYAUSD_CORE_PUBLIC
+    static MObject outIsStageIncomingAttr;


We already have a vector of all incoming layers, this attribute shouldn't be necessary. In isStageIncoming, you will check if incoming layers are not empty.

OK no worries I removed it. I had it for the test, but now I also removed those tests.

kxl-adsk · 2021-07-05T19:48:04Z

lib/mayaUsd/nodes/proxyShapeBase.h

+
+    // Keep track of source stage's root identifier (when we change the source we can swap it
+    // in-place in the layer heirarchy)
+    std::string _sourceRootIdentifier;


I'm not sure I understand the need for this identifier? You are already holding onto the stage root layer.

This is because I need to keep track of the source. On the proxy shape the source can be one of 4 things:

Stage Data In port

Stage Cache ID In Port

File Path (Valid)

New Anon Stage (if all else fails)

So when you are in unshared mode that source above is greyed out but you can still move it in the hierarchy. Meaning you can have

UnsharedRootLayer
|---> Some User Created Middle Layer
|---> The source (referenced above, buried in a complex hierarchy)

So now when we change the file or change the source we want to maintain that hierarchy, so we need to keep track of what the source was and what it will be.

After our discussion yesterday I was trying to access the old stage from the out port, but its hard for me to figure out which layer is which, the code gets really complicated if you don't want to keep track of the source layer. Simplest thing is to keep it as is.

kxl-adsk · 2021-07-05T20:00:06Z

lib/mayaUsd/nodes/proxyShapeBase.cpp

-        CHECK_MSTATUS_AND_RETURN_IT(retValue);
+    if (!remappedLayers.empty()) {
+        for (auto sublayer : sublayers) {
+            remapSublayerRecursive(sublayer, remappedLayers);


During iteration, you can discover that all layers were remapped. So you can save both searching for layers, vector insert and calls to remapSublayerRecursive if you do something like:

if (remappedLayers.empty()) return; for(const auto& sublayerPath : sublayerPaths) { SdfLayerRefPtr subLayer = SdfLayer::Find(sublayerPath); remapSublayerRecursive(sublayer, remappedLayers); if (remappedLayers.empty()) return; }

… crasher and adding new test

alsoussi · 2021-07-08T16:00:45Z

@kxl-adsk done!

kxl-adsk

Thank you. LGTM.

seando-adsk

Overall looks good - just a couple of minor changes.

lib/mayaUsd/utils/customLayerData.h

lib/usd/ui/layerEditor/mayaCommandHook.cpp

test/lib/CMakeLists.txt

seando-adsk · 2021-07-08T18:10:59Z

lib/usd/ui/layerEditor/mayaCommandHook.cpp

@@ -54,7 +59,40 @@ MString executeMel(const std::string& commandString)
 // maya doesn't support spaces in undo chunk names...
 MString cleanChunkName(QString name) { return quote(name.replace(" ", "_").toStdString()).c_str(); }

+std::string GetProxyShapeName(const std::string& proxyShapePath)


This function and the one below should be "lowerCamelCase" so "get" not "Get"

alsoussi · 2021-07-08T18:14:17Z

@seando-adsk Done!

…connection

alsoussi requested review from seando-adsk, kxl-adsk, luceric-adsk and spinell-adsk June 3, 2021 13:04

alsoussi assigned ecp-maya-devops-adsk Jun 3, 2021

luceric-adsk requested review from fowlertADSK and removed request for luceric-adsk June 3, 2021 21:13

MAYA-112039 - Fix formatting

fc3e51f

alsoussi assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jun 4, 2021

kxl-adsk requested review from ppt-adsk and removed request for spinell-adsk and fowlertADSK June 9, 2021 16:28

kxl-adsk added proxy Related to base proxy shape workflows Related to in-context workflows labels Jun 9, 2021

kxl-adsk requested a review from fowlertADSK June 9, 2021 16:37

ppt-adsk approved these changes Jun 15, 2021

View reviewed changes

seando-adsk requested changes Jun 17, 2021

View reviewed changes

kxl-adsk reviewed Jun 17, 2021

View reviewed changes

alsoussi and others added 3 commits June 21, 2021 11:59

Update plugin/adsk/scripts/mayaUsdMenu.mel

308b222

Co-authored-by: Sean Donnelly <[email protected]>

Update lib/usd/ui/layerEditor/layerEditorWidget.h

3fbfeb5

Co-authored-by: Krystian Ligenza <[email protected]>

MAYA-112039 - Bug fixes and PR comment fixes

643101b

kxl-adsk reviewed Jul 5, 2021

View reviewed changes

MAYA-112039 - Removing unnecessary attribute, general cleanup, fixing…

4b02d4d

… crasher and adding new test

kxl-adsk unassigned ecp-maya-devops-adsk Jul 8, 2021

kxl-adsk assigned ecp-maya-devops-adsk Jul 8, 2021

MAYA-112039 Fix clang format and remove unnecessary file

1bd1e2f

alsoussi assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 8, 2021

kxl-adsk approved these changes Jul 8, 2021

View reviewed changes

seando-adsk requested changes Jul 8, 2021

View reviewed changes

lib/mayaUsd/utils/customLayerData.h Outdated Show resolved Hide resolved

lib/usd/ui/layerEditor/mayaCommandHook.cpp Outdated Show resolved Hide resolved

test/lib/CMakeLists.txt Outdated Show resolved Hide resolved

seando-adsk reviewed Jul 8, 2021

View reviewed changes

MAYA-112039 - Minor fixes from Sean's review

dfc9153

alsoussi assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 8, 2021

MAYA-112039 - change to lowerCamelCase

73a4fdf

alsoussi assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 8, 2021

MAYA-112039 - Minor fix

4957ac1

alsoussi assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 8, 2021

seando-adsk approved these changes Jul 8, 2021

View reviewed changes

MAYA-112039 - Fix OSX issues and no UFE build break

e01873b

alsoussi assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 9, 2021

MAYA-112039 - Skipping test since we found a bigger issue with stage …

0163236

…connection

alsoussi assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 9, 2021

kxl-adsk merged commit 1646a8b into dev Jul 10, 2021

kxl-adsk deleted the t_al-sa/MAYA-112039/unsharable_stage branch July 10, 2021 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-112039 - As a user, I'd like to have an unshareable stage #1455

MAYA-112039 - As a user, I'd like to have an unshareable stage #1455

alsoussi commented Jun 3, 2021

ppt-adsk left a comment

seando-adsk left a comment

seando-adsk Jun 17, 2021

alsoussi Jun 20, 2021

alsoussi Jun 20, 2021

kxl-adsk left a comment

kxl-adsk Jun 17, 2021

alsoussi Jun 30, 2021

kxl-adsk Jun 17, 2021

alsoussi Jun 23, 2021

alsoussi commented Jun 30, 2021

kxl-adsk Jul 5, 2021

alsoussi Jul 6, 2021

kxl-adsk Jul 5, 2021

alsoussi Jul 6, 2021

kxl-adsk Jul 5, 2021

alsoussi Jul 7, 2021

kxl-adsk Jul 5, 2021

alsoussi Jul 7, 2021

alsoussi Jul 8, 2021

kxl-adsk Jul 5, 2021

alsoussi Jul 8, 2021

alsoussi commented Jul 8, 2021

kxl-adsk left a comment

seando-adsk left a comment

seando-adsk Jul 8, 2021

alsoussi Jul 8, 2021

alsoussi commented Jul 8, 2021

MAYA-112039 - As a user, I'd like to have an unshareable stage #1455

MAYA-112039 - As a user, I'd like to have an unshareable stage #1455

Conversation

alsoussi commented Jun 3, 2021

ppt-adsk left a comment

Choose a reason for hiding this comment

seando-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

kxl-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

alsoussi commented Jun 30, 2021

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

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

alsoussi commented Jul 8, 2021

kxl-adsk left a comment

Choose a reason for hiding this comment

seando-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

alsoussi commented Jul 8, 2021