Add scripting to Krita
ClosedPublic

Authored by rempt on Mar 1 2017, 10:55 AM.

Details

Reviewers
woltherav
Group Reviewers
Krita
Summary
  • Adds a wrapper library to Krita, libkis, that can be used to create bindings. The wrapper library is QObject based and shims between krita's internal API and an external API.
  • This is the first version of the API: it should be tested in practice, but going forward, changes and additions to the API are expected
  • pykrita is the combination of Python bindings to libkis and a plugin system, borrowed from Kate's Pate that allows users to create plugins in Python and enable and disable those plugins from Krita's settings dialog
  • There are a couple of sample scripts, too.

See T1625-python-scripting, and for easy building, check the rempt/T1625-python-scripting branch. You need Python3, PyQt and SIP. there are cmake external projects for those dependencies, but setting up a build environment with them can be tricky. Here's my cmake command line:

cmake ../krita -DCMAKE_INSTALL_PREFIX=/home/boud/dev/i-scripting \

-DCMAKE_PREFIX_PATH=/home/boud/dev/deps \
-DPACKAGERS_BUILD=ON \
-DBUILD_TESTING=OFF \
-DKDE4_BUILD_TESTS=OFF \
-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DPYTHON_EXECUTABLE=/home/boud/dev/deps/bin/python3 \
-DPYQT_SIP_DIR_OVERRIDE=/home/boud/dev/deps/share/sip/PyQt5

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
rempt created this revision.Mar 1 2017, 10:55 AM

I just had a quick glance, could it be that the patch needs some rebasing or so? It seems like there are quite some unrelated changes.

3rdparty/README.md
99

I don't quite understand this comment - to build Qt? how is that related to Py?

3rdparty/ext_frameworks/ecm_python3.diff
4

Shouldn't this be called FindPython3... ? and be part of the ECM stuff? I think this should be upstreamed and then it's still OK to carry a temporary patch.

32

Why is this needed?

Mainpage.dox
20

Unrelated, reviewing this huge patch would be easier without this kind of change.

46

missing comma after dialogs?

libs/image/kis_image.cc
868

rv is not very descriptive, rename to profileChanged?

libs/libkis/Action.h
27

Why is this needed? Why not use QAction?

Hi, @rempt!

I have checked the C++ part of the patch, and found quite a few issues, some of which are really critical. Please check them inline.

And as a general notes:

  1. As far as I can tell, you cannot compare python objects in the python implementation. That sounds wrong, because python may have different objects pointing to the same node. See a comment in Document::activeNode(). I guess it should be enough just to implement comparison (== + !=) operators for the wrapper objects.
  2. Document class needs also the following API:
void lock() { image->barrierLock(); }
void unlock() { image->unlock(); }
void waitForDone() {image->waitForDone(); }
  1. The patch needs unittests! There are quite a few places where neither you and me can tell whether the code works just with looking into the lines. It should be covered by the tests. Otherwise it'll bitrot like the previous scripting implementation. It needs at least teh following unittests:
    1. Channel
      • read/writeBytes for (8, 16, 16f, 32f) color spaces
      • if you pass an empty rect into readBytes() method, you'll see it asserts. That is an expected behavior of the iterator. Please add a test for it.
      • generation of a QImage for (8, 16, 16f, 32f) color spaces. Right now the generation doesn't work for anything other than 8-bit
    2. Document
      • read pixel data
      • thumbnails
    3. Krita
      • Nodes duplication and comparison in currentNode()
    4. Node
      • setColorSpace
      • setColorProfile
      • pixelData/projectionPixelData/thumbnails
      • save
      • mergeDown (if you tried to test it, you'd see it works incorrectly atm)
3rdparty/ext_frameworks/ecm_python3.diff
4

I'm totally agree about renaming the thing into FindPython3. It can be confusing for people who don't know our internals and which version of Python we use.

libs/image/brushengine/kis_paint_information.cc
24

Is this include really needed? It seems like it is unused.

libs/image/kis_idle_watcher.cpp
84

The real problem that is tried to solve with this check is that the document may not have an image (which seems to be wrong from the first glance), which is not related to the idle watcher itself. It is better just add a safe assert here and move the check up to KisPart::updateIdleWatcherConnections(). Logically, if you pass a null image to a connection, it is an error. This behavior is consistent with QObject::connect(), which asserts when you pass null as a connection object.

libs/image/kis_image.cc
870
  1. Please use m_d->signalRouter.emitNotification(ProfileChangedSignal) instead of direct emitting. Just for the sake consistency.
  2. 'if' is not needed here. The signal should be emitted whatever the return value of the visitor is, because the profile has already been changed in line 865.
libs/libkis/Canvas.cpp
105

Looks like a copy-paste error, should be:

return d->canvas->imageView()->canvasController()->wrapAroundMode();
libs/libkis/Channel.cpp
62

It should be an assert, because you already checked for the type correctness in if (!d->node->inherits("KisLayer")) return false;

66

Seems like is got forgotten to be removed :)

82

Same here, it should be an assert + forgotten 'break'

libs/libkis/Channel.h
54

Seems to be a misprint, should be setVisible

libs/libkis/Document.cpp
113

CRITICAL This code doesn't check if the nodeManager currently tracks this Document. If the script will ask to activate a node that doesn't belong to the currently active document (easily possible when the user switches the views), then we will get a crash.

158

This action is not undoable, because it is not used anywhere in GUI in Krita. If you want to make it a public API, it should be done undoable.

171

For convertImageColorSpace() the lock is not needed, because it is done by the undo system.

348

Why do you create a temporary buffer? It demands twice memory from the user's computer.

KisPaintDeviceSP dev = image->projection();
ba.resize(w * h * dev->pixelSize());
dev->readBytes(ba.data(), x, y, w, h);
return ba;
390

flatten() needs neither locking nor setModified() and initilaRefreshGraph(). Just call flatten() and everything is done.

397

Please reuse this method in setWidth() and setHeight(). Right now they duplicate the code.

There is also a small trouble that the original resizeImage() can be called with non-null origin of the rect. In this case it works like crop() but doesn't crop the layers. But that can be a separate API and be implemented in the future.

libs/libkis/Document.h
71

Misprint: topLevelNodes

291

Misprint: setYRes

libs/libkis/Filter.cpp
86

MAJOR Why using this weird locking here? Please use KisFilterStrokeStrategy instead. You can see a good example of usage in KisFilterManager::apply(). Right now the code will generate wrong result when there is a selection, not speaking about inefficiency.

libs/libkis/InfoObject.h
55

Misprint again

libs/libkis/Krita.cpp
124

What happens if we do the following?

doc1 = Krita::activeDocument()
doc2 = Krita::activeDocument()

if doc1 == doc2:
   print("Test passed")
else:
   print("Test failed")
libs/libkis/Node.cpp
202

This operation is not undoable, because it is not exposed to the GUI anywhere. Please make it undoable like the one for KisImage or just remove this from public API.

216

MAJOR Should be wrapped into image->undoAdapter()->beginMacro(kundo2_i18n("Convert Layer Type"));

See ColorSpaceConversion for an example

369

MAJOR no need to duplicate memory

385

MAJOR no need to duplicate memory

470

MAJOR You cannot keep a pointer to prev sibling, because it is removed from the image during the merge down operation.

You can use instead:

int index = indef_of(prevSibling);
d->image->mergeDown(qobject_cast<KisLayer*>(d->node.data()), KisMetaData::MergeStrategyRegistry::instance()->get("Drop"));
d->image->waitForDone();
return parent->node_by_index(index);
libs/libkis/Selection.cpp
109

MAJOR Please reuse the code from action factories, don't copy-paste it. I spent quite a lot of time on removing all the duplications of this very piece of code. Don't duplicate it back.

142

Again, use action factories instead, then you will not have to think about locking.

libs/pigment/colorspaces/KoAlphaF16ColorSpace.cpp
100

Commented out part cannot go into the release

123

Commented out part cannot go into the release. Especially convertToQimage() which is used in thumbnailing

libs/pigment/colorspaces/KoAlphaF16ColorSpace.h
58

MAJOR Mis-copy-paste

libs/pigment/colorspaces/KoAlphaF32ColorSpace.cpp
100

Please implement

123

Please implement

libs/pigment/colorspaces/KoAlphaF32ColorSpace.h
56

MAJOR Copy-paste error

libs/pigment/colorspaces/KoAlphaU16ColorSpace.cpp
101

Implementation needed

124

Implementation needed

libs/pigment/colorspaces/KoAlphaU16ColorSpace.h
56

MAJOR Same error

As far as I can tell, there are only three issues left:

  1. Filter.cpp doesn't use correct strategy for its work
  2. Duplicated objects in Kirta.cpp (compare operator for the objects)
  3. Discuss locking and undo policy for scripts
woltherav accepted this revision.Aug 4 2017, 4:21 PM
woltherav added a subscriber: woltherav.

This has long been in master now. I'm just gonna accept the revision so it can be closed properly. :/

This revision is now accepted and ready to land.Aug 4 2017, 4:21 PM
rempt closed this revision.Aug 5 2017, 8:59 AM
rempt marked 30 inline comments as done.
rempt added inline comments.
3rdparty/README.md
99

You need Python to build Qt on Windows, and we build Qt on Windows because of patches and because we want to slim down Qt.

3rdparty/ext_frameworks/ecm_python3.diff
4

It's actually not used in this version, it was used for the autogeneration of sip files. Deleted.

libs/image/brushengine/kis_paint_information.cc
24

Should be replaced by #include <Eigen/Core> -- all over the place in Krita, things transitively include eigen through kis_vec.

libs/image/kis_idle_watcher.cpp
84

Safe asserts aren't safe -- they crash our users' sessions and make them lose data. I prefer, strongly, to check whether my pointers are valid whenever I encounter a situation where a pointer can be invalid. So I don't intend to change this.

libs/image/kis_image.cc
868

we usually use something like rv or retval in Krita. It's not pretty, but consistent.

870

Okay, but _please_ add some documentation to that class. It's completly unclear what it's for, and how or when it is to be used.

libs/libkis/Action.h
27

Action has a special, wrapped, property to determine whether it should end up in the script menu or not.

libs/libkis/Channel.cpp
62

No, I disagree here. The check can go, but lets not add even more asserts.

82

No, the break is not forgotten. I'll remove the check.

libs/libkis/Document.cpp
158

No: none of the things done through scripting is undoable. Adding an undo api is something I want to do later on, but for now, nothing registers on the undo stack, intentionally.

171

That probably means the scripting plugin should call the visitor directly.

libs/libkis/Filter.cpp
86

I'm not sure about the locking -- but a) a filter can run on a paint device that's not associated with an image and b) KisFilterStrokeStragey is too high-level for use from a scripting language. We're not macro-scripting the ui, we're providing an alternative way to code for Krita.

Oh, and I tested with selections, and that works.

libs/libkis/Krita.cpp
124

That returns false:

from krita import *
d1 = Application.activeDocument()
d2 = Application.activeDocument()
print(d1 == d2)
====
False

We never discussed whether that should return True or False. In another iteration we can add comparison operators, that shouldn't be a problem.

libs/libkis/Selection.cpp
109

I'm sorry, but the action factories are not re-usable, and factoring out the code into something that can be used from both places also didn't work out, because the action factories version has so much transaction stuff mixed in. If we can find a good way to refactor it, I'll for it, but I won't be spending time on that again at this moment.

libs/pigment/colorspaces/KoAlphaF16ColorSpace.cpp
123

Replaced with warnPigment << i18n("Undefined operation in the alpha color space"); : this is actually not used at this point because nothing in Krita handles non-8 bit channel or mask objects.

libs/pigment/colorspaces/KoAlphaF32ColorSpace.cpp
100
warnPigment << i18n("Undefined operation in the alpha color space");
123

replaced with warnPigment << i18n("Undefined operation in the alpha color space");

libs/pigment/colorspaces/KoAlphaU16ColorSpace.cpp
101

Replaced with warnPigment << i18n("Undefined operation in the alpha color space");

124

replaced with a warning: we don't need the implementation at this point.