MAYA-122104 ProxyShapeSceneSegmentHandler and UsdCameraHandler::findCameras. #2307

williamkrick · 2022-04-22T19:36:31Z

No description provided.

lib/mayaUsd/ufe/ProxyShapeSceneSegmentHandler.cpp

williamkrick · 2022-04-22T19:38:16Z

lib/mayaUsd/ufe/UsdCameraHandler.cpp

@@ -52,5 +56,25 @@ Ufe::Camera::Ptr UsdCameraHandler::camera(const Ufe::SceneItem::Ptr& item) const
    return UsdCamera::create(usdItem);
 }

+#if defined(UFE_V4_FEATURES_AVAILABLE) && (UFE_PREVIEW_VERSION_NUM >= 4008)
+Ufe::Selection UsdCameraHandler::findCamerasInSceneSegment(const Ufe::Path& path) const


Native USD search for cameras.

vlasovi

Looks good. I'm only wondering how much faster is this approach compared to just traversing Ufe::Hierarchy. In the end we still have to traverse USD hierarchy in search of cameras.

williamkrick · 2022-04-25T15:01:03Z

Looks good. I'm only wondering how much faster is this approach compared to just traversing Ufe::Hierarchy. In the end we still have to traverse USD hierarchy in search of cameras.

You are right in that the complexity of the algorithm is the same. The speed difference comes from eliminating the overhead of calls to Ufe during the traversal. Using Ufe::Hierarchy to traverse means creating many additional Ufe::SceneItems, Ufe::Paths and Ufe::Hierarchy objects, which means allocating a bunch of std::vector<>, std::shared_ptr<> etc. Doing the iteration with USD means we don't do any allocation when the object is not a gateway item or a camera (which is most objects).

Avoiding the overhead of Ufe when doing the hierarchy traversal ends up saving a significant amount of time. This version is something like 10x faster then the original code.

ppt-adsk

Needs a test.

lib/mayaUsd/ufe/Global.cpp

lib/mayaUsd/ufe/ProxyShapeCameraHandler.cpp

lib/mayaUsd/ufe/ProxyShapeSceneSegmentHandler.cpp

lib/mayaUsd/ufe/ProxyShapeSceneSegmentHandler.h

lib/mayaUsd/ufe/UsdCameraHandler.cpp

williamkrick · 2022-05-02T16:04:40Z

These changes are already merged on top of @JGamache-autodesk work for #2315, so merge that one first.

The only thing I'm missing is a test for SceneSegmentHandler. I forgot the Ufe::RunTimeMgr Python API that I need to write the test, so it is going to come in another PR.

seando-adsk · 2022-05-03T14:20:14Z

I added the label "do not merge yet" as a reminder to myself to merge #2315 first.

ppt-adsk

Thanks for the tests. Would be great if you could adjust the test conditionals to follow the pattern we've done in the past, makes it easier to remove these obsolete conditionals at the next Maya preview release.

Was quite confusing to review your changes and Jerry's changes in the same pull request. Can't we just merge in Jerry's changes first, then you can update from dev and rebase your commits on top?

ppt-adsk · 2022-05-03T13:56:30Z

lib/mayaUsd/ufe/CMakeLists.txt

+        )
+    endif()
+
+    if (${UFE_PREVIEW_VERSION_NUM} GREATER_EQUAL 4010)


Ouch. Can't we just settle on GREATER_EQUAL 4010?

We're in kind of a strange state because we've had a bunch of UFE preview versions go by without getting them into Maya. Everyone is building locally slightly differently. But no QA-testable build with Ufe 0.4.9 is ever going to exist, which means these could all be testing for 0.4.10 (or 0.4.11 if my extra python API changes are merged). That simplifies the cmake. But we don't know which version to check for until something gets merged into Maya, and we can't merge into Maya until this gets merged...

It seems to me we can merge this with GREATER_EQUAL 4010 (or 4011) at any time. Local developer builds will have 0.4.10 (or 0.4.11, as the case may be), so maya-usd will work with the newer version, as well as with the older UFE version. Am I missing something?

test/lib/ufe/testCamera.py

ppt-adsk · 2022-05-03T14:59:31Z

test/lib/ufe/testCamera.py

+        result = ufe.CameraHandler.findAll(proxyShapeParentItem)
+        self.assertTrue(result.contains(camera1Path))
+        self.assertTrue(result.contains(camera2Path))
+        self.assertEqual(len(result), 2)


So why aren't we finding Maya cameras?

The Maya camera handler is not implemented, and there are no cameras that are children of proxyShapeParentItem. If we had the Maya camera handler and we had searched the Maya root path then we'd find front, side, top and persp.

Right, so do we plan on leaving the Maya camera handler unimplemented, because there is no demonstrated need for it? If so, would be nice to add a comment to that effect in the test.

Sure I'll add a comment.

ppt-adsk · 2022-05-03T15:01:22Z

test/lib/ufe/testSceneSegment.py

+        # Load plugins
+        self.assertTrue(self.pluginsLoaded)
+
+    def _StartTest(self, testName):


Why is this different from setUp()?

I'm not really sure. I copied most of the test from testCamera.py and it has this pattern too. Probably I copied it for testCamera from somewhere else.

I guess if all the tests are running with the same USD file (which for testCamera and testSceneSegment they are running with the same USD file, then the file could get loaded in setUp and each test could re-use it? That would make the test run faster. That should work relatively easily here because I'm not actually modifying the file at all.

I'll try it and see how it goes.

williamkrick · 2022-05-10T20:59:12Z

The new changes were around the feedback provided by @ppt-adsk on the tests for CameraHandler and SceneSegementHandler. I also re-based on top of dev to eliminate merge conflicts.

ppt-adsk

Not a big deal, but it feels like the tests could have some fat trimmed off, sorry for noticing this late. Otherwise looks great!

ppt-adsk · 2022-05-11T15:42:06Z

test/lib/ufe/testCamera.py

+        camera2Path = ufe.Path([mayaPathSegment, cam2UsdPathSegment])
+
+        # Test that the camera handlers can find USD cameras in a scene segment
+        proxyShapeParentSegment = mayaUtils.createUfePathSegment('|stage')


I wonder about these single-use UFE path segments --- can't we just use ufe.PathString.path() to create them out of concatenated strings? E.g.
mayaPathSegmentStr = '|stage|stageShape'
camerasParentPath = ufe.PathString.path(mayaPathSegmentStr+'/cameras/cam2')
I get that you're re-using the Maya path segment, and in real code there may be a performance benefit to doing this, but in a test it seems so verbose.

ppt-adsk · 2022-05-11T15:44:16Z

test/lib/ufe/testCamera.py

+        geoPath = ufe.Path([mayaPathSegment, geoPathSegment])
+        camera1Path = ufe.Path([mayaPathSegment, camera1PathSegment])
+
+        proxyShapeItem = ufe.Hierarchy.createItem(proxyShapePath)


Have a look at ufeUtils.createItem(). It will create a ufe.SceneItem directly out of a path string, I think this is what you want, and will remove a lot of boiler-plate code from your test.

… the CameraHandler and SceneSegment interfaces.

williamkrick · 2022-05-12T15:32:05Z

Updated the tests to create the required paths in a more concise way. Updated the Ufe preview version checks to the latest.

Don't merge until the Ufe PR is merged.

williamkrick commented Apr 22, 2022

View reviewed changes

lib/mayaUsd/ufe/ProxyShapeSceneSegmentHandler.cpp Outdated Show resolved Hide resolved

williamkrick commented Apr 22, 2022

View reviewed changes

williamkrick requested review from ppt-adsk and vlasovi April 22, 2022 19:39

williamkrick assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Apr 25, 2022

vlasovi previously approved these changes Apr 25, 2022

View reviewed changes

williamkrick dismissed vlasovi’s stale review via f2a5994 April 26, 2022 20:09

williamkrick assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Apr 29, 2022

ppt-adsk requested changes Apr 29, 2022

View reviewed changes

williamkrick force-pushed the krickw/MAYA-122104/SceneSegmentHandler branch from 949b990 to 0142456 Compare May 2, 2022 16:02

williamkrick requested a review from ppt-adsk May 2, 2022 16:04

williamkrick assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 2, 2022

seando-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label May 3, 2022

ppt-adsk requested changes May 3, 2022

View reviewed changes

krickw added 3 commits May 10, 2022 16:54

ProxyShapeSceneSegmentHandler and UsdCameraHandler::findCameras.

72fc747

Clang format.

685bb0c

Code review feedback.

e34ea9c

krickw added 8 commits May 10, 2022 16:54

Rename isAGateway.

61bd075

Code review feedback, clang format, test.

85b836b

Another clang format.

4aac63d

clang

0bf3951

Add testSceneSegment.

467c627

Only run the scene segment test when the python API is available.

4eabacf

Code review feedback on tests.

ea58f0b

Fix build.

34cd739

williamkrick force-pushed the krickw/MAYA-122104/SceneSegmentHandler branch from d32acd0 to 34cd739 Compare May 10, 2022 20:57

williamkrick requested a review from ppt-adsk May 10, 2022 20:58

williamkrick assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 10, 2022

williamkrick removed the do-not-merge-yet Development is not finished, PR not ready for merge label May 11, 2022

ppt-adsk reviewed May 11, 2022

View reviewed changes

krickw added 3 commits May 12, 2022 11:11

Updates to CameraHandler and SceneSegmentHandler interfaces.

2501668

Update protection to use the final Ufe preview version that modifiers…

b95057a

… the CameraHandler and SceneSegment interfaces.

Merge branch 'dev' into krickw/MAYA-122104/SceneSegmentHandler

d0a759e

williamkrick requested a review from ppt-adsk May 12, 2022 15:32

williamkrick assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 12, 2022

williamkrick added the do-not-merge-yet Development is not finished, PR not ready for merge label May 12, 2022

ppt-adsk approved these changes May 12, 2022

View reviewed changes

williamkrick added ready-for-merge Development process is finished, PR is ready for merge ufe Related to UFE component in Maya ufe-usd Related to UFE-USD plugin in Maya-Usd and removed do-not-merge-yet Development is not finished, PR not ready for merge labels May 12, 2022

seando-adsk merged commit 003226a into dev May 12, 2022

seando-adsk deleted the krickw/MAYA-122104/SceneSegmentHandler branch May 12, 2022 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-122104 ProxyShapeSceneSegmentHandler and UsdCameraHandler::findCameras. #2307

MAYA-122104 ProxyShapeSceneSegmentHandler and UsdCameraHandler::findCameras. #2307

williamkrick commented Apr 22, 2022

williamkrick Apr 22, 2022

vlasovi left a comment

williamkrick commented Apr 25, 2022

ppt-adsk left a comment

williamkrick commented May 2, 2022

seando-adsk commented May 3, 2022

ppt-adsk left a comment

ppt-adsk May 3, 2022

williamkrick May 3, 2022

ppt-adsk May 3, 2022

ppt-adsk May 3, 2022

williamkrick May 3, 2022

ppt-adsk May 3, 2022

williamkrick May 3, 2022

ppt-adsk May 3, 2022

williamkrick May 3, 2022

williamkrick commented May 10, 2022

ppt-adsk left a comment

ppt-adsk May 11, 2022

ppt-adsk May 11, 2022

williamkrick commented May 12, 2022 •

edited

Loading

MAYA-122104 ProxyShapeSceneSegmentHandler and UsdCameraHandler::findCameras. #2307

MAYA-122104 ProxyShapeSceneSegmentHandler and UsdCameraHandler::findCameras. #2307

Conversation

williamkrick commented Apr 22, 2022

Choose a reason for hiding this comment

vlasovi left a comment

Choose a reason for hiding this comment

williamkrick commented Apr 25, 2022

ppt-adsk left a comment

Choose a reason for hiding this comment

williamkrick commented May 2, 2022

seando-adsk commented May 3, 2022

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

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

williamkrick commented May 10, 2022

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

williamkrick commented May 12, 2022 • edited Loading

williamkrick commented May 12, 2022 •

edited

Loading