Support for building with Shiboken2 in Gui

[YakoYakoYokuYoku]

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

• I've followed the contributing guidelines to the best of my understanding
• I've read and understood the contributing guidelines
• I've formatted my code according to Natron's code style
• I've searched the pull requests tracker to ensure that this PR is not a duplicate

PR Description

What type of PR is this? (Check one of the boxes below)

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
• Breaking change (fix or feature that would cause existing functionality to change)
• My change requires a change to the documentation
  • I have updated the documentation accordingly

What does this pull request do?

See the dependant PR. Depends on #697.

Show a few screenshots (if this is a visual change)

N/A yet.

Have you tested your changes (if applicable)? If so, how?

See the dependant PR.

Futher details of this pull request

See the dependant PR.

[devernay]

Hi,

After merging the Engine PR, I had to re-add ENgine/NatronEngine/natron_namespace_wrapper, because init_NATRON_NAMESPACE() was missing.

My suggestion:

• use separate dirs for shiboken/shiboken2/shiboken6 generated files. we'll check in all generated files with the sources. let's Use Engine/NatronEngine, Engine/NatronEngine2, Engine/NatronEngine6 for example
• either use different QMake configs for shiboken/shiboken2/shiboken6, or add the sources&headers for shiboken-generated files conditionally in each .pro, using isEqual(QT_MAJOR_VERSION, 4), isEqual(QT_MAJOR_VERSION, 5) or isEqual(QT_MAJOR_VERSION, 6)
• you can make a PR with all shiboken-generated files once this one is merged (for easier review)

Could you please make these changes in this PR?

[YakoYakoYokuYoku]

In reality I wasn't expecting to get the changes in Engine merged that soon, though #704 ties the loose ends and builds with the path of Engine/NatronEngine5. This PR can also do the same as well too with Gui/NatronGui5. After this gets merged I'll try to add Qt5 + more OSes to the GH build workflow if desired.

[devernay]

RB-2.5 is still WIP, so I'd like to integrate your source code changes ASAP, making sure they don't break Qt4/PySide. It's OK if we break the build a few times in the process (and that would be nice to have at least working github actions in place to do early checks).

The goal is to release 2.5 with Qt4/PySide/Python3, with your non-breaking changes for Qt5/PySide2 integrated in the source code.

Then we will hopefully be able to start working on 2.6 with Qt5/PySide2/Python3. There may be some platform-dependent graphical stuff to fix. the master branch also builds with Qt5 and has a few graphical goodies that we may want to backport.

[YakoYakoYokuYoku]

Due to the merge of #704 there are no more blockers for this PR, marking as ready.

[devernay]

I had forgotten to post my comment above, can you check that?

[devernay]

Thank you very much for your work. It seems like you managed to add shiboken2/pyside2 support with very little sourcecode change, and I'm very grateful for that.

[devernay]

Did you ever try to build the "master" branch? I don't even know if it still builds, but you may want to look at the UI to see if there are UI/UX bits and improvements that we would want to backport.

The master branch is pretty much dead, because the render engine is very buggy and unreliable. And too much code was moved around/refactored to bring back the Natron 2 render engine (which has some bugs and limitations, but seems more reliable overall).

[YakoYakoYokuYoku]

Never took a look at the master branch due to my interest over porting Qt5/PySide2, dunno if scrambling around with that will lead to a trove.

[YakoYakoYokuYoku]

@devernay any further blockers?

[devernay]

I didn't have time to check, sorry. Is shiboken 1 still working with this PR? Did you get a chance to check?

[YakoYakoYokuYoku]

It generated with old Shiboken as well, but it had obviously needed to pass a typesystem location flag (--typesystem-path=Shiboken).

[devernay]

OK, I am testing with Qt4/shiboken.

I have an issue with the previous PR (the Engine one): did you have a specific reason for changing the namespace from "NATRON_NAMESPACE" to "Natron" in the typesystem? This removes all methods that use enums. Switching back to NATRON_NAMESPACE fixes that. Will it break for shiboken2?

And a second issue: Engine/NatronEngine/natron_namespace_wrapper.cpp and Engine/NatronEngine/natron_namespace_wrapper.h are not generated in RB-2.5. They were generated correctly in RB-2.4. Can you help me solve that? I don't understand why this happens.

[devernay]

ok that's because of the changes you did to Global/Enums.h

Were they necessary and why?

[YakoYakoYokuYoku]

The namespace changes were introduced due to issues with the enum generation. Due to the usage of NATRON_NAMESPACE as the enum namespace name and NATRON_NAMESPACE being defined to empty the enums of Global/Enums.h ended up being in an anonymous ns rather than NATRON_NAMESPACE or Natron, resulting in the omission of their generation. Because of that I've needed to be explicit with the namespace in #704. After that PR was merged the natron_namespace_wrapper.<cpp/h> was substituted by natron_wrapper.<cpp/h>, at least for Shiboken2.

[devernay]

The namespace changes were introduced due to issues with the enum generation.

Isn't that because you also did that change in the typesystem?

We can't affort to break shiboken1 for now, so I'll have to put back NATRON_NAMESPACE both in Enum.h and typesystem_engine.xml

Also can you confirm that the inject-code section is not necessary?

[YakoYakoYokuYoku]

Isn't that because you also did that change in the typesystem?

The issue with the enums not being generated was happening even before #697. Shiboken1 during its run doesn't expand NATRON_NAMESPACE in Global/Enums.h to empty, resulting with all the enums being placed under it (e.g. NATRON_NAMESPACE::AnimationParamEnum) and found in the typesystem, Shiboken2 and onwards does the expansion and the enums end up in the anonymous namespace, which results in the enums not being found in the typesystem because they were inside <namespace-type name="NATRON_NAMESPACE">.

Also can you confirm that the inject-code section is not necessary?

The code injection section is necessary, although the same could be achieved with runPostShiboken2.sh.

[devernay]

Enums are correctly generated using shiboken1 (I just reverted the relevant changes so that it works as in RB-2.4)/

The shiboken2 solution should not break shiboken1

[devernay]

If namespaces work fine in shiboken2, why not simply enable these on shiboken2 in Global/Macros.h? Is there a shiboken2-specifc define?

[YakoYakoYokuYoku]

If namespaces work fine in shiboken2, why not simply enable these on shiboken2 in Global/Macros.h? Is there a shiboken2-specific define?

Namespaces wouldn't work just fine in shiboken2 as they only support enums and because of that the rest of classes and methods cannot be namespaced. To begin with I'd say that the enums must be placed in a explicit namespace and not a define alias, so that we do not rely on many hacks after generation. In fact, almost all of the Qt namespaced enums of QtCore are placed under the Qt namespace in PySide1.

[devernay]

the enums are in a namespace, see https://github.com/NatronGitHub/Natron/blob/RB-2.5/Global/Enums.h#L32

[devernay]

I'm working on a solution (using a fake namespace just for enums)

[devernay]

@YakoYakoYokuYoku please review #713

[devernay]

Can you rebase & fix if necessary?
Please check that all methods in pypanel are generated

[YakoYakoYokuYoku]

Please check that all methods in pypanel are generated

The only changes were a couple of methods relocations and the NATRON_NAMESPACE to NATRON_ENUM for the enums.
I've used the following commands to generate the bindings again.

$ shiboken --avoid-protected-hack --enable-pyside-extensions \
    --include-paths=../Engine:../Global:/usr/include:/usr/include/qt4:/usr/include/PySide \
    --typesystem-paths=/usr/share/PySide/typesystems --output-directory=Engine \
    Engine/Pyside_Engine_Python.h Engine/typesystem_engine.xml
$ shiboken --avoid-protected-hack --enable-pyside-extensions \
    --include-paths=../Engine:../Gui:../Global:/usr/include:/usr/include/qt4:/usr/include/PySide \
    --typesystem-paths=/usr/share/PySide/typesystems:Engine:Shiboken --output-directory=Gui \
    Gui/Pyside_Gui_Python.h Gui/typesystem_natronGui.xml
$ ./tools/utils/runPostShiboken.sh

[devernay]

Just get rid of that "using namespace" in a header file, and we're good

[devernay]

Thank you!
Bonus question: do you know if there's a way to run shiboken so that it always generates the bindings in the same order?

[YakoYakoYokuYoku]

For Shiboken1 I have absolutely no idea, Shiboken2 and Shiboken6 do generate the bindings always in alphabetical order.

[devernay]

ok thanks!

[YakoYakoYokuYoku]

As a heads-up I'll start submitting for reviews, in the span from days to weeks, changes to make Qt API usages compatible with the 5.15 LTS release with the possibility of 6 support and the deprecation of the old usages.

[devernay]

what's your shiboken2 command? I can't get shiboken2 to find QDialog on macOS

[devernay]

and can you please push the shiboken2-generated bindings (no need to PR)?

[YakoYakoYokuYoku]

Forgot to post it...

$ shiboken2 --enable-parent-ctor-heuristic --use-isnull-as-nb_nonzero --avoid-protected-hack \
      --include-paths=.:Global:Engine:libs/OpenFX/include:/usr/include/PySide2:/usr/include/QtCore \
      --include-paths=/usr/include/QtGui:/usr/include/QtWidgets:/usr/include/python3.9 \
      --typesystem-paths=Engine:/usr/share/PySide2/typesystems --output-directory=Gui/Qt5 \
      Gui/PySide2_Gui_Python.h Gui/typesystem_natronGui.xml

There you go, although that all under both --include-paths flags was originally under a single one, posted like due to GH not wrapping lines.

[devernay]

no need for --enable-pyside-extensions?

[YakoYakoYokuYoku]

no need for --enable-pyside-extensions?

They are going to be needed if we want to use features like signal and slots in the Python scripts, so I'd pass the flag unless is found to be completely unnecessary.

[YakoYakoYokuYoku]

and can you please push the shiboken2-generated bindings (no need to PR)?

I'll have to PR due to the RB-2.5 branch is protected and if I want to push I need the review approved first.