From 449040be70e7e081357e3dc742c17ea742c6981c Mon Sep 17 00:00:00 2001 From: David Lanier Date: Wed, 15 Nov 2023 19:19:03 +0100 Subject: [PATCH] HYDRA-598 : Fixes from the code review. --- .../API/fvpInformationInterface.h | 13 ++++- .../fvpInformationInterfaceImp.cpp | 12 ++-- ...ormationAndSceneIndicesPerViewportData.cpp | 7 --- ...nformationAndSceneIndicesPerViewportData.h | 12 ++-- ...nAndSceneIndicesPerViewportDataManager.cpp | 55 +++++++++---------- ...ionAndSceneIndicesPerViewportDataManager.h | 4 +- .../mayaHydraSceneProducer.cpp | 10 ++-- .../hydraExtensions/mayaHydraSceneProducer.h | 2 +- ...testFlowViewportAPIViewportInformation.cpp | 10 ++-- 9 files changed, 63 insertions(+), 62 deletions(-) diff --git a/lib/flowViewport/API/fvpInformationInterface.h b/lib/flowViewport/API/fvpInformationInterface.h index d70156638c..98e15258ee 100644 --- 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& outHydraViewportInformationSet)const = 0; + virtual void GetViewportsInformation(std::set& outHydraViewportInformationArray)const = 0; }; ///Set of InformationInterface::ViewportInformation diff --git a/lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp b/lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp index f91f8bae47..5b3b7480fb 100644 --- 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& outHydraViewportInformationSet)const +void InformationInterfaceImp::GetViewportsInformation(std::set& 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/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.cpp b/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.cpp index 15771a06f5..f77b8b82a1 100644 --- a/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.cpp +++ b/lib/flowViewport/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/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.h b/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.h index 93f5b87cd0..1e45cc4bc4 100644 --- a/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.h +++ b/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.h @@ -41,7 +41,7 @@ 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;} @@ -49,10 +49,14 @@ class ViewportInformationAndSceneIndicesPerViewportData 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; +using ViewportInformationAndSceneIndicesPerViewportDataSet = std::set; } //End of namespace FVP_NS_DEF diff --git a/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp b/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp index 151197403d..4da7ef9fa3 100644 --- a/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp +++ b/lib/flowViewport/API/perViewportSceneIndicesData/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 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 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 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 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 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 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/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.h b/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.h index 2d09b21105..baf96f7dda 100644 --- a/lib/flowViewport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.h +++ b/lib/flowViewport/API/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 index 424eab2613..d2a19c748a 100644 --- 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 index e5713cb158..d5bbdb1bd4 100644 --- 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 index 7cad27c9b4..3b6f656e4b 100644 --- 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 }