Support for building with Shiboken2 in Engine #697
Conversation
YakoYakoYokuYoku
force-pushed
the
engine-sbk2
branch
from
ac00b96
to
ba44387
Compare
Engine/PyParameter.h
Outdated
| @@ -428,6 +428,7 @@ class Int2DParam | |||
|
|
|||
| Int2DTuple get() const; | |||
| Int2DTuple get(double frame) const; | |||
| using IntParam::set; | |||
There was a problem hiding this comment.
Both Int2DParam and Int3DParam require a function definition of get(int) (get(int, int) too for 3D) if not it will be shadowed, another way to solve this is by qualifying the method calls but is way harder to do so. See https://stackoverflow.com/questions/24342270/base-class-template-member-function-shadowed-in-derived-class-albeit-different
There was a problem hiding this comment.
Why do we need these functions? I think it's expected to have them shadowed, and I don't see what this has to do with this PR
There was a problem hiding this comment.
If hiding is expected then Shiboken2 was doing the very nasty thing of ignoring the shadowing and generating the get(int) where it wasn't supposed to be. IIRC these kind of exclusions can be done in the typesystem.
There was a problem hiding this comment.
Then I would say a proper solution would be to provide explicit implementations that throw an exception. Don't you think so?
There was a problem hiding this comment.
Sounds like it's the most sanish way to do so. One way could be immediately throwing a mismatched dimension error. Another putting those methods under a private field although at the cost of reporting a wrong error.
Engine/PyParameter.h
Outdated
| @@ -444,6 +445,8 @@ class Int3DParam | |||
|
|
|||
| Int3DTuple get() const; | |||
| Int3DTuple get(double frame) const; | |||
| using IntParam::set; | |||
| @@ -39,10 +39,8 @@ CLANG_DIAG_OFF(mismatched-tags) | |||
| GCC_DIAG_OFF(unused-parameter) | |||
| GCC_DIAG_OFF(missing-field-initializers) | |||
| #include <basewrapper.h> | |||
| #include <conversions.h> | |||
There was a problem hiding this comment.
use #if/#ifdef, please don't remove
There was a problem hiding this comment.
The FreeCAD codebase uses #if/#ifdef for the Python wrapper, gonna abstain the removals and use the preprocessor instead.
There was a problem hiding this comment.
OK!
also, I would remommend putting shiboken-generated files and shiboken2 generated files in two separate directories, so that we can check in both in the sources. That would be great
There was a problem hiding this comment.
Problem is that the Engine.pro is gonna need a good amount of tinkering, cannot get any idea to what to do here
| @@ -17,6 +17,9 @@ | |||
| * along with Natron. If not, see <http://www.gnu.org/licenses/gpl-2.0.html> | |||
| * ***** END LICENSE BLOCK ***** */ | |||
|
|
|||
| //Defined to avoid including some headers when running shiboken which may crash shiboken (particularly boost headers) | |||
There was a problem hiding this comment.
why move this here? this should be inside the include guard
There was a problem hiding this comment.
I don't know exactly yet but Shiboken (2 and 6) does something strange that it makes "two passes" over this file, and at the second "pass" it ignores the SBK_RUN definition. Will see if something can be done instead.
| rowVec[j] = ret; | ||
| } | ||
| } else if (PyUnicode_Check(pyString) ) { | ||
| if (PyUnicode_Check(pyString) ) { |
There was a problem hiding this comment.
instead of removing,
- replace PyString_Check by PyBytes_Check
- replace PyString_AsString by PyObject_Bytes
- put the if ( PyBytes_Check(pyString) ) { section after the if (PyUnicode_Check(pyString) section
There was a problem hiding this comment.
(this is to keep python2 compatibility)
There was a problem hiding this comment.
Gonna follow PEP-393 by adapting what you've suggested. May be better for Shiboken to be smart enough for #if PY_MAJOR_VERSION > 3/#else.
There was a problem hiding this comment.
I think the PyBytes_Check works both in python2 and 3 (PyString_Check disappeared from Python3), so that makes the code 2/3 compatible. Are you suggesting it should be done differently? There is similar code at several places in Natron
There was a problem hiding this comment.
Luckily enough one call to PyUnicode_Check is needed to check if the type is suitable for Python strings, according to the official docs. Although the PyBytes_Check method works too.
| @@ -28,11 +28,6 @@ CLANG_DIAG_OFF(deprecated) | |||
| CLANG_DIAG_ON(deprecated) | |||
|
|
|||
| NATRON_NAMESPACE_ENTER | |||
| #ifdef SBK_RUN | |||
There was a problem hiding this comment.
Newer C++ implementations let the preprocessor to expand the name of a namespace if it was defined. Wasn't portable to begin with.
| @@ -684,11 +679,6 @@ enum MergingFunctionEnum | |||
| //typedef QFlags<StandardButtonEnum> StandardButtons; | |||
| Q_DECLARE_FLAGS(StandardButtons, StandardButtonEnum) | |||
|
|
|||
| #ifdef SBK_RUN | |||
| @@ -66,10 +66,10 @@ CLANG_DIAG_ON(deprecated) | |||
| //#define foreach Q_FOREACH | |||
|
|
|||
|
|
|||
| typedef boost::uint32_t U32; | |||
There was a problem hiding this comment.
unrelated change. file a different PR if you think this is useful
There was a problem hiding this comment.
The later versions of Shiboken were ignoring the boost::*_t types. Given that C++11 has fixed width integers it compiled without nuisance. Can remain here in the case of being accepted, otherwise I can file a different PR.
| @@ -41,8 +41,6 @@ run-without-python { | |||
| # from <https://docs.python.org/3/c-api/intro.html#include-files>: | |||
| # "Since Python may define some pre-processor definitions which affect the standard headers on some systems, you must include Python.h before any standard headers are included." | |||
| CONFIG += python | |||
| QMAKE_CFLAGS += -include Python.h | |||
There was a problem hiding this comment.
Was giving me build errors to preinclude that file. If the current Natron build can do it without these flags, which I think it can and it is already included in many places of the codebase where it's needed.
There was a problem hiding this comment.
Thank you!
I would prefer if the code could be as compatible as possible with QT4/PySide during the transition.
Could you please split that into:
- a PR that fixes Qt5-related stuff, making sure it doesn't break Qt4 support (#if QT_VERSION >= QT_VERSION_CHECK(5, 0, 0) is your friend)
- a PR that adds PySide2/Shiboken2 support, making sure it doesn't break PySide/Shiboken support (same)
Similarly, we try to keep python2/python3 compat during the transition (see the PyBytes_Check comment)
|
Btw we're looking for a volunteer to setup github actions for CI (since travis is gone). That would be a good way to test the Qt4|Qt5/Py2|Py3 matrix in CI. |
You are welcome too.
Although no Qt5 is used (besides obviously PySide2) in this PR, I'll make sure to do the split in #698.
Sounds hella easy to do. |
|
Added a shiboken2 --enable-parent-ctor-heuristic --use-isnull-as-nb_nonzero --avoid-protected-hack \
--include-paths=.:Global:libs/OpenFX/include:/usr/include/PySide2:/usr/include/python3.9 \
--typesystem-paths=/usr/share/PySide2/typesystems --output-directory=Engine \
Global/Pyside_Shiboken2_Macros.h Engine/Pyside_Engine_Python.h Engine/typesystem_engine.xml |
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:
PR Description
What type of PR is this? (Check one of the boxes below)
What does this pull request do?
This is a partial attempt to bring support for Qt5+/Shiboken2+/PySide2+ support in Natron as Qt4 was deprecated by upstream. Although the Qt5 series are reaching End Of Support too this changes nonetheless may permit an smoother transition to Qt6.
In order to make the PR easier for review the
Engine/NatronEnginechanges have yet to be uploaded, although it can be done viashiboken2generation as described in theINSTALL_LINUX.mddoc.Show a few screenshots (if this is a visual change)
Have you tested your changes (if applicable)? If so, how?
I've followed this tutorial and rendered it successfully.
An even more rigorous testing is needed for the sake of coverage and to make sure things are ok.
Built under Solus 4.3.
Futher details of this pull request
Used configuration.
Example command output (TODO: notice the Python scripts).