Skip to content
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

Add Vulkan QML backend #467

Merged
merged 12 commits into from
Jan 5, 2023
Merged

Conversation

darksylinc
Copy link
Contributor

@darksylinc darksylinc commented Aug 14, 2022

🎉 New feature

Order in which PRs must be merged

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

Summary

Note: This PR depends on gazebosim/gz-rendering#706

Note: This PR is polluted with changes that are supposed to be merged by PR #357

This PR adds native Vulkan backend using Qt's QML Vulkan RHI.

I couldn't believe it so I opened it up on RenderDoc and indeed it's using Vulkan:

Screenshot_2022-08-14_19-51-44

This PR breaks ABI slightly, so it's best if it targets post-Garden.

With a few global variables we could workaround it and backport it to Garden though. Note that Garden supports Vulkan via a fallback that is very slow. This PR offers native, full speed integration.

Note: Qt 5.14 is the minimum requirement, which means it's not available on Ubuntu 20.04 but rather Ubuntu 22.04. I put macros to use at least 5.15.2 because that's what I tested against.

Note: Qt 5 considers Vulkan QML interface to be 'experimental' and I've submitted a patch to Qt to improve compliance with the Vulkan standard. Ideally Gazebo should eventually migrate to Qt 6.

Missing tasks:

  • Ensure code compiles without Qt 5.15.2+
  • Ensure code compiles when Vulkan isn't available
  • Gazebo needs to tell OgreNext to make a layout transition / synchronization for Qt (via BarrierSolver).
  • Run Vulkan validations to catch any potential issue
    • Done. Only validation errors show up on shutdown, but that's difficult to debug right now because gz-gui crashes on shutdown due to infamous crash at shutdown (can't find the ticket now) that could be the cause. May also be a side effect of Properly terminate render engine #17
  • Add vulkan toggle via command line

Test it

This PR can be tested once gazebosim/gz-sim#1735 (which comes after this PR) is merged, which adds the capability to enable the features in this PR via CLI.

Testing it in Ubuntu 20.04

It is possible to build with Ubuntu 20.04 against Qt 5.15.2. You need the following:

  1. Install Qt 5.15.2 using the online installer
  2. Compile Gazebo with: colcon build --cmake-args -DCMAKE_PREFIX_PATH=/opt/Qt/5.15.2/gcc_64 -DQT_QMAKE_EXECUTABLE=/opt/Qt/5.15.2/gcc_64/bin/qmake --merge-install
  3. To run gazebo go to the install folder and do:
export LD_LIBRARY_PATH=/opt/Qt/5.15.2/gcc_64/lib
. setup.bash
gz sim shapes.sdf

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.

src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
}

/////////////////////////////////////////////////
/////////////////////////////////////////////////
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/////////////////////////////////////////////////

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double comment is on purpose (it's also in MinimalSceneRhiOpenGL.cc.cpp & MinimalSceneRhiMetal.mm) to distinguish the switch from GzCameraTextureRhiVulkan class definitions to RenderThreadRhiVulkan.

(fun fact: GzCameraTextureRhi* don't seem to actually be used. It seems to be dead code but this theory needs to be tested; I added a TODO in class GzCameraTextureRhi).

@nkoenig nkoenig requested a review from mjcarroll August 22, 2022 18:31
@iche033
Copy link
Contributor

iche033 commented Aug 25, 2022

This PR breaks ABI slightly, so it's best if it targets post-Garden.

I think there is no ABI breakage? Adding a new function to Helpers.hh should be ok. The minimal scene plugin headers are not installed.

@darksylinc darksylinc marked this pull request as ready for review September 25, 2022 17:31
@jennuine jennuine added the needs upstream release Blocked by a release of an upstream library label Sep 26, 2022
@iche033
Copy link
Contributor

iche033 commented Sep 29, 2022

we need to bump gz-rendering dep version to 8.0.0 and update CI configurations. I'll do that in a separate PRs after all changes are forward ported to main.

@mjcarroll
Copy link
Contributor

we need to bump gz-rendering dep version to 8.0.0 and update CI configurations. I'll do that in a separate PRs after all changes are forward ported to main.

I thought this could land in garden?

@iche033
Copy link
Contributor

iche033 commented Sep 29, 2022

I thought this could land in garden?

gazebosim/gz-rendering#706 may have some ABI breaking changes. We could try to work around it.

@darksylinc
Copy link
Contributor Author

Originally yes. With a few hacks I think/thought it could still land on Garden.

However my original assessment was assuming #357 was already merged (I was surprised when I found out it was not), and AFAIK Garden will be released without it.

I'm not sure if we can avoid ABI breakage given that #357 wasn't included. I'd have to carefully look into it if we want to backport at least the fallback path.

@iche033
Copy link
Contributor

iche033 commented Sep 30, 2022

I think #357 can go in ign-gui6 and it touches only files in the minimal scene GUI plugin. Those headers are not installed so we don't have to worry about API/ABI compatibility.

@iche033
Copy link
Contributor

iche033 commented Oct 10, 2022

coming back to this, I think gazebosim/gz-rendering#706 is going to be hard to backport to garden. @mjcarroll are you ok with these PRs landing in main?

@mjcarroll
Copy link
Contributor

Yes, lets land them on main for expedience.

@azeey
Copy link
Contributor

azeey commented Nov 14, 2022

needs version bumps

@darksylinc
Copy link
Contributor Author

@azeey I'm trying to rebase this PR to main; but current main has a gazillion build errors.

gz-gui/main branch clearly needs to merge with gz-gui/gz-gui7 branch.

gz-sim/main is in the same state (merging has quite a number of merge conflicts).
gz-launch/main is in the same state.

On top of those build errors, they're also looking for the wrong project versions (e.g they look for gz-rendering7 instead of gz-rendering8).

Basically building Harmonic via collection-harmonic.yaml doesn't yet work out of the box.

Should I wait until Harmonic is buildable?

@azeey
Copy link
Contributor

azeey commented Nov 21, 2022

Should I wait until Harmonic is buildable?

Yes, there are a couple of PRs in flight bumping the major versions of gz-rendering and its dependees. These are tracked in gazebo-tooling/release-tools#853. Let's wait till all of those are merged.

@iche033
Copy link
Contributor

iche033 commented Dec 10, 2022

after #512 gets in, everything up to gz-gui should be buildable

@darksylinc
Copy link
Contributor Author

Another rebased and retargeted.

@codecov
Copy link

codecov bot commented Dec 11, 2022

Codecov Report

Merging #467 (99f887f) into main (fce993b) will decrease coverage by 0.09%.
The diff coverage is 61.72%.

@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
- Coverage   68.39%   68.30%   -0.10%     
==========================================
  Files          45       45              
  Lines        4980     5038      +58     
==========================================
+ Hits         3406     3441      +35     
- Misses       1574     1597      +23     
Impacted Files Coverage Δ
include/gz/gui/Helpers.hh 77.77% <ø> (ø)
src/plugins/minimal_scene/MinimalScene.hh 100.00% <ø> (ø)
src/plugins/minimal_scene/MinimalSceneRhi.hh 100.00% <ø> (ø)
src/plugins/minimal_scene/MinimalSceneRhiOpenGL.cc 91.11% <0.00%> (-1.12%) ⬇️
src/plugins/minimal_scene/MinimalSceneRhiOpenGL.hh 100.00% <ø> (ø)
src/Application.cc 83.79% <50.00%> (-2.08%) ⬇️
src/plugins/minimal_scene/MinimalScene.cc 62.59% <68.75%> (+0.42%) ⬆️
src/Helpers.cc 92.00% <75.00%> (-1.48%) ⬇️

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

@iche033 iche033 removed the needs upstream release Blocked by a release of an upstream library label Dec 20, 2022
@darksylinc
Copy link
Contributor Author

I need to rebase this PR now that the previous one has been merged.

It's working!

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
This fixes Vulkan validation warnings

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Fix some GCC warnings

Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Signed-off-by: Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
@darksylinc
Copy link
Contributor Author

darksylinc commented Dec 24, 2022

Are the tests flaky? Jammy always fails on these two:

51 - INTEGRATION_minimal_scene (Timeout)
53 - INTEGRATION_transport_scene_manager (SEGFAULT)

And Focal always succeeded except this last time (I made no significant changes that would cause this, possibly flaky):

45 - INTEGRATION_camera_tracking (SEGFAULT)

As for Windows CI, I fixed the problems caused by my PR; the new warnings don't look like caused by me (?)

And macOS was failing build, I fixed that. Now it fails various tests. I don't know if those are my fault.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested this with gazebosim/gz-sim#1735 on Jammy with Qt 5.13.3 (typo: it's Qt 5.15.3). It's working well for me with worlds like sensors_demo.sdf and segmentation_camera.sdf.

One thing I noticed is that gui crashes when I tried to resize the window when gui uses vulkan. If it's not trivial to fix, we can ticket an issue for this.

externalDevice.deviceExtensions.push_back(VkExtensionProperties{});
VkExtensionProperties &extProp = externalDevice.deviceExtensions.back();
strncpy(extProp.extensionName, ext.toStdString().c_str(),
VK_MAX_EXTENSION_NAME_SIZE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a compile warning here:

In function ‘char* strncpy(char*, const char*, size_t)’,
    inlined from ‘void fillQtDeviceExtensionsToOgre(gz::rendering::v8::GzVulkanExternalDevice&)’ at /home/ian/code/gz_h_ws/src/gz-gui/src/plugins/minimal_scene/MinimalScene.cc:655:12,
    inlined from ‘std::string gz::gui::plugins::GzRenderer::Initialize(gz::gui::plugins::RenderThreadRhi&)’ at /home/ian/code/gz_h_ws/src/gz-gui/src/plugins/minimal_scene/MinimalScene.cc:711:35:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:34: warning: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ specified bound 256 equals destination size [-Wstringop-truncation]
   95 |   return __builtin___strncpy_chk (__dest, __src, __len,
      |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
   96 |                                   __glibc_objsize (__dest));

```sh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be impossible? The macros:

#if QT_VERSION >= QT_VERSION_CHECK(5, 15, 2) && QT_CONFIG(vulkan)
#endif

Which means it should be excluded from complication with Qt 5.13.3

Anyway, the macro says we are doing this:

char data[256];
strncpy( data, somePtr, 256);

Which is dangerous because if strlen( somePtr ) >= 256; then data[255] will not contain the null terminator \0.

Which is why I do on the next line:

extProp.extensionName[VK_MAX_EXTENSION_NAME_SIZE - 1u] = 0;

in order to enforce the string is always valid.

Anyway, there may be other ways to fix this warning:

strncpy(extProp.extensionName, ext.constData(),
              VK_MAX_EXTENSION_NAME_SIZE - 1u);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah typo, I was testing with Qt 5.15.3 not Qt 5.13.3. I added the suggested fix to that line (and left other similar lines untouched) to get rid of the warning. 99f887f

@iche033
Copy link
Contributor

iche033 commented Dec 28, 2022

51 - INTEGRATION_minimal_scene (Timeout)
53 - INTEGRATION_transport_scene_manager (SEGFAULT)
45 - INTEGRATION_camera_tracking (SEGFAULT)

I've seen these tests fail from time to time. I think they are flaky.

And macOS was failing build, I fixed that. Now it fails various tests. I don't know if those are my fault.

I haven't seen those tests failed before. Not sure why. I restarted a new homebrew build and now it only has one expected test failure: https://build.osrfoundation.org/job/ignition_gui-ci-pr_any-homebrew-amd64/1893/. Will need to keep an eye on this.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI looks good. Tested together with gazebosim/gz-sim#1735:

gz sim -v 4 --render-engine-gui-api-backend vulkan  sensors_demo.sdf

GUI works for me in both vulkan and OpenGL.

@iche033 iche033 merged commit 91ed341 into gazebosim:main Jan 5, 2023
@iche033 iche033 added the 🎵 harmonic Gazebo Harmonic label Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants