Bug: https://bugs.kde.org/show_bug.cgi?id=306182
Plan of work: https://phabricator.kde.org/T3281
At now work:
- Gui-prototype
- Line stroke
- Choose size of line selection
- Choose color of line selection and brush selection
- Brush selection
Details
Details
- Reviewers
dkazakov - Commits
- R37:48e3929f838b: Add Stroke Selection Edge function
Diff Detail
Diff Detail
- Repository
- R37 Krita
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Hi, Alexey!
The patch is very nice, and it kind of works! :)
There are four small troubles right now:
- It is better to pass the color to the helper-resources_snapshot directly instead of editing the global resource temporarily.
- From the user point of view, I would expect the initial color of the color selector to be equal to the current global Krita color.
- It would be nice if the GUI elements would save their state into config. Though it can be postponed for later.
- [BIGGEST ONE] Right now the stroke you do with the brush is cut off by the selection, because it is taken into account :) The solution for this would be to add setSelectionOverride(KisSelectionSP selection) methods for both, KisFigureToolHelper and KisResourcesSnapshot.
| libs/ui/actions/kis_selection_action_factories.cpp | ||
|---|---|---|
| 594 | I would prefer passing a color to the helper and then to its internal KisResourcesSnapshot member object. Modifying global resources may cause really weird troubles with multithreading sometimes :( | |
Comment Actions
Hi, Alexey!
The stroking with the current brush works now. But I would really suggest talking to @scottpetrovic about the GUI :)
As I wrote before I would expect the default fill color to be equal to the current foreground color. And the selection of the coloring option should be not a set of checkboxes, but either a radiobutton or a combobox.
It might be a good idea to add the following options:
- Foreground Color
- Background Color
- Custom Color
If you do it this way, then the color of the "Custom Color" option can be saved into the config file. You can even make the GUI perfect if you let the user always click on the color button in any moment of time and the option will be switched automatically accordingly.
Comment Actions
Hi, Alexey!
The patch works fine now, except it creates shape layers, when the user selects "Use line selection". It should paint on the raster layer instead. You can use KisPainter::drawPainterPath for that.
And there is a couple of small issues in the code. Speaking truly I would not create two separate actions, for vectors and raster painting. I would just create a single action and choose the painting method depending on the type of the layer selected. That is, if a paint layer is selected, paint with raster lines. If a vector layer is selected, just add a vector shape.
And we should still talk to @scottpetrovic about the design of the GUI :)
| libs/ui/actions/kis_selection_action_factories.cpp | ||
|---|---|---|
| 543 | It would be better to split the params into a small struct, to break the dependency of the operation from a GUI object. struct StrokeSelectionParams {
KoColor color;
int width;
bool useBrush;
....
};Then you can just add a method to KisDlgStrokeSelection that returns this params object. With this split three problems will be solved:
| |
| libs/ui/actions/kis_selection_action_factories.h | ||
| 128 | The IDs of the actions should be unique :) We still don't have a registry for them, but can do in the future. | |
Comment Actions
I am not sure how many of these features are going to be implemented, but this is the direction I would go for the UI. I think showing everything at the same time might be too much for most people, so I think putting all of that styling information in another tab might be a good idea.
For the colors, I would default the color picked to the foreground or background color, but not have people actually pick "foreground color" from a drop-down
Comment Actions
Hi, @akapustin!
I have tested the patch. It basically works, but I have two comments for which I would like to get a @scottpetrovic feedback:
- The user has no way to select the currently active foreground color as the stroke color. Once you changed it, the saved custom color will be always selected. It might be a good idea to have a combo box like the one near the fill color selector. @scottpetrovic, your GUI draft doesn't have such selector, can it be added somehow? Or, probably, we need some different UIX?
- The filling works strange, more precisely, the filling is painted on the top of the stroke, covering some parts of it. I would expect it be painted behind the stroke. Though I would like to hear @scottpetrovic input about it.
How it works:
How I would expect it to work:
Scott, what do you think about it?
Comment Actions
Hi, @akapustin!
The patch works fine now. Except of the one thing. You use QColor everywhere, which causes your strokes be not color-managed. It can cause very difficult to find problems when the user works with linear gamma (*-g10) or HDR color spaces.
Please change all the interfaces (except the main GUI ones, like KColorButton) to work with KoColor instead of QColor. And when converting the color from KColorButton's QColor, please use KisCanvas2::displayColorConverter()::approximateFromRenderedQColor() to do the color conversion right.
Yes, I agree that the interface of KoColor might be a bit misleading providing a call for the direct conversion, but that is what we have here.
KisDisplayColorConverter also takes into account the HDR settings of the canvas, like exposure and other conversions done by OCIO engine, which is not available in pigment. Therefore we should use such long way for a workaround.
So, basically, you need to do the following:
- Get a QColor from KColorButton
- Convert (approximate) it into KoColor using KisDisplayColorConverter
- Use the resulting KoColor everywhere you need.
The only exception is a QPen which you pass to the painter. Here we cannot do anything, just convert the KoColor into QColor right before the constructor call using KoColor::toQColor(). In all higher level interfaces we should use KoColor.
| libs/ui/actions/kis_selection_action_factories.cpp | ||
|---|---|---|
| 587 | This conversion between KoColor<->QColor is really wrong. See the main comment | |
| libs/ui/tool/strokes/freehand_stroke.cpp | ||
| 134 | Why did you add a special method for this? You can just use the existing "Background" color. In a usual "painter" pattern, foreground and background colors mean just the colors of the stroke and the filling, they need not correspond to the real selected colors in the GUI. I guess, setCustomColor() could be deleted. | |
Comment Actions
I deleted setCustomColor().
There was problems with KisCanvas2::displayColorConverter()::approximateFromRenderedQColor() . It gives approximate result.
It is bad for comparing color, which obtained from KColorButton. There was troubles with GUI :(
Where is possible, I replaced QColor to KoColor.
Comment Actions
Dmitry helped me get the patch to apply. I wasn't doing right...now for the feedback about the UI. It looks pretty good for the most part.
If the current brush is selected, it doesn't look like you can change the brush size. You can probably just hide the labels and the input box since they aren't needed.
The whole thing is in a structure grouping. I think you can remove that since there are no other groups. It simplifies it a bit too.