After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 760481 - Add "diagonal neighbors" option to fuzzy-select and bucket-fill
Add "diagonal neighbors" option to fuzzy-select and bucket-fill
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Tools
git master
Other All
: Normal enhancement
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2016-01-11 17:01 UTC by Ell
Modified: 2016-01-17 13:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
"Diagonal neighbors" demonstration for fuzzy-select (21.96 KB, image/png)
2016-01-11 17:01 UTC, Ell
  Details
0001-app-Add-diagonal_neighbors-parameter-to.patch (6.50 KB, patch)
2016-01-11 17:05 UTC, Ell
none Details | Review
0002-app-Add-Diagonal-neighbors-option-to-the-fuzzy-selec.patch (4.74 KB, patch)
2016-01-11 17:06 UTC, Ell
committed Details | Review
0003-app-Add-Diagonal-neighbors-option-to-the-paint-bucke.patch (11.16 KB, patch)
2016-01-11 17:06 UTC, Ell
none Details | Review
0004-app-Add-diagonal_neighbors-parameter-to-gimp_channel.patch (6.12 KB, patch)
2016-01-11 17:07 UTC, Ell
committed Details | Review
0005-pdb-Add-diagonal-neighbors-as-GimpPDBContext-propert.patch (14.45 KB, patch)
2016-01-11 17:07 UTC, Ell
committed Details | Review
0006-pdb-Use-the-diagonal-neighbors-setting-from-GimpPDBC.patch (7.90 KB, patch)
2016-01-11 17:08 UTC, Ell
committed Details | Review
0001-app-Add-diagonal_neighbors-parameter-to.patch (6.53 KB, patch)
2016-01-11 17:52 UTC, Ell
committed Details | Review
0007-app-Add-a-modifier-key-to-toggle-the-diagonal-neighb.patch (3.20 KB, patch)
2016-01-12 17:50 UTC, Ell
rejected Details | Review
0003-app-Add-Diagonal-neighbors-option-to-the-bucket-fill.patch (11.16 KB, patch)
2016-01-14 20:03 UTC, Ell
committed Details | Review

Description Ell 2016-01-11 17:01:46 UTC
Created attachment 318767 [details]
"Diagonal neighbors" demonstration for fuzzy-select

The individual pixels of thin lines and curves, especially of non antialiased ones, are often connected to each other only by diagonal adjacency.  However, the fuzzy-select and bucket-fill tools can only calculate their affected region based on orthogonally adjacent pixels, which can make selecting, and filling, thin curves tricky.

The following patchset adds a "diagonal neighbors" option to both of these tools.  When this option is checked, the tools consider diagonally neighboring pixels, as well as orthogonally neighboring ones, when calculating the affected region.  In other words, instead of only looking at the four orthogonal neighbors of each pixel, they look at all eight neighbors.

The attached screenshot demonstrates the effect of this option when using fuzzy-select to select a thin curve.
Comment 1 Ell 2016-01-11 17:05:53 UTC
Created attachment 318769 [details] [review]
0001-app-Add-diagonal_neighbors-parameter-to.patch
Comment 2 Ell 2016-01-11 17:06:22 UTC
Created attachment 318770 [details] [review]
0002-app-Add-Diagonal-neighbors-option-to-the-fuzzy-selec.patch
Comment 3 Ell 2016-01-11 17:06:46 UTC
Created attachment 318771 [details] [review]
0003-app-Add-Diagonal-neighbors-option-to-the-paint-bucke.patch
Comment 4 Ell 2016-01-11 17:07:13 UTC
Created attachment 318772 [details] [review]
0004-app-Add-diagonal_neighbors-parameter-to-gimp_channel.patch
Comment 5 Ell 2016-01-11 17:07:55 UTC
Created attachment 318773 [details] [review]
0005-pdb-Add-diagonal-neighbors-as-GimpPDBContext-propert.patch
Comment 6 Ell 2016-01-11 17:08:17 UTC
Created attachment 318774 [details] [review]
0006-pdb-Use-the-diagonal-neighbors-setting-from-GimpPDBC.patch
Comment 7 Ell 2016-01-11 17:11:32 UTC
A few notes about the patches:  The entire "logic" is contained in the 5 new lines in find_contiguous_region(), in the first patch; the rest is boilerplate/UI/PDB.

Patches 2 and 3 add the option to the fuzzy-select and bucket-fill tools, respectively.

Patches 4-6 add the relevant PDB changes.
Comment 8 Ell 2016-01-11 17:52:28 UTC
Created attachment 318810 [details] [review]
0001-app-Add-diagonal_neighbors-parameter-to.patch

Avoid a theoretical integer overflow possibility in the previous version of the patch.  Be noted that patch 0001 will probably appear LAST now---it's still the first patch.
Comment 9 Michael Natterer 2016-01-11 20:02:09 UTC
Holy shit, this is a totally complete patch set with everything done right,
that almost never happens :)

I think this feature makes a lot of sense, I'll bring it up on IRC.
Comment 10 Michael Schumacher 2016-01-12 10:10:07 UTC
My feeling is that this should be a toggleable option with a modifier key. I can't tell whether it is or not by looking at the patches.
Comment 11 Ell 2016-01-12 12:44:10 UTC
(In reply to Michael Schumacher from comment #10)
> My feeling is that this should be a toggleable option with a modifier key. I
> can't tell whether it is or not by looking at the patches.

No modifier key ATM, but I agree it would be useful.  Taking a quick look at the keyboard shortcuts list, though, there doesn't seem to be modifier keys for any of the boolean tool options (and shift/ctrl/alt are all taken for selection), so that would be a first?
Comment 12 Ell 2016-01-12 17:50:25 UTC
Created attachment 318900 [details] [review]
0007-app-Add-a-modifier-key-to-toggle-the-diagonal-neighb.patch

The attached patch adds a modifier key to toggle the diagonal neighbors option.  AFAICT that's the only toggleable tool option with a programmable modifier key, so you might want to take a closer look at this.
Comment 13 Michael Natterer 2016-01-12 21:16:00 UTC
That patch is nice and correct too, but not what was meant :)

Michael really meant a modifier key, and we indeed have none left, so I guess
we can safely ditch that idea. IMO this feature is pretty special anyway
(if not scientific / totally nerdish), so let's just go with that toggle
without any modifier.

As to the feature itself, I think we can justify pushing these patches,
they seem to properly implement an 8-neighborhood by the book.
Comment 14 Ell 2016-01-12 22:09:11 UTC
(In reply to Michael Natterer from comment #13)
> That patch is nice and correct too, but not what was meant :)
> 
> Michael really meant a modifier key, and we indeed have none left, so I guess
> we can safely ditch that idea. IMO this feature is pretty special anyway
> (if not scientific / totally nerdish), so let's just go with that toggle
> without any modifier.
> 
> As to the feature itself, I think we can justify pushing these patches,
> they seem to properly implement an 8-neighborhood by the book.

Didn't realize there were two Michaels :)

FWIW, I'm completely fine with whatever: it is handy not to have to muck around with the mouse while doing a selection, and on the other hand it's completely awkward to have this as the only option with a key binding.
Comment 15 Ell 2016-01-14 20:03:47 UTC
Created attachment 319049 [details] [review]
0003-app-Add-Diagonal-neighbors-option-to-the-bucket-fill.patch

s/paint bucket/bucket fill/ in commit message.
Comment 16 Michael Natterer 2016-01-16 23:49:32 UTC
Thanks again for the patches! Fixed in master:

commit 7b89fe65366e37934be580756f68eefad92d7453
Author: Ell <ell_se@yahoo.com>
Date:   Mon Jan 11 10:44:21 2016 +0000

    pdb: Use the diagonal-neighbors setting from GimpPDBContext...
    
    ...in gimp-image-select-contiguous-color()
    
    This and the last 5 commits fix bug 760481.

commit 7b3e5ba1e94f078521ca362ae8133f47e2470d8e
Author: Ell <ell_se@yahoo.com>
Date:   Mon Jan 11 10:41:23 2016 +0000

    pdb: Add "diagonal-neighbors" as GimpPDBContext property
    
    and add PDB API to get/set it.

commit e0b1aa1c26660f99f65b72081ec0167d765eacb5
Author: Ell <ell_se@yahoo.com>
Date:   Mon Jan 11 11:02:27 2016 +0000

    app: Add diagonal_neighbors parameter to gimp_channel_select_fuzzy()
    
    and propagate it to gimp_pickable_contiguous_region_by_seed(), in
    preperation for adding a diagonal-neighbors setting to PDB.

commit 350c7ca338dcb37e3df6e13fb4dc603489aca166
Author: Ell <ell_se@yahoo.com>
Date:   Fri Jan 8 13:18:10 2016 +0000

    app: Add "Diagonal neighbors" option to the bucket fill tool
    
    When checked, diagonally neighboring pixels are considered connected
    when calculating the affected area.
    
    This commit also adds a corresponding diagonal_neighbors parameter to
    gimp_drawable_bucket_fill(), and modifies the callers, other than the
    bucket fill tool, to pass FALSE for this parameter, to retain the
    current behavior.

commit 070007d8914df8c586e679cb10726197b862d884
Author: Ell <ell_se@yahoo.com>
Date:   Fri Jan 8 13:09:45 2016 +0000

    app: Add "Diagonal neighbors" option to the fuzzy select tool
    
    When checked, diagonally neighboring pixels are considered connected
    when calculating the affected area.

commit 93bf78b83e4db6add88c79f1c1516c0aa1c881ea
Author: Ell <ell_se@yahoo.com>
Date:   Fri Jan 8 12:53:07 2016 +0000

    app: Add diagonal_neighbors parameter to...
    
    ...gimp_pickable_contiguous_region_by_seed(), in preperation
    for adding a similar option to the relevant tools.
    
    When this parameter is TRUE, all eight neighbors of each pixel
    are considered when calculating the resulting region, instead
    of just the four orthogonal ones.
    
    This commit also modifies all callers to pass FALSE for this
    parameter, to retain the current behavior.
Comment 17 Ell 2016-01-17 13:09:16 UTC
(In reply to Michael Natterer from comment #16)
> Thanks again for the patches! Fixed in master:

Thanks!

A small heads up: I already added gimp_context_{get,set}_diagonal_neighbors() to gimp.def in one of the patches, albeit not in alphabetical order, and your commit accidentally adds the set() variant twice, so there are some duplicates now.
Comment 18 Michael Natterer 2016-01-17 13:11:45 UTC
Argh, thanks, will fix this right away, it was late when I did this ;)
Comment 19 Michael Natterer 2016-01-17 13:11:53 UTC
Also, I can hardly remember when I last pushed patches without changing
a line. Those patches were great, can we keep you? :)

Would you like to join us on IRC? You obviously know what you are
doing, and we need contributors!
Comment 20 Michael Natterer 2016-01-17 13:24:37 UTC
For the record:

commit 80faa72617632140c8fb6ccca35a7d21cec33c66
Author: Michael Natterer <mitch@gimp.org>
Date:   Sun Jan 17 14:22:35 2016 +0100

    libgimp: remove duplicate entries from gimp.def

 libgimp/gimp.def | 2 --
 1 file changed, 2 deletions(-)