Symmetric Difference Selection Patch for Krita
ClosedPublic

Authored by Reptorian on Aug 26 2018, 5:32 AM.

Details

Summary

I have attempted to add a exclude selection for Krita, and while it does compile nicely and smoothly, there is a issue where Krita crash from the very beginning. Everything seem to check out.

The idea is just to finish the basic selection of Krita

Test Plan
  1. Fix the crash issue
  2. Change the math operation of exclude operation
  3. See if it works as expected
  4. If all goes all, this can be used to finish basic operations of selection of Krita

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
Reptorian created this revision.Aug 26 2018, 5:32 AM
Restricted Application added a reviewer: Krita. · View Herald TranscriptAug 26 2018, 5:32 AM
Restricted Application added a project: Krita. · View Herald Transcript
Reptorian requested review of this revision.Aug 26 2018, 5:32 AM
Reptorian edited the test plan for this revision. (Show Details)Aug 26 2018, 5:35 AM
Reptorian added a comment.EditedAug 26 2018, 3:45 PM


Here's the crash trace. Alternative crash log is provided too in case the other isn't useful. Segmentation fault.

Using what's below

if (m_d->outlineCacheValid) {
    m_d->outlineCache |= selection->outlineCache();

instead of

if (m_d->outlineCacheValid) {
    m_d->outlineCache -= selection->outlineCache();

wouldn't fix it.
-

Actually, never mind, it turns out that I may just need to only do the GUI part to fix the crash issue, and see what to do from there. Hopefully, it won't crash by then.

Reptorian updated this revision to Diff 40527.EditedAug 28 2018, 4:33 AM

I have added more lines related to GUI to attempt to resolve the crash. However, a new problem occured. 4 lines of this error happens:

‘class WdgSelectionOptions’ has no member named ‘exclude’
dkazakov requested changes to this revision.Aug 28 2018, 1:27 PM
dkazakov added a subscriber: dkazakov.

Hi, @Reptorian!

The error happens because you need to add actual buttons to the .ui file that defines WdgSelectionOptions class, that is to 'libs/ui/forms/wdgselectionoptions.ui'. Just open it in QtCreator, copy-paste an existing button and rename it into 'exclude'.

Could you also a bit elaborate what is the difference between "exclude" mode and "subtract" mode? I look at the formula and they look exactly the same...

This revision now requires changes to proceed.Aug 28 2018, 1:27 PM
Reptorian updated this revision to Diff 40560.Aug 28 2018, 2:52 PM

Added the GUI, however, there is still a major issue of crashing.

Reptorian updated this revision to Diff 40565.EditedAug 28 2018, 4:09 PM

Updated for current master. Now that I was able to test the result of the code, it turns out that the patch works partly. This patch forces every selection to be exclude selection, so that's something I don't where to start to look into. Krita no longer crashes.

The proof that it partly works -

Reptorian added a comment.EditedAug 29 2018, 2:17 AM

I have added a note to the commits here. Please see note to see current issue to be resolved before fixing the other bug. It's particularly a hard to fix issue.

Also, the other issue is that while mapping can resolve the forced to exclude mode only, it appears that it eliminates the option of being able to exclude unless you use keyboard shortcut.

libs/image/kis_pixel_selection.cpp
280

It seems that no matter the operator used here, the ants just doesn't show up in some area, or the ants just doesn't show up at all.

Reptorian updated this revision to Diff 40598.EditedAug 29 2018, 3:43 AM

Changed keyboard shortcut for maximizing productivity only. With keyboard shortcut, and global mask selection, you can preview how selections works altogether.

Here's a video to demonstrate - https://streamable.com/7e22h

rempt added a subscriber: rempt.Aug 29 2018, 7:34 AM

We're getting somewhere! The tooltip for the exclusion button needs to be changed, though, since it now also says "subtract".

Reptorian updated this revision to Diff 40643.EditedAug 29 2018, 1:12 PM

Only thing fixed is what Boud mentioned. I'm still having trouble fixing the issue that the button only activates subtract selection, but not exclude. A lot of lines were changed in search for an answer to that.

EDIT: In light of reddit post, name has to be looked into - https://www.reddit.com/r/krita/comments/9b4yj8/unofficial_new_minifeature_development_exclude/

"Cool. So is "exclude" another name for XOR? I found "exclude" a bit confusing, myself; just sounded like it would subtract as normal." - wthit56

"I second this. I know XOR is called "exclusive or", but in that case I feel it need to be mentioned what exactly is excluded from the result. "Symmetric difference", "Disjunctive union" or "Exclusive or (XOR)" would be better, or maybe it could be better to find artists who doesn't know Math as well as we and can name it even better (easier for other artists to grab)." - tiar

Reptorian updated this revision to Diff 40646.EditedAug 29 2018, 1:42 PM

Now, the button activates exclude selection tool thanks to boud. After I get the ants working, naming convention will be addressed by myself.

EDIT: There's a regression that need to be addressed. If I select, nothing shows up on layers. Also, there is a issue with ants at the moment. Did so many attempts that I do not believe that changing line 287 at kis_pixel_selection.cpp would change anything.

Reptorian added a comment.EditedAug 31 2018, 6:27 AM

I believe I may have bad news. This requires coding beyond Krita. QTGui:QPainterPath and QtGui:Qrect are the source of the issue.

Reptorian updated this revision to Diff 41004.Sep 4 2018, 7:47 PM

Updated to current master.

Fixed ant selection issue. There's still a few bug to be solved. Confident that it can be done.

Reptorian retitled this revision from Exclusion Selection Patch for Krita to Symmetric Difference Selection Patch for Krita.Sep 4 2018, 8:16 PM
Reptorian updated this revision to Diff 41016.Sep 5 2018, 1:54 AM
Reptorian marked an inline comment as done.

I have fixed the button issue. One blocker bug remains. It doesn't work for pixel selection, but it works very well for vector selection. I have no idea if Dmitry changes to Krita has to do anything with that or if it my fault that I somehow broke it. Dmitry has been notified shortly after this post.

Reptorian updated this revision to Diff 41558.Sep 13 2018, 4:38 PM

Updated for current master.
Same bug still remains.

Reptorian updated this revision to Diff 41917.Sep 18 2018, 9:05 PM

I have enabled the button to work. It actually does something.

One blocker bug remaining : The pixel ant selection. I have tried so many different lines, and it still gives wrong result. Vector selection ant selection works fine and dandy though. That it does.

dkazakov requested changes to this revision.Sep 19 2018, 8:45 AM

Hi, @Reptorian!

Here is a patch that fixes pixel selection in your patch. Basically, when processing a pixel selection you forgot to write the result somewhere.

1diff --git a/libs/image/kis_pixel_selection.cpp b/libs/image/kis_pixel_selection.cpp
2index 9686980..a6980f5 100644
3--- a/libs/image/kis_pixel_selection.cpp
4+++ b/libs/image/kis_pixel_selection.cpp
5@@ -151,7 +151,7 @@ void KisPixelSelection::applySelection(KisPixelSelectionSP selection, SelectionA
6 intersectSelection(selection);
7 break;
8 case SELECTION_SYMMETRICDIFFERENCE:
9- symmetricdifferenceSelection(selection);
10+ symmetricDifferenceSelection(selection);
11 break;
12 default:
13 break;
14@@ -258,7 +258,7 @@ void KisPixelSelection::intersectSelection(KisPixelSelectionSP selection)
15 m_d->invalidateThumbnailImage();
16 }
17
18-void KisPixelSelection::symmetricdifferenceSelection(KisPixelSelectionSP selection)
19+void KisPixelSelection::symmetricDifferenceSelection(KisPixelSelectionSP selection)
20 {
21 QRect r = selection->selectedRect();
22 if (r.isEmpty()) return;
23@@ -268,7 +268,7 @@ void KisPixelSelection::symmetricdifferenceSelection(KisPixelSelectionSP selecti
24 KisHLineConstIteratorSP src = selection->createHLineConstIteratorNG(r.x(), r.y(), r.width());
25 for (int i = 0; i < r.height(); ++i) {
26 do {
27- abs(*dst->rawData() - *src->oldRawData());
28+ *dst->rawData() = abs(*dst->rawData() - *src->oldRawData());
29
30 } while (src->nextPixel() && dst->nextPixel());
31 dst->nextRow();
32diff --git a/libs/image/kis_pixel_selection.h b/libs/image/kis_pixel_selection.h
33index 171642d..afca308 100644
34--- a/libs/image/kis_pixel_selection.h
35+++ b/libs/image/kis_pixel_selection.h
36@@ -156,7 +156,7 @@ private:
37 /**
38 * Invert a selection or intersect with the inverse of a selection
39 */
40- void symmetricdifferenceSelection(KisPixelSelectionSP selection);
41+ void symmetricDifferenceSelection(KisPixelSelectionSP selection);
42
43 private:
44 // We don't want these methods to be used on selections:
45diff --git a/libs/ui/tool/kis_selection_tool_helper.cpp b/libs/ui/tool/kis_selection_tool_helper.cpp
46index 50281fc..c2cd588 100644
47--- a/libs/ui/tool/kis_selection_tool_helper.cpp
48+++ b/libs/ui/tool/kis_selection_tool_helper.cpp
49@@ -101,12 +101,12 @@ void KisSelectionToolHelper::selectPixelSelection(KisPixelSelectionSP selection,
50 KisPixelSelectionSP pixelSelection = m_view->selection()->pixelSelection();
51 KIS_ASSERT_RECOVER(pixelSelection) { return 0; }
52
53- bool hasSelection = !pixelSelection->isEmpty();
54+ const bool hasSelection = !pixelSelection->isEmpty();
55
56 KisSelectionTransaction transaction(pixelSelection);
57
58 if (!hasSelection && m_action == SELECTION_SYMMETRICDIFFERENCE) {
59- pixelSelection->invert();
60+ m_action = SELECTION_REPLACE;
61 }
62
63 pixelSelection->applySelection(m_selection, m_action);

This revision now requires changes to proceed.Sep 19 2018, 8:45 AM
Reptorian updated this revision to Diff 41996.Sep 20 2018, 3:31 PM

In this patch, I have fixed the ant selection issue to the point where it's apparent that there has been a selection made with symmetric difference selection. I will have to look at Dmitry patch again to see if it fixes the absence of anti-aliased line, but I did observe a error somewhere during a compilation even though it works (could be false error as it works while the ant selection ant is still wrong with the patch from what I seen). When I do a circle with symmetric selection issue, and overlay it with another circle, I get two overlapping circle with visible selection lines on the middle which is the expected behavior. Not expected was that the lines gets pixelated.

Hi, @Reptorian!

The patch works kind of fine. Although it has a few blockers codewise. The main one is that the keyboard modifiers have changed. We need to know painters' opinion on that. The other comments are easily fixable.

libs/image/kis_pixel_selection.cpp
269

This check is incorrect because both values are quint8, which is unsigned it, therefore the condition is always true, unless *dst... == *src...

280

This line looks weird. Did you mean m_d->outlineCache?

libs/ui/tool/kis_selection_tool_helper.cpp
272

Is it correct that INTERSECT is not a part of the condition anymore?

plugins/tools/selectiontools/kis_selection_modifier_mapper.cc
95

Did you consult with the painters before changing modifiers?

dkazakov requested changes to this revision.Sep 25 2018, 6:39 PM
This revision now requires changes to proceed.Sep 25 2018, 6:39 PM

Okay, I talked to @woltherav and I think we both agree that the old keyboard modifiers should not be changed. So please revert this part of the patch.

Reptorian updated this revision to Diff 42751.EditedOct 2 2018, 7:55 PM

All the comments have been addressed, and thanks to one of the comment, one of the bug I just found during testing have been solved. Also, I had to make up a keyboard modifier for this patch as otherwise, you'd always be stuck on symmetric difference selection mode unless you use a keyboard shortcut. I chose cntrl+alt as a temporary keyboard shortcut.

By the way, the weird line was the only line I saw that made that selection ants works as expected (barring absence of vector in pixel selection).

Reptorian updated this revision to Diff 42752.EditedOct 2 2018, 8:03 PM
Reptorian marked 2 inline comments as done.

Erased one line in pixelselection cpp file. Didn't noticed that on the last update. Now it looks good.

Reptorian marked an inline comment as done.Oct 3 2018, 10:08 PM

I found something that may hint to the solution to the selection

There's this line

// parent selection is not supposed to be shared

If parent selection can be shared, then you would be able to shared selection outlines which is how the outline should look like when selected. But, I have no idea how to get around that issue. So, I have to say I cannot continue with this patch at all.

langkamp added inline comments.
libs/image/kis_pixel_selection.cpp
287

This should use &=. Otherwise it will alwas run regenerate the outline cache.

290

This should be the same as for the vector selection so m_d->outlineCache = (m_d->outlineCache | selection->outlineCache()) - (m_d->outlineCache & selection->outlineCache());

Reptorian updated this revision to Diff 43002.Oct 7 2018, 2:58 AM
Reptorian marked 3 inline comments as done.

Applied changes accordingly to Sven Langkamp instruction. It solved the blocky outline issue! Now, symmetric difference pixel selection behaves similar to the other selection modes.

Now the only thing that left to be done are shortcuts, and add the option layer-based selection which Dmitry has added.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 7 2018, 10:12 AM
This revision was automatically updated to reflect the committed changes.