HYDRA-598 : Merge viewport information interface #4

lanierd-adsk · 2023-11-09T16:10:20Z

No description provided.

lanierd-adsk · 2023-11-10T16:25:18Z

FYI : @lilike-adsk @roopavr-adsk @ppt-adsk

debloip-adsk

Hi, thanks for doing this! I know I've put a lot of comments, but a lot of them are nits, minor things or questions to help me understand things a little better.

lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp

lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.h

lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp

plugin/mayaHydraSceneBrowserTest/mayaHydraSceneBrowserTestCmd.cpp

plugin/mayaHydraSceneBrowser/mayaHydraSceneBrowser.cpp

lib/mayaHydra/mayaPlugin/renderOverride.cpp

lib/flowViewport/sceneIndex/fvpRenderIndexProxy.cpp

debloip-adsk · 2023-11-13T22:55:57Z

.../perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp

+                }
+            }
+
+            delete (*findResult); //delete the ViewportInformationAndSceneIndicesPerViewportData*


I'm worried about manually managing the ViewportInformationAndSceneIndicesPerViewportData : could we store the ViewportInformationAndSceneIndicesPerViewportData by value, or at least with smart pointers to reduce the risk of running into memory management issues?

I started to store them by value as you suggested but there is still the ViewportInformation pointer to manage (new/delete) which is inside this class so we would still have to manage some memory.
So is this something you still want ?

Also since this is data per viewport and is not shared by any other class I think it should be manageable.

I haven't managed to make it work with with unique_ptrs (there is a copy I can't find somewhere, but the error message only indicates lines in the standard library headers), but it does work with shared_ptrs. I'm not sure if the overhead would be a problem or not, but either ways, I think as long as we stick to the one new and the one delete we currently have, this should be fine as it's isolated in the implementation and very symmetrical.

(Grouping this conversation with this comment)

So after taking a closer look, the situation with ViewportInformation objects is quite similar, as like you said, those are also manually managed, with the exception that objects of that class are expected to be manually created outside of the Flow Viewport Toolkit, and their ownership then transferred to the Flow Viewport Toolkit. I feel like this should also be solvable with unique_ptrs.

That being said, if we can't get smart pointers to work, then I think we should at least explicitly document that the AddViewportInformation method will assume ownership of the ViewportInformation object it receives, so that users know not to have the object they pass in be deleted. And to make the method signature a bit more explicit, maybe we can also make it receive a raw pointer rather than a const reference?

AddViewportInformation is not called by users, but by maya hydra only. But you're right, I changed the method signature and added more comments about the ownership.

Explicit calls to delete are a maintenance burden and are not exception safe. They violate this C++ core guideline:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-newdelete
If we're still using an explicit call to delete (and it looks like we are), we should try to change the code to avoid it.

ok, I will try to find a way to avoid delete.

I am using a std::set with copies in it instead of arrays, the class to copy is not heavy and this set will have the size of the number of viewports in maya.

lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.cpp

lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp

lib/mayaHydra/mayaPlugin/renderOverride.cpp

ppt-adsk · 2023-11-15T14:58:31Z

.../perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp

+                }
+            }
+
+            delete (*findResult); //delete the ViewportInformationAndSceneIndicesPerViewportData*


Explicit calls to delete are a maintenance burden and are not exception safe. They violate this C++ core guideline:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-newdelete
If we're still using an explicit call to delete (and it looks like we are), we should try to change the code to avoid it.

lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.cpp

lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp

ppt-adsk · 2023-11-15T15:28:19Z

lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp

+{
+    std::lock_guard<std::mutex> lock(_viewportInformationClient_mutex);
+
+    InformationClient* clientNonConst = const_cast<InformationClient*>(&client);


This feels very awkward in terms of ownership. We shouldn't be storing a pointer to something that is conceptually passed by value, because the argument object may disappear if it goes out of scope. I think this should be changed.

lib/mayaHydra/hydraExtensions/sceneIndex/mayaHydraSceneIndex.cpp

ppt-adsk · 2023-11-15T15:34:31Z

lib/mayaHydra/hydraExtensions/mayaHydraLibInterface.h


 PXR_NAMESPACE_OPEN_SCOPE

-using SceneIndicesVector = std::vector<HdSceneIndexBasePtr>;
+using SceneIndicesVector                                        = std::vector<HdSceneIndexBasePtr>;//Be careful, these are not not Ref counted. Elements could become dangling


If it's a maintenance burden, then I would change to using HdSceneIndexBaseRefPtr.

...PI/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.h

ppt-adsk · 2023-11-15T15:56:13Z

...ort/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.cpp

+{
+    //The viewport info is own by this class
+    InformationInterfaceImp::Get().SceneIndexRemoved(_viewportInformation);
+    delete &_viewportInformation;


Again, an explicit delete violates
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-newdelete

lib/mayaHydra/mayaPlugin/renderOverride.cpp

lanierd-adsk · 2023-11-15T18:20:52Z

There should have only 1 remaining problem about the lifetime management which we are discussing offline.

lib/flowViewport/API/fvpInformationInterface.h

...wport/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.h

lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.cpp

…account the shared scene/render/index we use for all viewports

ppt-adsk

Thanks for the tests!

lib/flowViewport/API/fvpInformationInterface.h

...ort/API/perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportData.cpp

lib/flowViewport/sceneIndex/fvpRenderIndexProxyFwd.h

.../perViewportSceneIndicesData/fvpViewportInformationAndSceneIndicesPerViewportDataManager.cpp

HYDRA-598 : Merge viewport information interface

28f694d

lanierd-adsk requested a review from debloip-adsk November 9, 2023 16:10

lanierd-adsk self-assigned this Nov 9, 2023

lanierd-adsk marked this pull request as ready for review November 10, 2023 13:34

Merge branch 'dev' into lanierd/HYDRA-598

b2d5779

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 10, 2023

Fix build under Linux.

2748c35

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 13, 2023

debloip-adsk requested changes Nov 13, 2023

View reviewed changes

HYDRA-598 : Fixes from code review.

65b7b2c

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 14, 2023

lanierd-adsk requested a review from debloip-adsk November 14, 2023 14:05

HYDRA-598 : Attempt in fixing Linux crash in tests

390ff7a

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 14, 2023

debloip-adsk reviewed Nov 15, 2023

View reviewed changes

lib/mayaHydra/hydraExtensions/mayaHydraSceneProducer.cpp Outdated Show resolved Hide resolved

lib/flowViewport/API/interfacesImp/fvpInformationInterfaceImp.cpp Outdated Show resolved Hide resolved

lanierd-adsk and others added 2 commits November 15, 2023 11:05

HYDRA-598 : Changes from code review.

9b1827e

Merge branch 'dev' into lanierd/HYDRA-598

2ed7b37

lanierd-adsk requested a review from debloip-adsk November 15, 2023 10:45

lanierd-adsk commented Nov 15, 2023

View reviewed changes

lib/mayaHydra/mayaPlugin/renderOverride.cpp Outdated Show resolved Hide resolved

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 15, 2023

HYDRA-598 : Fix a possible crash by deleting the RenderIndexProxy last.

2128143

lanierd-adsk removed their assignment Nov 15, 2023

ppt-adsk requested changes Nov 15, 2023

View reviewed changes

HYDRA-598 : Fixes from the code review.

449040b

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 15, 2023

lanierd-adsk requested a review from ppt-adsk November 15, 2023 18:20

HYDRA-598 : Update a name from code review.

b376492

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 16, 2023

ppt-adsk reviewed Nov 16, 2023

View reviewed changes

HYDRA-598 : Add tests for multiviewports and update code taking into …

5e539d5

…account the shared scene/render/index we use for all viewports

lanierd-adsk requested a review from ppt-adsk November 17, 2023 13:13

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 17, 2023

ppt-adsk requested changes Nov 17, 2023

View reviewed changes

HYDRA-598 : Fixes from code review.

bb71253

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 20, 2023

HYDRA-598 : Fixes from code review.

ad0e1bb

lanierd-adsk requested a review from ppt-adsk November 20, 2023 14:41

lanierd-adsk assigned lanierd-adsk and unassigned lanierd-adsk Nov 20, 2023

ppt-adsk approved these changes Nov 20, 2023

View reviewed changes

debloip-adsk approved these changes Nov 20, 2023

View reviewed changes

lanierd-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 21, 2023

roopavr-adsk merged commit 9922683 into dev Nov 21, 2023
9 checks passed

roopavr-adsk deleted the lanierd/HYDRA-598 branch November 21, 2023 16:13

debloip-adsk mentioned this pull request Nov 24, 2023

HYDRA-703 : Fix mesh adapter transform update #20

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HYDRA-598 : Merge viewport information interface #4

HYDRA-598 : Merge viewport information interface #4

lanierd-adsk commented Nov 9, 2023

lanierd-adsk commented Nov 10, 2023

debloip-adsk left a comment

debloip-adsk Nov 13, 2023

lanierd-adsk Nov 14, 2023

lanierd-adsk Nov 14, 2023

debloip-adsk Nov 14, 2023 •

edited

Loading

debloip-adsk Nov 14, 2023 •

edited

Loading

lanierd-adsk Nov 15, 2023

ppt-adsk Nov 15, 2023

lanierd-adsk Nov 15, 2023

lanierd-adsk Nov 16, 2023

ppt-adsk Nov 15, 2023

ppt-adsk Nov 15, 2023

ppt-adsk Nov 15, 2023

ppt-adsk Nov 15, 2023

lanierd-adsk commented Nov 15, 2023

ppt-adsk left a comment

HYDRA-598 : Merge viewport information interface #4

HYDRA-598 : Merge viewport information interface #4

Conversation

lanierd-adsk commented Nov 9, 2023

lanierd-adsk commented Nov 10, 2023

debloip-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

debloip-adsk Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

debloip-adsk Nov 14, 2023 • 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

lanierd-adsk commented Nov 15, 2023

ppt-adsk left a comment

Choose a reason for hiding this comment

debloip-adsk Nov 14, 2023 •

edited

Loading

debloip-adsk Nov 14, 2023 •

edited

Loading