MAYA-125378 Implementation batched duplication #2687

JGamache-autodesk · 2022-10-26T18:41:08Z

This allows Maya to duplicate multiple items and to properly fixup connections and relationships between all these individual duplicates.

Also implements the "no connections" mode that is the default Maya behavior. Use "Duplicate Special" with the "Duplicate input connections" active when duplicating shaders.

This allows Maya to duplicate multiple items and to properly fixup connections and relationships between all these individual duplicates. Also implements the "no connections" mode that is the default Maya behavior. Use "Duplicate Special" with the "Duplicate input connections" active when duplicating shaders.

seando-adsk · 2022-10-31T19:57:54Z

lib/mayaUsd/ufe/UsdBatchOpsHandler.h

+    bool                               _inBatchedDuplicate = false;
+
+    // Clear all guard data.
+    void _clearGuardData();


I don't think this needs _
There is nothing in coding standard about that.

seando-adsk · 2022-10-31T19:59:40Z

lib/mayaUsd/ufe/UsdUndoDuplicateFixupsCommand.h

+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+#pragma once


We don't use pragma once. See the coding standard.

seando-adsk · 2022-10-31T20:00:43Z

lib/mayaUsd/ufe/UsdUndoDuplicateFixupsCommand.h

+    bool duplicateUndo();
+    bool duplicateRedo();
+
+    bool _updateSdfPathVector(


_ again - it is supposed to mean something ?

seando-adsk · 2022-11-01T21:49:00Z

lib/mayaUsd/ufe/UsdUndoDuplicateFixupsCommand.h

+    typedef std::map<SdfPath, SdfPath>                        TDuplicatePathsMap;
+    typedef std::unordered_map<Ufe::Path, TDuplicatePathsMap> TDuplicatesMap;


The coding standard says to prefer using over typedef. But in either case no "T".

ppt-adsk

Great work, just needs a few additional undo / redo tests IMO.

ppt-adsk · 2022-11-04T17:31:28Z

test/lib/ufe/testBatchOpsHandler.py

+        dGeomBindAPI = UsdShade.MaterialBindingAPI(dGeomPrim)
+        # Now seeing and using ss3SG1
+        self.assertEqual(dGeomBindAPI.GetDirectBinding().GetMaterialPath(), Sdf.Path("/mtl/ss3SG1"))
+


Would be nice to test undo / redo of the UFE command.

ppt-adsk · 2022-11-04T17:32:34Z

test/lib/ufe/testBatchOpsHandler.py

+        dGeomBindAPI = UsdShade.MaterialBindingAPI(dGeomPrim)
+        # Now seeing and using ss3SG1
+        self.assertEqual(dGeomBindAPI.GetDirectBinding().GetMaterialPath(), Sdf.Path("/mtl/ss3SG1"))
+


Would be nice to test undo / redo.

ppt-adsk · 2022-11-04T17:32:56Z

test/lib/ufe/testBatchOpsHandler.py

+        connections = connectionHandler.sourceConnections(f3pDupItem)
+        self.assertIsNotNone(connections)
+        self.assertEqual(len(connections.allConnections()), 0)
+


Undo / redo.

ppt-adsk · 2022-11-04T17:34:17Z

lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h

+#include <mayaUsd/undo/UsdUndoableItem.h>
+
+#include <pxr/usd/sdf/path.h>
+#include <pxr/usd/usd/prim.h>


Do we use this include?

ppt-adsk · 2022-11-04T17:34:58Z

lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp

+    , _copyExternalInputs(shouldConnectExternalInputs(duplicateOptions))
+{
+    // TODO: MAYA-125854. If duplicating /a/b and /a/b/c, it would make sense to order the
+    // operations by SdfPath, and always check if the previoulsy processed path is a prefix of the


Typo: "previoulsy"

External relationships are now always copied irrespective of the "imputConnections" option.

Geometries that had their own material scope, when duplicated, would lose their shading because connections deep in the hierarchy were considered external. Fixed the external check.

JGamache-autodesk · 2022-11-04T21:49:15Z

cmake/modules/FindUFE.cmake

@@ -11,6 +11,7 @@
 # UFE_VERSION         UFE version (major.minor.patch) from ufe.h
 # UFE_LIGHTS_SUPPORT  Presence of UFE lights support
 # UFE_SCENE_SEGMENT_SUPPORT Presence of UFE scene segment support
+# UFE_PREVIEW_BATCHOPS_SUPPORT Presence of UFE batched operations support


Should be UFE_PREVIEW_FEATURES ...

JGamache-autodesk · 2022-11-04T21:50:42Z

cmake/modules/FindUFE.cmake

+
+if (UFE_INCLUDE_DIR AND EXISTS "${UFE_INCLUDE_DIR}/ufe/batchOpsHandler.h")
+    list(APPEND UFE_PREVIEW_FEATURES BatchOps)
+endif()


Using a list to store keys looks like a viable solution to handle progressively added features.

I like it, but I'm not a CMake expert.

I think each feature added to UFE_PREVIEW_FEATURES should be tagged with the version first. So "v4_BatchOps". This would make them easier to find and remove/modify to normal ufe ifdef once Ufe v4 is released.

What happens when we backport v5_WorkflowX into v4 for an hypothetical Maya dot release?

Once Ufe v4 is released we remove all these "preview" checks and replace them with just "ufe v4" check.

ppt-adsk · 2022-11-07T15:00:15Z

cmake/modules/FindUFE.cmake

+
+if (UFE_INCLUDE_DIR AND EXISTS "${UFE_INCLUDE_DIR}/ufe/batchOpsHandler.h")
+    list(APPEND UFE_PREVIEW_FEATURES BatchOps)
+endif()


I like it, but I'm not a CMake expert.

seando-adsk · 2022-11-07T15:26:40Z

cmake/modules/FindUFE.cmake

+# Gather all preview features that might be there or not into a single list:
+list(APPEND UFE_PREVIEW_FEATURES v4)


We can't just say that it includes v4, because when compiling with older versions of Maya we aren't using Ufe v4.

Isn't this rather saying that UFE_PREVIEW_FEATURES is a list that pertains to UFE v4? Not sure what we're getting by placing this "v4" string into the list, because it will always be the case that preview features will pertain to the up-coming UFE version.

In fact, I would push one step further and say that maybe even "preview" is too specific. If we imagine this being only a list of Ufe features, then it enable them as soon as Ufe supports them in Maya, which becomes very interesteing from a feature backporting scenario. I do remember the MAYA_LIGHT_API_VERSION kicking in when I backported the light API fixes for 2020.3 without having to update MayaUSD.

Might be potential discussion subject for the UFE interest meeting.

Better initializer, since list can not be empty, would be "ufe".

Also did some updates following review.

…ctions_on_batched_duplication

seando-adsk · 2022-11-09T16:44:21Z

lib/mayaUsd/ufe/UsdBatchOpsHandler.cpp

+
+#include <mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h>
+
+PXR_NAMESPACE_USING_DIRECTIVE


Do you need this? All new code should use the fully qualified namespace name. But I don't see any USD code here.

seando-adsk · 2022-11-09T16:45:00Z

lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp

+
+#include <ufe/path.h>
+
+PXR_NAMESPACE_USING_DIRECTIVE


Again do you need this?

If its for the TF_VERIFY below, then put the macro inside that method.

seando-adsk

Just a couple of minor comments.

…ctions_on_batched_duplication

seando-adsk

Excellent thanks.

JGamache-autodesk requested review from hodoulp, neilh-adsk and seando-adsk October 26, 2022 18:41

JGamache-autodesk assigned ecp-maya-devops-adsk Oct 26, 2022

seando-adsk requested changes Nov 1, 2022

View reviewed changes

Reimplementd using duplicateSelection call

f1b8cd4

JGamache-autodesk changed the title ~~MAYA-125378 Implementation of a duplication guard~~ MAYA-125378 Implementation batched duplication Nov 4, 2022

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 4, 2022

Coding standard fixes.

ac62c4a

JGamache-autodesk requested a review from seando-adsk November 4, 2022 16:32

ppt-adsk reviewed Nov 4, 2022

View reviewed changes

Adressing review comment and spec change.

b524492

External relationships are now always copied irrespective of the "imputConnections" option.

JGamache-autodesk requested a review from ppt-adsk November 4, 2022 18:27

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 4, 2022

ppt-adsk previously approved these changes Nov 4, 2022

View reviewed changes

Fix issue discovered while testing.

0cc1d0b

Geometries that had their own material scope, when duplicated, would lose their shading because connections deep in the hierarchy were considered external. Fixed the external check.

JGamache-autodesk dismissed ppt-adsk’s stale review via 0cc1d0b November 4, 2022 20:02

ppt-adsk previously approved these changes Nov 4, 2022

View reviewed changes

Fix build when batchOps are absent.

ed11890

JGamache-autodesk dismissed ppt-adsk’s stale review via ed11890 November 4, 2022 21:04

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 4, 2022

More elegant CMake feature detection.

f0799ad

JGamache-autodesk commented Nov 4, 2022

View reviewed changes

Fix doc.

419275e

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 4, 2022

JGamache-autodesk requested a review from ppt-adsk November 7, 2022 14:18

ppt-adsk previously approved these changes Nov 7, 2022

View reviewed changes

seando-adsk reviewed Nov 7, 2022

View reviewed changes

JGamache-autodesk unassigned ecp-maya-devops-adsk Nov 7, 2022

JGamache-autodesk added 2 commits November 7, 2022 15:57

Updated to latest BatchOps API.

8da1e6c

Also did some updates following review.

Merge branch 'dev' into gamaj/MAYA-125378/fix_relationships_and_conne…

0e0e9f5

…ctions_on_batched_duplication

JGamache-autodesk dismissed ppt-adsk’s stale review via 0e0e9f5 November 7, 2022 21:21

JGamache-autodesk assigned ecp-maya-devops-adsk Nov 7, 2022

ppt-adsk previously approved these changes Nov 7, 2022

View reviewed changes

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 8, 2022

JGamache-autodesk requested a review from seando-adsk November 8, 2022 19:53

seando-adsk reviewed Nov 9, 2022

View reviewed changes

JGamache-autodesk added 2 commits November 9, 2022 11:49

Merge branch 'dev' into gamaj/MAYA-125378/fix_relationships_and_conne…

ac850c1

…ctions_on_batched_duplication

Remove using directive

acd172c

JGamache-autodesk dismissed ppt-adsk’s stale review via acd172c November 9, 2022 18:04

JGamache-autodesk requested a review from seando-adsk November 9, 2022 18:05

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 9, 2022

ppt-adsk approved these changes Nov 9, 2022

View reviewed changes

seando-adsk approved these changes Nov 9, 2022

View reviewed changes

JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 9, 2022

seando-adsk added the ufe-usd Related to UFE-USD plugin in Maya-Usd label Nov 10, 2022

seando-adsk merged commit 2bcac83 into dev Nov 10, 2022

seando-adsk deleted the gamaj/MAYA-125378/fix_relationships_and_connections_on_batched_duplication branch November 10, 2022 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-125378 Implementation batched duplication #2687

MAYA-125378 Implementation batched duplication #2687

JGamache-autodesk commented Oct 26, 2022

seando-adsk Oct 31, 2022

seando-adsk Oct 31, 2022

seando-adsk Oct 31, 2022

seando-adsk Nov 1, 2022

ppt-adsk left a comment

ppt-adsk Nov 4, 2022

ppt-adsk Nov 4, 2022

ppt-adsk Nov 4, 2022

ppt-adsk Nov 4, 2022

ppt-adsk Nov 4, 2022

JGamache-autodesk Nov 4, 2022

JGamache-autodesk Nov 4, 2022

ppt-adsk Nov 7, 2022

seando-adsk Nov 7, 2022

JGamache-autodesk Nov 7, 2022 •

edited

Loading

seando-adsk Nov 9, 2022

ppt-adsk Nov 7, 2022

seando-adsk Nov 7, 2022

ppt-adsk Nov 7, 2022

JGamache-autodesk Nov 7, 2022

JGamache-autodesk Nov 7, 2022

seando-adsk Nov 9, 2022

seando-adsk Nov 9, 2022

seando-adsk Nov 9, 2022

seando-adsk left a comment

seando-adsk left a comment

		typedef std::map<SdfPath, SdfPath> TDuplicatePathsMap;
		typedef std::unordered_map<Ufe::Path, TDuplicatePathsMap> TDuplicatesMap;

		# Gather all preview features that might be there or not into a single list:
		list(APPEND UFE_PREVIEW_FEATURES v4)


		#include <mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h>

		PXR_NAMESPACE_USING_DIRECTIVE

MAYA-125378 Implementation batched duplication #2687

MAYA-125378 Implementation batched duplication #2687

Conversation

JGamache-autodesk commented Oct 26, 2022

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGamache-autodesk Nov 7, 2022 • edited Loading

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

seando-adsk left a comment

Choose a reason for hiding this comment

seando-adsk left a comment

Choose a reason for hiding this comment

JGamache-autodesk Nov 7, 2022 •

edited

Loading