-
Notifications
You must be signed in to change notification settings - Fork 202
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
save and restore uniform buffer bindings during batch renderer selection #291
save and restore uniform buffer bindings during batch renderer selection #291
Conversation
Actually, I just saw that this was turned into an RAII class in mayaToHydra: Maybe it'd be better if I moved that to a public place (px_vp20/utils.h?) and used it from there instead? I'll try that and update the branch. |
@@ -1129,6 +1136,27 @@ UsdMayaGLBatchRenderer::_TestIntersection( | |||
GL_TEXTURE_BIT | | |||
GL_POLYGON_BIT); | |||
|
|||
// XXX: When Maya is using OpenGL Core Profile as the rendering engine (in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To help me understand better, when is _TestIntersection() called?
Is UsdMayaGLBatchRenderer being called from the draw override solution from Pixar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, UsdMayaGLBatchRenderer
is used to image pxrUsdProxyShape
and mayaUsdProxyShape
when VP2_RENDER_DELEGATE_PROXY
is disabled.
The MPxDrawOverride::userSelect()
override for proxy shapes is what ends up eventually calling _TestIntersection()
to do viewport-based picking:
UsdMayaProxyDrawOverride::userSelect( |
// across Hydra calls. We try not to bog down performance by saving and | ||
// restoring *all* GL_MAX_UNIFORM_BUFFER_BINDINGS possible bindings, so | ||
// instead we only do just enough to avoid issues. Empirically, the | ||
// problematic binding has been the material binding at index 4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this would only affect selection performance? Any other workflow that would be impacted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one other workflow that might be affected is when live surface is used. We end up using Hydra to do the intersection testing for that as well:
bool didIsect = renderer.TestIntersectionCustomPrimFilter( |
37d2924
to
0705c2c
Compare
I just pushed a re-worked version of this where the |
@@ -1196,6 +1198,8 @@ UsdMayaGLBatchRenderer::_ComputeSelection( | |||
|
|||
_selectResults.clear(); | |||
|
|||
UBOBindingsSaver bindingsSaver; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is slightly different from the previous version in that the instantiation of the UBOBindingsSaver
happens outside of _TestIntersection()
. In relatively infrequent cases, the loop below may go through more than one iteration, so this avoids repeatedly saving and restoring the bindings on each iteration.
This is also why a UBOBindingsSaver
was needed in TestIntersectionCustomPrimFilter()
above as well.
The original version of this PR is available for comparison in commit 37d2924.
Here's the simple test scene I was using for this: Extract that archive and launch Maya on the ma file with Then I had this incantation in the script editor (distilled from the unit test script mentioned in the description):
Without these changes, I was seeing testImage2.png coming out completely black. With the changes, it shows the selected cube instead. |
For what it's worth, it looks like the underlying issue is something that Animal Logic ended up tripping over as well:
|
lib/render/px_vp20/utils.h
Outdated
/// possible bindings, so instead we only do just enough to avoid issues. | ||
/// Empirically, the problematic binding has been the material binding at | ||
/// index 4. | ||
class UBOBindingsSaver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to add a GL post fix to the class name? I think this makes it clearer that the class is GL-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense. The name is a little opaque at a quick glance.
How does GLUniformBufferBindingsSaver
sound?
@mattyjams Thanks for making this into a utilities class that can be used elsewhere. |
This workaround was originally extracted from the Pixar batch renderer and turned into a more convenient and reusable RAII-style class. Since the workaround it provides is needed in multiple places, it would be good to have it defined publicly to take advantage of that reuse.
With UBOBindingsSaver now available from px_vp20/utils, this change uses that class to wrap the Hydra Execute() call made during rendering.
Uniform buffer bindings need to be saved and restored across Hydra Execute() calls as a workaround to the issue described in the documentation for the UBOBindingsSaver class. There are two such calls in the Pixar batch renderer, one during drawing, and another in the private _TestIntersection() method which is used when computing selections and during live surface workflows. The former was already protected by the workaround, but the latter was not, so this change applies the workaround there as well.
982acd5
to
80bf6f8
Compare
Which commit fixed it? |
I made the fix as part of an interactive rebase, so the export macros were added in commit 32def62. |
This addresses an issue when running the testProxyShapeSelectionPerformance test where only the first image would be drawn correctly and all images drawn after the first selection operation would be completely black.
This workaround was already being used during rendering, but this change applies it during selection as well.