[macOS] add a QML version of simple_demo

[srmainwaring]

🎉 New feature

Summary

This PR adds a QML version of the simple_demo example. It uses a similar mechanism to render as used
by the Scene3D plugin and so serves as a useful prototyping and test environment for investigating
rendering issues in QML applications without the complexity of the full ignition gui or plugin infrastructure.

The initial use for this example was to resolve issues around support of ignition guis on macOS (gazebosim/gz-gui#145).

Upstream dependencies

For macOS there is an upstream dependency on Ogre2 pending this PR: OGRECave/ogre-next#216 which adds support for the currentGLContext parameter when creating a render window.

Test it

The changes may be tested by building and running the example. The expected outcome is the window below showing the rendered ignition scene under a QML overlay (run on macOS Big Sur 11.4).

Checklist

• [x ] Signed all commits for DCO
• Added tests
• [x ] Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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

[iche033]

nice! About upgrading the version of ogre:

1. ignition uses a fork of ogre 2.1. In order to make use of this work, we'll need to pull the changes from [GL3+] add support for currentGLContext for macOS OGRECave/ogre-next#216 to our fork.
2. unfortunately I don't think we can just pull all the latest changes from ogre-next's v2-1 branch into our fork because of there are API changes, e.g. the ones that you had to make in this PR. If we do that, it'll break downstream users who use our ogre 2.1 packages.
3. From Ignition Fortress (ign-rendering6) onward, we will depend on ogre 2.2. Looks like with add GL3Plus support for macOS  OGRECave/ogre-next#209, we'll have GL3+ support on macOS. We can probably just update our ogre 2.2 fork to use the latest commit from the v2-2 branch once [GL3+] add support for currentGLContext for macOS OGRECave/ogre-next#216 is merged forward.

I think the best way forward for Edifice (ign-rendering5) and possibly Citadael (ign-rendering3) is to do 1. then make a new release of ogre 2.1 from our fork. @j-rivero is that do-able?

[srmainwaring]

1. ignition uses a fork of ogre 2.1. In order to make use of this work, we'll need to pull the changes from [GL3+] add support for currentGLContext for macOS OGRECave/ogre-next#216 to our fork.

I thought that would be the case but figured placing the Ogre2 PR against the head of the v2-1 branch would be the best way to get it accepted.

2. unfortunately I don't think we can just pull all the latest changes from ogre-next's v2-1 branch into our fork because of there are API changes, e.g. the ones that you had to make in this PR. If we do that, it'll break downstream users who use our ogre 2.1 packages.

I can rebase the PR against the Ogre2.1 commit used in the osrf/simulation formula for ogre2.1 if that makes it easier, or you can edit my branch if you prefer?

[iche033]

I thought that would be the case but figured placing the Ogre2 PR against the head of the v2-1 branch would be the best way to get it accepted.

yeah that's fine. Once the PR is accepted upstream, we can pull the commit into our fork.

I can rebase the PR against the Ogre2.1 commit used in the osrf/simulation formula for ogre2.1 if that makes it easier, or you can edit my branch if you prefer?

yes, that'll be easier for us to get this in, thanks!

[ahcorde]

Thank you for the contribution! I made a first pass

[srmainwaring]

@ahcorde thanks for your review. I've made the changes you suggested and pushed them.

I still need to rebase on the branch of Ogre2.1 used by ignition-rendering5 which will eliminate some of the other changes not directly related to the example.

[iche033]

OGRECave/ogre-next#216 is merged. I just opened a pull request to build our ogre 2.1 bottle with that patch:
osrf/homebrew-simulation#1555

[iche033]

OGRECave/ogre-next#216 is merged. I just opened a pull request to build our ogre 2.1 bottle with that patch:
osrf/homebrew-simulation#1555

looks like the patch failed because we're using an older commit of ogre 2.1 and there have been quite a few changes to the osx render system in the 2.1 branch since then.

I still need to rebase on the branch of Ogre2.1 used by ignition-rendering5 which will eliminate some of the other changes not directly related to the example.

Were you able to get this working with our version of ogre 2.1?

[srmainwaring]

Were you able to get this working with our version of ogre 2.1?

Yes, the other changes on the current version of Ogre2.1 were not specific to macOS. If you let me know the commit that you'd like me to rebase on I'll get the PR updated.

Having reviewed the Gazebo Scene3D plugin code in more detail I see that there is a synchronisation system in place to prevent a race condition between QML and Ogre. I've not implemented this in the demo, which does not seem to matter (mostly), probably because the window for contention is very small as explained in the dev notes by @darksylinc: Race condition between Qt and Ogre2 in presentation #304 and Fix race condition when rendering the UI #774 . It might be useful to add this in any case to make the example better reflect what happens in the main app.

The other issue I noticed when running the example on an Ubuntu VM hosted on macOS, is that I cannot necessarily rely on the scene graph calling updatePaintNode from the main thread - as this depends upon the value assigned to QSG_RENDER_LOOP. The default on macOS is QSG_RENDER_LOOP=basic so the example works, but it will fail if set to QSG_RENDER_LOOP=threaded.

@iche033's suggestion here of using event callbacks may provide a solution in this case as well.

[darksylinc]

probably because the window for contention is very small as explained in the dev notes by @darksylinc

Just to clear something up: Gazebo was rendering in two threads (Qt in one thread, Ogre in another) without proper synchronization. While "proper" synchronization is possible via a double buffer scheme; there were many difficulties so the solution we ended up implementing was brute force: serialize everything (i.e. thread A can't run while B is running, thread B can't run while A is running).

Such a simple concept ended up being more complex than we'd want to because of the signal/slot system Qt was using to communicate between threads; which could easily end up in either Deadlock or not serializing the threads as intended. But ultimately all the code is trying to do is to simply have A -> B -> A -> B executing as if it were just one thread.

[iche033]

Yes, the other changes on the current version of Ogre2.1 were not specific to macOS. If you let me know the commit that you'd like me to rebase on I'll get the PR updated.

our ogre 2.1 version is on this commit: b4c4fa785c03c2d4ba2a1d28d94394c7ca000358. Let me know if you're able to get a patch with your changes that can be applied on top of this particular ogre commit, and I'll apply that to our homebrew formula. Thanks!

[codecov]

Codecov Report

Merging #373 (aacc5ec) into ign-rendering5 (6dc5b81) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##           ign-rendering5     #373      +/-   ##
==================================================
- Coverage           57.77%   57.77%   -0.01%     
==================================================
  Files                 161      161              
  Lines               15947    15947              
==================================================
- Hits                 9214     9213       -1     
- Misses               6733     6734       +1     

Impacted Files	Coverage Δ
ogre2/src/Ogre2RenderEngine.cc	78.57% <ø> (ø)
...e/ignition/rendering/base/BaseGaussianNoisePass.hh	96.66% <0.00%> (-3.34%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dc5b81...aacc5ec. Read the comment docs.

[srmainwaring]

@iche033 this turns out to be a bit trickier.

macOS

Applying just the OpenGL shared context change 79be9e99 to the OSRF ogre2.1 version is not enough. The simple_demo_qml example will build, however there is a segfault deep within some of the Ogre OpenGL buffer management code. It's not clear to me which of the changes over the last three years on the v2.1 branch resolves this.

linux

On an Ubuntu 20.04 virtual machine the simple_demo_qml example does run correctly when linked against the OSRF distributed Ogre2.1 libraries. There are a number of additional settings required to get OpenGL hardware acceleration working with a macOS host, but it does work. I have not tested on a machine running Ubuntu natively.

Proposal

In order to move this along I suggest the following:

• Rebase the PR to link to this version of Ogre2.1 b4c4fa78, so there are no changes required upstream for linux, and the demo will build on macOS (but not run correctly).
• Remove all extraneous changes to other examples.
• Squash all the commits and tidy up the commit message [optional - but let me know if required]
• Complete the review and merge the PR

Then as a follow up we can deal with macOS. I suspect to support Ignition Edifice GUI apps on macOS we'll need to incorporate many of the changes on the ogre-next v2-1 branch (up to v2-1-2). @darksylinc may be able to advise if a smaller subset of changes will suffice - I'm not sure where to begin!

I don't think there is an advantage in releasing a new version of ogre2.1 on homebrew unless all the GUI functionality is available.

[iche033]

hi, thanks for looking into it. It does look like there have been quite a few changes to the osx render system since 2018:
https://github.com/OGRECave/ogre-next/commits/v2-1/RenderSystems/GL3Plus/src/windowing/OSX. I noticed one of these commits also include API deprecation / change, e.g. copyContentsToMemory, so ideally we'll just need to apply a minimal subset that's sufficient to make our 2.1 version work on macOS.

Squash all the commits and tidy up the commit message [optional - but let me know if required]

yeah it's optional. We'll typically squash the commits when merging this PR.

Agree to get this PR in first then follow-up on macOS afterwards. I'll continue reviewing this PR as is to make sure it works on linux,

[iche033]

Did some quick tests. The example built fine but ran into a segfault on launch. I see these error msgs in the console:

QObject::moveToThread: Current thread (0x5594874796b0) is not the object's thread (0x5594871ef5c0).
Cannot move to target thread (0x55948734d5d0)

Cannot make QOpenGLContext current in a different thread
Aborted (core dumped)

backtrace points to this line:
https://github.com/ignitionrobotics/ign-rendering/blob/96d2444a3df28cb8655d823bb8c581e2cf77a84d/examples/simple_demo_qml/ThreadRenderer.cpp#L224

I'm on Ubuntu Bionic, Qt 5.9.5

[srmainwaring]

Did some quick tests. The example built fine but ran into a segfault on launch.

Ah yes, the current version needs the Qt Quick Scene Graph to use the basic render loop:

export QSG_RENDER_LOOP=basic 

should get it working, but I think a better approach is needed.

[iche033]

ah yes thanks, that worked! I should've read your earlier comment about the env variable. I think this may not be obvious to the user. We could either add a README.md file with instructions on how to run the demo. Alternatively, just set the env for them with this patch:

diff --git a/examples/simple_demo_qml/Main.cpp b/examples/simple_demo_qml/Main.cpp
index c749f2c9..120bb0b0 100644
--- a/examples/simple_demo_qml/Main.cpp
+++ b/examples/simple_demo_qml/Main.cpp
@@ -28,6 +28,9 @@ int main(int _argc, char** _argv)
 {
     try
     {
+        // use single-threaded scene graph rendering
+        qputenv("QSG_RENDER_LOOP", "basic");
+
         // requested surface format
         QSurfaceFormat format = RenderThread::CreateSurfaceFormat();
         QSurfaceFormat::setDefaultFormat(format);

[iche033]

overall this looks good to me.

While we don't run linters or enforce coding style in examples directory, it's encouraged so we can keep the style consistent with the rest of the ignition library. Here are some suggestions for files that are not adapted from Qt:

• 2 space indentation
• wrap line to 80 chars
• use .hh and .cc file extensions
• function separator style: //////////////////////////////////////////////////

I'm also happy to do this in a separate PR afterwards if you don't mind me changing the coding style. We can also leave the style as is if you prefer.

[iche033]

hmm interesting. I remember this was added for a reason but looks like our CI homebrew builds are passing with this removed so this maybe ok.

[srmainwaring]

This will definitely break running under macOS if left in. The brew installer places the plugin libs only in the sub-directory and the plugin loader raises an (subsequently uncaught) exception if not.

[darksylinc]

If you run clang format with this file I'm attaching it should be pretty close to gazebo's style.
clang-format.zip

Things it does not cover:

• Naming conventions (e.g. function arguments start with _underscore)
• Member variable accessing convention, i.e. all member variables must be accessed via via this, for example this->myVariable
• Access declaration should be in the same line. Clang format will insist in the following:

public:
  int myVariable;
  int myVariable2;

while gazebo enforces the following for each variable:

  public: int myVariable;
  public: int myVariable2;

• Adding ////////////////////////////////////////////////// between every function.
• Space between C++ brackets. i.e. Gazebo wants int variable{5} clang-format will do: int variable{ 5 }

However it will enforce the 80 line columns, it will also enforce spaces between conditional control keyword, e.g. if (condition) and 2 space indentation

[srmainwaring]

I'm also happy to do this in a separate PR afterwards if you don't mind me changing the coding style. We can also leave the style as is if you prefer.

Happy for you to reformat to match house style.

[srmainwaring]

Alternatively, just set the env for them with this patch:

I like this, it keeps the env variable local and explains why it is there. I'd like to rework the initialisation in a follow up so its not needed - the same approach can then be used for the Scene3D plugins and sensor system that need similar changes to support macOS.

[srmainwaring]

It does look like there have been quite a few changes to the osx render system since 2018:
https://github.com/OGRECave/ogre-next/commits/v2-1/RenderSystems/GL3Plus/src/windowing/OSX

@iche033 the branch feature/v2-1-osrf should match the commit used in osrf/simulation for the ogre2.1 formula.

The branch feature/v2-1-osrf-macos contains a cherry-picked set of commits extracted from the v2-1 branch and applied to feature/v2-1-osrf.

These changes should be sufficient to support ign-rendering including simple_demo_qml on macOS without breaking the interface (it will also support ign-gui and ign-gazebo when combined with additional macOS changes to those repos).

You may want to omit the last commit on feature/v2-1-osrf-macos, I've added it because it's needed for me to build on macOS Big Sur without any of the additional patches in the osrf/simulation/ogre2.1.rb formula. It also includes a bash script that I use to install the ogre libraries into /usr/local/Cellar and then brew link them following the side-by-side installation approach used in the formula.

[iche033]

great, thanks for working through those commits. I'll give your branch a try later this week / early next week.

[iche033]

the feature/v2-1-osrf-macos branch without the last commit worked for me locally (on ubuntu). I've updated the homebrew formula with a new patch: https://github.com/ignition-forks/ogre-2.1-release/compare/b4c4fa785c03c2d4ba2a1d28d94394c7ca000358..81632330e3ab041345c7fa1075022cf6af30c658

Now just waiting for new bottles to be built 🤞

[srmainwaring]

Now just waiting for new bottles to be built 🤞

Just updated brew, reinstalled ogre2.1 and rebuilt ign-rendering. This demo working as are my local changes to support ignition gazebo 👍

[iche033]

awesome, thanks for verifying that the bottle works! I'm going to merge this and will update the coding style in a separate pull request.

[iche033]

@ahcorde, just went through your comments, I think most of them have been addressed. Some style changes will come in follow-up PR.

address some style changes in a separate PR