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.

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

TileSet Python export plugins (fixes #1865) #3857

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

DrDub
Copy link

@DrDub DrDub commented Dec 12, 2023

Extended the Python plugin to support custom tileset exporters and readers written in Python.

I understand the Python plugin has its issues and the JavaScript plugin is preferred, but this extension might make it more useful. (The issue of system Python vs. Tiled Python sadly remains.)

Extended the Python plugin to support custom tileset exporters and readers written in Python.
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Even if this plugin isn't very well maintained and I would discourage writing new plugins in Python, I still think this is a very welcome addition. Thanks!

I've left a number of comments which need to be addressed before this can be merged.

src/plugins/python/pythonbind.cpp Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.cpp Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.cpp Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.cpp Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.cpp Outdated Show resolved Hide resolved
src/plugins/python/tiledbinding.py Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.h Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.cpp Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.cpp Outdated Show resolved Hide resolved
src/plugins/python/pythonplugin.cpp Outdated Show resolved Hide resolved
@DrDub
Copy link
Author

DrDub commented Apr 3, 2024

Let me know how it looks now. I think I got all the changes in.

@bjorn
Copy link
Member

bjorn commented Apr 19, 2024

@DrDub I've pushed some changes, and also was running into a crash, likely the same you encountered. I've fixed it by adding a check on the return value of PyObject_Dir, but it does leave the plugin in a broken state in which it is not possible to actually implement a tileset format. When using your example plugin, I'm getting:

-- Loading tileset
TypeError: issubclass() arg 1 must be a class
No extension of tiled.Plugin or tiled.TilesetPlugin defined in script: tileset

It appears there is something broken about findPluginSubclass that makes it not possible to call it twice, but I haven't found the reason yet. When I have it search for a tileset plugin first and then a map plugin, it runs into the same problem but with a map plugin.

* Un-duplicated setPythonClass function

* Removed unused pluginmanager.h include

* Fixed cleanup code related to reloading plugins in case a plugin
  changes from defining a map format to a tileset format or vice versa.

* Allow a Python plugin to include both a map and a tileset format
  (though only one of each).
bjorn added 7 commits May 3, 2024 11:47
It appears Qt Creator's Qbs support does not include looking for compiler
flags setting include paths (-I). They need to be actually set using
cpp.includePaths.
There are actually no defines coming from python3-embed for me.
The problem was that handleError was not called when PyObject_IsSubclass
returned -1, which it commonly did when called with a value that wasn't
a class. With the error indicator still set, the next call to
PyObject_Dir was failing.

Since the errors aren't usually very useful, we just call PyErr_Clear when
PyObject_IsSubclass fails.

I couldn't find a convenient way to check if the value is a class, which
could avoid raising these errors in the first place.
The PyErr_Print call already clears the error indicator, according to
the docs.
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

I think it's working fine now. Can you give it one last try @DrDub before we merge this?

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.

None yet

2 participants