Glad somebody else wants this! But having an option for each tool would be tedious, instead just have it be a in the layer menu "Set Mirror Axis" a little dialog pops up with drop down box for the axis "vertical, horizontal, custom angle", drop box for the position "center, custom" and text boxes that are inactive unless custom is chosen that allow input for the x,y coordinates and angle (0-180). Then everything that gets done on one half of the layer regardless of tool is mirrored in real time. This would make it so much easier to do model sheets for blender because everything would be perfectly symmetrical.

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

Hey,

I agree, that's a nice tool. Actually Aryeom tested this at LGM at the krita workshop and now she wants this in GIMP.
In krita, they have 2 mirror mode: horizontal and vertical (you can use both mirror in same time). But I agree with Jon above. It would be awesome to have more customization possible. Basically angle and position of the mirror line should be editable, because I can easily imagine cases where you don't want to mirror only in the middle.
Also I think the mirror line should be displayable because I remember that Aryeom was sometimes looking like she was groping around the center of the canvas with her tablet pen for the mirror line.

Anyway I guess I'll implement this. :-)

Some small promotional message.
I am starting today a crowdfunding attempt for this feature.

Working proof-of-concept here: http://youtu.be/osSiETyae5c
To participate to the funding: http://funding.openinitiative.com/funding/1578/
Thanks!

Created attachment 299466 [details] [review]
Multi-Stroke core system and mirror symmetries.

Hi,

I finally took some days in the last weeks to write this feature!
Attached is a patch, but I also pushed a branch "multi-stroke" on the main repo that you can just checkout.

A few info on the design:

* I wrote this not as "just a mirror" which would just make the code messy, but as a very generic "multi-stroke" core system. That means that now when we ->paint(), we don't give as a parameter just 1 set of GimpCoords, but a GimpMultiStroke instead, which can hold as many coordinates as you want, and GEGL operation nodes to transform the brush (in case of a mirror for instance, the brush also has to be mirrored).

* To demonstrate the multi-stroke core being not only for mirror painting, I also implemented a "tiling" multi-stroke as an additional class, which was written in 15 minutes top, just to show how easy it is to add new multi-stroke classes (we could just as easily make mandalas with several rotated strokes around a central point, etc.). In some future, I could even imagine that we could quite easily have an API to allow users to just add their own multi-stroke for whatever purpose they may have.
This commit for "tiling" can be seen in the "multi-stroke" branch, I haven't attached it to this bug report.

* I have attached the multi-stroke list to the image (and not to the context for instance), for the reason that I imagine that a given transformation is adapted to an image. It usually does not make sense to have a given mirror set-up and keep it in the next image you open.
Also that means I also save the settings in the XCF (and incremented the version), so that the next time the user opens a file with say a mirror, it is still there at the right place one let it.

* Specifically for the mirror implementation, I also derived the GimpGuide into a GimpMirrorGuide (just added a GimpGuideStyle to allow for changing the style, specifically the color).

* I added this all in the playground.

* As a consequence of being very generic, any multi-stroke (Mirror, Tiling…) works with any of the painting tool (derived from GimpPaintCore basically). Even source tools (heal, clone and such).

* On any paint tool, you'll find the multi-stroke list in the tool options. When you select one, it gets you the available settings (this is also made very generic, so a new multi-stroke does not have to touch the Paint Options gui at all; a new class can just announce what properties are settable by the user, and it would generate the UI).

I guess that's it. Now you can just try. :-)

Nice stuff :) Can we discuss this on IRC please, I have quite some comments,
here a few quick ones:

- the XCF code essentially duplicates libgimpconfig, please simply make
  the object implement the GimpConfig interface

- the image can't keep around things from paint/, the core objects
  should be in core/, just like guides and brushes

- shouldn't the base class be called GimpSymmetry? Especially if it
  is in core/, it could be useful for other things

- the way the thing keeps around and adds/removes guides looks like
  it totally messes up undo

- keeping a "selected_multistroke" in the image feels quite wrong, that's
  the job of tools or whatever manages the symmetry

Disclaimer: I have not tried it yet.

> Can we discuss this on IRC please

I'm on IRC right now. :-)

> the XCF code essentially duplicates libgimpconfig, please simply make
  the object implement the GimpConfig interface

I'll have a look.

> the image can't keep around things from paint/, the core objects
  should be in core/, just like guides and brushes

I have been pondering a lot whether this had to go in paint/ or core/. So I'll just move things around.

> shouldn't the base class be called GimpSymmetry? Especially if it
  is in core/, it could be useful for other things

Well it's more than just "Symmetry", which is why I called the base class "GimpMultiStroke".
For instance the tiling feature is not a symmetry (it's more like some kind of repeated multiple translation all over the canvas). This is why I thought that the term "multi-stroke" was better suited as it is pretty explicit that the base concept is to paint multiple strokes at once.

> the way the thing keeps around and adds/removes guides looks like
  it totally messes up undo

Well I'll have a closer look. This adding/removing guide thing (while keeping a reference on it) was basically a trick to just reuse the guide with the same position. But since in my latter implementation, I keep the value of the guide positions in the GimpMirror class, I may as well just really remove it and create a new one on the same position.
That's probably better.

> keeping a "selected_multistroke" in the image feels quite wrong, that's
  the job of tools or whatever manages the symmetry

Well in my first implementation, the tools were managing the mirror, but after some time I decided that a mirror (or more generically a GimpMultiStroke) should be tied to an image. This allows to do some mirror-painting with some specific axis in one opened image, and a different axis in another.
So the image is the object actually tracking what multi-stroke it has selected, no the tool.

Small note: I noticed some issues with multi-painting with some dynamics. I'll work on this a little more, and make the various other fixes you asked for. :-)

Hi Mitch,

So I made a bunch of changes which addresses all or most of your remarks.

- I renamed GimpMultiStroke to GimpSymmetry. I still don't believe that "Symmetry" is the right naming, but I don't feel like discussing this hours. :P
Symmetry is as good as any name since I don't have better proposition right now.

- I moved this to core/gimpsymmetry.[ch], and also core/gimpimage-symmmetry.[ch].

- I serialize/deserialize with GimpConfig.

- I removed the GimpMirrorGuide, and instead simply added a concept of "custom" guide in GimpGuide, along with a new constructor gimp_guide_custom_new() which allows to create a guide with a custom style of your choice (i.e. other cairo patterns/colors).

These custom guides won't be saved (this is up to the "creator" of these guides to save their state if needed), other than this they act exactly as other guides. This allows to create guides very easily for other guiding purpose (on IRC, you said we could drive GEGL ops for instance, etc.).

- s/guint/gint/ where not necessary.
s/nfoo/n_foo/
No 0/NULL/FALSE initializing.

- I don't keep the mirror guides around. I just remove/unref them and create new ones when needed (you were afraid it would mess with undo).

- Various fixes and improvement.

As a bonus, I added a "Mandala" symmetry, which is basically a multiple rotation of a stroke around a single center. Another good example of using the custom guides for a specific purpose.

Mitch, any news on review? I have just rebased the branch multi-stroke so that it works good with any latter changes and just waiting for review. :-)

Few comments from me having built it for the first time:


I checked out the branch multi-stroke and tried to build it,
but it doesn't build. It seems that a generated file (app/pdb/guides-cmds.c)
has been modified and it gets overwritten when building from scratch.

I think that 'tools/pdbgen/pdb.pl' should be modified instead
to make the modification permanent.



Saving and loading an xcf with a symmetry parasite, valgrind
shows few memory leaks.

The GList*  returned by 'gimp_image_symmetry_list' is not freed in few places
and the char* returned by 'gimp_symmetry_parasite_name' is passed directly to
functions that do not free it and so it is leaked.

And I'm not sure that not updating the iter->prev here:

https://git.gnome.org/browse/gimp/tree/app/core/gimpbrushcache.c?h=multi-stroke#n240

is correct, I think the next time g_list_remove_link is called for
that link the wrong 'prev' will see its 'next' pointer changed to the
wrong link.

(In reply to Massimo from comment #12)
> And I'm not sure that not updating the iter->prev here:
> 
> https://git.gnome.org/browse/gimp/tree/app/core/gimpbrushcache.c?h=multi-
> stroke#n240
> 
> is correct, I think the next time g_list_remove_link is called for
> that link the wrong 'prev' will see its 'next' pointer changed to the
> wrong link.

sorry that's correct because g_list_remove_link sets it to NULL

> I checked out the branch multi-stroke and tried to build it,
> but it doesn't build. It seems that a generated file (app/pdb/guides-cmds.c)
> has been modified and it gets overwritten when building from scratch.

Indeed! How silly of me.

> I think that 'tools/pdbgen/pdb.pl' should be modified instead
> to make the modification permanent.

After searching, this was actually tools/pdbgen/pdb/image_guides.pdb. which had to be modified. (a @headers list in the end).

> Saving and loading an xcf with a symmetry parasite, valgrind
> shows few memory leaks.

> The GList*  returned by 'gimp_image_symmetry_list' is not freed in few places
> and the char* returned by 'gimp_symmetry_parasite_name' is passed directly to
> functions that do not free it and so it is leaked.

Argh!

All issues reviewed here fixed. I also rebased the branch against last origin/master and updated origin/multi-stroke, now rebased and fixed.

Anything else? :-)

For the record, Massimo answered on IRC that it was all ok on his side. He was just surprised by the default of the tiling which are all at 0 at start.

The reason is that I don't want too many strokes at the same time by a random default, because it could slow down GIMP (like tiling with hundreds of strokes in your canvas). It could be possible though to make nicer defaults.

Mitch > You were right about the symmetries being in tool options as a weird stuff, because they are attached to the image. So I changed. Now the symmetries are managed through their own dock.

I have also rebased to recent code, and squashed a bunch of commits together. This way, it will be a lot easier to review for you. This is in the branch origin/symmetry.
Could you review? :-)

Review of attachment 299466 [details] [review]:

This patch is old. Please see the branch origin/symmetry.

I just checked out the symmetry branch and played a little with it.

I think that the "symmetry" image property should not be
a gint property because on my platform a GType is declared
gsize (aka gulong) and gints are too small to contain all
possible values a GType assumes. (It exists a g_param_spec_gtype)

https://git.gnome.org/browse/gimp/tree/app/core/gimpimage.c?h=symmetry#n632

running:

LD_PRELOAD=libasan.so.2 LC_ALL=C gimp-2.9 |& tee out.log

gimp crashes as soon as I select a symmetry type or as soon as
I open an xcf with symmetries. This is AddressSanitizer output

>(gimp-2.9:29692): GLib-GObject-CRITICAL **: g_object_new: assertion 'G_TYPE_IS_OBJECT (object_type)' failed
>ASAN:SIGSEGV
>=================================================================
>==29692==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000048 (pc 0x0000006f9ab6 bp 0x7ffc3570a0a0 sp 0x7ffc3570a090 T0)
>    #0 0x6f9ab5 in gimp_image_symmetry_new /home/massimo/devel/tmp/gimp/app/core/gimpimage-symmetry.c:69
>    #1 0x6df6c6 in gimp_image_set_property /home/massimo/devel/tmp/gimp/app/core/gimpimage.c:900
>    #2 0x7f72130551e2 in object_set_property /home/massimo/devel/obj/glib/gobject/gobject.c:1421
>    #3 0x7f72130551e2 in g_object_set_valist /home/massimo/devel/obj/glib/gobject/gobject.c:2165
>    #4 0x7f72130558be in g_object_set /home/massimo/devel/obj/glib/gobject/gobject.c:2275
>    #5 0x6f9c63 in gimp_image_symmetry_select /home/massimo/devel/tmp/gimp/app/core/gimpimage-symmetry.c:163
>    #6 0x6052f3 in xcf_load_image /home/massimo/devel/tmp/gimp/app/xcf/xcf-load.c:303
>    #7 0x60241b in xcf_load_invoker /home/massimo/devel/tmp/gimp/app/xcf/xcf.c:308
>    #8 0x67512b in gimp_procedure_execute /home/massimo/devel/tmp/gimp/app/pdb/gimpprocedure.c:333
>    #9 0x66fa39 in gimp_pdb_execute_procedure_by_name_args /home/massimo/devel/tmp/gimp/app/pdb/gimppdb.c:322


BTW here:

https://git.gnome.org/browse/gimp/tree/app/core/gimpimage-symmetry.c?h=symmetry#n179

'last_image' is initialized to NULL and never assigned, probably
'image' should be assigned to it when identity is reassigned, but
I don't understand what's going on here, I've just seen that
'gimp_image_symmetry_new' is called once per brush stamp.

Anyway without AddressSanitizer it works well and I've read on forums
people that tried it appreciate it, well done.

Yes I noticed the same, please use a GType member and g_param_spec_gtype()

Thanks for the review, Massimo, as always!
I'll have a look, and I'll bother you later this week to review again, Mitch! :-)

I have fixed the GType issue. I had to create a GimpGTypeCombobox since I was using a GimpIntCombobox for the UI. This is mostly based on GimpIntCombobox code though (these are the times you wish for templates or duck typing in C to avoid code duplication), as well as the identity transformation issue raised by Massimo.

I have also rebased the code on origin/symmetry.
Mitch, could you have a closer look now? :-)

After additional review by Mitch on IRC, I reviewed the last commit. I don't create a GimpGTypeCombobox and uses a GimpIntCombobox instead, in which I simply save the pointer value in the user-data.

As requested on IRC, the saving/loading part is in a separate commit which is still in holding until we test further.

Pushed:

commit 1f4839288e8463138ec98f19478e2999f221dbdd
Author: Jehan <jehan@girinstud.io>
Date:   Thu Jan 21 21:43:15 2016 +0100

    Bug 648776 - fixes symmetry painting after Massimo and Mitch's reviews.
    
    Use a GType for the PROP_SYMMETRY property of GimpImage, and create
    a default "identity" symmetry for an image.
    I still use a GimpIntComboBox but store the property value in the
    user-data column because gpointer isn't a subset of gint.
    Adds in libgimpwidgets:
    - gimp_int_combo_box_set_active_by_user_data()
    - gimp_int_combo_box_get_active_user_data()
    - gimp_int_store_lookup_by_user_data()
    - gimp_prop_pointer_combo_box_new() to create a GimpIntComboBox and
      attach it to a gpointer property.
    Thanks Massimo and Mitch for reviewing my code.

commit eb25d0cead015493112e1451d609b746c0935235
Author: Jehan <jehan@girinstud.io>
Date:   Thu Mar 26 16:10:48 2015 +0100

    po: add symmetries to POTFILES.in.

commit 1c0a0a47def58c5812c67e92d3f03dc3b9360762
Author: Jehan <jehan@girinstud.io>
Date:   Wed Mar 25 17:19:33 2015 +0100

    app: add a "Mandala" symmetry.
    
    This is basically a multiple rotation around a given center.

commit b5811b05ad64ed9afb8106d26c7510a012481397
Author: Jehan <jehan@girinstud.io>
Date:   Sun Mar 15 17:21:02 2015 +0100

    app: add a "Tiling" symmetry.

commit 76f573c981522162cb68596706931b037bc85867
Author: Jehan <jehan@girinstud.io>
Date:   Wed Jan 27 19:13:17 2016 +0100

    Bug 648776 - mirror symmetries.
    
    You can now set any paint tool to mirror painting relatively
    horizontal/vertical axis or a central point (any combination of these 3
    symmetries).
    This has been implemented as a new multi-stroke core, where every stroke
    is actually handled as a multi-stroke (default of size 1).
    This is also the first usage of custom guides for symmetry guiding.
    Current version has to be activated in the playground.

commit b8fadf3ad7a0416a740bbba6bfe526c7b0d51c3c
Author: Jehan <jehan@girinstud.io>
Date:   Tue Dec 15 02:54:04 2015 +0100

    app: add a "custom" guide concept.
    
    With gimp_guide_custom_new(), you can create a custom guide with a different
    style on canvas (other pattern/color/width). A custom guide won't be saved
    and could be used, for instance, for specific GEGL op guiding.

Mitch > Can I get the save/load commit in the master branch, now that the feature is out of playground? I think it really adds to the feature.

I haven't even started thinking about the logic yet, just did mindless
cleanups, it wasn't really "in" the playground anyway. Please keep that
commit out for a bit. Is it on the branch?

It's not on the remote branch because I split the save/load code, along with some locale rebase, few before I committed to master. I'll force push origin/symmetry with this commit later tomorrow. :-)

origin/symmetry rebased over master, and forced pushed.
Check out the commit cafc616da5ffea79e0b3457ea0fcde8a0eaf66ed which adds the saving/loading of symmetries in parasites.

So... what is the current state of this?

The main stuff is done. I have another bug report that I will look into soon. 

I'd like to know the if save/load part, which had been split and not merge could be merged now, before closing this. Mitch?

Michael, it seems that only remains a bit of feedback to Jehan for finish properly this bug, could you check it please?

I'll finish this myself and self-review I guess.
Anyway I remember what Mitch said last: it needs versionning.
Also I will change a few things in the format of the saving.

Set as blocker for the release.

I have decided saving symmetries is not a blocker. There are much more pressing things we need to work on for 2.10.

Also the feature is there. Only saving the symmetries with the image for further reload is missing. But this is not part of the base feature. So let's just close this as FIXED.

I opened bug 792520 as a reminder for myself of the next improvement by saving symmetries.