New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request : GUI-level group [$75 awarded] #462

Closed
zozorg opened this Issue Nov 17, 2017 · 37 comments

Comments

4 participants
@zozorg

zozorg commented Nov 17, 2017

When a complex transformation involving several layers is repeated over and over, the layers panel tends to become painful to browse. It's not possible to group the layers since a group layer limits the scope.
A GUI-level group would merely group layers at the GUI level, not limiting their scope.
For example, if a GUI-level group contains a Translate layer and a Stretch layer, it acts as a translate+stretch layer.


The $75 bounty on this issue has been claimed at Bountysource.

@zozorg

This comment has been minimized.

zozorg commented Nov 20, 2017

Posted a $60 bounty

@zozorg zozorg changed the title from Feature request : "..." ( suspension point ) layer to Feature request : GUI-level group Nov 20, 2017

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Nov 21, 2017

Hello @zozorg! Thank you very much for giving extensive support to Synfig on Bountysource! Right now I am looking for possibility to extend our team with one more hired developer. I already found one candidate with a powerful skills. Bringing him to the team will take some time, but the support you provided with bounties makes this task easier for me. Thank you!

@zozorg

This comment has been minimized.

zozorg commented Nov 21, 2017

Awesome :-) How much does it cost to hire a developer?

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Nov 21, 2017

The candidate I am talking about needs minimum $1k per month. But for first month I can hire him for $700 (because in first month he needs to learn how everything works in the project and thus he will be less effective comparing to consequent months). With your contributions I can reduce the amount of first month to $500-$400, since he can fulfill the remaining amount through bounties. I am considering to get this amount by setting up a dedicated Patreon page.

But first I have to release version 1.3.5. Tomorrow I will finish building binary packages and will start writing release notes (maybe I will even get some demonstration video about new features). ^__^

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Nov 21, 2017

@zozorg, you can currently use "sets" feature for logical layer grouping, though that's quite far from what you're proposing.

Another possible non-radical solution would be to add layer filtering (by name/type/"sets"/etc). As far as i can tell, that might be easier than to do than your "GUI-level group" (at least it was easy to implement this in Qt); it's also more flexible, but on the other hand less permanent (i.e. you need to change filter line each time you need to get to required layer).

As for your proposal, personally i find it too complicated to implement with current code base to be a high priority. If current synfig status is "maintaining mode with patching bugs and minor improvements", it's too radical solution (for one, it breaks forward compatibility, unless it is somehow made optional; for another, it requires changes in multiple parts of code). On the other hand, if we want real development, i think to implement this effectively we'd need rewrite large portions of code. But then i might be biased about this since i mostly abandoned synfig for my from scratch project.

While i was writing this, i came across another idea: combine current "sets" feature with layer tree. E.g. adjacent layers in the same set are visually grouped. This would reduce necessary code changes to layer tree model. This variant of implementing your FR might actually work, i think.

@zozorg

This comment has been minimized.

zozorg commented Nov 21, 2017

@caryoscelus
I didn't think about forward compatibility. But I can see it's possible to add a XML tag to a layer that synfig will simply ignore.
<layer type="circle" active="true" exclude_from_rendering="false" IGNORED_TAG="BLAH BLAH" version="0.2" desc="Circle">
... simple version :
A layer can be marked as "hidden", adjacent hidden layers are grouped ( so not totally hidden ^^ )
... more complex version
A layer can be marked with a "gui_group_ID", adjacent layers of same gui_group_ID are grouped ( GUI groups can't nest. I'm OK with that )
... even more complex:
gui_group_id is some kind of "path" to allow GUI group nesting.

Older versions will simply ignore this tag.
Does this sound feasible?

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Nov 21, 2017

Well, yeah, this

A layer can be marked with a "gui_group_ID", adjacent layers of same gui_group_ID are grouped ( GUI groups can't nest. I'm OK with that )

is pretty much what i suggested in the latter part of my post (except that sets are actually already exist ^_^).

It's just that your initial suggestion was worded in a way that suggested this to be implemented as layer.

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Nov 21, 2017

Interestingly enough, i just discovered (by reading code) that sets can be nested. So basically if you don't mind extra work of tagging everything, it's possible to recreate whatever tree structure you desire.

Example: sets-recreate-groups.sif.txt (remove .txt extension to open in synfig)

sets

@zozorg

This comment has been minimized.

zozorg commented Nov 21, 2017

But I don't always want layers of the same set to group ^^

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Nov 21, 2017

I'm just showing a way how something like this can work now.

As for FR implementation, adding new tags would surely add some work, but i suppose the most of it would still be with tree model itself anyway. The problem is (as usual with synfig) is that code there is not very good and so there's usual dilemma: rewrite that piece or try to understand and then patch it up.

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Nov 22, 2017

So yeah, might work.
gui-groups-1

@zozorg

This comment has been minimized.

zozorg commented Nov 22, 2017

Oh, "REcreate" a tree structure, got it ^^

@zozorg

This comment has been minimized.

zozorg commented Nov 27, 2017

@morevnaproject
How can I message you privately?

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Nov 28, 2017

@zozorg Please drop me a message via this form - https://morevnaproject.org/about/contact/

@zozorg

This comment has been minimized.

zozorg commented Dec 1, 2017

added $15, total $75

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Dec 9, 2017

I can help with implementing this if anyone would be interested in coding it in proper way (i.e. rewrite layer tree model class), but otherwise i'm not interested enough (and i don't like bountysource for its fees and withdrawal terms).

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Dec 10, 2017

Ok, so we agreed with @zozorg on additional off-bounty payment and so i will be implementing this (unless someone would express their interest before i start).

Basic idea (as discussed above) is that grouping is "fake" (and thus can be toggled on/off instantly), i.e. it does not interfere with layer structure in xml in any way. Instead it would use either tags or flag. For the sake of simplicity i'd probably prefer to implement via existing "set" functionality, but if there's a reason not to, i can add separate tagging.

Planned features:

  • toggleable grouping of adjacent layers with same tag
  • drag&drop working in the same way as with normal groups
  • easy tag assigning (current way of setting "sets" is very unfriendly)

Stretch goals:

  • make up/down buttons work the same way as with normal groups (without additional tinkering, they would ignore grouping and break into the group)
  • make visibility toggle work the same way as with normal groups (the most basic implementation would work as set visibility toggle works now, i.e. change layers visibility status)

Also, since i will be rewriting layer tree model (https://github.com/synfig/synfig/blob/master/synfig-studio/src/gui/trees/layertreestore.cpp - it is using push model based on tree store, which leads to increased complexity, overhead and bug proneness), i might be able to improve some of related behaviour along the way, so suggestions are welcome.

Hopefully i haven't forgot anything.

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Dec 11, 2017

First of all I am happy to see some positive dynamic for this feature request. Thank you @zozorg for pushing this and to @caryoscelus for considering this improvement.

That being said, I do not like the idea of implementing this feature by adding new layer tags (or through sets). From my point of view this doesn't fit well into application design. Although it might look easier to implement, this approach leads to a sequence of top-level "quick hacks" and workarounds.

For example, consider following structure:

  • Circle 1 (tag A)
  • Circle 2 (tag A)
  • Circle 3 (no tag)
  • Circle 4 (tag A)
  • Circle 5 (tag A)

Here we have two "fake-groups". If we will select bottom group and move it one step upper (I am assuming that up/down buttons work as described in "Stretch goals"), then the groups suddenly merge together:

  • Circle 1 (tag A)
  • Circle 2 (tag A)
  • Circle 4 (tag A)
  • Circle 5 (tag A)
  • Circle 3 (no tag)

That's counter-intuitive behavior from user's point of view. Also we will have similar problems with copy-pasting operations.

I am perfectly understand the intention to implement this as top-level GUI solution, with minimum intervention to synfig core and keeping forward-compatibility.

But I think in this case it is ok to break forward-compatibility in this case and do full implementation. So I am proposing to consider the following possible ways:

  1. Extend the functionality of existing Group Layer, by adding a "same_context" tag to it. The "same_context" tag instruct group to NOT limit the scope.

I.e.:
<layer type="group" active="true" exclude_from_rendering="false"> - regular group.
<layer type="group" active="true" exclude_from_rendering="false" same_context="true"> - fake group.

  1. Add a new type of group layer (in the same way as we have Group Layer and Switch Group Layer).

<layer type="group" active="true" exclude_from_rendering="false"> - regular group.
<layer type="group_fake" active="true" exclude_from_rendering="false"> - fake group.

With both approaches it is required to instruct core renderer to properly handle fake groups.

@blackwarthog Your opinion?

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Dec 11, 2017

@caryoscelus your email address (the one that linked to your forum account) is giving me bounce (Mail delivery failed).

@zozorg

This comment has been minimized.

zozorg commented Dec 11, 2017

If we go for full implementation, at the GUI level I would expect "fake group layers" to behave pretty much exactly like group layers, with one possible difference.
To keep the panel readable ( = to keep the actual scope of layers obvious ), a fake group could have a special behavior when expanded : its name would get greyed ( or put in italic as if it was "exclude_from_rendering" ), and its content would NOT get indented. I realize some nesting information can be lost this way, but nesting of fake groups isn't really relevant, actual scope is much more important. If we give fake groups an indentation, the scope of the different layers will not be as obvious when everything is expanded.

I suggest a "..." icon ( without "folder", so it clearly stands out from real groups ).

@zozorg

This comment has been minimized.

zozorg commented Dec 11, 2017

@morevnaproject
I think it's not that counter-intuitive if the tag is the name/description of the fake group. If it's called "outlines", you would expect all contiguous outlines to be hidden in a single "outlines" fake group.
If it's not what we want, that kind of aggregation can be prevented by adding some random string after the name/description ( tag="outlines-134fe56d" ), not visible to the user, and the tag would be lost as soon as a layer is moved out of the fake group. Would prevent a tag from being split like in your example, and still have fake groups with the same name.

( I realize it's a "quick hack" and going for full implementation is probably wiser ^^ that's up to you anyway )

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Dec 11, 2017

its name would get greyed ( or put in italic as if it was "exclude_from_rendering" ), and its content would NOT get indented

That sounds like an interesting idea, but i'm not sure that's technically possible to enable expander handles, but remove indentation on per-layer basis (without heavily altering tree view code). So then hiding/expanding would have to be implemented in other way.

your email address (the one that linked to your forum account) is giving me bounce (Mail delivery failed).

That's strange, i'm receiving mails on it. If everything is fine on your side, i can only assume the server could have banned your address because it was used to send notifications.

As for "gui-level" vs "core-level" implementation:

I'm not really sure it would be easier to implement gui-level version. Depending on how renderer works, core-level might be actually simpler.

So setting aside implementation difficulties for now. I find that the whole concept of 'fake groups' can be counter-intuitive to users, no matter how implemented. The general logic of synfig (being an example of layer stack based apps) is that each layer is rendered on top of 'squashed' layers below it (with respect to any grouping, if present). Fake groups break that assumption and implementing them via real layers breaks it more than implementing via toggleable gui-level solution, because in the case of latter it clearly shows that it is something additional that can be turned off.

As far as i can see, the main purpose of the fake groups is to group transforms&filters, because any regular objects can be grouped with normal ones. With that in mind, i don't think it would be as often to encounter up/down & other operations that may work different with the fake groups.

The only real (i.e. present regardless of habit and not considering compatibility) advantage of layer-based implementation is the possible ability to link the whole group as current groups can be (via canvas property). Ah, and also the ability to turn group visibility properly without additional code.

The only real advantage of gui-level implementation is the ability to switch presentation on and off.

Speaking of compatibility,

<layer type="group" active="true" exclude_from_rendering="false"> - regular group.
<layer type="group" active="true" exclude_from_rendering="false" same_context="true"> - fake group.

would break forward compatibility even worse than introducing new type of layer: old versions would load file, but render it wrong and saving it without the attribute.

In conclusion, i should say that, all things considered, in my opinion gui-level implementation is conceptually less of a hack than layer-based.

@zozorg

This comment has been minimized.

zozorg commented Dec 11, 2017

@caryoscelus
Maybe there is another way to put it that wouldn't warrant any GUI "hack", and wouldn't break the "layer stack" aspect.
Fake groups can be seen as filters, so let's call them "filter layers", "composite layers", "custom effect layers" or something along that line, and give them a "puzzle pieces" icon. This would make (somewhat) clear that they are a composition of several elementary transformations.

In my case for example, I would be grouping rotate/translate/stretch layers to go from one set of coordinates to another ; for instance sun -> earth -> moon -> space ship.
I could pack a bunch in one "filter layer" and call it "set_spaceship_coordinates" for example.

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Dec 11, 2017

@zozorg
Well, yeah, i suppose it can be rationalized that way. "Filter stack layer" or "Filter combining layer"..

I think the most priority in deciding this should go to potential usability (by educated user) and implementation complexity (if there's much difference).

Anyway, i've just spent some time investigating renderer and i think the whole system is too tangled for me to find out the point where so-called rendering context is created, i.e. where renderer decides how layer children are processed.

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Dec 12, 2017

and its content would NOT get indented

But if content will not be indented, then how user will determine where the fake group ends (when expanded)?

that kind of aggregation can be prevented by adding some random string after the name/description ( tag="outlines-134fe56d" )

Yes, that would allow to avoid the group merging, if we will go with tag-based approach. And removing tag when layer moved out of group (or copied/cut to clipboard) will solve problem with copy-pasting. Still, at the moment I am more inclined to full-feature solution.

I find that the whole concept of 'fake groups' can be counter-intuitive to users, no matter how implemented.

I do not find this feature counter-intuitive. I think, it is OK to have one type of groups that limits the scope and other type of groups that does not. As long as those groups can be visually distinguished (have different icons).

Speaking of compatibility,
<layer type="group" active="true" exclude_from_rendering="false"> - regular group.
<layer type="group" active="true" exclude_from_rendering="false" same_context="true"> - fake group.
would break forward compatibility even worse than introducing new type of layer: old versions would load file, but render it wrong and saving it without the attribute.

You right. This is not good solution, because saving in old version will remove attribute. Implementing a different layer type would be better. Yes, it will render wrong, but at least user can manually fix file.

Here I have made a quick example by manually adding new group type into SIF file:

fake-group.sif.zip

screenshot_003

You can see the fake-group is displayed as unsupported layer, but it is still possible to access its content. At the moment its content is not rendered (because this is not supported type of layer), so only thing required to do is to instruct rendering engine to handle its content (without limiting the scope).

@caryoscelus

That's strange, i'm receiving mails on it. If everything is fine on your side, i can only assume the server could have banned your address because it was used to send notifications.

Ah, yes. I have wrote this because I am getting a lot of bounces of your forum notifications. This is why I thought something is wrong with your email. ^__^

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Dec 12, 2017

Fake groups can be seen as filters, so let's call them "filter layers", "composite layers", "custom effect layers" or something along that line

Yes, it is a question how to call those fake-groups. We cannot use "Filter Layers" because this is already referenced in documentation to all layers that do filtering (i.e. Distortions, Blurs, etc). "Composite Layers" also often referenced to layers that draw something (Circle, Rectangel, Spline, etc.).

So, yes, we need to carefully select name for this type of layer/feature.

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Dec 12, 2017

But if content will not be indented, then how user will determine where the fake group ends (when expanded)?

Perhaps it could be highlighted in some way, or a separate column with tag could be added.

I do not find this feature counter-intuitive. I think, it is OK to have one type of groups that limits the scope and other type of groups that does not. As long as those groups can be visually distinguished (have different icons).

I suppose it depends on how you think about layers. For me, if we call "fake group" layers, it is counter-intuitive (and it would be implied to be layer in any case since anything in layer dock is called a layer currently). But being counter-intuitive shouldn't really matter that much, i suppose. It is an optional feature, unless you have to work with someone else's files.

Ah, yes. I have wrote this because I am getting a lot of bounces of your forum notifications. This is why I thought something is wrong with your email. ^__^

And there i was wondering why those stopped coming ;)

@zozorg

This comment has been minimized.

zozorg commented Dec 12, 2017

wow, that was quick o_O

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Dec 13, 2017

@zozorg Please test if it works as you like:

Select several layers, then right-click and choose "Group Layer into Filter". ^__^
I think that type of group can be called "Filter Group" (this is exactly what it does).

@morevnaproject morevnaproject added this to In Progress in Releases Dec 13, 2017

@zozorg

This comment has been minimized.

zozorg commented Dec 13, 2017

Seems to work fine! ( linux version )
Thanks! The name/icon is up to you ^^ The current "stacked layers" icon is actually not that bad... Maybe we could just call this a "stack" instead of a group...

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Dec 16, 2017

Glad to hear it works! Thanks to @blackwarthog for implementation.

I also like "stack" word and assigned the same stack icon to "Group Layers into Filter" action.
Still I think it might be better to keep "group" into the name (on code level it is a group). On the other hand, if we will stick to the statement that "Group limits the scope", then it would be logical to replace "Group" with "Stack".

Opinions?

@zozorg

This comment has been minimized.

zozorg commented Dec 16, 2017

Users don't know what happens at the code level anyway. I think the word and icon express well that it's several layers stacked ( and not a single layer like a group ) and thus it can have multiple effects.
The action should be called "group into stack".
The icon for "skeletal deformation" and "free time" should be replaced for a version with only 1 layer ( or a bone and a clock, if you have time for that ^^ )
So that's my opinion...

@caryoscelus

This comment has been minimized.

Collaborator

caryoscelus commented Dec 16, 2017

Incidentally, i think the names in code should reflect the names in interface. While nobody might bother to fix existing code (though that should be as simple as applying refactoring tools in ide or running replace), all new code should really avoid terminology mismatch.

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Dec 16, 2017

Users don't know what happens at the code level anyway.

Agree.

all new code should really avoid terminology mismatch.

Yes, unfortunately in Synfig this is a common issue of mismatching code and UI terminology.

@blackwarthog I found an issue with latest Synfig version (after this feature got introduced):
If you select any layer inside of regular group, then no handles are displayed on the workarea.

@zozorg

This comment has been minimized.

zozorg commented Dec 16, 2017

@caryoscelus
Hopefully @morevnaproject will hire that new developer soon, and this issue will be fixed ; changing the name afterward will add confusion for users.

blackwarthog added a commit to blackwarthog/synfig that referenced this issue Dec 16, 2017

@blackwarthog

This comment has been minimized.

Collaborator

blackwarthog commented Dec 16, 2017

@blackwarthog I found an issue with latest Synfig version (after this feature got introduced):
If you select any layer inside of regular group, then no handles are displayed on the workarea.

Oops! Fixed here: 116761e

@morevnaproject morevnaproject moved this from In Progress to Released in Releases Feb 22, 2018

@morevnaproject

This comment has been minimized.

Member

morevnaproject commented Feb 22, 2018

Now shipped with 1.3.6 - https://morevnaproject.org/2018/02/22/synfig-studio-1-3-6/

There is an interesting effect possible with this new feature ^__^ - https://www.youtube.com/watch?v=lPjY847IO28

@morevnaproject morevnaproject changed the title from Feature request : GUI-level group to Feature request : GUI-level group [$75 awarded] Mar 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment