Add necessary headers for Vulkan QML GUI backend #706

darksylinc · 2022-08-14T23:17:31Z

🎉 New feature

No ticket is associated with this feature.

Order in which PRs must be merged

This PR.
Add fallback rendering for other APIs gz-gui#357
- or optionally, merge this one as part of the next one aka PR 467, which includes it
Add Vulkan QML backend gz-gui#467
Add CLI to switch to Vulkan & Metal backends gz-sim#1735

Summary

The GUI component needs to send the Vulkan device to Rendering component, which in turns sends it to OgreNext. There were various possible solutions:

gz-gui gets linked against OgreNext
gz-rendering gets linked against Qt
The solution in this PR, where a few structs are added to get passed around and perform Qt -> OgreNext conversions while both components are reasonably isolated from each other.

This PR adds the following structs:

GzVulkanExternalInstance (mirrors Ogre::VulkanExternalInstance without depending on OgreNext)
GzVulkanExternalDevice (mirrors Ogre::VulkanExternalDevice without depending on OgreNext)

These structs are filled by gz-gui and passed to gz-rendering via "external_instance" and "external_device".

The new code is guarded using #ifdef OGRE_BUILD_RENDERSYSTEM_VULKAN because if OgreNext wasn't built with Vulkan support, then it is extremely likely the Vulkan SDK isn't present (i.e. Apple systems) which means including RenderEngineVulkanExternalDeviceStructs.hh will cause errors.

Test it

To test this code we need gazebosim/gz-gui#467 however we could add test coverage by creating a Vulkan Device in a test and testing this interface.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
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.

codecov · 2022-08-14T23:38:23Z

Codecov Report

Merging #706 (5639156) into main (581aaab) will decrease coverage by 0.10%.
The diff coverage is 28.12%.

@@            Coverage Diff             @@
##             main     #706      +/-   ##
==========================================
- Coverage   74.18%   74.07%   -0.11%     
==========================================
  Files         159      159              
  Lines       14381    14408      +27     
==========================================
+ Hits        10668    10673       +5     
- Misses       3713     3735      +22

Impacted Files	Coverage Δ
include/gz/rendering/Camera.hh	`100.00% <ø> (ø)`
include/gz/rendering/RenderTarget.hh	`100.00% <ø> (ø)`
ogre2/src/Ogre2Camera.cc	`84.57% <0.00%> (-1.98%)`	⬇️
ogre2/src/Ogre2RenderTarget.cc	`81.17% <14.28%> (-2.20%)`	⬇️
include/gz/rendering/base/BaseCamera.hh	`66.66% <33.33%> (-0.94%)`	⬇️
include/gz/rendering/base/BaseRenderTarget.hh	`79.12% <50.00%> (-1.57%)`	⬇️
ogre2/src/Ogre2RenderEngine.cc	`76.67% <75.00%> (+0.09%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

include/gz/rendering/RenderEngineVulkanExternalDeviceStructs.hh

iche033 · 2022-08-25T00:45:41Z

we could target / backport this to gz-rendering7 in case gazebosim/gz-gui#467 works on Garden

Signed-off-by: Matias N. Goldberg <[email protected]>

Needed by Vulkan QML integration Signed-off-by: Matias N. Goldberg <[email protected]>

Needed by QML Vulkan integration. Signed-off-by: Matias N. Goldberg <[email protected]>

Signed-off-by: Matias N. Goldberg <[email protected]>

iche033

changes look fine. Just need to fix Jammy build by adding vulkan dependency

include/gz/rendering/RenderEngineVulkanExternalDeviceStructs.hh

Signed-off-by: Matias N. Goldberg <[email protected]>

iche033 · 2022-10-10T22:06:41Z

Can you add libvulkan-dev in the jammy apt packages file to see if it fixes Jammy CI build?

Signed-off-by: Matias N. Goldberg <[email protected]>

darksylinc · 2022-10-10T23:59:04Z

OK I just did. Let's cross fingers 🤞

iche033 · 2022-10-13T18:19:00Z

the jammy build got further. Now it complains about the 2 macros in this block.

build error here

  /github/workspace/ogre2/src/Ogre2RenderEngine.cc:47:59: error: expected identifier before numeric constant
     47 | #    define VK_ACCESS_ACCELERATION_STRUCTURE_READ_BIT_KHR 0x00200000
        |                                                           ^~~~~~~~~~
  /github/workspace/ogre2/src/Ogre2RenderEngine.cc:47:59: error: expected '}' before numeric constant
  In file included from /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanQueue.h:37,
                   from /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanDevice.h:34,
                   from /github/workspace/ogre2/src/Ogre2RenderEngine.cc:51:
  /usr/include/vulkan/vulkan_core.h:1941:31: note: to match this '{'
   1941 | typedef enum VkAccessFlagBits {
        |                               ^
  /github/workspace/ogre2/src/Ogre2RenderEngine.cc:47:59: error: expected unqualified-id before numeric constant
     47 | #    define VK_ACCESS_ACCELERATION_STRUCTURE_READ_BIT_KHR 0x00200000
        |                                                           ^~~~~~~~~~
  In file included from /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanQueue.h:37,
                   from /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanDevice.h:34,
                   from /github/workspace/ogre2/src/Ogre2RenderEngine.cc:51:
  /usr/include/vulkan/vulkan_core.h:1976:3: error: declaration does not declare anything [-fpermissive]
   1976 | } VkAccessFlagBits;
        |   ^~~~~~~~~~~~~~~~
  In file included from /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanQueue.h:37,
                   from /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanDevice.h:34,
                   from /github/workspace/ogre2/src/Ogre2RenderEngine.cc:51:
  /usr/include/vulkan/vulkan_core.h:14312:1: error: expected declaration before '}' token
  14312 | }
        | ^
  In file included from /github/workspace/ogre2/src/Ogre2RenderEngine.cc:51:
  /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanDevice.h:172:59: error: 'VK_ACCESS_SHADING_RATE_IMAGE_READ_BIT_NV' was not declared in this scope; did you mean 'VK_ACCESS_2_SHADING_RATE_IMAGE_READ_BIT_NV'?
    172 |           VK_ACCESS_ACCELERATION_STRUCTURE_READ_BIT_KHR | VK_ACCESS_SHADING_RATE_IMAGE_READ_BIT_NV |
        |                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        |                                                           VK_ACCESS_2_SHADING_RATE_IMAGE_READ_BIT_NV
  /usr/include/OGRE-2.3/RenderSystems/Vulkan/OgreVulkanDevice.h:173:11: error: 'VK_ACCESS_FRAGMENT_DENSITY_MAP_READ_BIT_EXT' was not declared in this scope; did you mean 'VK_ACCESS_2_FRAGMENT_DENSITY_MAP_READ_BIT_EXT'?
    173 |           VK_ACCESS_FRAGMENT_DENSITY_MAP_READ_BIT_EXT | VK_ACCESS_COMMAND_PREPROCESS_READ_BIT_NV );
        |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        |           VK_ACCESS_2_FRAGMENT_DENSITY_MAP_READ_BIT_EXT
  make[2]: *** [ogre2/src/CMakeFiles/gz-rendering8-ogre2.dir/build.make:594: ogre2/src/CMakeFiles/gz-rendering8-ogre2.dir/Ogre2RenderEngine.cc.o] Error 1

I reproduced it on Jammy. If I comment those two #defines, it builds though.

Signed-off-by: Matias N. Goldberg <[email protected]>

darksylinc · 2022-10-13T22:54:45Z

Ahhhhh!!! That error is so annoying.

The #define VK_ACCESS_ACCELERATION_STRUCTURE_READ_BIT_KHR is needed as a workaround because the flag is not defined in the headers included by focal, but jammy uses a newer version.

I think the problem is that # ifndef VK_NV_device_generated_commands and #ifndef VK_NV_ray_tracing guards are failing because vulkan/vulkan_core.h had not been included yet.

I just pushed a fix. Let's hope that fixes it.

iche033

CI is green!

darksylinc requested a review from iche033 as a code owner August 14, 2022 23:17

darksylinc mentioned this pull request Aug 14, 2022

Add Vulkan QML backend gazebosim/gz-gui#467

Merged

14 tasks

ahcorde reviewed Aug 15, 2022

View reviewed changes

include/gz/rendering/RenderEngineVulkanExternalDeviceStructs.hh Outdated Show resolved Hide resolved

iche033 changed the base branch from main to gz-rendering7 August 25, 2022 00:10

iche033 changed the base branch from gz-rendering7 to main August 25, 2022 00:11

darksylinc added 3 commits September 24, 2022 19:35

Add necessary code to get Qt 5.15 + Vulkan working

8e111cc

Signed-off-by: Matias N. Goldberg <[email protected]>

Add Camera::PrepareForExternalSampling

e92b969

Needed by Vulkan QML integration Signed-off-by: Matias N. Goldberg <[email protected]>

Add _reinterpretable parameter to Camera::SetImageFormat

15137d2

Needed by QML Vulkan integration. Signed-off-by: Matias N. Goldberg <[email protected]>

darksylinc force-pushed the qml-vulkan-rebased-main branch from 60c1d9e to 15137d2 Compare September 24, 2022 22:35

Fix include error when OGRE2 is named OGRE-Next

9b299b1

Signed-off-by: Matias N. Goldberg <[email protected]>

This was referenced Sep 25, 2022

Add CLI to switch to Vulkan & Metal backends gazebosim/gz-sim#1735

Merged

Add fallback rendering for other APIs gazebosim/gz-gui#357

Merged

iche033 reviewed Sep 29, 2022

View reviewed changes

include/gz/rendering/RenderEngineVulkanExternalDeviceStructs.hh Outdated Show resolved Hide resolved

Cosmetic changes

84c03e2

Signed-off-by: Matias N. Goldberg <[email protected]>

Add libvulkan-dev to jammy CI

4c3fc86

Signed-off-by: Matias N. Goldberg <[email protected]>

Fix build error in jammy CI

5639156

Signed-off-by: Matias N. Goldberg <[email protected]>

iche033 approved these changes Oct 15, 2022

View reviewed changes

iche033 merged commit b197260 into gazebosim:main Oct 15, 2022

iche033 mentioned this pull request Dec 9, 2022

Look for vulkan in CMakeLists #782

Merged

8 tasks

iche033 added the 🎵 harmonic Gazebo Harmonic label Feb 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add necessary headers for Vulkan QML GUI backend #706

Add necessary headers for Vulkan QML GUI backend #706

darksylinc commented Aug 14, 2022 •

edited

Loading

codecov bot commented Aug 14, 2022 •

edited

Loading

iche033 commented Aug 25, 2022

iche033 left a comment

iche033 commented Oct 10, 2022

darksylinc commented Oct 10, 2022

iche033 commented Oct 13, 2022

darksylinc commented Oct 13, 2022 •

edited

Loading

iche033 left a comment

Add necessary headers for Vulkan QML GUI backend #706

Add necessary headers for Vulkan QML GUI backend #706

Conversation

darksylinc commented Aug 14, 2022 • edited Loading

🎉 New feature

Order in which PRs must be merged

Summary

Test it

Checklist

codecov bot commented Aug 14, 2022 • edited Loading

Codecov Report

iche033 commented Aug 25, 2022

iche033 left a comment

Choose a reason for hiding this comment

iche033 commented Oct 10, 2022

darksylinc commented Oct 10, 2022

iche033 commented Oct 13, 2022

darksylinc commented Oct 13, 2022 • edited Loading

iche033 left a comment

Choose a reason for hiding this comment

darksylinc commented Aug 14, 2022 •

edited

Loading

codecov bot commented Aug 14, 2022 •

edited

Loading

darksylinc commented Oct 13, 2022 •

edited

Loading