Refactor of way serialization of layers is done within a maya scene file #53

pmolodo · 2017-12-13T02:39:14Z

Removed per-layer nodes, and mapping of inter-relationships between them, as they were not needed to handle serialization. Instead now uses a single layerManager node to store serializations (one per maya scene.) Fixes some stability issues, and some issues with serializations being lost in some cases.

previously, was using connection callbacks, which is fragile; now uses normal dependency graph push / pull of data to trigger updates of stage / prim

simplifies serialization, makes it more reliable in edge cases

...and not for every proxyShape in the scene, every time any reference is loaded

on the transform node, a connection is made from the stageOut to the stageIn using the modifier, but then the primPath is set without it. Therefore, the primPath set happens FIRST, when there is no stage

dbaz

Thanks for your time! There is a few comments below for discussion.

Seems like we may not need layer.h anymore? nuke it if you get the chance or we will do it at some point.

~~I'll talk with the team about the singleton style part of the LayerManager and let you know.~~ Talked with Eoin, he mentioned he talked to you about it and it was OK'd by us.

In regards to the mutex locks in the LayerManager, are you guys expecting, or are, adding layers in different threads? Just for my interest

dbaz · 2017-12-20T23:04:47Z

lib/AL_USDMaya/AL/usdmaya/Global.cpp

@@ -153,8 +208,8 @@ void Global::onPluginLoad()
  m_fileNew = MSceneMessage::addCallback(MSceneMessage::kAfterNew, onFileNew);
  m_preSave = MSceneMessage::addCallback(MSceneMessage::kBeforeSave, preFileSave);
  m_postSave = MSceneMessage::addCallback(MSceneMessage::kAfterSave, postFileSave);
-  m_preOpen = MSceneMessage::addCallback(MSceneMessage::kBeforeOpen, preFileOpen);
-  m_postOpen = MSceneMessage::addCallback(MSceneMessage::kAfterOpen, postFileOpen);
+  m_preRead = MSceneMessage::addCallback(MSceneMessage::kBeforeFileRead, preFileRead);


From quickly browsing the docs it looks like k(Before|After)FileRead also triggers when a Reference in Maya is created. So if I had a variantSet and switched it to a variant that contained a Prim of type MayaReference, which will load a MayaReference, it will trigger preFileRead(); From my understanding. Will that cause problems in your postFileRead?

No, this should work fine - we control whether assets are brought in as usd/hydra or MayaReference types with variants in our own pipe, and it hasn't caused problems. In theory, if your referenced scene in turn had it's own ProxyShape, you'd want this to trigger anyway (though I wouldn't really recommend doing that, it's not really heavily tested).

Oh, and regarding layer.h - it's there just to provide a stub implementation of the layer nodes. They no longer do anything, but the node is still provided by the plugin so that if you open a scene that was saved using old-style-layer serialization, it won't error - all the same attributes, etc are there.

Of course, any serialized layer changes you'd made in the old format will no longer be recognized / will be lost. At some point in the future, we could potentially write some upgrade / transition code, which reads the old layer nodes. Actually, probably not even be hard to do... would just had to add a loop inside of the postRead callback that loads all the layers...

Sounds good about the references not being a problem still!

Sure, no harm in leaving the node code there I guess, we will remove it at some point.

Cheers!

Oh, and regarding the mutex locks - as far as I know, we're not loading layers in a multithreaded manner. The locks are more from paranoia... I don't know they COULDN'T be loaded in a multithreaded manner (I haven't really looked into the way maya's new multi-threaded node evaluation works, for instance), and it's essentially a globally shared resource, so I figured better be safe.

dbaz · 2017-12-20T23:22:28Z

lib/AL_USDMaya/AL/usdmaya/cmds/LayerCommands.cpp

-  return MObject::kNullObj;
-}
+////----------------------------------------------------------------------------------------------------------------------
+//MObject LayerCommandBase::getSelectedNode(const MArgDatabase& args, const MTypeId typeId)


Reminder to just delete this commented out code before merging

dbaz · 2017-12-20T23:59:47Z

lib/AL_USDMaya/AL/usdmaya/cmds/LayerCommands.cpp

-        MGlobal::displayError("Cannot query many names on muted layers (they layers haven't been imported into Maya)");
-        return MS::kFailure;
-      }
+      // TODO: figure out what to do with "-identifiers" flag. Unlike other


I'm thinking that we should make it like the others and return the identifier when the flag is on and the displayName otherwise, and make sure we document it in the ChangeLog

I'll talk with the other peeps and see what they think

dbaz · 2017-12-21T00:59:50Z

lib/AL_USDMaya/AL/usdmaya/cmds/LayerCommands.cpp

@@ -1041,6 +674,9 @@ AL_MAYA_DEFINE_COMMAND(LayerSave, AL_usdmaya);
 MSyntax LayerSave::createSyntax()
 {
  MSyntax syn = setUpCommonSyntax();
+  // TODO: -l is currently required, convert to an arg... but need


Your comment about converting -l to an arg sounds good

dbaz · 2017-12-21T02:52:51Z

lib/AL_USDMaya/AL/usdmaya/nodes/LayerManager.cpp

+    }
+    return status;
+  }
+}


Do you need to do a dgmod.doit() at the end?

dbaz · 2017-12-21T05:41:31Z

lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.h

@@ -636,6 +623,18 @@ class ProxyShape
  /// \param[in] changedPaths are child paths that existed previously and may not be existing now.
  void onPrimResync(SdfPath primPath, const SdfPathVector& changedPaths);

+
+  // \brief Serialize information unique to this shape
+  void serializeThis();


Drop the This I think it is self documenting that it will save itself, since is a member method

dbaz · 2017-12-21T05:47:36Z

lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.cpp

@@ -962,12 +952,11 @@ void ProxyShape::variantSelectionListener(SdfNotice::LayersDidChange const& noti
 }

 //----------------------------------------------------------------------------------------------------------------------
-void ProxyShape::reloadStage(MPlug& plug)
+void ProxyShape::loadStage()


I liked the reloadStage() name, because when stuff changes, such as the m_fileName, the name made a bit more sense in that context, because you want to reload the stage. What do you think @robthebloke ?

@dbaz I still like loadStage because: If the file name changes, its reading from a whole new source and USD treats it like a different stage in the cache. Part of the motivation for changing it was that it was getting called repeatedly (for other reasons than the file name change) and each time it was adding a new stage to the stage cache because they each had a unique anonymous session layer. Naming it loadStage() discourages using it simply to refresh the data on the same stage.

Yep, sure. Thanks for the explanation.

dbaz · 2017-12-21T05:59:03Z

lib/AL_USDMaya/AL/usdmaya/nodes/Transform.cpp

  if(plug == m_primPath)
  {
    MDataBlock dataBlock = forceCache(*(MDGContext *)&context);
    MString path = handle.asString();
    outputStringValue(dataBlock, m_primPath, path);
    SdfPath primPath(path.asChar());
    StageData* data = inputDataValue<StageData>(dataBlock, m_inStageData);
-    if(data)
+    if (data && data-> stage)


Remove the space 'data-> stage'

dbaz · 2017-12-21T06:04:28Z

lib/AL_USDMaya/AL/usdmaya/nodes/TransformationMatrix.cpp

@@ -116,7 +116,7 @@ void TransformationMatrix::setPrim(const UsdPrim& prim)
    m_xform = UsdGeomXform();
  }
  m_time = UsdTimeCode(UsdTimeCode::Default());
-  m_flags = 0;
+  m_flags &= kPreservationMask;


I think this should stay = 0, there is some code that will inspect the prim and set the correct flags, I hope?

For most of the flags, yes, they are read from the prim - however, there are two flags, kPushToPrimEnabled and kReadAnimated, that are controlled not by the prim, but by attributes on the node. These should NOT be reset when the prim is reset, thus the need for this flag.

I document this above the definition of kPreservationMask, but I'll copy that here for clarity.

dbaz · 2017-12-21T06:19:26Z

lib/AL_USDMaya/AL/usdmaya/nodes/LayerManager.cpp

+
+
+//----------------------------------------------------------------------------------------------------------------------
+// TODO: make it take an optional MDGModifier


sounds handy

dbaz · 2017-12-21T07:16:04Z

lib/AL_USDMaya/AL/usdmaya/nodes/LayerManager.h

+
+  /// \brief  Find the already-existing non-referenced LayerManager node in the scene, or return a nullptr
+  /// \return the found LayerManager, or a nullptr
+  static LayerManager* findManager();


Can we have one method, findOrCreateManager()? and remove findManager? If we a buying into a singleton, I'd expect it to be created once the first time I access the LayerManager, or straight up create a LayerManager Node when the AL_USDMaya plugin is loaded and we have a scene. Otherwise I think havint the two methods can complicate the code, since it seems like some code should call findOrCreateManager and some code should call findManager, but who really should create it?

Not sure I completely follow what you mean by this:

"some code should call findOrCreateManager and some code should call findManager, but who really should create it?"

As for why I want two methods - basically, findManager() doubles as a "doesManagerExist()" query. Yes, we could take the approach of always force-creating a manager whenever we do any sort of querying (or even on plugin load), but I'm strongly against this, because this involves creating a node in the scene. That then means that your scene now requires this plugin, even if it really doesn't - ie, if you have AL_USDMaya loaded, open a new scene, then save out a blank scene, it now has a requirement to load AL_USDMaya.

We've had a few plugins from third-party vendors that operated like this, and they're highly annoying - the plugin ends up acting like a virus that infects scenes: when anyone opens such a scene, then opens and saves out another scene - even if that other scene doesn't need the plugin at all - they spread the "infection" to the other scene.

In our two studios, we may not care much, because I'm guessing we'll both nearly always want this plugin loaded, anyway, but it's good plugin-hygiene to only create nodes if the scene in question actually needs them.

I guess I was seeing the LayerManager as the style of singleton where if you "access it, you create it" because otherwise you will have 2 methods as you have, a FindAndCreate and a Find, which I think can cause confusion about when the creation of the LayerManager should happen. i.e. when should I call FindAndCreate?

But I see why you have done it this way, because you only want to create it when there is actually a ProxyShape in the scene which seems fair enough!

alekamca · 2017-12-21T21:45:59Z

I like the idea of the singleton LayerManager in general, but I wanted to flag that we've also made a bunch of changes with the Layer nodes (many of which we were planning to contribute back). I'm sure we can find a way to make this new way work for us, but I thought I'd share what we're trying to do in case it throws up any red flags.

We've added a lot of guards related to handling cases where something bad has happened to the usd file associated with the proxy shape, including missing layer files. This is much more likely for us because we also are storing unresolved partial paths for some layers to support the way we publish assets. Long story short, we utilize the process of layer initialization in this PR (#28 sorry--still getting around to cleaning this up and resubmitting) to detect if something has gone wrong at the layer level. If it has, we notify the associated ProxyShape and put it into an error state (with hopefully enough debug information to help users find their way out of the problem).

pmolodo · 2017-12-22T00:06:47Z

@alekamca - Hmm... I can see your concern. The previous node system actually traversed / mapped out the whole layer hierarchy / inter-relationships, and recorded them in the maya nodes... initially just for the purposes of visualization, but it wasn't necessary for serialization, so I removed this traversal in my PR.

However, I think this sort of roll-your-own layer traversal is probably not preferred anyway... and I image there must be a way either be notified of errors on layer load, or finding them after the fact. I'll ask in the USD mailing list...

alekamca · 2017-12-22T01:00:41Z

@elrond79 Yes, I agree that we should be using whatever facilities USD would prefer us to use. My only other thought is that by encoding the layer hierarchy in the maya scene, we can also tell whether or not the hierarchy has changed. We aren't doing this right now, but I can imagine cases where this would be useful to know, otherwise our layer deserialization may not make sense.

Somewhat related, I don't know that we even care to have the maya files carry around this layer serialization anyways. We might prefer a preFileSave callback that just saves the USD stage, so it might all be irrelevant.

chadrik · 2017-12-22T01:08:53Z

My only other thought is that by encoding the layer hierarchy in the maya scene, we can also tell whether or not the hierarchy has changed.

@alekamca I believe that continually rewiring the node network to reflect the stage requires a lot of work to make accurate (and may not even be possible based on lack of valid layer identifiers over time) and Maya is not very resilient when it comes to transient nodes and connections. It's just asking for trouble without much benefit. That's my high level understanding of the problem: I'll let @elrond79 fill in the details.

Somewhat related, I don't know that we even care to have the maya files carry around this layer serialization anyways. We might prefer a preFileSave callback that just saves the USD stage, so it might all be irrelevant.

Yes, we also considered this, and there's a lot to like about storing this data in a side-car file that can be accessed outside of Maya. It'll take some wrangling to make sure that callbacks are always firing at the right moments so that data is not lost. Xgen pre-2017 is a good example of how this approach can go wrong, but I think it's a solvable problem and we'd love to see this become an option.

…er flags)

...thanks!

pmolodo · 2017-12-23T01:52:59Z

@dbaz - ok, I think that addresses all of your notes, save for the issue with re-serialization of session layers, as I noted above. I'll hopefully have a fix for that soon...

Thanks greatly for the extra eyes on this, always appreciated!

dbaz · 2018-01-01T23:26:25Z

thanks @elrond79 all seems good. I just have a question about the session layer saving code and if you are going to do the change you mentioned now or later?

pmolodo · 2018-01-03T20:45:28Z

I'm starting working on it now, should hopefully be done in a few days...

chadrik · 2018-01-03T20:56:22Z

@dbaz @robthebloke Would you prefer to wait for that change, or merge this now and take the change as a second PR?

dbaz · 2018-01-03T21:43:17Z

Prefer it to be done during this PR since I want to reduce the possibility of serialisation/deserialisation maya file issues for people.

Conflicts: lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.cpp

…K_ERROR_RETURN inserted makes sense for AL_MAYA_CHECK_ERROR_RETURN / AL_MAYA_CHECK_ERROR_RETURN_NULL_MOBJECT to be next to each other (also makes merge with pr/use_PxrUsdMayaXformStack easier)

...because boost::multi_index won't allow us to have two identifiers for same layer

…uplicate serialization

…th serializedSessionLayer

also, if have a transform with multiple shapes, check all to try to find a proxyShape (not just the first)

...not just the ones already in the LayerManager also, added a helper to allow unconditionally adding flags to the command to execute - CommandGuiHelper::addExecuteText

...now that I've realized I can just forward declare LayerManager, since I only need a pointer for the header...

...then, when we go to serialize we do one last check, to see if current target is dirty

pmolodo · 2018-01-13T01:47:06Z

Ok, made it so session layers are now serialized on the LayerManager, which should resolve the duplicate-serialization. Also fixed a few other bugs (one where layers were not being serialized in some situations, and one with the SetEditTarget gui).

dbaz

Cheers! Few small comments, mainly the ones referring to compilation/maya compatibility issues I'm getting when building.

dbaz · 2018-01-14T22:24:33Z

lib/AL_USDMaya/AL/usdmaya/cmds/LayerCommands.cpp

 {
+  MStringArray result;
+  MSelectionList sl;
+  AL_MAYA_CHECK_ERROR_RETURN_VAL(MGlobal::getActiveSelectionList(sl), result,


Where is AL_MAYA_CHECK_ERROR_RETURN_VAL declared? I'm getting a compilation error regarding it

dbaz · 2018-01-14T23:19:19Z

plugin/AL_USDMayaTestPlugin/AL/usdmaya/nodes/test_LayerManager.cpp

+    MPlug anonymousPlug = layersPlug0.child(tempNonConst, &status);
+    ASSERT_TRUE(status);
+
+    ASSERT_EQ(MString(realLayer->GetIdentifier().c_str()), idPlug.asString(&status));


This asString isn't compatible with maya2017 Update3, can we use asString(DGContext::fsNormal, &status), also for the asBool calls?

dbaz · 2018-01-15T00:38:47Z

lib/AL_USDMaya/AL/maya/CommandGuiHelper.cpp

+    {
+      // Just throwing, if we ever need to use a string with weird chars, need to
+      // update this function...
+      throw std::logic_error("CommandGuiHelper::addExecuteText may only be called with normal printable characters");


Having a bit more information about which character is the 'bad' one would be nice, Could we print out the index of the character in the string?

Also, could make this a std::cerr and a return. Seems a bit harsh to throw an exception for this case.

note that addLayer/removeLayer changed to take a LayerHandle (WeakPtr) instead of a RefPtr, due to behavor where python wrapping downgrades a RefPtr to a WeakPtr after it has been passed to a function taking a RefPtr

migrated from pr/use_PxrUsdMayaXformStack branch

pmolodo · 2018-01-16T01:48:21Z

Ok, adressed your notes - thanks!
Also added a wrapper for the LayerManager (7209fae) since we needed that over here...

dbaz · 2018-01-16T03:04:23Z

Thanks! I will look at merging this ASAP

pmolodo added 9 commits December 4, 2017 14:39

Transform: remove unset / unused outStageData attr

bbed679

Transform: simplify way inStageData is consumed

166a593

previously, was using connection callbacks, which is fragile; now uses normal dependency graph push / pull of data to trigger updates of stage / prim

layer refactor: replace layer nodes with a single layer manager node

bac1231

simplifies serialization, makes it more reliable in edge cases

fix for loading of plugin on scene open via requires

c021970

ProxyShape: make sure outStageData is marked dirty after stageReload()

59524c8

rename reloadStage to loadStage

2931f9f

make sure loadStage is only called once on node creation

37351e4

...and not for every proxyShape in the scene, every time any reference is loaded

fix makeUsdTransformChain

68d99c7

on the transform node, a connection is made from the stageOut to the stageIn using the modifier, but then the primPath is set without it. Therefore, the primPath set happens FIRST, when there is no stage

test_LayerManager: newFile at start of tests

ed4bcc1

dbaz reviewed Dec 21, 2017

View reviewed changes

alekamca mentioned this pull request Dec 22, 2017

Allow automatic saving of USD stage rather than serialization #56

Open

pmolodo added 6 commits December 22, 2017 10:00

remove commented out getSelectedNode

51484ee

make LayerGetLayers -muted return display layers by default (like oth…

80e1763

…er flags)

LayerSave command - formally make layer a mandatory arg

f1033fb

[LayerManager] Add notes about multithreading / mutex locks

5dd57e6

[LayerManager] fix disconnectCompoundArrayPlug, add test

2585ef4

[LayerManager] address miscellaneous code review notes from dbaz

c1239a7

...thanks!

dbaz mentioned this pull request Jan 2, 2018

Initialise all layers on file load. #28

Closed

pmolodo added 2 commits January 3, 2018 16:39

Merge branch 'develop' into pr/layer_refactor

5a8abe3

Conflicts: lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.cpp

maya/Common.h: move where AL_MAYA_CHECK_ERROR_CONTINUE / AL_MAYA_CHEC…

a4b2d44

…K_ERROR_RETURN inserted makes sense for AL_MAYA_CHECK_ERROR_RETURN / AL_MAYA_CHECK_ERROR_RETURN_NULL_MOBJECT to be next to each other (also makes merge with pr/use_PxrUsdMayaXformStack easier)

pmolodo added 11 commits January 10, 2018 09:34

LayerManager: use custom LayerDatabase class to store layers

a1d35b8

...because boost::multi_index won't allow us to have two identifiers for same layer

LayerManager: allow findOrCreateNode/Manager to take optional DGModifier

6e57f65

LayerManager: some tweaks to debug prints

ffb96a0

test_ProxyShapeImport: add test for ProxyShapeImport with sessionLayer

5c00bd6

LayerManager: serialize session layers on layer manager, to prevent d…

46f18e9

…uplicate serialization

LayerManager: add some backward-compatibility code for ProxyShapes wi…

bde37e7

…th serializedSessionLayer

LayerCommands: find proxyShape from selected ProxyShapeTransform

7b6923a

also, if have a transform with multiple shapes, check all to try to find a proxyShape (not just the first)

Merge branch 'pr/getProxyFromSelectedTransform' into pr/layer_refactor

64766d6

CommandGuiHelper: setEditTarget gui should show ALL layers in stage

f11c856

...not just the ones already in the LayerManager also, added a helper to allow unconditionally adding flags to the command to execute - CommandGuiHelper::addExecuteText

ProxyShape: move trackEditTargetLayer back as a method on ProxyShape

db8afe1

...now that I've realized I can just forward declare LayerManager, since I only need a pointer for the header...

ProxyShape: on editTarget change, check if PREVIOUS target was dirty

8f17acf

...then, when we go to serialize we do one last check, to see if current target is dirty

dbaz reviewed Jan 15, 2018

View reviewed changes

pmolodo added 4 commits January 15, 2018 10:16

LayerManager: add python wrapper

7209fae

note that addLayer/removeLayer changed to take a LayerHandle (WeakPtr) instead of a RefPtr, due to behavor where python wrapping downgrades a RefPtr to a WeakPtr after it has been passed to a function taking a RefPtr

add AL_MAYA_CHECK_ERROR_RETURN_VAL

15546c8

migrated from pr/use_PxrUsdMayaXformStack branch

CommandGuiHelper::addExecuteText - print offending characters

85c1aa2

test_LayerManger: compatibility fixes for maya 2017

a9dfcaf

dbaz approved these changes Jan 16, 2018

View reviewed changes

dbaz merged commit 9fd4545 into AnimalLogic:develop Jan 16, 2018



		//----------------------------------------------------------------------------------------------------------------------
		// TODO: make it take an optional MDGModifier

Refactor of way serialization of layers is done within a maya scene file #53

Refactor of way serialization of layers is done within a maya scene file #53

Conversation

pmolodo commented Dec 13, 2017

dbaz left a comment • edited

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

dbaz Dec 21, 2017 • edited

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

pmolodo Dec 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekamca commented Dec 21, 2017

pmolodo commented Dec 22, 2017

alekamca commented Dec 22, 2017

chadrik commented Dec 22, 2017

pmolodo commented Dec 23, 2017 • edited

dbaz commented Jan 1, 2018

pmolodo commented Jan 3, 2018

chadrik commented Jan 3, 2018

dbaz commented Jan 3, 2018

pmolodo commented Jan 13, 2018

dbaz 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

pmolodo commented Jan 16, 2018

dbaz commented Jan 16, 2018

dbaz left a comment •

edited

dbaz Dec 21, 2017 •

edited

pmolodo Dec 23, 2017 •

edited

pmolodo commented Dec 23, 2017 •

edited