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.

Sign up for GitHub

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

Jump to bottom

Support for building with Shiboken2 in Engine #697

Merged
merged 2 commits into from Nov 7, 2021
Merged

Support for building with Shiboken2 in Engine #697

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba44387
Shiboken2 for NatronEngine
YakoYakoYokuYoku Nov 3, 2021
7a6b160
Preprocessor suggestions
YakoYakoYokuYoku Nov 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions Engine/AppManager.h
View file
Open in desktop
Expand Up @@ -44,11 +44,9 @@ CLANG_DIAG_ON(deprecated)
#include <QtCore/QProcess>
#include <QtCore/QMap>

#if !defined(Q_MOC_RUN) && !defined(SBK_RUN)
YakoYakoYokuYoku marked this conversation as resolved.
Show resolved Hide resolved
#include <boost/scoped_ptr.hpp>
#include <boost/shared_ptr.hpp>
#include <boost/noncopyable.hpp>
#endif

#include "Engine/AfterQuitProcessingI.h"
#include "Engine/Plugin.h"
Expand Down
2 changes: 0 additions & 2 deletions Engine/Engine.pro
View file
Open in desktop
Expand Up @@ -238,7 +238,6 @@ SOURCES += \
NatronEngine/intparam_wrapper.cpp \
NatronEngine/itembase_wrapper.cpp \
NatronEngine/layer_wrapper.cpp \
NatronEngine/natron_namespace_wrapper.cpp \
NatronEngine/natronengine_module_wrapper.cpp \
NatronEngine/nodecreationproperty_wrapper.cpp \
NatronEngine/outputfileparam_wrapper.cpp \
Expand Down Expand Up @@ -383,7 +382,6 @@ HEADERS += \
PyParameter.h \
PyRoto.h \
PyTracker.h \
Pyside_Engine_Python.h \
ReadNode.h \
RectD.h \
RectDSerialization.h \
Expand Down
2 changes: 0 additions & 2 deletions Engine/ImagePlaneDesc.h
View file
Open in desktop
Expand Up @@ -31,7 +31,6 @@
#include <string>
#include <vector>

#if !defined(Q_MOC_RUN) && !defined(SBK_RUN)
YakoYakoYokuYoku marked this conversation as resolved.
Show resolved Hide resolved
GCC_DIAG_UNUSED_LOCAL_TYPEDEFS_OFF
GCC_DIAG_OFF(unused-parameter)
// /opt/local/include/boost/serialization/smart_cast.hpp:254:25: warning: unused parameter 'u' [-Wunused-parameter]
Expand All @@ -41,7 +40,6 @@ GCC_DIAG_OFF(unused-parameter)
#include <boost/serialization/version.hpp>
GCC_DIAG_UNUSED_LOCAL_TYPEDEFS_ON
GCC_DIAG_ON(unused-parameter)
#endif

#define IMAGEPLANEDESC_SERIALIZATION_INTRODUCES_ID 2
#define IMAGEPLANEDESC_SERIALIZATION_VERSION IMAGEPLANEDESC_SERIALIZATION_INTRODUCES_ID
Expand Down
2 changes: 0 additions & 2 deletions Engine/Knob.h
View file
Open in desktop
Expand Up @@ -33,10 +33,8 @@
#include <set>
#include <map>

#if !defined(Q_MOC_RUN) && !defined(SBK_RUN)
YakoYakoYokuYoku marked this conversation as resolved.
Show resolved Hide resolved
#include <boost/enable_shared_from_this.hpp>
#include <boost/scoped_ptr.hpp>
#endif

#include <QtCore/QReadWriteLock>
#include <QtCore/QMutex>
Expand Down
3 changes: 3 additions & 0 deletions Engine/PyParameter.h
View file
Open in desktop
Expand Up @@ -428,6 +428,7 @@ class Int2DParam

Int2DTuple get() const;
Int2DTuple get(double frame) const;
using IntParam::set;
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Then I would say a proper solution would be to provide explicit implementations that throw an exception. Don't you think so?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

void set(int x, int y);
void set(int x, int y, double frame);
};
Expand All @@ -444,6 +445,8 @@ class Int3DParam

Int3DTuple get() const;
Int3DTuple get(double frame) const;
using IntParam::set;
Copy link
Member

Choose a reason for hiding this comment

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

why?

using Int2DParam::set;
void set(int x, int y, int z);
void set(int x, int y, int z, double frame);
};
Expand Down
2 changes: 0 additions & 2 deletions Engine/PySideCompat.cpp
View file
Open in desktop
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

use #if/#ifdef, please don't remove

Copy link
Member Author

Choose a reason for hiding this comment

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

The FreeCAD codebase uses #if/#ifdef for the Python wrapper, gonna abstain the removals and use the preprocessor instead.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem is that the Engine.pro is gonna need a good amount of tinkering, cannot get any idea to what to do here

#include <sbkconverter.h>
#include <gilstate.h>
#include <typeresolver.h>
YakoYakoYokuYoku marked this conversation as resolved.
Show resolved Hide resolved
#include <bindingmanager.h>
CLANG_DIAG_ON(mismatched-tags)
GCC_DIAG_ON(unused-parameter)
Expand Down
8 changes: 4 additions & 4 deletions Engine/Pyside_Engine_Python.h
View file
Open in desktop
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

why move this here? this should be inside the include guard

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

#define SBK_RUN

#ifndef PYSIDE_ENGINE_PYTHON_H
#define PYSIDE_ENGINE_PYTHON_H

Expand All @@ -33,10 +36,7 @@
* Do not include it when compiling Natron.
**/

//Defined to avoid including some headers when running shiboken which may crash shiboken (particularly boost headers)
#define SBK_RUN

#include <pyside_global.h>
#include <pyside2_global.h>
YakoYakoYokuYoku marked this conversation as resolved.
Show resolved Hide resolved
#include <string>
//Global
#include <GlobalDefines.h>
Expand Down
22 changes: 9 additions & 13 deletions Engine/typesystem_engine.xml
View file
Open in desktop
Expand Up @@ -18,10 +18,13 @@
- along with Natron. If not, see <http://www.gnu.org/licenses/gpl-2.0.html>
- ***** END LICENSE BLOCK ***** -->
<typesystem package="NatronEngine">


<inject-code class="native" position="beginning">
NATRON_NAMESPACE_USING NATRON_PYTHON_NAMESPACE_USING
</inject-code>

<!--Load QtCore typesystem-->
<load-typesystem name="typesystem_core.xml" generate="no" />

<!--Primitives-->
<primitive-type name="bool"/>
<primitive-type name="double"/>
Expand Down Expand Up @@ -209,8 +212,8 @@
</conversion-rule>
</container-type>

<!--Natron global enums-->
<namespace-type name="NATRON_NAMESPACE">
<!--Natron global enums
<namespace-type name="NATRON_NAMESPACE">-->
<enum-type name="StatusEnum"/>
<enum-type name="MergingFunctionEnum"/>
<enum-type name="StandardButtonEnum" flags="StandardButtons"/>
Expand All @@ -225,7 +228,7 @@
<enum-type name="OrientationEnum"/>
<enum-type name="PlaybackModeEnum"/>
<enum-type name="DisplayChannelsEnum"/>
</namespace-type>
<!--</namespace-type>-->


<object-type name="RectI">
Expand Down Expand Up @@ -1506,14 +1509,7 @@

for (int j = 0; j &lt; subSize; ++j) {
PyObject* pyString = PyList_GET_ITEM(subList,j);
if ( PyString_Check(pyString) ) {
char* buf = PyString_AsString(pyString);
if (buf) {
std::string ret;
ret.append(buf);
rowVec[j] = ret;
}
} else if (PyUnicode_Check(pyString) ) {
if (PyUnicode_Check(pyString) ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(this is to keep python2 compatibility)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

PyObject* utf8pyobj = PyUnicode_AsUTF8String(pyString); // newRef
if (utf8pyobj) {
char* cstr = PyBytes_AS_STRING(utf8pyobj); // Borrowed pointer
Expand Down
10 changes: 0 additions & 10 deletions Global/Enums.h
View file
Open in desktop
Expand Up @@ -28,11 +28,6 @@ CLANG_DIAG_OFF(deprecated)
CLANG_DIAG_ON(deprecated)

NATRON_NAMESPACE_ENTER
#ifdef SBK_RUN
Copy link
Member

Choose a reason for hiding this comment

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

why remove?

Copy link
Member Author

Choose a reason for hiding this comment

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

Newer C++ implementations let the preprocessor to expand the name of a namespace if it was defined. Wasn't portable to begin with.

// shiboken doesn't generate SbkNatronEngine_StandardButtonEnum_as_number unless it is put in a class or namespace
NATRON_NAMESPACE_EXIT
namespace NATRON_NAMESPACE {
#endif

enum ScaleTypeEnum
{
Expand Down Expand Up @@ -684,11 +679,6 @@ enum MergingFunctionEnum
//typedef QFlags<StandardButtonEnum> StandardButtons;
Q_DECLARE_FLAGS(StandardButtons, StandardButtonEnum)

#ifdef SBK_RUN
Copy link
Member

Choose a reason for hiding this comment

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

why remove?

}
NATRON_NAMESPACE_ENTER
#endif


NATRON_NAMESPACE_EXIT

Expand Down
8 changes: 4 additions & 4 deletions Global/GlobalDefines.h
View file
Open in desktop
Expand Up @@ -66,10 +66,10 @@ CLANG_DIAG_ON(deprecated)
//#define foreach Q_FOREACH


typedef boost::uint32_t U32;
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change. file a different PR if you think this is useful

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

typedef boost::uint64_t U64;
typedef boost::uint8_t U8;
typedef boost::uint16_t U16;
typedef std::uint32_t U32;
typedef std::uint64_t U64;
typedef std::uint8_t U8;
typedef std::uint16_t U16;


NATRON_NAMESPACE_ENTER
Expand Down
2 changes: 0 additions & 2 deletions global.pri
View file
Open in desktop
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

QMAKE_CXXFLAGS += -include Python.h
python3 {
PYV=3
PY_PKG_SUFFIX=-embed
Expand Down
15 changes: 8 additions & 7 deletions tools/utils/runPostShiboken.sh
View file
Open in desktop
Expand Up @@ -47,7 +47,7 @@ GCC_DIAG_OFF(uninitialized)\
GCC_DIAG_UNUSED_LOCAL_TYPEDEFS_OFF\
#include <shiboken.h> // produces many warnings@' -i'.bak' Engine/NatronEngine/*.cpp Gui/NatronGui/*.cpp

sed -e 's@// Extra includes@// Extra includes\
YakoYakoYokuYoku marked this conversation as resolved.
Show resolved Hide resolved
sed -e 's@// inner classes@// inner classes\
NATRON_NAMESPACE_USING NATRON_PYTHON_NAMESPACE_USING@' -i'.bak' Engine/NatronEngine/*.cpp Gui/NatronGui/*.cpp

# replace NATRON_NAMESPACE with Natron for enums with flags (e.g. StandardButtonEnum)
Expand All @@ -56,9 +56,10 @@ sed -e 's@"NatronEngine\.NATRON_NAMESPACE@"NatronEngine.Natron@g' -e 's@, Natron
# re-add the Natron namespace
#sed -e 's@" ::\([^s][^t][^d]\)@ NATRON_NAMESPACE::\1@g' -i'.bak' Engine/NatronEngine/*.cpp Engine/NatronEngine/*.h Gui/NatronGui/*.cpp Gui/NatronGui/*.h

sed -e 's@SbkType< ::@SbkType<NATRON_NAMESPACE::NATRON_PYTHON_NAMESPACE::@g' -e 's@SbkType<NATRON_NAMESPACE::NATRON_PYTHON_NAMESPACE::QFlags<@SbkType< ::QFlags<NATRON_NAMESPACE::NATRON_PYTHON_NAMESPACE::@g' -e's@NATRON_NAMESPACE::NATRON_PYTHON_NAMESPACE::Rect@NATRON_NAMESPACE::Rect@g' -i'.bak' Engine/NatronEngine/natronengine_python.h Gui/NatronGui/natrongui_python.h
sed -e '/AnimatedParam/ {:y;n;by};s@SbkType< ::\(.*\) >@SbkType<NATRON_NAMESPACE::\1 >@g' -i Engine/NatronEngine/natronengine_python.h
YakoYakoYokuYoku marked this conversation as resolved.
Show resolved Hide resolved
sed -e 's@SbkType< ::@SbkType<NATRON_NAMESPACE::NATRON_PYTHON_NAMESPACE::@g' -e 's@SbkType<NATRON_NAMESPACE::QFlags<@SbkType< ::QFlags<NATRON_NAMESPACE::@g' -e's@NATRON_NAMESPACE::NATRON_PYTHON_NAMESPACE::Rect@NATRON_NAMESPACE::Rect@g' -i'.bak' Engine/NatronEngine/natronengine_python.h Gui/NatronGui/natrongui_python.h
sed -e 's@^class @NATRON_NAMESPACE_ENTER NATRON_PYTHON_NAMESPACE_ENTER\
class @g' -e 's@^};@};\
class @g;T;{:y;/^};/! {n;by}};s@^};@};\
NATRON_PYTHON_NAMESPACE_EXIT NATRON_NAMESPACE_EXIT@g' -i'.bak' Engine/NatronEngine/*.h Gui/NatronGui/*.h

# replace NATRON_NAMESPACE::NATRON_NAMESPACE with NATRON_NAMESPACE in the enums wrappers
Expand All @@ -67,18 +68,18 @@ sed -e 's@NATRON_NAMESPACE::NATRON_PYTHON_NAMESPACE::NATRON_NAMESPACE@NATRON_NAM
sed -e 's@^#include <pysidemetafunction.h>$@CLANG_DIAG_OFF(header-guard)\
#include <pysidemetafunction.h> // has wrong header guards in pyside 1.2.2@' -i'.bak' Engine/NatronEngine/*.cpp Gui/NatronGui/*.cpp

sed -e 's@^#include <pyside_qtcore_python.h>$@CLANG_DIAG_OFF(deprecated)\
sed -e 's@^#include <pyside2_qtcore_python.h>$@CLANG_DIAG_OFF(deprecated)\
CLANG_DIAG_OFF(uninitialized)\
CLANG_DIAG_OFF(keyword-macro)\
#include <pyside_qtcore_python.h> // produces warnings\
#include <pyside2_qtcore_python.h> // produces warnings\
CLANG_DIAG_ON(deprecated)\
CLANG_DIAG_ON(uninitialized)\
CLANG_DIAG_ON(keyword-macro)@' -i'.bak' Engine/NatronEngine/*.cpp Gui/NatronGui/*.cpp Engine/NatronEngine/*.h Gui/NatronGui/*.h

sed -e 's@^#include <pyside_qtgui_python.h>$@CLANG_DIAG_OFF(deprecated)\
sed -e 's@^#include <pyside2_qtgui_python.h>$@CLANG_DIAG_OFF(deprecated)\
CLANG_DIAG_OFF(uninitialized)\
CLANG_DIAG_OFF(keyword-macro)\
#include <pyside_qtgui_python.h> // produces warnings\
#include <pyside2_qtgui_python.h> // produces warnings\
CLANG_DIAG_ON(deprecated)\
CLANG_DIAG_ON(uninitialized)\
CLANG_DIAG_ON(keyword-macro)@' -i'.bak' Engine/NatronEngine/*.cpp Gui/NatronGui/*.cpp Engine/NatronEngine/*.h Gui/NatronGui/*.h
Expand Down