Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict UFE nodes according to proxyNode's primPath #3640

Merged
merged 6 commits into from
Mar 14, 2024

Conversation

dj-mcg
Copy link
Collaborator

@dj-mcg dj-mcg commented Feb 28, 2024

Only create UFE nodes that are ancestors or descendants of the proxy node's primPath. This addresses a performance issue we encountered in our Reference Assembly workflows. For these workflows, there were reference assemblies that were referencing very large Usd scenes, but drawing a very small subset of it. While Maya would only image the subset, UFE was populating every node from the Usd stage.

This caused several performance problems, including:

  • viewport -> panel -> perspective slowness due to it traversing all the UFE nodes looking for cameras
  • selection with the transform tool

This fix limits creation of UFE nodes to those "relevant" to the prim path

This fix also changes behavior of imaging a proxy node with an invalid prim path. Previously, a non-empty invalid path would cause the entire stage to be imaged. With this change, we image nothing.

Only create UFE nodes that are ancestors or descendants of the proxy
node's primPath. This addresses a performance issue we encountered in
our Reference Assembly workflows. For these workflows, there were
reference assemblies that were referencing very large Usd scenes, but
drawing a very small subset of it. While Maya would only image the
subset, UFE was populating every node from the Usd stage.

This caused several performance problems, including:
* viewport -> panel -> perspective slowness due to it traversing all the
UFE nodes looking for cameras
* selection with the transform tool

This fix limits creation of UFE nodes to those "relevant" to the prim
path

This fix also changes behavior of imaging a proxy node with an invalid
prim path. Previously, a non-empty invalid path would cause the entire
stage to be imaged. With this change, we image nothing.
This will hopefully resolve a merge conflict that was encountered with
the last push.
@seando-adsk seando-adsk added the core Related to core library label Feb 29, 2024
@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Feb 29, 2024

I fixed the typo in the comment in this branch but it looks like that part of the change is still being identified as a conflict. I would just accept my change in the conflict but I appear not to have permission to resolve it.

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Mar 1, 2024

Also, note that we added a test with the change. This is enabled internally, but I did not enable it in the open source build. I made a naive attempt, but I'm woefully undereducated when it comes to CMake so I gave up after I ran into some import errors with ufe and mayaUsd. It would be a good idea to enable it if you don't mind updating the makefiles to include it. Thank you!

@seando-adsk seando-adsk requested review from pierrebai-adsk and removed request for ppt-adsk March 6, 2024 13:55
Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like the function to be renamed for clarity.

@pierrebai-adsk
Copy link
Collaborator

pierrebai-adsk commented Mar 6, 2024

I fixed the typo in the comment in this branch but it looks like that part of the change is still being identified as a conflict. I would just accept my change in the conflict but I appear not to have permission to resolve it.

You need to pull the dev branch, merge it in your branch and resolve conflicts locally on your computer and push your branch again.

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Mar 6, 2024

I fixed the typo in the comment in this branch but it looks like that part of the change is still being identified as a conflict. I would just accept my change in the conflict but I appear not to have permission to resolve it.

You need to pull the dev branch, merge it in your branch and resolve conflicts locally on your computer and push your branch again.

Will do, thanks - stand by for the commit

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Mar 6, 2024

OK the review notes and the conflict have been addressed. Thanks!

pierrebai-adsk
pierrebai-adsk previously approved these changes Mar 7, 2024
@seando-adsk seando-adsk added the ufe Related to UFE component in Maya label Mar 7, 2024
@seando-adsk seando-adsk assigned dj-mcg and unassigned dj-mcg Mar 7, 2024
@seando-adsk
Copy link
Collaborator

@dj-mcg I ran the preflight for you and it failed. Currently because of a security issue the build logs are not being uploaded to github. So I've looked at the logs and included the error here. It is on all the Maya 2022 builds:

S:\jenkins\workspace\ECP\ecg-maya-usd\ecg-mayausd-branch-preflight-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\nodes\proxyShapeBase.cpp(1931): error C2039: 'isEmpty': is not a member of 'Autodesk::Maya::OpenMaya20220000::MString'
P:\shared\Artifactory\Windows\rundev-2022\202303271415\runtime\795869a\runTime\include\maya/MString.h(49): note: see declaration of 'Autodesk::Maya::OpenMaya20220000::MString'

@pierrebai-adsk
Copy link
Collaborator

@dj-mcg I ran the preflight for you and it failed. Currently because of a security issue the build logs are not being uploaded to github. So I've looked at the logs and included the error here. It is on all the Maya 2022 builds:

S:\jenkins\workspace\ECP\ecg-maya-usd\ecg-mayausd-branch-preflight-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\nodes\proxyShapeBase.cpp(1931): error C2039: 'isEmpty': is not a member of 'Autodesk::Maya::OpenMaya20220000::MString'
P:\shared\Artifactory\Windows\rundev-2022\202303271415\runtime\795869a\runTime\include\maya/MString.h(49): note: see declaration of 'Autodesk::Maya::OpenMaya20220000::MString'

You need to replace isEmpty() by retrieving the length and comparing to zero. Unfortunate, but required for 2022 compat.

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Mar 7, 2024

@dj-mcg I ran the preflight for you and it failed. Currently because of a security issue the build logs are not being uploaded to github. So I've looked at the logs and included the error here. It is on all the Maya 2022 builds:

S:\jenkins\workspace\ECP\ecg-maya-usd\ecg-mayausd-branch-preflight-2022-python3-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\nodes\proxyShapeBase.cpp(1931): error C2039: 'isEmpty': is not a member of 'Autodesk::Maya::OpenMaya20220000::MString'
P:\shared\Artifactory\Windows\rundev-2022\202303271415\runtime\795869a\runTime\include\maya/MString.h(49): note: see declaration of 'Autodesk::Maya::OpenMaya20220000::MString'

You need to replace isEmpty() by retrieving the length and comparing to zero. Unfortunate, but required for 2022 compat.

OK, thanks! I should've tried building it against 2022 internally before pushing -- sorry about that!

@seando-adsk seando-adsk added ready-for-merge Development process is finished, PR is ready for merge do-not-merge-yet Development is not finished, PR not ready for merge and removed ready-for-merge Development process is finished, PR is ready for merge labels Mar 7, 2024
@seando-adsk
Copy link
Collaborator

Added do not merge label, while we test this internally.

@seando-adsk seando-adsk added ready-for-merge Development process is finished, PR is ready for merge and removed do-not-merge-yet Development is not finished, PR not ready for merge labels Mar 14, 2024
@seando-adsk
Copy link
Collaborator

Internal Autodesk QA tested these changes and confirmed it with design. Okay for merging now.

@seando-adsk seando-adsk merged commit 0d774ac into Autodesk:dev Mar 14, 2024
13 checks passed
@dj-mcg dj-mcg deleted the pr/Respect_ProxyShape_PrimPath branch March 14, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to core library ready-for-merge Development process is finished, PR is ready for merge ufe Related to UFE component in Maya
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants