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

Buckettool fill bugs #1772

Merged

Conversation

MrStevns
Copy link
Member

@MrStevns MrStevns commented Jul 10, 2023

Fixes #1735 as well as another unreported bug.

This PR also removes the Fill to layer feature as i've come to the conclusion that its usecase is too specific, while also discovering bugs that would complicate the code further, in order to fix.

It's been a growing annoyance of mine that sometimes a fill won't happen, sometimes caused by a bug, other times because of the rules with are to be applied when filling while dragging the mouse.
@MrStevns MrStevns changed the title Buckettool fill bugs release review Buckettool fill bugs Jul 10, 2023
@J5lx J5lx added this to Needs Review in Bug Fix Priority via automation Aug 8, 2023
@J5lx J5lx added this to Needs Review in Pull Request Priority via automation Aug 8, 2023
@J5lx J5lx added this to the v0.6.7 milestone Aug 8, 2023
Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

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

I’m a bit confused about the undo bug fix. The other parts should be good to go if no one objects to the removal of the fill to feature.

core_lib/src/graphics/bitmap/bitmapbucket.cpp Outdated Show resolved Hide resolved
Bug Fix Priority automation moved this from Needs Review to Review in Progress Aug 8, 2023
Pull Request Priority automation moved this from Needs Review to Waiting for Requested Changes Aug 8, 2023
@MrStevns MrStevns force-pushed the buckettool-fill-bugs-release-review branch from 35a655c to cc359c1 Compare August 28, 2023 17:57
Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

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

LGTM! This new approach is indeed cleaner, well done. I’m a little hesitant to merge the removal of a feature when no one else has chimed in on the PR, but other than that this PR is good to go as far as I’m concerned.

Bug Fix Priority automation moved this from Review in Progress to Merge Ready Aug 30, 2023
Pull Request Priority automation moved this from Waiting for Requested Changes to Approved Aug 30, 2023
@MrStevns
Copy link
Member Author

Awesome and thanks for fixing my typos... 😅 incredible that I managed to look at it for so long and not notice.

@Jose-Moreno
Copy link
Member

LGTM! This new approach is indeed cleaner, well done. I’m a little hesitant to merge the removal of a feature when no one else has chimed in on the PR, but other than that this PR is good to go as far as I’m concerned.

Hey. Sorry about the delay. I'll only try to answer from a user POV if that helps.

First off I'm not going to argue against the removal, but rather I'll try to lay out which coloring workflows might need improvement after it is removed.

Frankly I don't quite understand why it's being removed. I liked the feature, but the technical details elude me and the provided explanation does not seem to quite suffice as a justification for removal.

I do acknowledge that for the main use case which is arguably tv/web production, such feature is not an extreme necessity since other apps work differently.

Potential enhancements suggested after removal

Disclaimer: I tried to limit the scope about digital Ink & Paint approaches for Pencil2D. From a professional animation user POV I took into account the very common need to send animation deliverables to external compositing software.
Most enhancements would require their own PR, if there isn't one already.

Coloring in the current layer

I estimate that when removing the entirety of the FillTo/Reference features, this way of working will surely continue being the prominent way to colorize drawings for animation in Pencil2D.

When handling both aliased & anti-aliased lineart, users might only require the expand feature to fill enclosed areas. Particularly if they want the final result straight from Pencil2D to be flattened.

However I feel it's important for the workflow to become a bit more straightforward to be compatible with other stages in a pipeline (such as compositing), either by implementing one or all of the following in Pencil2D:

  • Have an easy & officially supported way to export to different software that can trace / binarize the lines for coloring (e.g OpenToonz)
  • Facilitate lineart binarization (i.e tracing process)
  • Inform via documentation & training materials that clean-up / lineart drawings have to be made without anti-alias using a specific tool

Note: Quite frankly, the Pen tool drawing algorithm behaves horribly and I don't want to use it at all to draw, so enhancing this should also be part of a proposed solution for binarized lineart drawing.

Coloring in a different layer

If users want to work color separately, for compositing or labor division purposes, they'll have to manually paint each drawing by using brush (outline & color details) + fill tool (color surfaces).

The possibility of duplicating layers (lineart) as well as merging layers (#1206 ) could optimize work efficiency as well. I feel such features should be prioritized as improvements after FillTo is gone.

Conclusion

Overall, removing the FillTo feature might not impact many users right now, but a recommended coloring workflow should be offered in a future release, along proper documentation and training materials.

IMO right now we could focus on streamlining both UI & UX of #1178 since most of the enhancements I've mentioned already have an implementation in that PR.

Regarding the Reference layers option when considering a potential replacement I would suggest discussing layer-based alpha clipping modes such as:

  • masks (paints everything in the canvas except the mask)
  • stencils (paints everything inside the stencyl)

This would allow not just the Bucket / Fill Tool Tool, but any drawing tool to be clipped when drawing & coloring by virtue of the bit masking process, also removing the need for the "reference layers" option.

@MrStevns
Copy link
Member Author

Although i can no longer 100% recall the reasons for removing the fillTo feature, it was related to the usability of the feature and the combination of using the drag to fill feature which caused mismatch in what to expect, when filling on the layer below.

It's not that I don't see the usability of the feature, rather it's because in its current state, it's too easy to make mistakes with it. In order to fix that, I think we need to create a tighter integration to the timeline UI, so that it becomes more intuitive and visually apparent where your fill and stroke appears

@Jose-Moreno
Copy link
Member

@MrStevns Certainly. As my previous message implies, I'm not opposed to remove the feature as long as we have a future replacement planned.

Ofc when keeping any feature that becomes a problem it's better to remove everything until when we have sufficient technical foundations to support and develop said feature. No one wants technical to have debt before even implementing something 😓

While I did outline potential enhancements as alternatives, those are also meant for future iterations of this problem. Either way thank you again for taking a look into all of this.

Hopefully it's not difficult to implement something that fulfills artistic & workflow requirements later on considering some of the suggestions already exist in one way or another🙏

@MrStevns
Copy link
Member Author

MrStevns commented Oct 1, 2023

Merging this one now. Thanks for reviewing Jakob and chiming in on the discussion Jose. I'll think about what we can do in the future to improve on our different pipeline needs like you mentioned.

@MrStevns MrStevns merged commit 1914718 into pencil2d:master Oct 1, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Bucket tool does not take camera transforming into account when filling
3 participants