Add ability to remove duplicates in SdfListOp::ModifyOperations

and update list editing proxies. This avoids coding errors that would occur if the modification callback produced duplicate values. The Sdf_ListEditor proxies are where the check for duplicate values currently happen, so these proxies all opt in to remove duplicates. SdfListOp itself does not do this by default to maintain current behavior, since it does not itself restrict duplicate values in lists. This change builds off of PR #1282 from @marktucker. Fixes #1282 (Internal change: 2252577)
PixarAnimationStudios · Oct 21, 2022 · a072198 · a072198
1 parent dc2ecb8
commit a072198
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 17 deletions.
diff --git a/pxr/usd/sdf/listEditor.h b/pxr/usd/sdf/listEditor.h
@@ -126,7 +126,9 @@ class Sdf_ListEditor
     /// Modifies the operations stored in all operation lists.
     /// \p callback is called for every key.  If the returned key is
     /// invalid then the key is removed, otherwise it's replaced with the
-    /// returned key.
+    /// returned key. If the returned key matches a key that was previously
+    /// returned for the list being processed, the returned key will be
+    /// removed.
     virtual void ModifyItemEdits(const ModifyCallback& cb) = 0;
 
     typedef std::function<

diff --git a/pxr/usd/sdf/listOp.cpp b/pxr/usd/sdf/listOp.cpp
@@ -28,6 +28,7 @@
 #include "pxr/usd/sdf/payload.h"
 #include "pxr/usd/sdf/reference.h"
 #include "pxr/usd/sdf/types.h"
+#include "pxr/base/tf/denseHashSet.h"
 #include "pxr/base/tf/diagnostic.h"
 #include "pxr/base/tf/iterator.h"
 #include "pxr/base/tf/registryManager.h"
@@ -594,21 +595,29 @@ template <typename T>
 static inline
 bool
 _ModifyCallbackHelper(const typename SdfListOp<T>::ModifyCallback& cb,
-                      std::vector<T>* itemVector)
+                      std::vector<T>* itemVector, bool removeDuplicates)
 {
     bool didModify = false;
 
     std::vector<T> modifiedVector;
-    TF_FOR_ALL(item, *itemVector) {
-        boost::optional<T> modifiedItem = cb(*item);
+    TfDenseHashSet<T, TfHash> existingSet;
+
+    for (const T& item : *itemVector) {
+        boost::optional<T> modifiedItem = cb(item);
+        if (removeDuplicates && modifiedItem) {
+            if (!existingSet.insert(*modifiedItem).second) {
+                modifiedItem = boost::none;
+            }
+        }
+
         if (!modifiedItem) {
             didModify = true;
         }
-        else if (*modifiedItem != *item) {
-            modifiedVector.push_back(*modifiedItem);
+        else if (*modifiedItem != item) {
+            modifiedVector.push_back(std::move(*modifiedItem));
             didModify = true;
         } else {
-            modifiedVector.push_back(*item);
+            modifiedVector.push_back(item);
         }
     }
 
@@ -621,17 +630,24 @@ _ModifyCallbackHelper(const typename SdfListOp<T>::ModifyCallback& cb,
 
 template <typename T>
 bool 
-SdfListOp<T>::ModifyOperations(const ModifyCallback& callback)
+SdfListOp<T>::ModifyOperations(const ModifyCallback& callback,
+                               bool removeDuplicates)
 {
     bool didModify = false;
 
     if (callback) {
-        didModify |= _ModifyCallbackHelper(callback, &_explicitItems);
-        didModify |= _ModifyCallbackHelper(callback, &_addedItems);
-        didModify |= _ModifyCallbackHelper(callback, &_prependedItems);
-        didModify |= _ModifyCallbackHelper(callback, &_appendedItems);
-        didModify |= _ModifyCallbackHelper(callback, &_deletedItems);
-        didModify |= _ModifyCallbackHelper(callback, &_orderedItems);
+        didModify |= _ModifyCallbackHelper(
+            callback, &_explicitItems, removeDuplicates);
+        didModify |= _ModifyCallbackHelper(
+            callback, &_addedItems, removeDuplicates);
+        didModify |= _ModifyCallbackHelper(
+            callback, &_prependedItems, removeDuplicates);
+        didModify |= _ModifyCallbackHelper(
+            callback, &_appendedItems, removeDuplicates);
+        didModify |= _ModifyCallbackHelper(
+            callback, &_deletedItems, removeDuplicates);
+        didModify |= _ModifyCallbackHelper(
+            callback, &_orderedItems, removeDuplicates);
     }
 
     return didModify;

diff --git a/pxr/usd/sdf/listOp.h b/pxr/usd/sdf/listOp.h
@@ -215,9 +215,15 @@ class SdfListOp {
     /// \p callback is called for every item in all operation vectors.  If the 
     /// returned key is empty then the key is removed, otherwise it's replaced 
     /// with the returned key.
+    /// 
+    /// If \p removeDuplicates is \c true and \p callback returns a key that was
+    /// previously returned for the current operation vector being processed,
+    /// the returned key will be removed.
     ///
     /// Returns true if a change was made, false otherwise.
-    SDF_API bool ModifyOperations(const ModifyCallback& callback);
+    SDF_API
+    bool ModifyOperations(const ModifyCallback& callback,
+                          bool removeDuplicates = false);
 
     /// Replaces the items in the specified operation vector in the range
     /// (index, index + n] with the given \p newItems. If \p newItems is empty

diff --git a/pxr/usd/sdf/listOpListEditor.h b/pxr/usd/sdf/listOpListEditor.h
@@ -255,7 +255,7 @@ Sdf_ListOpListEditor<TP>::ModifyItemEdits(const ModifyCallback& cb)
     modifiedListOp.ModifyOperations(
         [this, &cb](const value_type &t) {
             return _ModifyCallbackHelper(cb, _GetTypePolicy(), t);
-        });
+        }, /* removeDuplicates = */ true);
 
     _UpdateListOp(modifiedListOp);
 }

diff --git a/pxr/usd/sdf/vectorListEditor.h b/pxr/usd/sdf/vectorListEditor.h
@@ -273,7 +273,7 @@ Sdf_VectorListEditor<TP, FST>::ModifyItemEdits(const ModifyCallback& cb)
     valueListOp.ModifyOperations(
         [this, &cb](const value_type &t) {
             return _ModifyCallbackHelper(cb, _GetTypePolicy(), t);
-        });
+        }, /* removeDuplicates = */ true);
 
     _UpdateFieldData(valueListOp.GetItems(_op));
 }