Skip to content

appleseed.studio python module improvements#1578

Merged
est77 merged 3 commits intoappleseedhq:masterfrom
glebmish:studio-python-module
Aug 5, 2017
Merged

appleseed.studio python module improvements#1578
est77 merged 3 commits intoappleseedhq:masterfrom
glebmish:studio-python-module

Conversation

@glebmish
Copy link
Copy Markdown
Contributor

@glebmish glebmish commented Aug 5, 2017

Implemented wrapper over wrapinstance functions used in different Qt Python bindings to provide unified interface for them and complement Qt.py
Refactored build process to copy changed .py files to sandbox every build

set(module_path "${py_path}/${relative_module_path}")

add_custom_command (TARGET appleseed.python POST_BUILD
add_custom_command (TARGET appleseed.python.copy_py_files POST_BUILD
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you need to make the same changes in cmake/config/windows.txt and the one for OSX too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah yes, forgot about it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't really like the target name I chose. Maybe you have some ideas?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it's bad. I don't have anything better now.
It's an internal name that is not visible anywhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is, that's why I wonder

$ make 
[  0%] Built target lz4
[ 44%] Built target appleseed
[ 44%] Built target SANDBOX
[ 59%] Built target appleseed.shaders
[ 59%] Built target appleseed.shared
[ 60%] Built target appleseed.cli
[ 63%] Built target appleseed.python
[ 63%] Built target appleseed.python.copy_py_files
[ 98%] Built target appleseed.studio
[ 98%] Built target animatecamera
[ 98%] Built target convertmeshfile
[ 99%] Built target dumpmetadata
[ 99%] Built target makefluffy
[100%] Built target projecttool


import Qt

def wrapinstance(addr, type):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

type is maybe a reserved python keyword. We can rename the argument to cls (short for class) just in case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked docs and made it consistent with what names are used in modules being wrapped
https://shiboken.readthedocs.io/en/latest/shibokenmodule.html
http://pyqt.sourceforge.net/Docs/sip4/python_api.html

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok. Lets keep type then.


def wrapinstance(addr, type):
# Function won't change and will throw Exception for PySide2 and PyQt5.
raise Exception("No wrapinstance function defined for " + Qt.__binding__)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can do all in wrapinstance. I see that you are trying to import sip or shiboken only once, but importing a module that's already imported is not expensive, just a dictionary lookup, and this function is not going to be used in any kind of performance critical code.

I suggest:

if Qt.binding == 'PyQt4':
import sip
return sip.wrapInstance(...)
elif Qt.binding == 'PySide':
...
else:
raise Exception(...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I tried to avoid if-elif chain and repetitive importing. If it's not too expensive I'll make function easier as you suggested

@est77 est77 merged commit bfca5ec into appleseedhq:master Aug 5, 2017
@glebmish glebmish deleted the studio-python-module branch August 25, 2017 19:35
@glebmish glebmish changed the title Studio python module appleseed.studio python module improvements Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants