ensure that GL is initialized when mtoh creates the Storm render delegate #1007
Conversation
…gate For core USD prior to 21.02, this is done by initializing GLEW, but for 21.02 and later where the GLEW dependency has been removed, we instead call GarchGLApiLoad().
| // We also disable clang-format for this block, since otherwise v10.0.0 fails | ||
| // to recognize that "utils.h" is the related header. | ||
| // clang-format off |
There was a problem hiding this comment.
Another clang-format oddity here, but when I added this block of includes and the conditional, clang-format v10.0.0 starts failing to recognize that "utils.h" is the related header for this cpp file, and it tries to put it at the bottom of the "private" header group at line 32. I opted to disable clang-format here rather than absorb that change.
There was a problem hiding this comment.
Do you happen to know if v11.0.0 will has it fixed?
There was a problem hiding this comment.
Do you happen to know if v11.0.0 will has it fixed?
I just built clang v11.0.0 and gave it a try (using the current configuration) and I get the same results. It seems to get tripped up by the conditional that wraps the glew.h #include. If I remove the #if/#else, it correctly figures out that utils.h should stay where it is, just after glew.h but before the other "local" headers.
| #if USD_VERSION_NUM >= 2102 | ||
| #include <pxr/imaging/garch/glApi.h> | ||
| #endif |
There was a problem hiding this comment.
Unlike GLEW, this garch include does not need to come first.
There was a problem hiding this comment.
I had a follow-up chat with the Hydra team about this. They were saying that the newly added GL loader doesn't break the way glew.h does if it's not #include'd first, it's probably still good practice to include it early. I've got a new PR coming shortly with a bunch more instances of this, so I'll revisit this one as part of those changes. I'll also amend the clang-format config so that it includes both glew.h and glApi.h in the first group.
The Hydra team also just made a change to GlfContextCaps::InitInstance() so that it calls GarchGLApiLoad() itself, so we'd then be able to remove the call I had to add here for core USD 21.02+.
There was a problem hiding this comment.
Hmm... well, note sure why @mattyjams was encountering this, and I didn't. (Nor Hamed or the pre-flight tests...) However, since it should be fine to call GlfGlewInit more than once, this looks good / makes sense to me.
|
Apologies @mattyjams, this somehow pass undetected in our pre-merge validation. I tested again, and you are correct that dev was broken with dev post 20.11. |
…gate in mtoh For core USD 21.02, GlfContextCaps::InitInstance() was updated to call GarchGLApiLoad() itself in the following core USD commit: PixarAnimationStudios/OpenUSD@4034139 As a result, the call to GarchGLApiLoad() that was added in Autodesk#1007, and the accompanying include of glApi.h, were made redundant and are no longer necessary. The call to GlfGlewInit() remains for earlier versions of core USD.
I'm not sure how others aren't running into this, but we're seeing all of the newly added
mtohtests crashing because GL/GLEW isn't fully initialized at the point where it discovers the Storm renderer plugin and tries to callGlfContextCaps::InitInstance().Prior to core USD 21.02, this was handled by calling
GlfGlewInit(), but from 21.02 on, the GLEW dependency has been removed, andGarchGLApiLoad()is used instead.The same pattern of calling
GlfGlewInit()/GarchGLApiLoad()before callingGlfContextCaps::InitInstance()can be seen in some of the unit tests in core USD, for example:https://github.com/PixarAnimationStudios/USD/blob/e9172af0ccea9896990f5ac093c3d6063ac7cab5/pxr/imaging/hdSt/unitTestGLDrawing.cpp#L94