Created attachment 102552 [details] [review]
Proposed patch

Created attachment 102554 [details] [review]
Proposed patch

Removed an out of place orphaned comment.

I've tried your patch, Alexia, and it draws much cleaner and responsively:). When zoom is <= 100%, drawing becomes sluggish -- that is, it takes much more movement of the device to travel the same amount of pixels across the canvas. This is particularly noticable with a tablet mapped absolutely to the full screen -- moving near to the canvas edges while drawing probably WON'T end up drawing near to the edges.
In short, it works well when zoom > 100%, less well with mouse at zoom <=100%, and even less well with tablet at zoom <= 100%.
(most of my testing was done with pencil tool + size sensitivity, since ink tool behaves identically to before.)

I believe that it will be necessary to expose the smoothing factor, too. Sven earlier suggested that this could be done by considering jitter factors <0 as smoothing factors (that is, jitter -0.25 == smoothing 0.25). That should definitely be good enough for testing the code

Combining Jitter and Smoothing into a single parameter, controlled by a single slider, seems to make a lot of sense to me. Or does it actually make sense to use both Smoothing and Jitter at the same time?

We should probably do some profiling to find out where the time is spent when working on a zoomed-out view. The patch shouldn't make this case even slower than it already is. sysprof has shown to be a useful tool for profiling GIMP (see http://live.gnome.org/Sysprof).

Created attachment 102582 [details] [review]
patch after coding style cleanup, with some FIXMEs added

I have done some coding style cleanups to the patch and added a few FIXME comments that should be looked at.

The problem that David outlines in comment #3 becomes evident when you enable "Show pointer for paint tools" in the Image Windows section in the Preferences. This behavior is not acceptable and could become a show-stopper for this approach. Can this problem be avoided?

The slowdown is due to overly intensive smooth causing noticeable gap between the  position of the cursor and the the pointer. I made smooth auto dependent on velocity, a behaviour that must be disabled once theres a slider to control it. There's a little algorithmic kink in the smooth that is based on the average, when drawing straight lines. If the smooth factor exceeds the actual need for correction it will start throttling the  cursor movement. Perhaps a detection for this condition is needed, so that this would not happen. With manual smooth factor control this can be minimized however by simply not using factors higher than need.

Adressing FIXME-s
// FIXME: Why use sqrt() here? 
Linear velocity is near unusable in tools. A sqrt function is a) most natural, b) wont change the way ink tool expects its speed. Pippin has suggested that pow(val,0<pow<1) should be used and the pow option made configurable. I have no problem with that, I just dont know how to get this information from GUI and believe this should be a chore for Part Two, dynamics configuration overhaul.

// FIXME: move calculations out of the macros that are
//        evaluated multiple times
          xy_sfactor = MAX (inertia_factor,
                            MIN (shell->last_coords.velocity / 25.0, 0.9));
This is the auto smooth factor boosting by speed that needs to be removed once theres a gui slider for this. Inertia factor is all that should be used, once thats done, and xy_sfactor removed. For testing you can disable the auto smooth boost easily by changing the baove line to

 xy_sfactor =inertia_factor;

P.S Small cursor lag is to be expected with smooth, thats what inertia does. It wont let the tool follow the overly quick noise movements of your hand.

The point is not that there's a small delay. The point is that the stroke does not any longer follow the pointer movement, not even the averaged, smoothened pointer movement. This is unacceptable. If events are discarded, that is fine. But the errors introduced by this needs to be taken into account and corrected with the next event that is not discarded.

About the sqrt(). The variable is called speed, so it should be speed, not the square root of speed. sqrt() is a very expensive operation and it should only be applied when it is needed. So please keep it out of the event filtering and leave it up to the tool to apply a square root when that is needed. At some point we will introduce a configurable curve to map between speed and tool parameters and the square root will go away then. So keep it out of the event filtering.

You also seem to have misunderstood my concern about using expressions in the MIN(), MAX() macros. These macros evaluate their parameters multiple times, so you must not do any calculations in the macros. Just introduce some local variables and move the calculations out of the macros.

The lag is not due discarding either. Discarded events have no influence on the tool. Its an algorithm error. I agree that it needs to be managed and I have just had an idea on how I will do just that. Ill make a new patch for tomorrow.

About the square root, ok. Ill move the sqrt to ink tool then and add a comment that this is to be removed when the curve adjustment becomes possible.

I did understand, and I will do that, my point was that this was temporary code anyway, something that should go away as soon as the slider is in place.

Created attachment 102683 [details] [review]
New way of smoothing, auto smooth force off

This patch does not have lagging issues. The proposal to use the jitter slider for smooth has one drawback, ink tool does not have one. Still, Id love it if somebody could make a patch that brings smooth factor up to th UI level.

Another thing that might be worth bringing out is filter threshold. Lifting the threshold decreases accuracy but can help on computers where tablets overload the CPU.

I'm just testing this now. The patch to app/paint/gimpink.c doesn't work for me, I had to apply it manually. The rest of the patch works fine for me.

That's odd. I generated it from my working copy, applied to a clean trunk, made a new patch, reverted my working copy and patched that with the new one without issues... The regenerated patch was what I posted.

This patch does not include the coding style cleanup that I did on your previous patch. Please redo this patch based on the patch that I uploaded to this bug report.

Created attachment 102838 [details] [review]
Stylefixed and updated patch.

Ive done as neo asked. Along with new smooth method velocity is now represented with a value form 0 to 1. New to this patch is that coasting movement is smoothed to to avoid the adverse effects of smooth suddenly kicking in on touch.

I've commited this change (after rewrapping a comment that extended 80 chars per line):

2008-01-14  Sven Neumann  <sven@gimp.org>

	* app/core/core-types.h
	* app/display/gimpdisplayshell-callbacks.c
	* app/display/gimpdisplayshell-coords.[ch]
	* app/display/gimpdisplayshell.h
	* app/paint/gimpink.[ch]
	* app/paint/gimpinkundo.[ch]: applied patch from Alexia Death that
	adds an event evaluation function that decides if an event is
	needed or can be discarded. As a side-product some useful dynamics
	parameters like velocity are added to the GimpCoords struct. The
	Ink tool is changed to use this information. See bug #508639.

Let's keep this report open until as we expect more changes to come...

I could implement smoothing adjustment via the jitter control, if I could just figure out how to get the calculated smoothing value from the GimpPaintOptions to gimp_display_shell_eval_event.

Oh, come on... This is based on the patch that introduces smoothing in the GimpPaintOptions:


static gdouble
gimp_display_shell_get_smooth_factor (GimpTool *active_tool)
{
  if (active_tool)
    {
      GimpToolOptions *options = gimp_tool_get_options (active_tool);

      if (GIMP_IS_PAINT_OPTIONS (options))
         return GIMP_PAINT_OPTIONS (option)->smooth;
    }

   return 1.0;  /* default smooth factor */
}

See, you're assuming I even understand the gsignal system, I figured out that connecting to tool-changed would provide the appropriate behaviour, but don't understand all the parameters of g_signal_connect or the way that tool-changed seems to accept two different function signatures for signals. And no, I don't have the time to look it up. (more accurately, I don't have the spare mental real estate.). So, this is not approachable enough for me to implement.

David, you would help us a lot if you could refrain from making further comments. So far you only added noise and confusion to this bug report. The code snippet I added was for Alexia; after all she is the one who is working on this.

Created attachment 102929 [details] [review]
Update to already commited smooth patch.

Update to smoothing. This patch introduces a completely new and much more sane smooth algorithm and the temporary GUI sliders to paint tools to adjust it from Sven to play with.

Oh, as cleanup it also moves assignment of the last Coord set into the event_eval body. It's only ever assigned with the function call. No point in keeping it separate.

I have removed the UI part from this patch before applying it and done some further cleanup:

2008-01-16  Sven Neumann  <sven@gimp.org>

	* app/display/gimpdisplayshell-callbacks.c
	* app/display/gimpdisplayshell-coords.c: applied parts of a change
	from Alexia Death. This improves the event smoothing (bug #508639).

Try playing with the pencil tool with 1pixel brush. Something is not quite right there -- if I'm not moving almost purely horizontally or vertically, there is a chance that , when I move the mouse and stop -- then click to paint a pixel, the pointer suddenly moves into the adjacent pixel, so I end up changing the wrong pixel.

The easiest way to see this is to get a curvy shaped image, and begin tracing it with one color, one pixel at a time, with pencil tool. (make sure brushscale == 1.0, and jitter is off)

Can't reproduce this problem. The pixel is always applied exactly where the brush outline shows that it will be applied.

I'll agree with that. It's just that the outline moves just before the brush is applied. It seems to be a rounding error, that means moving the mouse fractionally results in a rollover between pixels. It makes things as simple as drawing a 45 degree line, pixel by pixel, difficult. It makes the cursor try to 'run around' some pixels.
This bug behaves exactly the same, whether I use tablet or mouse.

I think the smoothing is being applied even when drawing is not occurring, and in order to fix this bug , smoothing should only be applied when a paint tool is active and some device button is pressed. Applying this limitation would also fix the other problem I have noticed, when I was drawing hair strands I noted that it was difficult to make very close-together strokes because the smoothing was making my movement across the canvas between strokes inaccurate.

The interaction with the paths tool is also.. interesting. Distances < 1.0 being ignored means that no matter the zoom level, you cannot make fine adjustments (<1 pixel movements) to paths.

Well, this is work in progress. The event filtering should take the brush size and spacing into account when discarding events. And for tools like the path tool, perhaps event filtering shouldn't happen at all. Alexia, will you look into this?

I will. Ive been pretty sure for some time that the tool should dictate the threshold for the filter. Sometimes it is even beneficial to let the user set the threshold. It is the matter of bringing this up one level and defaulting it so nothing breaks.

I will be be able to work on this on Sunday the earliest. I may have something along the lines of a patch on Sunday or Monday evening.

Created attachment 103353 [details] [review]
Patch to bring threshhold up to shell level and default to to 0.5

What needs to be done is setting defaults (and parameters if needed) to tools to adjust this. However, I'm not enough aware of the structure to know where where and how this needs to be done.

David, I failed to repeat any of the pixel line issues you described... Loss of some accuracy is expected when intensive smoothing is used. Thats why smoothing should be easy to adjust when needed. The suitable smooth factor depends both on the use case and the user. Some peoples hands are more shaky than others etc...

Created attachment 106387 [details] [review]
Patch that corrects filter according to zoom level to avoid tracking issues.

Sven pointed out that using 1px brush at >100% zoom level creates tracking issues for drawing cursor. This patch corrects it making sure that filter is not below screen resolution and smooth is off since it is useless at these zooms.

2008-03-02  Sven Neumann  <sven@gimp.org>

	* app/display/gimpdisplayshell-callbacks.c
	* app/display/gimpdisplayshell-coords.c
	(gimp_display_shell_eval_event): applied slightly modified patch
	from Alexia Death as attached to bug #508639.

Are there still further issues with this patch or can the bug report be closed?

If this is closed, then another bug, relating to exposing the smooth factor in some way for the user, should be opened. for example smudge tool could really benefit from that.

Theres one more patch coming up in these series I believe. The one that puts tools in control of the filter threshold and max smooth that can be applied. From my experiments this kind of smooth is something that does not really require user interaction and is of more value when applied silently. Its a simple angular smooth that is best value at large zooms(try 2.4 vs svn using a tablet at any zoom, and you'll see the difference).

User controlled smooth is something that I think deserves a separate solution using rubber band and event buffer, something that is only useful for paint tools and could be part of the code there IMHO.

Created attachment 107858 [details] [review]
Final smooth and filter patch

This patch makes the filter completely automatic, corrects any tracking issues and gives control over max smooth to tools. Currently all gimp tools get a default smooth of 0 and paint tools get max smooth. By the nature of this smooth, it should have no adverse effects(haven't seen any in my testing) on usability but keeps strokes at large zooms from becoming that jagged.

Created attachment 108493 [details] [review]
Filter and smooth finalization for current trunk

Finalized for current trunk

Applied after some minor cleanups:

2008-04-07  Sven Neumann  <sven@gimp.org>

	* app/display/gimpdisplayshell-callbacks.c
	* app/display/gimpdisplayshell-coords.[ch]
	* app/tools/gimppainttool.c
	* app/tools/gimptool.[ch]: applied patch from Alexia Death as
	attached to bug #508639. This change makes the smoothing depend on
	the active tool.

Can this bug report be closed now?

Yes. Anything else will get a new report.

Ok so let's close it

Thanks for the patch Alexia!