Brush Editor refactoring project
Open, Needs TriagePublic

Description

Brush editor was historically the most convoluted part of Krita, so we needed a refactoring for ages. I will try to gather all the ideas and notes about that in this task

Problems

  1. Out GUI classes for the brush options do a lot of logic about brushes, some brush features will be disabled or enabled by the GUI controls. That is, the GUI elements basically perform the role of "controller" for the brushes. See KisLodAvailabilityWidget for the worst example.
  2. We have no C++-typed representation of the brush preset. It means that to modify any brush property we need to instantiate GUI widgets (or at least GUI-targeted objects).
  3. We cannot access brush properties without constructing a KisBrush object, which is an expensive operation. The problem is worsened by the fact that KisBrush is a dual-purpose object. It is also used for painting, but for painting it needs additional "state", like BrushPyramid, OutlineCache, Gradients and Textures. We don't create these extra objects in the GUI elements, which makes KisBrush object incomplete there. This incomplete state may also leak into the stroke, which causes issues.
  4. Since we store the state of the brush in GUI elements, we have circular updates problem: every time we modify a GUI control, it emits valueChanged() signal, which modifies the preset and requests another GUI update.
  5. When any single property of the preset is changed we need to reload all the GUI elements. Currently, this problem is mitigated by the heavy usage of signal compressors, but signal compressors bring back the circular updates problem
  6. Some of the brush settings have interdependencies. For example,
    • "lod availability" depends on the "brush size"
    • "colorsmudge paint thickness" depends on "colorsmudge new algorithm"
    • "colorsmudge lightness mode" depends on "colorsmudge new algorithm"
    • "paintbrush lightness strength" depends on "brush mode"

Notes

Lager library

There is Lager library. It is designed to solve the circular updates problem. It can theoretically help us solve problems 4) and 5).

What the library does, it converts a model in a from struct Model { int a; int b; }; into a QObject with Qt's properties. Every property will have a getter, setter and a notification signal. All the boilerplate for proper signal emission is done by Lager. That is, you just assign a new model into the view, and the library emits only update for the properties that have changed.

Here is the list of pros and cons:

  • [pro] it has boilerplate to bind a normal struct to Qt's properties. It means that the resulting model can be connected to QML very easily
  • [pro] it solves "circular updates" and "updates granularity" problems, which is about 30% of our problems
  • [pro] the implementation of binding to properties is something we need anyway
  • [con] the library might be a bit non-obvious for usage for an unexperienced developer (it uses boost::hana internally)
  • [con] it needs C++17
  • [con] it doesn't seem to address interdependencies issues (problem 6) ). It can probably be solved at the higher level of abstraction, though I'm not sure.

Even if we decide not to use Lager, we can still borrow a lot of ideas from it. Especially the part, where it binds model to Qt properties.

Tentative Plan

  1. Refactor KisLodAvailabilityWidget to avoid being a controller for EffectiveLodAvailablility resource
  2. Refactor-out the usage of KisBrush from GUI elements. The factories should provide raw data about the brush in some custom lightweight form
  3. Move static members like KisBrushBasedPaintOp::prepareLinkedResources into the factory
  4. ...
  5. Profit.
dkazakov created this task.Dec 22 2021, 9:52 AM
dkazakov updated the task description. (Show Details)Dec 22 2021, 9:56 AM
dkazakov updated the task description. (Show Details)Dec 22 2021, 10:18 AM

Adding scott, as he was interested in this. Though, right now it seems like it's mostly about how to tackle the refactor.

In any case, happy to see qproperties are considered ahead of time :)

mwein added a subscriber: mwein.Dec 22 2021, 12:31 PM

@dkazakov - These all seem like great ideas. The whole 'unidirectional data flow' pattern would be neat to try, as well as reducing any property dependencies we currently have. If we use lager and the concepts are a bit more abstract, I think just providing more documentation in the code is really all we need. I guess our code base works ok with C++17, so not sure how big of an issue that will be.

With breaking some of these dependencies, I am not sure if that will mean the UI aspect will need to change at all. Sometimes breaking dependencies can make properties more versatile as they aren't directly tied to something else.

Hi, @scottpetrovic!

With breaking some of these dependencies, I am not sure if that will mean the UI aspect will need to change at all

No, GUI aspects don't have to change much. It is mostly about lower level things. Right now these dependencies are controlled by the GUI, which is bad. It would be nice to move it away from that. Then changing any aspect of the brush editor will be easy: one would just have to map a different control to an existing property.