Skip to content
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

Add ability to source all layers to bucket fill tool #908

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blurymind
Copy link
Contributor

@blurymind blurymind commented Nov 11, 2019

This makes the bucket fill tool use all layers as reference for what area to fill, not just the one you are on. This is made the default.
69063084-c55bb700-0a13-11ea-9516-caa94a78b312

The ability to also source only current layer (old behaviour) is preserved and assigned to a modifier key (ctrl)

Copy link
Collaborator

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @blurymind . I tried the PR, I can see how it might be useful in some situations but I don't think it can be the default behavior. Are there other drawing applications that handle paint bucket + layers in this way? I think such a common tool should really not challenge user expectations.

Even as a modifier, it seems very difficult to predict the behavior of the tool. The current implementation has some bugs when used on transparent pixels and when using undo.

ScreenFlow

So right now, I am not convinced we should move forward with this. Sorry about the negative feedback :/ Not completely closing the door to the modifier option, but it would help if there was any prior art I could look at in other applications.

Copy link
Collaborator

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(switching to request changes)

@blurymind
Copy link
Contributor Author

blurymind commented Feb 3, 2020

Hi Thank you for having a look at this :)
Would you be ok to have it assigned to the modifier then?

So what you are doing there is clicking on a filled area on the layer below. That is why nothing happens - you are filling with black an area that is already black.
This is what happens in other software too, but it is not entirely obvious in piskel, because piskel applies a transparency effect when displaying layers that you are not on. This is what is unique in piskel and is causing your confusion there.

Perhaps the display should not have that effect when you are using the bucket tool, so it doesnt look like something is wrong with the tool.

The way the tool works in all other software is:

  1. get the pixels from all visible layers and combine them into one layer
  2. source that combination and use it to generate a fill area result
  3. draw the resulting pixels on the currently active layer

@juliandescottes
Copy link
Collaborator

juliandescottes commented Feb 3, 2020

Hi @blurymind, thanks for the added info!

This is what happens in other software too

I would be interested to look at examples. I checked GIMP and Aseprite yesterday, they don't work like that. I don't seem to remember that photoshop or paint.net handle the fill/paint bucket tools in this manner either?

Would you be ok to have it assigned to the modifier then?

If I can look at prior art in other applications I will consider it. That's the only way I can gauge if this is somewhat usual for graphical tools.

Thanks!

@blurymind
Copy link
Contributor Author

blurymind commented Feb 3, 2020

@juliandescottes sorry I had to mention that in gimp, in order to get this behaviour, you have to enable "Sample merged"

To be clear, this feature is called Bucket tool - sample merged mode in gimp (I just dont like that name)
sampleMergedGimp

I believe mypaint does it the same way - has the same option

@juliandescottes
Copy link
Collaborator

Thanks!

In the PR the behavior of the tool is not the same as in GIMP. When I clicked in the transparent area above the square from the below layer, then it should have filled the pixels of my current layer in black, in the area corresponding to the connected pixels from the "merged" frame.

Now that I have another implementation example, I will take another look. The issue that will remain is how much we should expose this. We can't possibly add all the options of more advanced tools as modifiers :) I think if we want to open the door to more tool settings, we'll need to have an actual UI to select tool settings

@blurymind
Copy link
Contributor Author

Now that I have another implementation example, I will take another look. The issue that will remain is how much we should expose this. We can't possibly add all the options of more advanced tools as modifiers :) I think if we want to open the door to more tool settings, we'll need to have an actual UI to select tool settings

Hi yes I agree the default behaviour should not be what is implemented, but the way it was before. The implementation should be on a modifier instead.

Ideally I would like it to be a tick box like in gimp, but the current UI doesnt allow for checkbox style tool modifiers and I didn't want to risk making the pr harder to take in by introducing new ui concepts.

Would you be ok with allowing the new behaviour as a modifier, under the condition that it works the same as gimp with "sample merged" enabled? Then once we have some sort of infrastructure to add tool inputs, I will do another pr to turn it into a checkbox rather than a modifier?

@blurymind
Copy link
Contributor Author

blurymind commented Feb 3, 2020

I added a gif on an example use case where this bucket fill mode is needed:
69063084-c55bb700-0a13-11ea-9516-caa94a78b312

The ability to separate your fill layer from your lineart layer!
This is sort of the problem I would like to address. It ties in with overall colouring workflow, which is kind of limited in piskel

@blurymind
Copy link
Contributor Author

I need to update this to work exactly like Gimp, but there will be no point in doing so if it cant be assigned to a modifier

@juliandescottes
Copy link
Collaborator

I need to update this to work exactly like Gimp, but there will be no point in doing so if it cant be assigned to a modifier

Right, sorry for not giving a clear answer. Let's go for a fixed version of the tool, behind a modifier. The last example you shared seems useful for keeping the lineart separated from the rest.

@blurymind
Copy link
Contributor Author

blurymind commented Feb 7, 2020

In that case I will look into making it behave exactly like gimp's, assign it to the modifier instead of being the default and update this pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants