Ogre2RenderEngine: on Windows if useCurrentGLContext is specified, set the externalWindowHandle ogre-next option

[traversaro]

🦟 Bug fix

Fixes part of gazebosim/gz-sim#2089 .

Summary

Without this fix, ogre-next fails with error:

wglMakeCurrent failed: The pixel format is invalid.
 in Win32Window::create at D:\bld\ogrenext_1684687719477\work\RenderSystems\GL3Plus\src\windowing\win32\OgreWin32Window.cpp (line 624)`

After this fix, I was able to correctly start gz sim -g on Windows:

In theory, the fix can be easily backported to gz-rendering7 and ign-rendering6, but as I am only working on getting Gazebo Harmonic to work well on Windows, I am not sure if there is any benefit of doing that. Anyone interested is welcome to backport the fix to earlier versions of gz-rendering.

The fix requires the Windows ID to be passed down to the rendering engine, but this is already happening in gz-sim and gz-gui thanks to gazebosim/gz-gui#292 and gazebosim/gz-sim#1063 .

This PR adds a required on Windows for winID to be set if useCurrentGLContext, I would be happy to document this additional constraint, but I could not find a place where these parameters are documented in the code or in the documentation, so for the time being I just added a (hopefully) descriptive error message.

I guess a similar fix may be required to get ogre (1) to run with gz-sim -g, but to honest now I am focusing on the minimal set of features for our Windows users to be able to migrate from Gazebo Classic.

Long and boring explanation

Why is this happening? First of all, it is important to know that on Windows (see https://stackoverflow.com/questions/50014602/wglcreatecontext-fails-with-error-the-pixel-format-is-invalid):

• Before calling wglMakeCurrent on an OpenGL context on a device context, you need to set the PixelFormat of the device,
• You can't set the pixel format of the NULL/0 device context.

So, in short, you can't call wglMakeCurrent passing as first argument 0/NULL. However, this is exactly what was happening in ogre-next at https://github.com/OGRECave/ogre-next/blob/v2.3.3/RenderSystems/GL3Plus/src/windowing/win32/OgreWin32Window.cpp#L620-L625, when the useCurrentGLContext option was passed.

This was happening because in https://github.com/OGRECave/ogre-next/blob/v2.3.3/RenderSystems/GL3Plus/src/windowing/win32/OgreWin32Window.cpp#L620-L625 if useCurrentGLContext is not passed, the mHDC argument is not zero, as it corresponds to the device context of newly created dummy window (see https://github.com/OGRECave/ogre-next/blob/v2.3.3/RenderSystems/GL3Plus/src/windowing/win32/OgreWin32Window.cpp#L492 and https://github.com/OGRECave/ogre-next/blob/v2.3.3/RenderSystems/GL3Plus/src/windowing/win32/OgreWin32Window.cpp#L516). However, if useCurrentGLContext is passed, no new windows is created, and both mHDC and mHwnd remain to the their default values (0), resulting in the aforementioned The pixel format is invalid. error.

Interestingly, exactly the same mechanism (the Windows handle passed to ogre with the externalWindowHandle) is exactly how the rendering in the window works on Windows for Gazebo Classic, see:

• https://github.com/gazebosim/gazebo-classic/blob/gazebo11_11.14.0/gazebo/rendering/WindowManager.cc#L101
• https://github.com/gazebosim/gazebo-classic/blob/gazebo11_11.14.0/gazebo/gui/GLWidget.cc#L247
• https://github.com/gazebosim/gazebo-classic/blob/gazebo11_11.14.0/gazebo/gui/GLWidget.cc#L964

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[traversaro]

fyi @xela-95, hopefully with this fix we should be able (soonish) to run gz-sim-yarp-plugins simulation on Windows as well.

[iche033]

Thanks for looking into this! Fix looks good. A few lines earlier in the same function, we're setting params["externalWindowHandle"] on Windows for the case when useCurrentGLContext is false (rendering on the server side). So it looks like we should be doing this regardless of what the useCurrentGLContext value is.

[traversaro]

A few lines earlier in the same function, we're setting params["externalWindowHandle"] on Windows for the case when useCurrentGLContext is false (rendering on the server side). So it looks like we should be doing this regardless of what the useCurrentGLContext value is.

Yes, but in case useCurrentGLContext is false, the value passed is different (_handle instead of winID) so I guess it is still useful to differentiate the two cases.