Modifications from code review

Autodesk · May 6, 2024 · 43ffab6 · 43ffab6
1 parent ccb0231
commit 43ffab6
Show file tree

Hide file tree

Showing 13 changed files with 64 additions and 122 deletions.
diff --git a/lib/flowViewport/sceneIndex/fvpSelectionSceneIndex.cpp b/lib/flowViewport/sceneIndex/fvpSelectionSceneIndex.cpp
@@ -39,32 +39,20 @@
 #include "flowViewport/sceneIndex/fvpSelectionSceneIndex.h"
 #include "flowViewport/sceneIndex/fvpPathInterface.h"
 #include "flowViewport/selection/fvpSelection.h"
-#include "flowViewport/fvpUtils.h"
 
 #include "flowViewport/debugCodes.h"
 
-#include <pxr/imaging/hd/retainedDataSource.h>
-#include <pxr/imaging/hd/selectionsSchema.h>
-#include <pxr/imaging/hd/primvarsSchema.h>
-#include <pxr/imaging/hd/tokens.h>
+#include "pxr/imaging/hd/retainedDataSource.h"
+#include "pxr/imaging/hd/selectionSchema.h"
+#include "pxr/imaging/hd/selectionsSchema.h"
 
 #include <ufe/pathString.h>
 #include <ufe/selection.h>
 
 PXR_NAMESPACE_USING_DIRECTIVE
 
 namespace {
-    //Handle primsvars:overrideWireframeColor in Storm for wireframe selection highlighting color
-    TF_DEFINE_PRIVATE_TOKENS(
-         _primVarsTokens,
-
-         (overrideWireframeColor)    // Works in HdStorm to override the wireframe color
-     );
-
-    const HdDataSourceLocatorSet selectionsAndPrimvarsColorsLocatorSet{HdSelectionsSchema::GetDefaultLocator(), 
-                                                                HdPrimvarsSchema::GetDefaultLocator().Append(_primVarsTokens->overrideWireframeColor),//Also dirty override wireframe color
-                                                                HdPrimvarsSchema::GetDefaultLocator().Append(HdTokens->displayColor)//and display color
-                                                                };
+const HdDataSourceLocatorSet selectionsSchemaDefaultLocator{HdSelectionsSchema::GetDefaultLocator()};
 }
 
 namespace FVP_NS_DEF {
@@ -173,9 +161,8 @@ SelectionSceneIndex::AddSelection(const Ufe::Path& appPath)
     TF_DEBUG(FVP_SELECTION_SCENE_INDEX)
         .Msg("    Adding %s to the Hydra selection.\n", sceneIndexPath.GetText());
 
-    HdSceneIndexObserver::DirtiedPrimEntries dirtiedPrims;
     if (_selection->Add(sceneIndexPath)) {
-        _SendPrimsDirtied({{sceneIndexPath, selectionsAndPrimvarsColorsLocatorSet}});
+        _SendPrimsDirtied({{sceneIndexPath, selectionsSchemaDefaultLocator}});
     }
 }
 
@@ -188,9 +175,8 @@ void SelectionSceneIndex::RemoveSelection(const Ufe::Path& appPath)
     // index path.
     auto sceneIndexPath = SceneIndexPath(appPath);
 
-    HdSceneIndexObserver::DirtiedPrimEntries dirtiedPrims;
     if (_selection->Remove(sceneIndexPath)) {
-       _SendPrimsDirtied({{sceneIndexPath, selectionsAndPrimvarsColorsLocatorSet}});
+        _SendPrimsDirtied({{sceneIndexPath, selectionsSchemaDefaultLocator}});
     }
 }
 
@@ -208,7 +194,7 @@ SelectionSceneIndex::ClearSelection()
     auto paths = _selection->GetFullySelectedPaths();
     entries.reserve(paths.size());
     for (const auto& path : paths) {
-        entries.emplace_back(path, selectionsAndPrimvarsColorsLocatorSet);
+        entries.emplace_back(path, selectionsSchemaDefaultLocator);
     }
 
     _selection->Clear();
@@ -228,7 +214,7 @@ void SelectionSceneIndex::ReplaceSelection(const Ufe::Selection& selection)
     auto paths = _selection->GetFullySelectedPaths();
     entries.reserve(paths.size() + selection.size());
     for (const auto& path : paths) {
-        entries.emplace_back(path, selectionsAndPrimvarsColorsLocatorSet);
+        entries.emplace_back(path, selectionsSchemaDefaultLocator);
     }
 
     _selection->Clear();
@@ -247,7 +233,7 @@ void SelectionSceneIndex::ReplaceSelection(const Ufe::Selection& selection)
         sceneIndexSn.emplace_back(sceneIndexPath);
         TF_DEBUG(FVP_SELECTION_SCENE_INDEX)
             .Msg("    Adding %s to the Hydra selection.\n", sceneIndexPath.GetText());
-        entries.emplace_back(sceneIndexPath, selectionsAndPrimvarsColorsLocatorSet);
+        entries.emplace_back(sceneIndexPath, selectionsSchemaDefaultLocator);
     }
 
     _selection->Replace(sceneIndexSn);

diff --git a/lib/flowViewport/selection/fvpSelection.cpp b/lib/flowViewport/selection/fvpSelection.cpp
@@ -38,8 +38,8 @@
 
 #include "flowViewport/selection/fvpSelection.h"
 
-#include <pxr/imaging/hd/selectionSchema.h>
-#include <pxr/imaging/hd/selectionsSchema.h>
+#include "pxr/imaging/hd/selectionSchema.h"
+#include "pxr/imaging/hd/selectionsSchema.h"
 
 PXR_NAMESPACE_USING_DIRECTIVE
 
@@ -74,23 +74,19 @@ Selection::_PrimSelectionState::GetVectorDataSource() const
     );
 };
 
-Selection::Selection()
-{
-}
-
 bool
-Selection::Add(const SdfPath& primPath)
+Selection::Add(const PXR_NS::SdfPath& primPath)
 {
     if (primPath.IsEmpty()) {
         return false;
     }
 
     _pathToState[primPath].selectionSources.push_back(selectionBuilder.Build());
-    
+
     return true;
 }
 
-bool Selection::Remove(const SdfPath& primPath)
+bool Selection::Remove(const PXR_NS::SdfPath& primPath)
 {
     return (!primPath.IsEmpty() && (_pathToState.erase(primPath) == 1));
 }
@@ -101,7 +97,7 @@ Selection::Clear()
     _pathToState.clear();
 }
 
-void Selection::Replace(const SdfPathVector& selection)
+void Selection::Replace(const PXR_NS::SdfPathVector& selection)
 {
     Clear();
 
@@ -114,7 +110,7 @@ void Selection::Replace(const SdfPathVector& selection)
     }
 }
 
-void Selection::RemoveHierarchy(const SdfPath& primPath)
+void Selection::RemoveHierarchy(const PXR_NS::SdfPath& primPath)
 {
     auto it = _pathToState.lower_bound(primPath);
     while (it != _pathToState.end() && it->first.HasPrefix(primPath)) {
@@ -156,12 +152,12 @@ SdfPathVector Selection::GetFullySelectedPaths() const
 }
 
 HdDataSourceBaseHandle Selection::GetVectorDataSource(
-    const SdfPath& primPath
+    const PXR_NS::SdfPath& primPath
 ) const
 {
     auto it = _pathToState.find(primPath);
     return (it != _pathToState.end()) ? 
         it->second.GetVectorDataSource() : nullptr;
 }
 
-} // end of namespace FVP_NS_DEF
+}
diff --git a/lib/flowViewport/selection/fvpSelection.h b/lib/flowViewport/selection/fvpSelection.h
@@ -21,7 +21,6 @@
 
 #include <pxr/imaging/hd/retainedDataSource.h>
 #include <pxr/usd/sdf/path.h>
-#include <pxr/base/gf/vec4f.h>
 
 #include <map>
 
@@ -43,9 +42,6 @@ class Selection
 {
 public:
 
-    FVP_API
-    Selection();
-
     // Add primPath to selection and return true if the argument is not empty.
     FVP_API
     bool Add(const PXR_NS::SdfPath& primPath);

diff --git a/lib/mayaHydra/hydraExtensions/mhLeadObjectPathTracker.cpp b/lib/mayaHydra/hydraExtensions/mhLeadObjectPathTracker.cpp
@@ -24,9 +24,9 @@
 
 //ufe
 #include <ufe/globalSelection.h>
-#include <ufe/selection.h>
 #include <ufe/observableSelection.h>
 #include <ufe/selectionNotification.h>
+#include <ufe/sceneItem.h>
 
 PXR_NAMESPACE_USING_DIRECTIVE
 
@@ -40,62 +40,29 @@ class GlobalSelectionChangedObs : public Ufe::Observer
     {
         public:
             GlobalSelectionChangedObs(MayaHydra::MhLeadObjectPathTracker& leadObjectPathInstance)
-            : _leadObjectPathInstance(leadObjectPathInstance)
+            : _leadObjectPathTracker(leadObjectPathInstance)
             {
             }
 
         private:
             void operator()(const Ufe::Notification& notification) override
             {
-                const Ufe::SelectionChanged& selectionChanged = notification.staticCast<Ufe::SelectionChanged>();
-
-                auto leadObjectUfePath = _leadObjectPathInstance.getLeadObjectUfePath();
-
-                switch (selectionChanged.opType()) {
-                    case Ufe::SelectionChanged::OpType::Append: {
-                        const Ufe::SelectionItemAppended& appended = notification.staticCast<Ufe::SelectionItemAppended>();
-                        auto newLeadObjectUfePath = appended.item()->path();
-                        _leadObjectPathInstance.setNewLeadObjectSceneItem(newLeadObjectUfePath);
-                        break;
-                    }
-                    case Ufe::SelectionChanged::OpType::Remove: {
-                        const Ufe::SelectionItemRemoved& removed = notification.staticCast<Ufe::SelectionItemRemoved>();
-                        auto removedItemUfePath = removed.item()->path();
-                        //Check if the lead object has been removed
-                        if (leadObjectUfePath == removedItemUfePath) {
-                            UpdateLeadObjectSceneItem(leadObjectUfePath);
-                        }
-                        break;
-                    }
-                    case Ufe::SelectionChanged::OpType::Clear: {
-                        _leadObjectPathInstance.ClearLeadObject();
-                        break;
-                    }
-
-                    case Ufe::SelectionChanged::OpType::Insert://Fall into
-                    case Ufe::SelectionChanged::OpType::SelectionCompositeNotification://Fall into
-                    case Ufe::SelectionChanged::OpType::ReplaceWith: {
-                        UpdateLeadObjectSceneItem(leadObjectUfePath);
-                        break;
-                    }
-                    default: break;
-                }
-            }
-
-            void UpdateLeadObjectSceneItem(const Ufe::Path& currentLeadObjectUfePath) { 
+                //selection has changed.
+
                 //Update the lead object if it has changed
                 auto globalSelection = Ufe::GlobalSelection::get();
                 if (globalSelection->size() > 0) {
                     auto newLeadObjectUfePath = globalSelection->back()->path();
+                    auto currentLeadObjectUfePath = _leadObjectPathTracker.getLeadObjectUfePath();
                     if (newLeadObjectUfePath != currentLeadObjectUfePath){
-                        _leadObjectPathInstance.setNewLeadObjectSceneItem(newLeadObjectUfePath);
+                        _leadObjectPathTracker.setLeadObjectUfePath(newLeadObjectUfePath);
                     }
                 }else {
-                    _leadObjectPathInstance.ClearLeadObject();
+                    _leadObjectPathTracker.setLeadObjectUfePath({});
                 }
             }
 
-            MayaHydra::MhLeadObjectPathTracker& _leadObjectPathInstance;
+            MayaHydra::MhLeadObjectPathTracker& _leadObjectPathTracker;
     };
 
 }
@@ -104,6 +71,7 @@ namespace MAYAHYDRA_NS_DEF {
 MhLeadObjectPathTracker::MhLeadObjectPathTracker(const HdSceneIndexBaseRefPtr& sceneIndexWithPathInterface, 
                                             MhDirtyLeadObjectSceneIndexRefPtr& dirtyLeadObjectSceneIndex) 
     : _pathInterface(dynamic_cast<const Fvp::PathInterface*>(&*sceneIndexWithPathInterface))
+    , _ufeSelectionObserver (std::make_shared<GlobalSelectionChangedObs>(*this))
     , _dirtyLeadObjectSceneIndex(dirtyLeadObjectSceneIndex)
 {
     TF_AXIOM(_pathInterface);
@@ -114,20 +82,19 @@ MhLeadObjectPathTracker::MhLeadObjectPathTracker(const HdSceneIndexBaseRefPtr& s
         auto leadObjectSceneItem = selection.back();//get last selected
         _leadObjectUfePath = leadObjectSceneItem->path();
         //_leadObjectPrimPath can be empty with a valid _leadObjectUfePath when the lead object is in a data producer scene index not yet added to the merging scene index
-        //This is fixed at some point by calling updateAfterDataProducerSceneIndicesLoaded()
+        //This is fixed at some point by calling updatePrimPath()
         _leadObjectPrimPath = _pathInterface->SceneIndexPath(_leadObjectUfePath); 
     }
 
    // Add ourself as an observer to the selection
-   _ufeSelectionObserver = std::make_shared<GlobalSelectionChangedObs>(*this);
+
    ufeSelection->addObserver(_ufeSelectionObserver);
 }
 
 MhLeadObjectPathTracker::~MhLeadObjectPathTracker()
 {
     Ufe::GlobalSelection::get()->removeObserver(_ufeSelectionObserver);
     _ufeSelectionObserver = nullptr;
-    _dirtyLeadObjectSceneIndex = nullptr;
 }
 
 bool MhLeadObjectPathTracker::isLeadObject(const PXR_NS::SdfPath& primPath) const
@@ -137,8 +104,9 @@ bool MhLeadObjectPathTracker::isLeadObject(const PXR_NS::SdfPath& primPath) cons
     return primPath.HasPrefix(_leadObjectPrimPath);
 }
 
-void MhLeadObjectPathTracker::setNewLeadObjectSceneItem(const Ufe::Path& newLeadObjectUfePath)
+void MhLeadObjectPathTracker::setLeadObjectUfePath(const Ufe::Path& newLeadObjectUfePath)
 {
+    //newLeadObjectUfePath be an empty Ufe::Path
     if (_leadObjectUfePath == newLeadObjectUfePath) {
        return;
     }
@@ -154,13 +122,7 @@ void MhLeadObjectPathTracker::setNewLeadObjectSceneItem(const Ufe::Path& newLead
     }
 }
 
-void MhLeadObjectPathTracker::ClearLeadObject()
-{
-    _leadObjectPrimPath   = SdfPath::EmptyPath();
-    _leadObjectUfePath    = Ufe::Path();
-}
-
-void MhLeadObjectPathTracker::updateAfterDataProducerSceneIndicesLoaded() 
+void MhLeadObjectPathTracker::updatePrimPath() 
 { 
    // Update the lead object prim path in case it was not valid yet
     if ( (_leadObjectUfePath.size() > 0) && _leadObjectPrimPath.IsEmpty()) {

diff --git a/lib/mayaHydra/hydraExtensions/mhLeadObjectPathTracker.h b/lib/mayaHydra/hydraExtensions/mhLeadObjectPathTracker.h
@@ -26,7 +26,7 @@
 
 //ufe
 #include <ufe/observer.h>
-#include <ufe/sceneItem.h>
+#include <ufe/path.h>
 
 //Hydra headers
 #include <pxr/imaging/hd/sceneIndex.h>
@@ -53,21 +53,17 @@ class MhLeadObjectPathTracker
     Ufe::Path getLeadObjectUfePath() const {return _leadObjectUfePath;}
 
     MAYAHYDRALIB_API
-    void setNewLeadObjectSceneItem(const Ufe::Path& newLeadObjectUfePath);
+    void setLeadObjectUfePath(const Ufe::Path& newLeadObjectUfePath);
 
     MAYAHYDRALIB_API
-    void ClearLeadObject();
-
-    MAYAHYDRALIB_API
-    void updateAfterDataProducerSceneIndicesLoaded(); // This is called after the data producer
-                                                      // scene indices are loaded
+    void updatePrimPath(); // For example : this is called after the data producer scene indices are loaded
 
 private:
     const Fvp::PathInterface* const _pathInterface {nullptr};
     PXR_NS::SdfPath                 _leadObjectPrimPath;
     Ufe::Observer::Ptr              _ufeSelectionObserver {nullptr};
     Ufe::Path                       _leadObjectUfePath;
-    MhDirtyLeadObjectSceneIndexRefPtr _dirtyLeadObjectSceneIndex;
+    const MhDirtyLeadObjectSceneIndexRefPtr _dirtyLeadObjectSceneIndex;
 };
 
 }//end of namespace MAYAHYDRA_NS_DEF