GNOME Bugzilla – Bug 348291
Increase the discrimination of the Fuzzy Select tool
Last modified: 2006-08-15 16:21:40 UTC
The current Fuzzy Select seems to use as a criterion the maximum of the absolute differences of each of the RGB color components. It would be useful to have it optionally discriminate on different characteristics (e.g. hue, so that a solid color background in an image could be selected regardless of lighting variations that, with the present method, fail to select the entire background. I"ve written a patch against gimp-2.3.9 that adds a radio box to the Fuzzy Select menu offering a selection of selection criteria: Traditional (the present method), Red, Green, Blue, Hue, Saturation, And Value. The RGB options compare only the specified components; the HSV options compare only the HSV components after conversion from RGB. This screen doesn't let me append the patch (463 lines, 17869 bytes); I'll try to do that later.
Created attachment 69360 [details] [review] patch to implement the described enhancement
Created attachment 69747 [details] [review] patch to add select criteria to both fuzzy and by-color selects. Replaces the earlier patch.
The new replacement patch adds the select-criteria capability to by-color selects as well as to fuzzy selects.
Very nice. However you must not include any GUI files from the core. This enum belongs into app/core/core-enums.h. When you've done this there is no need any more to include this file explicitely, since it's automatically included when including "tools-types.h". Also, "TRADITIONAL" means nothing since it implies that the reader of the code knows the state before your patch. Your patch contains tabs, please convert them to spaces. Please do not simply say "cvs diff", you included a useless change to po/Makefile.in.in Apart from these stylistic things, and perhaps the enum name and descs (which I have not yet made my mind up about completely), the whole idea and the patch look very good.
Created attachment 69781 [details] [review] Third stab at adding the select criteria capability. Per above, I moved the enum to app/core/core-enums.h, renamed "Traditional" to "Composite", generated the diff using the `--expand-tabs' switch, and made sure there's no unnecessary stuff in the patch. In addition, I've made sure that none of the patched files are generated files (at least, none of them say "generated by..." or anything of that sort). Tracking that stuff down, I discovered that one of the generated files uses tools/pdbgen/pdb/selection_tools.pdb, so the patch tinkers with that too. tools/pdbgen/pdb/selection_tools.pdb, by the way, contains tabs so when I used the --expand-tabs switch, it stripped the tabs from one of the patch context lines. When you apply the patch, you have to use 'patch -l...' to tell patch to normalise whitespace--if you don't, it rejects because it can't match the context. The patch works on cvs HEAD as of 2239 East-coast US time and the result builds fine. It also runs to the extent that the select criteria radio boxes show up when they're supposed to, but it won't open existing .jpg files or let me create a new image--I assume those are unrelated problems.
transferring my emailed comments on this patch here: (applied to CVS HEAD as of 2006-08-03) I get the impression that PDB function signatures are not intended to be changed so often, because of the compatibility issues it could cause. However, it's exactly the PDB interface to this that I'm most interested in. Maybe you should make a variant named by-color-select-full, it could include also the control of whether to select transparent areas, which I've found very important (and the lack of it is somewhat difficult to work around). If you do, then also make a corresponding fuzzy-select-full. gimp_channel_select_fuzzy and gimp_drawable_bucket_fill_full should also have the select_criteria parameter added; gimp_contiguous_region_by_seed seems to support it. Why does the PDB function by_color_select accept the criteria parameter but not use it? gimpbycolorselect.c doesn't compile: gimp_by_color_select_tool_get_mask fails to provide the criteria parameter to gimp_image_contiguous_region_by_color. Nor does gimpfuzzyselect.c provide the criteria parameter to gimp_image_contiguous_region_by_seed. The parameter order for gimp_image_contiguous_region_by_seed should probably be criteria, x ,y) rather than x,y,criteria), to keep the options grouped separately from the seed. For more completeness, you could also add a set of radiobuttons arranged horizontally under the main SelectionEditor display ('r','g','b','h','s','v','*') to change between the different selection criteria usable when operating it.
Comments on the user interface: Select criteria: * select on Composite * select on red * ... .. etc could be turned into Compare by: * Composite * Red ... which is much more manageable. Anyway, even with that setup, it's fairly big.. you should probably put it into an GtkExpander, like the Pressure-Sensitivity options are.
Ok sorry, I got distraced by other things... I agree with David that in the "Select on foo" strings the "Select on" is redundant. I also agree that the SelectCriteria param needs to be added to all functions which finally call the contiguous region functions. Also, PDB functions can't be changed for compatibility reasons, you need to add a new function. I'd even suggest to apply this in two stages: first the internal stuff and then the PDB wrappers, since as David says also the "select-transparent" stuff needs to be exposed in some way. Another thing: isn't "criteria" the plural form? It should actually be "criterion" to be consistent with the naming of other types. Also, this are just too much enum values for aradio group, in such cases we use a combo box instead of a radio group. Setting milestone to 2.4 so I don't forget about it again ;)
Created attachment 70215 [details] [review] patch 0.4 Yet another spin that: Leaves pdb intact, as well as leaving intact a couple of files that were touched by the last patch just to make the parameters match--this version adds gimp_image_contiguous_region_by_seed_with_criteria() and gimp_image_contiguous_region_by_color_with_criteria() to the existing gimp_image_contiguous_region_by_seed() and gimp_image_contiguous_region_by_color() just to avoid tinkering with things that can't or don't yet use the select criteria stuff. Changed the radio-box format to a combo-box. Changed the combo box text to be less redundant. Didn't touch the channel_select, bucket_fill, or Selection Editor stuff--adding that stuff would take a bit of research on my part and gimp is changing so fast that I'm afraid this patch will be obsolete by the time I can get it done. (That seems to be what happened last time--in the week between when I created the patch and when people applied it, things had changed. This patch applies and the result builds and works with the cvs HEAD as of 2006-08-04 13:23 EDT (17:23 UTC).) Would it be possible to commit this branch as-is just I don't have to keep playing catch-up with a moving target?
This patch looks good to me, apart from the following points: * 'criteria' is still used throughout the code, despite it being displayed as 'criterion'. (I have no preference and think both make sense; I just think it should be consistent.) * 'this version adds gimp_image_contiguous_region_by_seed_with_criteria() and gimp_image_contiguous_region_by_color_with_criteria() to the existing gimp_image_contiguous_region_by_seed() and gimp_image_contiguous_region_by_color()' -- this is unneeded, the only places where gimp_image_contiguous_region_by_color() or gimp_image_contiguous_region_by_seed() are used are in the files you already changed: app/core/gimpdrawable-bucket-fill.c app/core/gimpchannel-select.c app/tools/gimpfuzzyselecttool.c app/tools/gimpbycolorselecttool.c -- only Gimp itself uses those functions. Plugins can only use the functions exposed by libgimp. Fix the second point and I'd agree that it would be good to commit now.
Yes, please change it to GimpSelectCriterion, we don't use plural forms in enum names. And actually, it was just bad luck that the two tools changed, GIMP is not moving that fast (I with it would, though :-)
Mapped "criteria" to "criterion" Consolidated the _with_criteria() fcns back to their base fcns and modifed app/core/gimpchannel-select.c and app/core/gimpdrawable-bucket-fill.c to match. Patches, builds, and runs vs. cvs HEAD as of 1635 EDT.
Created attachment 70225 [details] [review] patch 0.5 Would have helped a lot if I'd actually appended the patch...
Thanks, looks fine apart from the added tabs (GIMP source uses spaces). Will commit now anyway and apply some additional formatting changes.
Sorry, forgot the --expand-tabs switch. Thanks.
Notes for after the patch is committed: Changing gimp_channel_select_fuzzy() and gimp_channel_select_by_color()'s parameters is a very small change (only two funcs in the PDB and one in the selection editor call these functions). Bucket fill parameters and tooloptions is a bigger change, involving: * Change the parameters and tool options, adjusting function calls to match. notably, app/widgets/gimpdrawabletreeview.c and app/display/gimpdisplayshell-dnd.c are surprising places to find bucket fill usage. * app/core/gimpdrawable-bucket-fill.c also needs the gimp_drawable_bucket_fill_full() and gimp_drawable_bucket_fill() parameters changed. They are called from a total of 12 lines in various source files (one in the PDB) * Make a new PDB function, variant of gimp-edit-bucket-fill, which exposes the ability to fill transparent areas as well as select by different criterion. I'm willing to do either of those if you want.
You obviously know a bunch more about it than I do--have at it.
Um, too late :) I already did all the changes David suggested while cleaning up the original patch. Will commit later today.
Fixed in CVS. Leaving open so we don't forget about the PDB interface. 2006-08-05 Michael Natterer <mitch@gimp.org> Applied (modified and enhanced) patch from Chris Moller which allows tools to distinguish similar colors not only by composite, but also by R, G, B, H, S and V. Fixes bug #348291. * app/core/core-enums.[ch]: added new enum GimpSelectCriterion which can be one of { COMPOSITE, R, G, B, H, S, V }. * app/core/gimpimage-contiguous-region.[ch]: added select_criterion params and create the region based on difference by the selected criterion. * app/core/gimpchannel-select.[ch] * app/core/gimpdrawable-bucket-fill.[ch]: take criterion params and pass them through to the contiguous region functions. * app/tools/gimpbucketfilloptions.[ch] * app/tools/gimpselectionoptions.[ch]: added criterion properties and GUI to select it. * app/tools/gimpbucketfilltool.c * app/tools/gimpbycolorselecttool.c * app/tools/gimpfuzzyselecttool.c: pass the selected criterion to the resp. core functions. * app/widgets/gimpdrawabletreeview.c * app/widgets/gimpselectioneditor.c * app/display/gimpdisplayshell-dnd.c * tools/pdbgen/pdb/edit.pdb * tools/pdbgen/pdb/selection_tools.pdb: changed accordingly (simply pass GIMP_SELECT_CRITERION_COMPOSITE in most cases). * app/pdb/edit_cmds.c * app/pdb/selection_tools_cmds.c: regenerated.
Created attachment 70389 [details] [review] provides extended pdb interface Adds the following pdb functions: gimp_edit_bucket_fill_full gimp_by_color_select_full gimp_fuzzy_select_full All have been tested.
Thanks for the patch. I am not overly happy with the names you've chosen for the new API but since I can't come up with anything better, I've committed the patch as is. 2006-08-15 Sven Neumann <sven@gimp.org> * app/core/core-enums.h * tools/pdbgen/pdb/edit.pdb * tools/pdbgen/pdb/selection_tools.pdb: applied patch from David Gowers that adds extended PDB interface for gimp-edit-bucket-fill, gimp-by-color-select and gimp-fuzzy-select. Fixes bugs #348291 and #347499. * app/pdb/edit_cmds.c * app/pdb/internal_procs.c * app/pdb/selection_tools_cmds.c * libgimp/gimpedit_pdb.[ch] * libgimp/gimpenums.c.tail * libgimp/gimpenums.h * libgimp/gimpselectiontools_pdb.[ch] * tools/pdbgen/enums.pl: regenerated. * libgimp/gimp.def: updated.