-
Notifications
You must be signed in to change notification settings - Fork 203
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
[mtoh] Add camera adapter to support physical camera parameters and configurable motion blur and depth-of-field. #1282
[mtoh] Add camera adapter to support physical camera parameters and configurable motion blur and depth-of-field. #1282
Conversation
a26e4cb
to
8bd4e6f
Compare
8bd4e6f
to
6fe2dd9
Compare
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.
Overall makes sense to me - just a few small questions / suggestions.
Think I covered it all. Closed out the -easy- issues, but left the more complicated ones open for you to do the same! |
44a6910
to
b25908f
Compare
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.
All the changes look good!
@marsupial Can you please apply clang-format so we can get PF results. Thank you. |
b6c386d
to
05abb31
Compare
Hopefully latest will pass pre-flight. |
@marsupial Clang-format worked...but there are some build failures on windows. |
…onfigurable motion blur.
05abb31
to
fb929d3
Compare
I took a wild guess at why MSVC is complaining about my perfectly legal code. -Thanks |
@marsupial There is one additional point that makes me slightly nervous about this change - there is no regression test added to cover the changes you made. I wouldn't want us to rely on manual testing to validate that things continue to work since that's not going to work with all permutations of versions that we must support. |
Thanks for the patch...woulda taken me forever to figure that out change-submit-change-submit style. I agree about the testing, but sadly Storm (and your VP-2 delegate) doesn't support HdCamera or motion-blur, best I could offer is some images proving our delegate matches the GL-Viewport in all fit-modes, and editing shutter start, end, and angle gives the expected results. Nothing was changed in the delegate for this patch, which matches what HdKarma does in Houdini. This was written so we can start doing more advanced play-blasting from Maya, so it should be getting exercised and any breakage will be reported to me pretty quickly. |
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.
@marsupial Looks like there is still something to investigate. We currently don't run interactive tests as part of PF and only run them before merging changes (it's something we are hoping to fix very soon). The following test is failing with your change:
49/148 Test #36: testMtohCommand .................................***Failed 84.24 sec
Maya.env file copied from C:/Users/ligenzk/Documents/maya/2020/ to E:/Dev/usd/_workspaces_latest/Maya_USD_PR_2020/build/RelWithDebInfo/test/Temporary/testMtohCommand/2020/Maya.env
MAYA_DEBUG_NO_SAVE_ON_CRASH is set.
Initialized VP2.0 renderer {
Version : 2016.11.53.12. Feature Level 5.
Adapter : Quadro K5000/PCIe/SSE2
Vendor ID: 4318. Device ID : 0
Driver : .
API : OpenGL V.4.5.
Max texture size : 16384 * 16384.
Max tex coords : 32
Shader versions supported (Vertex: 5, Geometry: 5, Pixel 5).
Shader compiler profile : (Best card profile)
Active stereo support available : 0
GPU Memory Limit : 4096 MB.
CPU Memory Limit: 62004.9 MB.
MultiDraw consolidation: enabled
}
OpenCL evaluator is attempting to initialize OpenCL.
OpenCL evaluator failed to initialize clew.
F.....
======================================================================
FAIL: test_createRenderGlobals (__main__.TestCommand)
----------------------------------------------------------------------
Traceback (most recent call last):
File "E:/Dev/usd/_workspaces_latest/Maya_USD_PR_2020/build/RelWithDebInfo/test/lib/mayaUsd/render/mayaToHydra\testMtohCommand.py", line 97, in test_createRenderGlobals
"defaultRenderGlobals.mtohEnableMotionSamples"))
AssertionError: False is not true
----------------------------------------------------------------------
Ran 6 tests in 4.761s
FAILED (failures=1)
Runtime Error: in GlfPostPendingGLErrors at line 85 of E:\Dev\usd\usd\pxr\imaging\glf\diagnostic.cpp -- GL error: invalid operation, reported from void __cdecl pxrInternal_v0_20__pxrReserved__::GlfSimpleShadowArray::_FreeBindfulTextures(void)
Should be passing now, just a pref-name change. |
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.
LGTM, thank you.
Running the test-suite locally to verify nothing has been broken, but as the change-set is quite large posting it now
Currently mtoh does motion-blur with two hard-coded samples t=frame and t=frame+1. We'd like this to be configurable, as these values/ranges are rarely used by our camera's, which are usually centered around the current frame. Additionally, reliance in the Hydra-projection matrix only for the camera means depth-of-field isn't currently supported.
These changes shouldn't affect Storm or other delegates that only use the Hydra render-pass view matrix.
TODO: Need a way to use the Maya viewport camera in a UsdProxyShape. Waiting until we can actually get the USD camera path when selected in the viewport to see what else can be done....The only way I can think of doing this right now is to build up a camera in the UsdProxyShape's stage, which has some downsides.