Merge multithreaded version of Pixel Brush into master
ClosedPublic

Authored by dkazakov on Nov 1 2017, 9:15 PM.

Details

Summary

This patch represents the contents of 'kazakov/multithreaded-brushes' branch.

Now Pixel Brush engine is fully multithreaded. The number of used threads is controlled by the Threads Limit option in "Performance" tab of the preferences.

Test Plan

All the features of Pixel Brush should work as usual, but several times faster :)

Diff Detail

Repository
R37 Krita
Branch
kazakov/multithreaded-brushes
Lint
No Linters Available
Unit
No Unit Test Coverage
dkazakov created this revision.Nov 1 2017, 9:15 PM
dkazakov edited the summary of this revision. (Show Details)Nov 1 2017, 9:17 PM
dkazakov edited the test plan for this revision. (Show Details)
dkazakov added reviewers: Krita, rempt.
dkazakov added a project: Krita.

Okay, most of this goes above my head, but I found a typo that might need to be fixed.

I do have a vague comprehension now of how the new system works, it'll be interesting to get the hsv patch for colored brush masks weaved in anew.

libs/pigment/KoColor.cpp
40

This is a bit silly, but maybe this typo could be fixed before merge? Then we won't have to force people to build all of krita again when someone fixes the typo later :p

plugins/paintops/defaultpaintops/brush/kis_brushop.cpp
265

This and line 257... their results seem virtually the same. Any reason why you check them seperately?

I mostly just spent some time and tested painting using different brushes, instant preview, and looked at the live preview. Everything seems to work really well!

Great job! This is really exciting.

rempt accepted this revision.EditedNov 2 2017, 8:05 AM

Patches of this size aren't reviewable... So it'll testing in practice.

Bugs and notes are to be collected here:

https://phabricator.kde.org/T7309

This revision is now accepted and ready to land.Nov 2 2017, 8:05 AM
dkazakov marked 2 inline comments as done.Nov 2 2017, 9:42 AM

Hi, @woltherav!

Thank you for the notes! I have fixed them locally and will push directly into the branch :)

libs/pigment/KoColor.cpp
40

Fixed! Thank you!

plugins/paintops/defaultpaintops/brush/kis_brushop.cpp
265

Yes, in case we have both types of mirrors, we should copy the dab 3 times :) Just note, that there is no "else" branch. It just adds one more mirroring step in case the both mirrors are active. It allows us to avoid one copying.

Anyway, I will add a comment into the code giving details about that.

There, uh, was a minor bug with the brush preview that I went and fixed: https://phabricator.kde.org/R37:06ea68a92974976c96a6f4560e47af3caadba654

Not sure what exactly caused it, but we might be mindful that the new kocolor default might cause this in other places.

dkazakov closed this revision.Nov 2 2017, 5:17 PM
dkazakov marked 2 inline comments as done.

Closed by 89f7b9e2a971