GNOME Bugzilla – Bug 124225
Paintbrush tool opacity pressure unusable
Last modified: 2004-06-05 16:22:13 UTC
The paintbrush tool tablet pressure sensitivity for opacity handles as if it made many single strokes instead of one long stroke. It still clips the maximum opacity at the tool opacity slider setting, but overlapping lines add to each other. The problem is that the alpha should handle as "darken" within a stroke, ie. the logic should be: if (overlapping_alpha > pixel_alpha) { pixel_alpha = overlapping_alpha; } instead of the current "add": pixel_alpha += overlapping_alpha; // this is wrong This behavior is demonstrated in the following image: http://kig.misfiring.net/gimp_paintbrush.jpg
Please specify the version of GIMP you are using. You can check the version number in the About dialog. Can you please also check the state of the "Incremental" toggle in the paint tool options.
1.3.21 Incremental doesn't affect. Pressure acts as if incremental is on - up to the tool opacity setting, where it clips.
Created attachment 20620 [details] [review] Quick hack patch for paintbrush pressure behavior
I managed to fix the paintbrush with a quick hack, though the fix probably broke something else. http://kig.misfiring.net/gimp_newpaintbrush.jpg
Uh, the above diff is for app/paint-funcs/paint-funcs-generic.h
If you send patches, please create unified diff (cvs -u), preferably from the CVS repository. No need to attach a new diff now since it is small; just a suggestion for the future...
Ah okay, thanks. Sorry for my ignorance. The patch breaks the airbrush tool by the way. I fooled around a bit trying to find if it'd be viable to have the paintbrush use the patched function and other tools the old one, but haven't had success thus far, and my attempts seemed to just introduce unelegantness. Oh well.
I may be missing something, but it looks like all you need to do is to lower the spacing attribute of your brush?
The spacing attribute controls the distance between the brush atoms. The problem is that the brush atoms add to (actually, multiply) each other's alpha even within a single stroke. If you look at the top picture, you can see how it's impossible to paint smooth gradients and surfaces using just tablet pressure. This makes quick painting a good bit more difficult. With the changed behavior, it becomes possible to do things like in the second picture, which has two brushstrokes done in black with the paintbrush, tablet pressure set to control opacity and brush size.
The URLs pointed to by this report don't seem to be working any longer. Unless we get some feedback from the bug reporter, we'll have to close this report.
The domain reg expired, but has been renewed again, so the images should work now. Oh and I was using an Wacom Intuos2 tablet to do the images above, the settings are the same except for the patch. To reproduce the bug, use a tablet with pressure sensitivity, set the pressure to control paintbrush opacity, and try to paint a smooth gradient from zero opacity to full opacity with a single stroke or do smooth medium-opacity areas with a single stroke. The messy painting below was done with using only 100% opacity black paintbrush and 100% white - controlling the opacity and size with pressure. http://kig.misfiring.net/forums/dottington.gif
Created attachment 26185 [details] Image demonstrating unpatched behaviour The tools that use this function are: paintbrush, airbrush (via paintbrush), clone, eraser, smudge, dodgeburn, convolve Here's the call path: combine_mask_and_alpha_channel <- combine_mask_and_sub_region <- combine_mask_and_region <- brush_to_canvas_tiles <- gimp_paint_core_paste <- gimp_paint_core_paste_canvas used by gimppaintbrush, gimpclone, and gimperaser however, there are two calls to gimp_paint_core_paste gimp_paint_core_replace is the other one that seems to be used by convolve, dodgeburn, and smudge
Created attachment 26186 [details] Patched behaviour Note clone tool and eraser tool.
Created attachment 26187 [details] [review] Unified format diff
Created attachment 26188 [details] [review] A patch for using low paintbrush pressure opacities This patch changes the paintbrush pressure opacity to 1.0*pressure, rather than 2.0*pressure, resulting in increased control over opacity at low and high pressures. Eraser tool and others using this double pressure behaviour would benefit from a similar patch.
See also bug #142808.
Created attachment 27911 [details] soft brush behavior I think this is a good idea, but after using the patch, something about the behavior strikes me as a little off (I'd have to think about that a bit more). More importantly, though, the patch breaks soft brushes as you can see in the attachment. Also, I don't think this should be used with the eraser- with a real eraser, you keep rubbing the same spot until the mark is gone. Maybe it could be a new tool or a tool option?
Created attachment 27920 [details] [review] Fixed patch that works with soft brushes
Created attachment 27921 [details] Fixed patch demonstration Just in case you're not convinced, I'm attaching an image to show the behavior of the fixed patch. The patch still needs to be made tool-specific, though.
Created attachment 27922 [details] [review] Fixed patch that works with soft brushes, fixed Just a speed improvement.
Created attachment 27925 [details] [review] fixed patch with tool specifics, proposed for commit I've added a gboolean "stipple" to GimpPaintCore, which is set during each paint tool's INIT_PAINT mode. If it is TRUE, brush marks are made in the usual 'dotted' additive fashion, and if it is FALSE they are made as in the image I posted earlier. These are the settings I came up with for each tool: airbrush: TRUE (the only way it works) clone: FALSE (see Ilmari's examples) convolve: TRUE (generally you want the image to get sharper or more blurry as you keep making marks) dodgeburn: FALSE eraser: TRUE (so it acts like a real eraser) paintbrush: FALSE smudge: FALSE (note: if you're testing and you think it looks broken (especially with the Size option checked), you probably just need to lower the brush spacing.)
If the stipple parameter is a fixed property of the painttool then it should be part of the paint tool class. Here's a short diff that illustrates how to do that: Index: app/paint/gimpairbrush.c =================================================================== RCS file: /cvs/gnome/gimp/app/paint/gimpairbrush.c,v retrieving revision 1.93 diff -u -p -r1.93 gimpairbrush.c --- app/paint/gimpairbrush.c 15 Jul 2003 15:38:24 -0000 1.93 +++ app/paint/gimpairbrush.c 22 May 2004 10:42:22 -0000 @@ -109,6 +109,8 @@ gimp_airbrush_class_init (GimpAirbrushCl object_class->finalize = gimp_airbrush_finalize; + paint_core_class->stipple = FALSE; + paint_core_class->paint = gimp_airbrush_paint; } Index: app/paint/gimppaintcore.c =================================================================== RCS file: /cvs/gnome/gimp/app/paint/gimppaintcore.c,v retrieving revision 1.107 diff -u -p -r1.107 gimppaintcore.c --- app/paint/gimppaintcore.c 21 May 2004 15:23:28 -0000 1.107 +++ app/paint/gimppaintcore.c 22 May 2004 10:42:23 -0000 @@ -188,6 +188,8 @@ gimp_paint_core_class_init (GimpPaintCor object_class->finalize = gimp_paint_core_finalize; + klass->stipple = TRUE; + klass->paint = NULL; } @@ -1880,7 +1882,8 @@ brush_to_canvas_tiles (GimpPaintCore *co xoff * maskPR.bytes); /* combine the mask to the canvas tiles */ - combine_mask_and_region (&srcPR, &maskPR, brush_opacity * 255.999); + combine_mask_and_region (&srcPR, &maskPR, brush_opacity * 255.999, + GIMP_PAINT_CORE_GET_CLASS (core)->stipple); } static void Index: app/paint/gimppaintcore.h =================================================================== RCS file: /cvs/gnome/gimp/app/paint/gimppaintcore.h,v retrieving revision 1.34 diff -u -p -r1.34 gimppaintcore.h --- app/paint/gimppaintcore.h 17 Sep 2003 12:05:10 -0000 1.34 +++ app/paint/gimppaintcore.h 22 May 2004 10:42:23 -0000 @@ -132,6 +132,8 @@ struct _GimpPaintCoreClass { GimpObjectClass parent_class; + gboolean stipple; + /* virtual function */ void (* paint) (GimpPaintCore *core, I'd like to accept such a patch for gimp-2.2 only because it seems to much of a change for the stable branch.
Philip: Good job with the soft brushes, thanks a lot! The logic behind having the eraser tool behave as they are, is to make it possible to paint gradients using pen pressure. If you make the stipples additive, it's very much like throwing the pressure data away and just using the mouse. Also, there already is a switch to toggle additive stipple -- the 'Incremental' checkbox (Well, kinda. It doesn't cap at the opacity setting.) So, in my opinion, it would be preferable to either add a switch to toggle this in-stroke-additive thing, or leave the job to the 'Incremental' switch. The non-additive-stippled eraser can be simulated using a mask and the paintbrush, but, imho, this defeats the purpose of the tool. If you really want to erase at the maximum opacity, turn pressure control off. For an example, try making a smooth opacity gradient with the eraser to quickly make a fuzzy edge to a photo. Yes, masks are a better match for this problem, but there are times when you just don't want to bother with one.
That sounds like a good idea. Make the behaviour depend on the setting of the Incremental toggle. Can we agree on that or do you think that we need an extra option for this? In other words, does it make sense to use "Incremental" without "Stipple" or "Stipple" without setting "Incremental"?
Sorry, but I've never really used the incremental option. Judging the description, though, it sounds like incremental is already the default and only behavior? If this is the case, I think it should be removed from the airbrush tool options (airbrush only works in incremental), and I support linking it to stipple. I agree that it is a fairly big change... but since the old behavior will be available through the incremental option, isn't it simply making the pressure tools and the incremental option 'work'?
The default value for "Incremental" varies from tool to tool. Have a look at the tool options (press Reset to make sure you are looking at the defaults). The change introduces new functionality (and new functions) and is thus not strictly speaking a bug-fix. Since we are not too far from gimp-2.2 it is IMO not necessary to do this change in the stable 2.0 branch. It definitely needs to be made first in the HEAD branch. We can then consider to backport it.
I didn't mean the tool default - I was just wondering if incremental actually changed the current behavior.
Well, yes, of course it works. Try setting a brush to low opacity and draw with the mouse. Compare the result of drawing with and w/o incremental mode.
Ah.. that makes a lot of sense. In that case, airbrush incremental should be kept but should not change the value of stipple. As for paintbrush/eraser... I can't imagine a case in which I would prefer stippling over this behavior without also wanting incremental, so I think a second option would be unnecessary.
Created attachment 27941 [details] [review] cvs head patch with requested changes - put stipple in _GimpPaintCoreClass - changed eraser's stipple to FALSE - added stipple = FALSE to gimp_pencil_class_init Because tools in incremental mode use apply_mask_* instead of combine_mask_*, they act like they always have. The airbrush and convolve tools are the only tools that use stippling otherwise, but the value of stipple doesn't affect convolve or smudge because they also use another function. This should address everything that was discussed.
Wasn't the idea not to have a stipple parameter and use GimpPaintOptions::application_mode instead? Sorry, perhaps I just lost track but I wonder what we still need GimpPaintToolClass::stipple for then.
Created attachment 27962 [details] [review] tool check instead of stipple check No, because like I said, tools in incremental mode don't use the combine_mask functions, which were the ones modified. The only reason that stipple is needed currently is the airbrush. Possibly future tools might use it, but I kind of doubt it. I'm new to the glib class system and the gimp code, so I didn't realize this (see patch) could be done before.
Sorry, but I still don't get it. The patch looks fine but I thought we came to the conclusion that if Incremental is set we'd like to keep the old behaviour and if it is not set we use a modified combine_mask_and_alpha_channel(). Since the function used is a different one for incremental mode anyway, all that would have to be done is to modify the behaviour of combine_mask_and_alpha_channel(). No need to introduce a new function, no need for stipple parameters or to alter the behaviour depending on the type of the paint tool. Is there something I missed?
If the airbrush is in constant mode (not incremental), it does not function properly with the modified combine_mask_and_alpha_channel(). It needs to use the old stipple behavior; otherwise it is useless in constant mode.
Does it make sense to use the airbrush in constant mode at all?
Yes, if you want to cap the opacity with the opacity slider.
But you can't do that with the airbursh anyway. Or at least it doesn't work here and given the way that the airbrush is implemented, I don't see how it could work.
Excuse me, I am wrong; it does work and it seems to make sense to keep that behaviour. Looks like we need to add that new function then :(
Applied to the HEAD branch: 2004-06-05 Sven Neumann <sven@gimp.org> * app/paint/gimppaintcore.c * app/paint-funcs/paint-funcs-generic.h * app/paint-funcs/paint-funcs.[ch]: applied a patch from Philip Lafleur that changes the way that paint is applied during a paint stroke. Fixes bug #124225.