Smoother behavior for airbrush Rate curve
ClosedPublic

Authored by allenmarshall on Jun 18 2017, 11:50 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Summary

This revision fixes an annoyance in the airbrush feature implemented in D5845, where if the Rate curve went to zero momentarily while airbrushing (which often happens at the start of a stroke), the user may have to wait 1 second for airbrushing to resume. The issue is fixed by allowing paintops to update the spacing/timing information of a KisDistanceInformation in between painted dabs. This is something I tried to do in an early version of D5845, but it introduced bugs and performance issues, so it was removed. I believe I have fixed the bugs, and the performance is improved by having spacing updates occur only once every 50 ms, instead of every tablet event. These updates only happen if airbrushing is enabled, to minimize the performance impact on other brush settings.

The main change is allowing paintops to update the spacing every 50 ms when airbrushing, even if no dab is painted. The patch also removes the lower limit of 1 dab per second previously imposed on the Rate curve, as that limit existed mainly to mitigate the problem addressed by this revision.

Test Plan

The problem of having to wait 1 second with a Rate curve linked to pressure should no longer be present; this should be tested. Since this patch makes minor changes to the code for every KisPaintOp subclass (and KisLiquifyPaintop), it should be verified that brush engines (and the liquify tool) still behave as before when airbrushing is disabled. The patch also makes modifications to the information stored in macros by the freehand tool, so it should be verified that macros work the same as before (although there seem to be significant bugs in the macro system even without the patch). The revision author has experimented with the Rate curve, the liquify tool, the macro system, and all selectable brush engines, and they seemed to work as expected.

This patch causes no new unit test failures on the author's machine. It adds a unit test for some of the new functionality in KisDistanceInformation and related classes.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
allenmarshall created this revision.Jun 18 2017, 11:50 PM
Restricted Application added a subscriber: woltherav. ยท View Herald TranscriptJun 18 2017, 11:50 PM
dkazakov accepted this revision.Jun 21 2017, 1:39 PM
dkazakov added a subscriber: dkazakov.

Hi, @allenmarshall!

I have thoroughly tested your patch and tried to scrutinize the code, but I failed to find anything that can cause bugs. Your patch is simply perfect! =)

I also tried to think about consequences of updating the spacing value on a timeout instead of at the moment of painting. The only case when it can change behavior is when 'Spacing' option is active and bound to pressure sensor. I tested it and I have a feeling that with your timeout approach the effect is much nicer than before. I have a feeling that some time in the future we could even make an option for it (or even make default?).

Actually, the effect can be tested even without that special option. To test the effect one should just activate Airbrush feature and set its rate to minimum :)

Test plan of the new feature:)

  1. Set 'Spacing' to 3.0+
  2. Enable Spacing Option and bind it to Pressure sensor
  3. Try to draw a line with very hard pressure so that you see regular circles with spacing 3.0. Then, while you are in between points, slightly release the pen and press it back. Without your feature (old behavior), no additional points appear.
  4. Enable 'Airbrush' feature and set rate to 1.0
  5. Try to draw a line with very hard pressure so that you see regular circles with spacing 3.0. Then, while you are in between points, slightly release the pen and press it back. You will see small circles in between the regular ones.

Personally, I do really like the effect in 5) (with your feature)

libs/image/brushengine/kis_paintop_utils.h
93

Could you add a comment like this?

// 1) This 'if' executes if and only if the the loop above doesn't paint anything. 
//    Any paint operation will reset the accumulators and needsSpacingUpdate() 
//    will automatically become false
// 2) The accumulated time value is updated not during the paint operation, but
//    during the call to getNextPointPosition(), that is, updated during every 
//    paintLine() call
libs/image/kis_distance_information.cpp
364

[nitpicking mode on, please simply discard the comment if you don't agree = ) ]

Using t = 0.0 instead of return would make the code look like other 'if' branches and make it a bit more obvious :)

458

Same t = 0.0 here :)

This revision is now accepted and ready to land.Jun 21 2017, 1:39 PM

Okay, great! I will make the changes you suggested and merge to master.

I see what you mean about the behavior when Spacing is bound to pressure. Once this revision is finished, I will experiment with adding a setting to enable the spacing updates even when not airbrushing.

allenmarshall marked 3 inline comments as done.Jun 21 2017, 7:09 PM

I must be doing something wrong with my commit messages. I included "Differential Revision: https://phabricator.kde.org/D6263" in the merge commit message and pushed, but it doesn't seem to have closed the revision. I don't see an option to close the revision through the Web interface either. Do you have any ideas as to what I am missing?

dkazakov closed this revision.Jun 21 2017, 7:22 PM

Weird... Phabricator has extremely complicated rules about who can close the revision and in what state. I seem to have this option so I'll just close it :)