HYDRA-598 : Fixes from the code review.

Autodesk · Nov 15, 2023 · 449040b · 449040b
1 parent 2128143
commit 449040b
Show file tree

Hide file tree

Showing 9 changed files with 63 additions and 62 deletions.
diff --git a/lib/flowViewport/API/fvpInformationInterface.h b/lib/flowViewport/API/fvpInformationInterface.h
@@ -60,6 +60,15 @@ namespace FVP_NS_DEF
 
             ///_rendererName is the Hydra viewport renderer name (example : "GL" for Storm or "Arnold" for the Arnold render delegate)
             const std::string _rendererName;
+
+            ///Comparison operator
+            bool operator ==(const ViewportInformation& other)const{
+                return  _viewportSceneIndex == other._viewportSceneIndex &&
+                        _cameraName == other._cameraName &&
+                        _viewportWidth == other._viewportWidth &&
+                        _viewportHeight == other._viewportHeight&&
+                        _rendererName == other._rendererName;
+            }
         };
 
         /**
@@ -80,9 +89,9 @@ namespace FVP_NS_DEF
         /**
         *  @brief      Get the Hydra viewports information. 
         *
-        *  @param[out] outHydraViewportInformationSet is a set of ViewportInformation to have information about each Hydra viewport in use in the current DCC.
+        *  @param[out] outHydraViewportInformationArray is a set of ViewportInformation to have information about each Hydra viewport in use in the current DCC.
         */
-        virtual void GetViewportsInformation(std::set<const ViewportInformation*>& outHydraViewportInformationSet)const  = 0;
+        virtual void GetViewportsInformation(std::set<const ViewportInformation*>& outHydraViewportInformationArray)const  = 0;
     };
 
     ///Set of InformationInterface::ViewportInformation

diff --git a/lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp b/lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp
@@ -85,14 +85,14 @@ void InformationInterfaceImp::SceneIndexRemoved(const InformationInterface::View
     }
 }
 
-void InformationInterfaceImp::GetViewportsInformation(std::set<const ViewportInformation*>& outHydraViewportInformationSet)const
+void InformationInterfaceImp::GetViewportsInformation(std::set<const ViewportInformation*>& outHydraViewportInformationArray)const
 {
-    outHydraViewportInformationSet.clear();
-    const ViewportInformationAndSceneIndicesPerViewportDataSet& viewportInformationAndSceneIndicesPerViewportDataSet = 
+    outHydraViewportInformationArray.clear();
+    const ViewportInformationAndSceneIndicesPerViewportDataSet& viewportInformationAndSceneIndicesPerViewportDataArray = 
         ViewportInformationAndSceneIndicesPerViewportDataManager::Get().GetViewportInformationAndSceneIndicesPerViewportDataSet();
-    for (const ViewportInformationAndSceneIndicesPerViewportData* viewportInformationAndSceneIndicesPerViewportData : viewportInformationAndSceneIndicesPerViewportDataSet){
-        const InformationInterface::ViewportInformation& viewportInfo = viewportInformationAndSceneIndicesPerViewportData->GetViewportInformation();
-        outHydraViewportInformationSet.insert(&viewportInfo);
+    for (const ViewportInformationAndSceneIndicesPerViewportData& viewportInformationAndSceneIndicesPerViewportData : viewportInformationAndSceneIndicesPerViewportDataArray){
+        const InformationInterface::ViewportInformation& viewportInfo = viewportInformationAndSceneIndicesPerViewportData.GetViewportInformation();
+        outHydraViewportInformationArray.insert(&viewportInfo);
     }
 }
 

diff --git a/.../API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.cpp b/.../API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.cpp
@@ -27,12 +27,5 @@ ViewportInformationAndSceneIndicesPerViewportData::ViewportInformationAndSceneIn
 {
 }
 
-ViewportInformationAndSceneIndicesPerViewportData::~ViewportInformationAndSceneIndicesPerViewportData()
-{
-    //The viewport info is own by this class
-    InformationInterfaceImp::Get().SceneIndexRemoved(_viewportInformation);
-    delete &_viewportInformation;
-}
-
 } //End of namespace FVP_NS_DEF {
 
diff --git a/...rt/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.h b/...rt/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.h
@@ -41,18 +41,22 @@ class ViewportInformationAndSceneIndicesPerViewportData
     };
 
     ViewportInformationAndSceneIndicesPerViewportData(const CreationParameters& creationParams);
-    virtual ~ViewportInformationAndSceneIndicesPerViewportData();
+    ~ViewportInformationAndSceneIndicesPerViewportData() = default;
 
     const InformationInterface::ViewportInformation& GetViewportInformation()const { return _viewportInformation;}
     const PXR_NS::HdSceneIndexBaseRefPtr& GetLastFilteringSceneIndexOfTheChain() const {return _lastFilteringSceneIndexOfTheChain;}
     const RenderIndexProxy& GetRenderIndexProxy() const {return _renderIndexProxy;}
     void SetInputSceneIndex(const PXR_NS::HdSceneIndexBaseRefPtr& inputSceneIndex) {_inputSceneIndex = inputSceneIndex;}
     const PXR_NS::HdSceneIndexBaseRefPtr&   GetInputSceneIndex() const {return _inputSceneIndex;}
 
+    //Needed by std::set
+    bool operator < (const ViewportInformationAndSceneIndicesPerViewportData& other)const{
+        return true; //don't care about ordering, is just for std::set.
+    }
 
 private:
-    ///Hydra viewport information, it is owned by this class
-    const InformationInterface::ViewportInformation&                        _viewportInformation;
+    ///Hydra viewport information
+    const InformationInterface::ViewportInformation                         _viewportInformation;
 
     ///Is the scene index we should use as an input for the custom filtering scene indices chain
     PXR_NS::HdSceneIndexBaseRefPtr                                          _inputSceneIndex {nullptr};
@@ -63,7 +67,7 @@ class ViewportInformationAndSceneIndicesPerViewportData
     Fvp::RenderIndexProxy&                                                  _renderIndexProxy;
 };
 
-using ViewportInformationAndSceneIndicesPerViewportDataSet = std::set<ViewportInformationAndSceneIndicesPerViewportData*>;
+using ViewportInformationAndSceneIndicesPerViewportDataSet = std::set<ViewportInformationAndSceneIndicesPerViewportData>;
 
 } //End of namespace FVP_NS_DEF
 

diff --git a/...rViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp b/...rViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp
@@ -39,30 +39,28 @@ ViewportInformationAndSceneIndicesPerViewportDataManager& ViewportInformationAnd
     return theViewportInformationAndSceneIndicesPerViewportDataManager;
 }
 
-//A new Hydra viewport was created, this method takes the ownership of viewportInfo
-void ViewportInformationAndSceneIndicesPerViewportDataManager::AddViewportInformation(const InformationInterface::ViewportInformation* viewportInfo, Fvp::RenderIndexProxy& renderIndexProxy)
+//A new Hydra viewport was created
+void ViewportInformationAndSceneIndicesPerViewportDataManager::AddViewportInformation(const InformationInterface::ViewportInformation& viewportInfo, Fvp::RenderIndexProxy& renderIndexProxy)
 {
-    TF_AXIOM(nullptr != viewportInfo);
-
     //Add it in our array if it is not already inside
     {
         std::lock_guard<std::mutex> lock(_viewportInformationAndSceneIndicesPerViewportDataSet_mutex);
 
-        auto findResult = std::find_if(_viewportInformationAndSceneIndicesPerViewportDataSet.begin(), _viewportInformationAndSceneIndicesPerViewportDataSet.end(),
-                    [&viewportInfo](const ViewportInformationAndSceneIndicesPerViewportData* other) { 
-                        return &(other->GetViewportInformation()) == viewportInfo;
+        auto findResult = std::find_if(_viewportInformationAndSceneIndicesPerViewportDataSet.cbegin(), _viewportInformationAndSceneIndicesPerViewportDataSet.cend(),
+                    [&viewportInfo](const ViewportInformationAndSceneIndicesPerViewportData& other) { 
+                        return other.GetViewportInformation() == viewportInfo;
                     }
         );
-        if (findResult == _viewportInformationAndSceneIndicesPerViewportDataSet.end()){
-            const ViewportInformationAndSceneIndicesPerViewportData::CreationParameters creationParams(*viewportInfo, renderIndexProxy);
-            _viewportInformationAndSceneIndicesPerViewportDataSet.insert(new ViewportInformationAndSceneIndicesPerViewportData(creationParams));
+        if (findResult == _viewportInformationAndSceneIndicesPerViewportDataSet.cend()){
+            const ViewportInformationAndSceneIndicesPerViewportData::CreationParameters creationParams(viewportInfo, renderIndexProxy);
+            _viewportInformationAndSceneIndicesPerViewportDataSet.insert(ViewportInformationAndSceneIndicesPerViewportData(creationParams));
         }else{
             return; //It is already inside our array
         }
     }
 
     //Let the registered clients know a new viewport has been added
-    InformationInterfaceImp::Get().SceneIndexAdded(*viewportInfo);
+    InformationInterfaceImp::Get().SceneIndexAdded(viewportInfo);
 }
 
 void ViewportInformationAndSceneIndicesPerViewportDataManager::RemoveViewportInformation(const HdSceneIndexBaseRefPtr& viewportSceneIndex)
@@ -72,22 +70,21 @@ void ViewportInformationAndSceneIndicesPerViewportDataManager::RemoveViewportInf
         std::lock_guard<std::mutex> lock(_viewportInformationAndSceneIndicesPerViewportDataSet_mutex);
 
         auto findResult = std::find_if(_viewportInformationAndSceneIndicesPerViewportDataSet.begin(), _viewportInformationAndSceneIndicesPerViewportDataSet.end(),
-                    [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData* other) { return other->GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
+                    [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData& other) { return other.GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
         if (findResult != _viewportInformationAndSceneIndicesPerViewportDataSet.end()){
 
-            const RenderIndexProxy* renderIndexProxy = & (*findResult)->GetRenderIndexProxy();//Get the pointer on the renderIndexProxy
+            InformationInterfaceImp::Get().SceneIndexRemoved(findResult->GetViewportInformation());
+
+            const RenderIndexProxy& renderIndexProxy = findResult->GetRenderIndexProxy();//Get the pointer on the renderIndexProxy
 
             //Destroy the custom filtering scene indices chain
-            if (renderIndexProxy){
-                //Following code is equivalent to calling FilteringSceneIndicesChainManager::GetManager().DestroyFilteringSceneIndicesChain(*renderIndexProxy);
-                //But we cannot do so because of the lock above.
-                auto renderIndex = renderIndexProxy->GetRenderIndex();
-                if (renderIndex && (*findResult)->GetLastFilteringSceneIndexOfTheChain()){
-                    renderIndex->RemoveSceneIndex((*findResult)->GetLastFilteringSceneIndexOfTheChain());//Remove the whole chain from the render index
-                }
+            //Following code is equivalent to calling FilteringSceneIndicesChainManager::GetManager().DestroyFilteringSceneIndicesChain(*renderIndexProxy);
+            //But we cannot do so because of the lock above.
+            auto renderIndex = renderIndexProxy.GetRenderIndex();
+            if (renderIndex && findResult->GetLastFilteringSceneIndexOfTheChain()){
+                renderIndex->RemoveSceneIndex(findResult->GetLastFilteringSceneIndexOfTheChain());//Remove the whole chain from the render index
             }
 
-            delete (*findResult); //delete the ViewportInformationAndSceneIndicesPerViewportData*
             _viewportInformationAndSceneIndicesPerViewportDataSet.erase(findResult);
         }
     }
@@ -99,9 +96,9 @@ ViewportInformationAndSceneIndicesPerViewportDataManager::GetViewportInformation
     std::lock_guard<std::mutex> lock(_viewportInformationAndSceneIndicesPerViewportDataSet_mutex);
 
     auto findResult = std::find_if(_viewportInformationAndSceneIndicesPerViewportDataSet.cbegin(), _viewportInformationAndSceneIndicesPerViewportDataSet.cend(),
-                [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData* other) { return other->GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
+                [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData& other) { return other.GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
     if (findResult != _viewportInformationAndSceneIndicesPerViewportDataSet.cend()){
-        return &((*findResult)->GetViewportInformation());
+        return &(findResult->GetViewportInformation());
     }
 
     return nullptr;
@@ -113,9 +110,9 @@ ViewportInformationAndSceneIndicesPerViewportDataManager::GetViewportInformation
     std::lock_guard<std::mutex> lock(_viewportInformationAndSceneIndicesPerViewportDataSet_mutex);
 
     auto findResult = std::find_if(_viewportInformationAndSceneIndicesPerViewportDataSet.cbegin(), _viewportInformationAndSceneIndicesPerViewportDataSet.cend(),
-                [&renderIndexProxy](const ViewportInformationAndSceneIndicesPerViewportData* other) { return &(other->GetRenderIndexProxy()) == &renderIndexProxy;});
+                [&renderIndexProxy](const ViewportInformationAndSceneIndicesPerViewportData& other) { return &(other.GetRenderIndexProxy()) == &renderIndexProxy;});
     if (findResult != _viewportInformationAndSceneIndicesPerViewportDataSet.cend()){
-        return (*findResult);
+        return &(*findResult);
     }
 
     return nullptr;
@@ -127,9 +124,9 @@ ViewportInformationAndSceneIndicesPerViewportDataManager::GetRenderIndexProxyFro
     std::lock_guard<std::mutex> lock(_viewportInformationAndSceneIndicesPerViewportDataSet_mutex);
 
     auto findResult = std::find_if(_viewportInformationAndSceneIndicesPerViewportDataSet.cbegin(), _viewportInformationAndSceneIndicesPerViewportDataSet.cend(),
-                [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData* other) { return other->GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
+                [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData& other) { return other.GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
     if (findResult != _viewportInformationAndSceneIndicesPerViewportDataSet.cend()){
-        return &((*findResult)->GetRenderIndexProxy());
+        return &(findResult->GetRenderIndexProxy());
     }
 
     return nullptr;
@@ -141,9 +138,9 @@ const ViewportInformationAndSceneIndicesPerViewportData* ViewportInformationAndS
     std::lock_guard<std::mutex> lock(_viewportInformationAndSceneIndicesPerViewportDataSet_mutex);
 
     auto findResult = std::find_if(_viewportInformationAndSceneIndicesPerViewportDataSet.cbegin(), _viewportInformationAndSceneIndicesPerViewportDataSet.cend(),
-                [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData* other) { return other->GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
+                [&viewportSceneIndex](const ViewportInformationAndSceneIndicesPerViewportData& other) { return other.GetViewportInformation()._viewportSceneIndex == viewportSceneIndex;});
     if (findResult != _viewportInformationAndSceneIndicesPerViewportDataSet.cend()){
-        return (*findResult);
+        return &(*findResult);
     }
 
     return nullptr;

diff --git a/...perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.h b/...perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.h
@@ -38,8 +38,8 @@ class FVP_API ViewportInformationAndSceneIndicesPerViewportDataManager
     /// Manager accessor
     static ViewportInformationAndSceneIndicesPerViewportDataManager& Get();
 
-    ///A new Hydra viewport was created, this method takes the ownership of _viewportInfo which has been created on the heap and will be deleted by Flow Viewport
-    void AddViewportInformation(const InformationInterface::ViewportInformation* _viewportInfo, RenderIndexProxy& renderIndexProxy);
+    ///A new Hydra viewport was created
+    void AddViewportInformation(const InformationInterface::ViewportInformation& _viewportInfo, RenderIndexProxy& renderIndexProxy);
 
     ///An Hydra viewport was deleted
     void RemoveViewportInformation(const PXR_NS::HdSceneIndexBaseRefPtr& viewportSceneIndex);

diff --git a/lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.cpp b/lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.cpp
@@ -54,11 +54,9 @@ MayaHydraSceneProducer::MayaHydraSceneProducer(
         _sceneIndex = MayaHydraSceneIndex::New(initData, lightEnabled);
         TF_VERIFY(_sceneIndex, "Maya Hydra scene index not found, check mayaHydra plugin installation.");
 
-        //Create an HydraViewportInformation instance
-        FVP_NS::InformationInterface::ViewportInformation* hydraViewportInformation = 
-            new FVP_NS::InformationInterface::ViewportInformation(_sceneIndex, initData.cameraName, initData.viewportWidth, initData.viewportHeight, initData.rendererName);
-        //This method takes the ownership of the hydraViewportInformation passed to it and will delete it
-        FVP_NS::ViewportInformationAndSceneIndicesPerViewportDataManager::Get().AddViewportInformation(hydraViewportInformation, _renderIndexProxy);
+        //Create an HydraViewportInformation 
+        const Fvp::InformationInterface::ViewportInformation hydraViewportInformation(_sceneIndex, initData.cameraName, initData.viewportWidth, initData.viewportHeight, initData.rendererName);
+        Fvp::ViewportInformationAndSceneIndicesPerViewportDataManager::Get().AddViewportInformation(hydraViewportInformation, _renderIndexProxy);
     }
     else
     {
@@ -103,7 +101,7 @@ MayaHydraSceneProducer::~MayaHydraSceneProducer()
 {
     if (enableMayaNativeSceneIndex())
     {
-        FVP_NS::ViewportInformationAndSceneIndicesPerViewportDataManager::Get().RemoveViewportInformation(_sceneIndex);
+        Fvp::ViewportInformationAndSceneIndicesPerViewportDataManager::Get().RemoveViewportInformation(_sceneIndex);
         _renderIndexProxy.RemoveSceneIndex(_sceneIndex);
         _sceneIndex->RemoveCallbacksAndDeleteAdapters();//This should be called before calling _sceneIndex.Reset(); which will call the destructor if the ref count reaches 0
         _sceneIndex.Reset();

diff --git a/lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.h b/lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.h
@@ -131,7 +131,7 @@ class MAYAHYDRALIB_API MayaHydraSceneProducer
     SdfPath GetPrimPath(const MDagPath& dg, bool isSprim);
 
     HdRenderIndex& GetRenderIndex();
-    FVP_NS::RenderIndexProxy&  GetRenderIndexProxy(){ return _renderIndexProxy;}
+    Fvp::RenderIndexProxy&  GetRenderIndexProxy(){ return _renderIndexProxy;}
 
     SdfPath GetLightedPrimsRootPath() const;
 

diff --git a/test/lib/mayaUsd/render/mayaToHydra/cpp/testFlowViewportAPIViewportInformation.cpp b/test/lib/mayaUsd/render/mayaToHydra/cpp/testFlowViewportAPIViewportInformation.cpp
@@ -30,20 +30,20 @@
 
 PXR_NAMESPACE_USING_DIRECTIVE
 
-//Subclass FVP_NS::InformationClient to register this class to receive callbacks.
-class InfoClientTest : public FVP_NS::InformationClient
+//Subclass Fvp::InformationClient to register this class to receive callbacks.
+class InfoClientTest : public Fvp::InformationClient
 {
 public:
     InfoClientTest() = default;
     ~InfoClientTest() override = default;
 
-    //From FVP_NS::InformationClient
-    void SceneIndexAdded(const FVP_NS::InformationInterface::ViewportInformation& viewportInformation)override
+    //From Fvp::InformationClient
+    void SceneIndexAdded(const Fvp::InformationInterface::ViewportInformation& viewportInformation)override
     {
         ++_numSceneIndexAdded;//We want to count the number of times this is called
     }
 
-    void SceneIndexRemoved(const FVP_NS::InformationInterface::ViewportInformation& viewportInformation)override
+    void SceneIndexRemoved(const Fvp::InformationInterface::ViewportInformation& viewportInformation)override
     {
         ++_numSceneIndexRemoved;//We want to count the number of times this is called
     }