PixarAnimationStudios
Autodesk: Revert "[HdSt, Hdx, HgiMetal] Full Masking Viewport issue on Mac"
Description of Change(s)
The intention of the commit ab0520a2 was to make the ComputeViewport the same in OpenGL and Metal. https://github.com/PixarAnimationStudios/USD/commit/ab0520a28a2f0e88140e64bb455ed1babaae141b
Originally, in Metal, ComputeViewport will just output the dataWindow as the viewport. And in HgiMetalGraphicsCmds::SetViewport, it uses "viewportHeight - viewportMinY" as the final minY. So we can know: finalMinY = viewportHeight - viewportMinY = windowHeight - windowMinY.
In the commit of ab0520a2, it uses the OpenGL calculation in ComputeViewport for both OpenGL and Metal. So ComputeViewport will use "frameBufferHeight - windowMinY - windowHeight" as the minY of viewport. And in HgiMetalGraphicsCmds::SetViewport, it uses "viewportHeight + viewportMin" as the final minY. So now : finalMinY = viewportHeight + viewportMinY = windowHeight + frameBufferHeight - windwMinY - windowHeight = framebufferHeight - windowMinY.
Indeed after this change, the finalMinY is different from the original finalMinY. If framebufferHeight is the same as the windowHeight, there is no problem. If they are different, the rendering result will be indirect.
So here we just revert the change.
Fixes Issue(s)
- N/A
- [X] I have verified that all unit tests pass with the proposed changes
- [X] I have submitted a signed Contributor License Agreement
Hi @erikaharrison-adsk ! Do you know if this issue is related to the Vulkan backend or another backend?
Hi @erikaharrison-adsk ! Do you know if this issue is related to the Vulkan backend or another backend?
Hi @jesschimein we found the issue on Metal backend.
Hi @erikaharrison-adsk! Do you have any examples or perhaps a USD asset that demonstrates the current behavior not working for HgiMetal? From what I can tell, what we have currently is correct. You are right that the final computed value of viewport y is different from what it was previously, but the previous code was incorrect and would result in drawing errors.
Here's an example using the current code where the data window height is a different size from the framebuffer height:
- Say we have a data window (0, 5, 100, 100) with a framebuffer size (100, 125).
- _ComputeViewport will return (0, 125 - (5 + 100), 100, 100) = (0, 20, 100, 100).
- The Metal viewport will be set to (0, 20 + 100, 100, -100) = (0, 120, 100, -100).
- Since the Metal viewport is y-down, this will result in a viewport that draws from pixels 20 to 120 in the y direction in our 125-height framebuffer. We then flip this image (whether interop-ing to GL or in writing it to image file), which means the area in which we drew matches the original data window.
Hi @erikaharrison-adsk! Do you have any examples or perhaps a USD asset that demonstrates the current behavior not working for HgiMetal? From what I can tell, what we have currently is correct. You are right that the final computed value of viewport y is different from what it was previously, but the previous code was incorrect and would result in drawing errors.
Here's an example using the current code where the data window height is a different size from the framebuffer height:
- Say we have a data window (0, 5, 100, 100) with a framebuffer size (100, 125).
- _ComputeViewport will return (0, 125 - (5 + 100), 100, 100) = (0, 20, 100, 100).
- The Metal viewport will be set to (0, 20 + 100, 100, -100) = (0, 120, 100, -100).
- Since the Metal viewport is y-down, this will result in a viewport that draws from pixels 20 to 120 in the y direction in our 125-height framebuffer. We then flip this image (whether interop-ing to GL or in writing it to image file), which means the area in which we drew matches the original data window.
Thank you. There was already some days for the change. I will review the change and answer you.
Hi @PierreWang! Friendly follow-up on this question -- are you able to provide any assets or examples of this not working on HgiMetal? Thanks!
Hi @PierreWang! Friendly follow-up on this question -- are you able to provide any assets or examples of this not working on HgiMetal? Thanks!
Sorry. Indeed the problem was discovered from a local app team of my company. So I need to review the message from that team and need their help to provide more information about this. But I was busy with some other issues and still didn't have time to talk with them.
No worries! I'm going to close this comment for now, but please feel free to reopen if the discussions indicate it is something that requires a fix on our end!
@jesschimein @clach I have verified that the problem that we met is because we mis-used the GfRect2i class. There is no problem with the current viewport calculation. You can close this pr. Thank you for all your comments, and sorry for my misleading in the pr.