Optional spacing updates between dabs
ClosedPublic

Authored by allenmarshall on Jun 24 2017, 8:00 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Summary

This patch refactors some of the spacing control code to separate the concepts of distance-based and time-based (i.e. airbrush) spacing in a stroke, so that each can be updated independently of the other. It adds a checkbox in the Spacing option that, when checked, causes the distance-based spacing to be updated every 50 ms during a stroke instead of just when dabs are painted. For time-based spacing, similar updates happen every 50 ms if and only if the Airbrush option is enabled. (The time-based spacing updates for airbrushing were not made optional because airbrushing tends not to work very well without them, at least when the Rate curve option is enabled.)

Before the patch, both distance-based and time-based spacing would be updated between dabs when and only when airbrushing. With the patch, the updating of distance-based spacing can be configured independently of airbrushing.

Test Plan

The changes made in this revision should only be noticeable when the spacing can change during a stroke, that is, when the Spacing curve option is enabled. The following steps (based on @dkazakov 's comments on D6263) can be used to test the new feature.

  1. Set the brush spacing to a high value, e.g. 3.0.
  2. Enable the Spacing option and bind it to the Pressure sensor (with an upward curve going from 0% to 100%).
  3. Start a stroke with the freehand tool, and draw with high pressure. You should see dabs appear far apart.
  4. Just after a dab is painted, reduce the pressure and then continue painting. (Make sure not to reduce pressure so much that the stroke ends.)

There are two possibilities for what can happen when you lighten the pressure and continue moving the cursor:
A. Painting does not resume until you have moved a distance equal to the large distance between dabs when you were using high pressure. After that, painting resumes with the new smaller spacing.
B. Painting immediately resumes with the new smaller spacing based on the lighter pressure.

Before the patch, outcome B would happen if and only if airbrushing was enabled. After the patch, outcome B happens if and only if the Update Between Dabs checkbox is checked in the Spacing option, regardless of whether airbrushing is enabled. (To test this easily without having airbrushing interfere with the test, you can enable the Rate option and set its Strength to zero so that no airbrushing actually happens, even when airbrushing is enabled.)

The revision author has tested this behavior in all selectable brush engines that have the Spacing option, and the behavior seemed to be as expected.

This revision makes changes to code affecting all KisPaintOp subclasses (and KisLiquifyPaintop), so it should be verified that all brush engines (and the liquify tool) behave the same as before when Update Between Dabs is not enabled. The revision author has experimented with all selectable brush engines and the liquify tool, and the behavior seemed to be as expected.

This revision causes no new unit test failures on the author's machine.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
allenmarshall created this revision.Jun 24 2017, 8:00 PM
Restricted Application added a subscriber: woltherav. ยท View Herald TranscriptJun 24 2017, 8:00 PM
dkazakov accepted this revision.Jun 30 2017, 6:57 AM

Hi, Allen!

I have finally managed to review and test your patch! The patch is perfect, please push it! :)

(not related to the patch, just a though about the future)
Our brush system becomes curiosier and curiosier. I fully understand that it does quite a lot of stuff with tons of optional features. I'm just wondering if we could simplify it somehow in the future? :)

This revision is now accepted and ready to land.Jun 30 2017, 6:57 AM

Okay, I have pushed the patch. (I still can't figure out how to close the revision.)

I agree that the brush system could use some simplification. Currently, I don't have many good ideas on how to do that, but I will try to think about it in the future.

rempt closed this revision.Jun 30 2017, 10:04 AM
rempt added a subscriber: rempt.

Adding

Differential Revision: https://phabricator.kde.org/DXXXX

To the commit should autoclose the revision