Sven, can you please stop opening enhancement requests in our bug-tracker without prior discussion on the gimp-developer mailing-list. Thanks.

This was my last one.
In the past few days I used GIMP frequently and I didn't want these ideas to
be lost.

I look, if I found any of these in the Bugtracker, but I didn't found
any with the keywords I used. So sorry for the duplicates.

With other projects we use the bugtracker for discussion too, so I wasn't aware of this policy...

Doesn't look like Sven will open a discussion on the mailing-list, so let's reopen this one.

*** Bug 522705 has been marked as a duplicate of this bug. ***

In case someone wants to start working on this, here's a rough outline
of what needs to be done:

(1) Rename the scale() method of GimpBrush to transform().
(2) Add an angle parameter to the transform method.
(3) Implement the rotation for all derived brush classes.
    The difficult part is to do the rotation, efficiently,
    for pixmap brushes.

When we are this far, we can start to consider how the angle should be
controlled by the user. As a start one would add methods to GimpContext
to rotate the current brush so that keyboard shortcuts can be added for
this. Later we can add more sophisticated things then.

The next logical step after adding rotation would be to change the
GimpBrush::transform method to take a GimpMatrix3 parameter and to allow
for arbitrary affine transformations (that includes shearing). I am sure
that there is some efficient code for this out there that we could
use...

Isn't this a duplicate of Bug 323923 ?

Yes, but this report has a better description of what needs to be done in order to implement this.

*** Bug 323923 has been marked as a duplicate of this bug. ***

Assigning it to myself but with future milestone. I will be looking into it.

Created attachment 127670 [details] [review]
Rename all scale related infrastructure to transform

Preparations for adding more transforms.

Commited after cleanups and file renames to trunk rev 27975:

2009-02-01  Martin Nordholts  <martinn@svn.gnome.org>

	Applied patch from Alexia Death that prepares brushes for
	arbitrary transforms by renaming stuff with 'scale' to
	'transform'. Takes us one step closer to fixing bug #520078.

	* app/core/gimpbrush.[ch]
	* app/core/gimpbrushgenerated.c
	* app/paint/gimpbrushcore.[ch]
	* app/paint/gimpsmudge.c

	* app/core/gimpbrush-transform.[ch]: New names of
	* app/core/gimpbrush-scale.[ch]

	* app/core/Makefile.am: Update.

Created attachment 127752 [details] [review]
Very basic rotatio patch

The infrastructure should be OK, but rhe rotation algorthm is not good. The rotation is linear without any interpolation and thus very ugly... Ideas in that department are very welcome. Dynamics support for rotation is not there and wont be Dynamics rework for both tilt and rotation is IMHO its own chore.

Created attachment 127947 [details] [review]
Brush rotation with full integration

Kudos to Sven for doing the serious bit, where I got stuck :)

Created attachment 128027 [details] [review]
Brush rotation with full integration 2

Angle scale now runs from -180 to 180. Patch format should be different.

2009-02-05  Sven Neumann  <sven@gimp.org>

	Bug 520078 – Rotate brushes

	Applied patch from Alexia Death:

	* app/core/gimpbrush.[ch]
	* app/core/gimpbrushgenerated.c
	* app/core/gimpbrush-transform.[ch]: affine transformations for
	brushes. So far only scaling and rotation is supported. The
	transformation is done using nearest-neighbour. This is a
	regression and we need to add back interpolation before the next
	release.

	* app/paint/gimpsmudge.c
	* app/paint/gimppaintoptions.[ch]
	* app/paint/gimpbrushcore.[ch]: allow to control the brush
	rotation angle.

	* app/tools/gimppaintoptions-gui.c
	* app/tools/gimpbrushtool.c: added UI for controlling the
	brush rotation angle.

	* app/actions/tools-actions.c
	* app/actions/tools-commands.[ch]: add actions for controlling the
	brush rotation angle.

I guess we should keep this report open until we have looked at the interpolation issue.

Created attachment 128093 [details] [review]
Fix to make rotation roll over when shortcuts are used.

A minor issue here is the action step size -- it's tiny, even '-skip' variants are extremely slow to change angle. I suggest to use "0.1, 1.0, 15.0" rather than "0.01, 0.1, 1.0", 

Sounds reasonable to me. Can you make a patch for this change?

I agree. Ill make a patch a little later. first, patch that adds direction dynamic.

Created attachment 128196 [details] [review]
Direction dynamic. Makes it possible to make the brush follow the direction of the stroke.

Committed the last patch. Alexia, does this count as FIXED now?

2009-02-08  Michael Natterer  <mitch@gimp.org>

	Bug 520078 – Rotate brushes

	Applied a slightly modified patch from Alexia Death:

	* app/core/core-types.h (struct GimpCoords): add "direction" member.

	* app/core/gimpcoords.c: take direction into account in mix(),
	scalarprod(), length_squared(), manhattan_dist() and equal().

	* app/core/gimpcoords-interpolate.c
	(gimp_coords_interpolate_catmull): same here.
	* app/display/gimpdisplayshell-coords.c

	(gimp_display_shell_eval_event): same here.

	* app/paint/gimppaintoptions.[ch]: add properties for direction
	dynamics and adapt dynamics mixing accordingly.

	* app/paint/gimpbrushcore.c (gimp_brush_core_interpolate):
	"interpolate" direction too (in fact, just copy it from
	last_coords since it doesn't change along a straight line).

	* app/paint/gimppaintcore-stroke.c
	(gimp_paint_core_stroke_emulate_dynamics): emulate direction too.

	* app/tools/gimppaintoptions-gui.c: add GUI for direction dynamics.

Id say its fixed, but Sven wanted to look at possible interpolation on transforming the brushes... So I'm not sure.

Looking at the results of mapping direction to brush rotation, I'd say we still have work to do. There are frequent outliers. Looks like there is more smoothing needed in the direction domain...

Created attachment 128254 [details] [review]
Patch to change the action steppings as proposed

Created attachment 128255 [details] [review]
Fixes to rotation.

fixes (for now) generated brushes rotating the wrong way and adds smooth to direction.

2009-02-10  Sven Neumann  <sven@gimp.org>

	* app/display/gimpdisplayshell-coords.c
	(gimp_display_shell_eval_event): applied patch from Alexia Death
	that introduces smoothing for the stroke direction (bug #520078).

I haven't committed the brush direction change in the attached patch because I think this needs to be addressed differently. Let's mark the patch as committed nevertheless...

Created attachment 128411 [details] [review]
Fixes issues fith rotation direction

2009-02-10  Sven Neumann  <sven@gimp.org>

	* app/core/gimpcoords-interpolate.c
	* app/core/gimpbrush-transform.c
	* app/paint/gimppaintoptions.c
	* app/display/gimpdisplayshell-coords.c: applied patch from Alexia
	Death that fixes the direction of brush rotation (bug #520078).

test, please ignore.

Created attachment 128486 [details] [review]
Fix bug accidentaly introduced with last patch.

Created attachment 128712 [details] [review]
More fixes to rotation

All the obvious quirks should be out now.

Committed with some coding style cleanups:

2009-02-14  Sven Neumann  <sven@gimp.org>

	* app/core/gimpcoords-interpolate.c
	* app/display/gimpdisplayshell-coords.c: applied patch from Alexia
	Death that fixes issues with the new stroke direction code
	(bug #520078).

closing as fixed. Feature is there now and looks stable enough.

We still have a regression here. The brush transform code is really just a placeholder as it is now. It doesn't perform any interpolation.

yes, and that lack of interpolation means that the fix for bug #548631 has become ineffective (since scale < 1.0 with a 1-pixel brush already makes the result disappear). I would reopen it, however I think the problem would be properly addressed by the interpolation required here.

Created attachment 129686 [details] [review]
efficient bilinear interpolation of transformed brushes

I have a feeling the speed could still be improved somehow, so any performance gurus looking for a challenge are welcome to give it a go.

This patch works for most brushes, but very small brushes (1xNpx or Nx1px) it doesn't work at all on (any brush scale other than 1.00 causes the brush to become completely empty/transparent). I think you may need to special case that.

Might make sense to add specific algorithms for these corner cases (just one src row or just one src column). That code should be significantly less complex and it would not hurt performance of the general case.

Without having tried it, the patch looks good otherwise. Tal, would you prefer that we commit this patch and you continue to work on it based on that version or would you prefer to attach an updated patch that addresses the problem outlined in comment #38?

Sven, I prefer we commit the patch now and then continue working on it based on that version.

I need to think carefully about how to implement special algorithms for the 1x1, 1xN and Nx1 source cases.

David, I will try to see what could be causing the issue you mentioned in #38.

Attn Alexia: 
Related to the polishing of the rotation interface, mapping Direction to Color doesn't work correctly. Half of the possible directions map to a range of the gradient, and the other half map to that same range. This is easily detectable when you reset colors to default, select 'FG to BG (RGB)' gradient, and draw using that with Pencil or Paintbrush.
We might also need to review other Direction mappings. IMO Direction -> Hardness is also dubious, and Direction -> Size is definitely wrong (jumps from small -> large after a certain angle ,about the same angle as the split that occurs with mapping to Color). Direction -> Opacity looks correct though :)

Re: comment #16

Something weird is happening, which means that only backward wrapping (eg. -180 becomes +165) works correctly. Try using shortcuts to move past +180 to -165, nothing happens.

There is also a problem with this code when rotating a square brush. There are edges cropped off in the result. I have committed the patch anyway, in the hope that we can solve the remaining issues together...

Created attachment 129832 [details] [review]
Bilinear algorithm accuracy boost and brush_transform_matrix bugfix

re#42 Boosted fraction accuracy from 8 bits to 12 bits to limit the cropping issue.

re#38 Fixed bug in gimp_brush_transform_matrix (corrected y-translation value). Solves brushes sometimes partially or completely disappearing and possibly other issues. 1xn, nx1 and 1x1 brushes work fine for me apart from issues noted below.

Unresolved issues:

1) At some scales < 1 and for some angles, sometimes brush edges are lost or brushes disappear completely if they are very small already. Possibly due to source image undersampling.

2) At some scales < 1, 1x1 pixel brush will disappear at angles < -90 and >= 90. If a 1x1 pixel brush at scale < 1.0 is special cased to give a 1x1 pixel result, the brush no longer disappears at these angles. However, for scale 1.0 the 1x1 brush appears thinner at these angles compared to the other angles. In particular, at scale 1.0, through angles > -90 and < 90 and at -180 the 1x1 brush appears thicker and may occupy 2x3, 3x2, 1x3, or 3x1 pixels despite the function returning a forced 1x1 pixel result early (prior to the bilinear algorithm processing). To prevent 1x1 pixel brushes scaled < 1.0 appearing thicker than at 1.0 the special case has been commented out in the patch (see gimp_brush_real_transform_mask) but is there so developers can investigate this.

3) Scaling 1xn and nx1 brushes below 0.5 does not make them smaller. Instead, they keep their 0.5 scale size.


Anyone who wishes to solve any of these issues or improve upon the bilinear algorithm presented, please feel free to do so. Issues 2 and 3 may have something to do with infrastructure code.

3) is enforced elsewhere in the code. Making a brush smaller than half a pixel makes little sense anyway.

With the current state of SVN head, somehow smudge tool ignores brush scale and angle..
So, the brush outline is shown with the scale and angle set in the tool options, but the smudge operation is performed as if brush scale = 1.0 and angle = 0.0.

IMO it's important to fix this before 2.8.


Also, the following tools respect brush scale and angle OK while not supporting 'angle' dynamic mapping, which IMO makes sense for them.

Dodge/Burn
Heal
Clone + perspective-clone
Convolve

This seems relatively simple to implement, perhaps it could be included in the 'brush dynamics' proposed SoC project. Support for Smudge also makes sense but Smudge support could be quite demanding (rotating the smudge buffer as well as the brush mask)

Thank you for the bug finding. I will check out smudge bug later tonight.

Allowing for tools that already respect angle to have angle dynamic is most likely a change of one, perhaps two lines of code. Just enabling the controls is probably all that needs to be done.

It would be nice if we could have a real name instead of just "Tal" in athors.xml. Tal, would you mind providing your real name please?

Alexia, what is the current state?

There are the corner cases for interpolation Tal worked on still open, the core of the bug report is done tho... So Im not sure if it can be closed or not.

Martin, thanks for adding me to authors.xml. I'm happy to be credited with my real name: Tal Trachtman

Ok thanks I've updated your name now

commit 34bccb087617cebb22a0ac6ac31cffde5016de11
Author: Martin Nordholts <martinn@src.gnome.org>

    authors: Tal's real name is Tal Trachtman

Since the bulk of this is done now, let's close this as FIXED and handle remaining issues in separate to-the-point bug reports.