Differential D5845

Movable Airbrush Feature
ClosedPublic
Actions

Authored by allenmarshall on May 13 2017, 9:39 PM.

Details

Reviewers
dkazakov
rempt
Group Reviewers
Krita
Summary

This revision replaces the existing airbrush feature with a newly implemented version that differs in the following ways:

  • With airbrushing enabled, paint dabs will accumulate with time even if the cursor is moving. Previously, airbrushing only occurred in the absence of pointer events.
  • A new curve option for Rate allows the airbrushing rate to depend on factors such as pressure.
  • There is an option to disable the regular distance-based spacing and let the spacing be determined solely by time.
  • Rate is specified as a speed (dabs per second) rather than an interval (milliseconds). (An interval is used internally, but the speed is what shows up in the GUI.)

When airbrushing and distance-based spacing are both enabled, the new airbrush feature behaves similarly to GIMP's Airbrush Tool. Each new dab is applied when the cursor has moved the specified distance (from the last dab) or the airbrush time interval has passed (since the last dab), whichever happens first.

Test Plan

For the paintops that have the Airbrush and Rate options, the settings in these tabs should be tested. This includes testing the Rate and Override Spacing options from the Airbrush tab, and making sure the Rate curve option can control the airbrushing rate. The Rate curve should not be able to reduce the rate to lower than 1 dab per second, even if the curve goes to zero. The Rate curve should not have any effect if the Airbrush option is not checked. The revision author has experimented with these settings in all selectable brush engines that have the airbrush feature, and they seemed to work correctly.

Note: Since the Dyna, Particle, and Sketch brush engines paint constantly when the pointer is moving, it can be difficult to see the difference between airbrushing and not airbrushing when using a tablet with these engines. The difference is easier to see when using a mouse, as it is easier to keep the pointer still. For the Dyna brush engine, setting the Mass to a positive value also makes the difference easier to see.

It has been verified that the patch does not cause any 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.May 13 2017, 9:39 PM
Restricted Application added a subscriber: woltherav. · View Herald TranscriptMay 13 2017, 9:39 PM
rempt added a subscriber: rempt.May 15 2017, 8:16 AM

Looks like a good job to me! I've got two small questions, and Dmitry will later on do a review and check whether everything is fine with isotropic/anisotropic spacing.

plugins/paintops/particle/kis_particle_paintop_settings.h
32

Did you intentionally remove the override here?

plugins/paintops/sketch/kis_sketch_paintop_settings.cpp
37

Is this intentionally removed?

allenmarshall added inline comments.May 15 2017, 10:50 AM
plugins/paintops/particle/kis_particle_paintop_settings.h
32

Removing the destructor override was unintentional. I will update the patch.

plugins/paintops/sketch/kis_sketch_paintop_settings.cpp
37

The removal of some overrides from KisSketchPaintOpSettings was intentional, because the code would have been the same as what is inherited from KisBrushBasedPaintOpSettings (unless I am overlooking something).

allenmarshall updated this revision to Diff 14542.May 15 2017, 11:14 AM

Removed an accidental change that had removed the "override" keyword from a destructor declaration.

dkazakov requested changes to this revision.May 17 2017, 8:39 AM
dkazakov added a subscriber: dkazakov.

Quick testing shows that the timing of the "times spacing" seems to depend on the events flow. And tablet events flow is not uniform. As a result, the spacing between points becomes non-uniform as well. Here is a screenshot of doing quick strokes with airbrush (4ms) and timed spacing (1/250sec), which ideally should have the same spacing, but in real life the result of timed spacing looks not nice.

As far as I can tell, the best thing we can do to make Airbrush work nicely is to disable usual dab generation using KisDistanceInformation when Airbrush is active.

@allenmarshall, could you please ping me on IRC, so we could discuss this feature with painters? I'm not sure what should be the next steps. We need to choose: either to implement some complex algorithm for smoothing time stamps received from the system, or to simplify the system and try to reuse the current Airbrush capabilities and make them more useful for painters.

The bug can also happen because of the problem I mentioned inline. I'm not sure about it, but from the first glance the code doesn't look correct.

PS:
The basic invariant of the feature that we can communicate to the users should be:
"With any settings of Spacing (SP) and Timed spacing (TSP) the dabs should be painted either on every 1/TSP ms or every SP pixels, whatever happens first."

Right not this invariant is not satisfied :(

PPS:
I can also see that the Brush editor became a bit broken, but I'm not yet sure if it is a problem of this patch or just a regression in master.

libs/image/kis_distance_information.cpp
283

As far as I remember the code, the interval should be divided not by newAccumTime, but by (endTime - startTime), because the result returned by getNextPointPosition() is a proportion between start and end, not a proportion between "the last painted point and end". That actually can be the reason of the bug I described in the review.

This revision now requires changes to proceed.May 17 2017, 8:39 AM
dkazakov added a comment.May 17 2017, 8:51 AM

Ok, I tested a clean master and I couldn't see the editor bug, so it might be related to the patch. The bug was the following:

For some reason the "Size" slider on the toolbox was disabled after I played around with Timed spacing option.

The bug might be related to some conflict with recently push "Image Patch" tool, which introduced some disabling of this slider on the toolbox.

allenmarshall added a comment.May 17 2017, 12:59 PM

Thanks for the feedback. I will look into the issues you mentioned. I agree with the invariant "With any settings of Spacing (SP) and Timed spacing (TSP) the dabs should be painted either on every 1/TSP ms or every SP pixels, whatever happens first." If that isn't happening, there is something wrong with my code. I will try to get on IRC soon, though IRC communication may be a bit difficult, as I am in the UTC-5 time zone.

dkazakov added a comment.May 17 2017, 2:23 PM

It might be either your code or non-uniform flow of the tablet events.
You might also want to check the related topic discussed here:
https://bugs.kde.org/show_bug.cgi?id=369349#c32

It is not directly related, but still tells about the flow of tablet
events in Krita.

allenmarshall updated this revision to Diff 14651.May 18 2017, 1:42 AM
allenmarshall edited edge metadata.

Fixed faulty logic in KisDistanceInformation::getNextPointPositionTimed

allenmarshall added a comment.May 18 2017, 1:48 AM

I think I have fixed the bad logic in KisDistanceInformation::getNextPointPositionTimed now, but the timing stability issue is still present. I am experiencing some level of trouble even with the existing airbrush tool, as shown in the attached screenshot. The problem seems to be less noticeable when a slower airbrush rate is used, as the regular distance-based spacing can then "take over" for fast-moving strokes. I noticed that GIMP's airbrush tool limits the rate to 15 dabs per second, while MyPaint limits it to 80. Maybe we just need to set a lower limit on the rate? I will try to get on IRC to discuss this when I can.
I think I have found the cause of the bug where the Size slider would get disabled. I have submitted a fix in D5907: Fix disabling of Size and Flow sliders.

dkazakov added a comment.May 18 2017, 9:59 AM

Ok, I tested the patch, now technically the feature works perfectly and can already substitute the normal Airbrush feature. Now we need to think about the GUI.

Right now there are two problems:

  1. Timed spacing duplicates the functionality of the Airbrushing feature.
  2. One cannot disable normal "distance" spacing functionality and use airbrushing only.

While the second point is "not a regression" the first one is. Probably, we could merge the new feature into the old Airbrushing tab and remove the old airbrushing feature complemtely?

libs/image/kis_distance_information.cpp
281

I'm wondering if such situation can ever happen? Is it legit logically? Probably we could add a safe assert instead of the check?

KIS_SAFE_ASSERT_RECOVER (nextPointInterval <= 0.0) {
    resetAccumulators();
    return 0.0;
}

PS:
You can ignore this comment, it is just nitpicking :)

dkazakov added subscribers: gdquest, Deevad, timotheegiet, kamathraghavendra.May 18 2017, 10:11 AM

Ok, we need to get input from the painters.

@Deevad, @kamathraghavendra, @gdquest, @timotheegiet, @woltherav ping :)

Hi! Could you please tell your opinion, can we remove the only Airbrushing feature and substitute it with the new one? The only difference between them is that the two spacing options are coordinated now, if the Airbrush painted a dab, the next dab will appear either in "spacing" distance after it, or after airbrushing timeout happens. Theoretically, it shouldn't break your presets much, but I would like to know your opinion about it.

In other words, the new Airbrushing feature works according to this rule:
"With any settings of Spacing (SP) and Timed spacing (TSP) the dabs are painted either on every 1/TSP ms or every SP pixels, whatever happens first."

woltherav added a comment.May 18 2017, 1:20 PM

I am okay with removing the airbush and replacing it with this one. Especially as that would also solve this bug: https://bugs.kde.org/show_bug.cgi?id=360414

timotheegiet added a comment.May 18 2017, 3:44 PM

I'm also ok for replacing the old bugged airbrush feature with this one.
And even if it breaks a little a few presets, 4.0 would be a good time for it.

allenmarshall added inline comments.May 18 2017, 5:13 PM
libs/image/kis_distance_information.cpp
281

I wouldn't expect this situation to happen, but I wasn't confident enough in my understanding of floating point math to say for sure that it can't be caused by floating point roundoff error. I believe nextPointInterval can (legitimately) be arbitrarily close to zero, and I feel like it might then get tipped over by roundoff. If you are more confident than I am that that can't happen, I will go ahead and change it to a safe assert.

dkazakov added inline comments.May 19 2017, 7:01 AM
libs/image/kis_distance_information.cpp
281

Ok, if unsure, let's push it as it is (after we get a reply from @gdquest) and experiment with the assert after that :)

kamathraghavendra added a comment.May 19 2017, 8:31 AM

+1 for removing the older airbrush system and replacing it with this one in 4.0. We mush also provide notification to users and brush makers that in 4.0 there is a possibility of preset breakage and advice them to update the brush presets accordingly.

allenmarshall updated this revision to Diff 14729.May 21 2017, 5:31 AM
allenmarshall edited the summary of this revision. (Show Details)
allenmarshall edited the test plan for this revision. (Show Details)

This new patch removes the existing airbrush feature and replaces it with the new movable airbrush feature. The timed spacing settings have been moved into the Airbrush tab. An option to disable the regular distance-based spacing when airbrushing has also been added to the Airbrush tab.

Some notes:

  • Adding the option to disable distance-based spacing required more substantial changes to the code than I expected, so the code should probably be reviewed again. I found it necessary to change the behavior of KisPaintOp a bit, and as a result, the patch contains changes to the code for every paintop. I have experimented with the non-airbrushing paintops to make sure they seem to behave the same as before, but testing from others would probably be helpful as well.
  • The Override Spacing feature is hidden/disabled for the Dyna, Particle, and Sketch paintops, because they don't use the same notion of distance-based spacing as most other paintops. I wasn't sure how an airbrush-only mode for these paintops would be expected to behave, so I just disabled the option.
  • I have updated the revision Summary and Test Plan to reflect the changes.
allenmarshall edited the summary of this revision. (Show Details)May 21 2017, 5:37 AM
allenmarshall edited the summary of this revision. (Show Details)
rempt accepted this revision.May 23 2017, 7:16 AM

I've followed most of the test plan and didn't find anything weird.

dkazakov added a comment.May 23 2017, 7:19 AM

I will check the patch a bit later today :)

rempt added a comment.May 23 2017, 8:01 AM

Oh, there is some superfluous debug still:

krita.general: KisPaintInformation::drawingDistance() Cannot access Distance Info last dab data

dkazakov added a comment.May 23 2017, 8:08 AM
In D5845#111272, @rempt wrote:

Oh, there is some superfluous debug still:

krita.general: KisPaintInformation::drawingDistance() Cannot access Distance Info last dab data

Is it a regression? It usually means the painting inforamtion is used uninitialized with the distance information :(

rempt added a comment.May 23 2017, 8:17 AM

I'll rebuild without the patch and check.

rempt added a comment.May 23 2017, 8:21 AM

Yes that output isn't present in master.

dkazakov added a comment.May 23 2017, 3:09 PM

Hi, @allenmarshall!

I've tested the patch. From the user point of view it works perfectly fine (I couldn't reproduce Boud's problem). But I didn't manage to check the patch codewise. I'll do it late in the evening today.

Just a quick question: is it possible to remove KisPaintOp::paintAt() and KisPaintOp::updateSpacing() completely and keep only KisPaintOp::paintAtAndUpdateSpacing()? Theoretically, it should remove a bit of confusion...

I would really like to avoid this ugly template stuff we have, but I'm not yet sure it is possible...

rempt added a comment.May 23 2017, 3:23 PM

Could that be because of the specific tablet I was testing with, the intuos 3?

allenmarshall added a comment.May 23 2017, 3:31 PM

@dkazakov:
Now that you mention it, I think we could remove KisPaintOp::paintAt. KisPaintOp::updateSpacing is used in KisPaintOpUtils::paintLine to allow for updating the spacing between dabs. I needed to include the ability to update spacing information between dabs because otherwise, if the user has regular spacing disabled and the airbrush rate temporarily goes to zero due to the pressure curve, no more dabs will be painted until the spacing information changes. Without spacing updates between dabs, this causes the stroke to get stuck and not paint any more dabs.

I haven't had a chance to check if I can reproduce the drawingDistance debug message yet. I will look into that as soon as I can.

dkazakov added a comment.May 23 2017, 4:31 PM
In D5845#111340, @allenmarshall wrote:

@dkazakov:

I needed to include the ability to update spacing information between dabs because otherwise,

From the first glance it looks a bit dangerous, since not painting anything is a perfectly normal situation for spatial spacing case: tablet events come much more often than dabs are painted. I will look more closely when get home in about 5 hour. I just looked through the patch in the train :)

allenmarshall added a comment.May 23 2017, 4:39 PM

I guess my changes could potentially cause a performance problem with the high frequency of tablet events. Maybe I should change it so that the spacing information doesn't update on every tablet event, but instead every x milliseconds?

dkazakov added a comment.May 23 2017, 6:53 PM

The initial design of the spacing was that the spacing is updated on every
paint operation. Right now i cannot tell what will be the consequeces of
relaxing this requirement. I'm still not at home

23 мая 2017 г. 19:39 пользователь "Allen Marshall" <
noreply@phabricator.kde.org> написал:

dkazakov added a comment.May 23 2017, 10:33 PM

I briefly looked through the patch. It seems like it doesn't have any serious bugs. Unconditional setting the spacing in paintAt has only one drawback, that is speed and you already mentioned it. I don't know if it is possible to resolve it. I would need to think about it.

I haven't yet fully understand the new logic in KisToolFreehandHelper. Could you explain a bit, why do we need this choice in paintPointOrLine()?

PS:
I will continue looking through the patch tomorrow morning.

libs/image/brushengine/kis_paintop_settings.cpp
320

When loading an (old) preset without the airbrush property, it'll crash either because of division by zero or because of trying to start a timer with NaN interval.

libs/image/brushengine/kis_paintop_utils.h
100

It doesn't look it can break anything, but as you mentioned it may be a trouble with professional tablets that generate events every 4 ms.

libs/ui/tool/kis_tool_freehand_helper.cpp
859

Why do we want to paint a single point when we do already have two distinct paint info objects?

allenmarshall added inline comments.May 24 2017, 3:45 AM
libs/image/brushengine/kis_paintop_settings.cpp
320

Oops. I assumed 1000.0 / 0.0 would evaluate to positive infinity, which should cause KisResourcesSnapshot::needsAirbrushing to return false and prevent the infinity from being passed to a timer. Some quick searching indicates that I was wrong, and floating point division by zero has undefined behavior in C++. I could either change the code to explicitly return infinity when the rate is zero (or not present), or maybe change it to use something like an interval of 1 year instead of infinity, to avoid relying on any floating point edge case behavior.

allenmarshall added a comment.May 24 2017, 3:54 AM

The reason I added paintPointOrLine was because of a weird problem I was having where KisToolFreehandHelper::paintLine wouldn't seem to paint anything (when airbrushing) until KisToolFreehandHelper::paintAt had been called at least once. So I added it to make sure KisToolFreehandHelper::paintAt would get called at the start of the stroke. I will study the code more and try to come up with a better solution, as paintPointOrLine was really just a workaround for a problem I didn't fully understand.

dkazakov added a comment.May 24 2017, 10:01 AM

Ok, I looked through the patch again. Please read the inlined comment about the updateSpacing() thing. I guess I've found the reason why that workaround was needed.

So basically, we have the following points left:

  1. updateSpacing() bug. Probably, we should remove that call completely and check how the curve in "Rate" option affects the stroke. Probably, we should return to the old "Interval" setting? With interval setting it is less easy to shoot into the user's own foot :) Or somehow limit how the curve affects the setting.
  2. The workaround in KisToolFreehandHelper. It might also depend on that division by zero bug, could you check that?
  3. Division by zero in reading the option
  4. [minor] Remove updateSpacing() and paintAt() calls

I would also like to tell that the patch is really awesome! There are still a couple of small bugs, but I eager to see it master! I hope we will do it very soon! :)

Btw, @allenmarshall, have you already got a developer account? I guess pushing the patch into a branch and doing fixes in smaller commits might help us a bit :)

libs/image/brushengine/kis_paintop_settings.cpp
320

Yes, it also depends on the CPU model and compiler. I cannot remember the exact details, but it might also depend on the state of the registers or a compiler flag that issues a special instruction to select the desired behavior every time you do the floating point maths. Without special treatment it can either return +inf or NaN.

Yes, returning something like 1000/0.01 would be the best approach, and consistent with the GUI :)

libs/image/brushengine/kis_paintop_utils.h
100

Strange thing: if I make this 'if' unconditional, then the checkbox "Override Spacing" stops working and the spatial spacing always generates dabs.

In my understanding, always calling updateSpacing() shouldn't change anything, because it just "updates information about previously (potentially) painted dab". So either I don't understand something, or it can lead to some artifacts when airbrushing rate is set to very low values...

UPD:
Yes, just tested:

  1. Set low value to Airbrushing rate (e.g. 0.01)
  2. Set low spacing value (also 0.01)
  3. Set "Override spacing" checkbox

In this case the line is still controlled by spacing, while it is expected to not to be :)

There are some circumstances where the effective spacing can temporarily be such that no dabs will ever be painted

What should be the spacing value that the brush is stuck?

In the spatial case, spacing means "the distance hovered by the pointer since the last painted dab until the next dab is painted". That is, even if the effectiveSpacing value is 0, the dab will be painted anyway.

For the timed case I guess it should work like that, except that spacing means "the time passed since the last painted dab until the next one is painted".

Can it be that your "stuck" case happens because the "Rate" curve multiplies the current "Airbrush Rate" (using pen pressure) by zero, and the timed spacing is set to +inf? Then this 'if' updates the spacing with a different pen pressure and '+inf' is substituted with something else and the brush in "unstuck" again?

I guess that was the reason why initially the author of the feature used "interval" instead of "rate" :)

Btw, in the slider the user can also set the Rate to 0. I guess it should be limited to something like 0.01.

dkazakov added a comment.May 24 2017, 10:20 AM

Hi, @kamathraghavendra, @woltherav, @timotheegiet!

Could you help us a bit?

In this patch we changed the meaning the Airbrushing option from "Interval" to "Rate per second". From the first glance, the difference is very subtle, but it is not. The point is that we also have a "Rate" option that can be linked to the airbrushing rate. And this option can be controlled by the pen pressure, and the pen pressure can be zero(!). It means that with the default curve, Airbrushing Rate can be easily become zero as well. Which would mean that the Airbrushing interval will become +infinity and one would never see a dab :) Could you suggest us any idea how to workaround this issue from the user's point of view?

Right now I have the following ideas:

  1. Limit the maximum airbrushing interval by some value, e.g. 1 second... (Rate is not smaller than 1).
  2. Return back to "interval" options (I'm not sure if the curve works in a user-friendly way in this case).

And if we allow airbrush rates below 1, we will have to handle one more probelm. What happens if the first dab of the stroke has been painted with very slight pressure and the rate is "once 10 seconds", but while we were waiting for these 10 seconds to pass, the pressure changed and the rate become "10 times per second"? Should we update our timer or not?

Basically, limiting the rate by not slower than once per second partially resolves this problem: the rate will be updates at least once per second :)

allenmarshall added a comment.May 25 2017, 7:11 PM

I was able to reproduce the KisPaintInformation::drawingDistance() warning message. It seems to happen whenever certain curve options, including Size and Rate, are set to depend on Distance. I think the problem comes from my separation of the painting code from the spacing update code. I believe the distance info is registered with the KisPaintInformation when calling doPaintAt, but not when calling doUpdateSpacing. Therefore, spacing updates that depend on the distance information being registered encounter problems.

I just finished applying for a developer account, so I should be able to push to a branch and make more incremental changes there soon.

I think setting a minimum rate of 1 dab per second makes sense. I thought allowing the rate to go to zero wouldn't be a problem, but it seems to be causing issues. We could implement the restriction by switching back to an Interval curve instead of a Rate curve, or by adding some extra logic in the code that handles spacing. I am inclined to do the latter, as I find the Rate curve to be a more intuitive idea from a UI perspective. However, I am not sure if the same is true for other users. In any case, disallowing very slow rates will probably make it possible to switch back to only updating the spacing when painting a dab.

allenmarshall updated this revision to Diff 14900.May 27 2017, 11:59 PM
allenmarshall edited the test plan for this revision. (Show Details)

I have made a new version of the diff that addresses some of the issues, but I think further discussion is needed. In the new version, KisDistanceInformation clamps the airbrush interval to the range [0.5, 1000.0], meaning the rate can't be less than 1 dab per second. I changed the code back to only updating the spacing when a dab is painted, which should fix the problems I introduced with the previous diff. It is still possible to get an unpleasant effect where the pressure can make the dab rate slow (but no slower than 1 dab/s), forcing the user to wait 1 second before the next dab. This especially happens at the start of strokes, as the pressure is low when the pen first touches the tablet. The ideas I have for solving this are:

  1. Increase the minimum rate to more than 1 dab per second, so that it won't be as noticeable.
  2. Implement some form of spacing updates that can happen between dabs. From my previous attempt at this, it seems prone to causing bugs and performance issues.
  3. Leave it as is and let users work around it by setting a Rate curve that doesn't go too low. (Possibly changing the default Rate curve as well?)

We need to discuss which idea works best, or if there are any other solutions.

I also looked into the issue that led to the paintPointOrLine workaround. I have removed paintPointOrLine, but the workaround is still present in a cleaner form by having KisToolFreehandHelper::initPaintImpl call KisToolFreehandHelper::paintAt at the end. I figured out that the reason I needed a workaround like this was that, when painting a stroke, the spacing information is initialized to a default value that has a distance spacing of zero and airbrushing disabled. The spacing information from the actual paintop doesn't get used until after the paintop's paintAt method has been called. Normally, this happens as soon as the cursor is moved due to the default spacing of zero. However, some paintops (Dyna, Particle, Sketch) don't always call paintAt in their paintLine methods, so the spacing never actually gets updated during the stroke. This wasn't very noticeable before because the default spacing of 0 works fine with the Dyna, Particle, and Sketch paintops. However, the default of having airbrushing disabled can cause the whole stroke to never airbrush. Some ideas for fixing the issue are as follows:

  1. Leave the (now cleaner) workaround in place.
  2. Change the initial default spacing to have airbrushing enabled. (This might cause the opposite problem, making Dyna, Particle, and Sketch airbrush when they shouldn't.)
  3. Add some function in KisPaintOp allowing paintops to provide the initial spacing.

I feel like the first option is the best, but discussion is needed. A side effect of the workaround is that the first dab now gets painted on the first pointer event rather than the second. This is easiest to see using a mouse. If you click the mouse button on the canvas and hold the button without moving the mouse, a dab now gets painted when you click, while without the patch it would only get painted when you start moving the mouse. I don't think this causes any problems, but I could be overlooking something.

There is also the issue of whether it is better, from a UI perspective, to have a Rate curve that gets ignored when it goes below a certain minimum rate, or to have an Interval curve that could be slightly confusing since higher means slower. I have implemented the former in the new version of the patch, but I am not really sure which is better.

I have also removed the division by zero and reliance on the behavior of floating point infinity. I have pushed the code changes into the branch allenmarshall/movable-airbrush in the Git repository.

rempt added a comment.May 30 2017, 10:12 AM
  1. Leave the (now cleaner) workaround in place.
  2. Change the initial default spacing to have airbrushing enabled. (This might cause the opposite problem, making Dyna, Particle, and Sketch airbrush when they shouldn't.)
  3. Add some function in KisPaintOp allowing paintops to provide the initial spacing.

    I feel like the first option is the best, but discussion is needed. A side effect of the workaround is that the first dab now gets painted on the first pointer event rather than the second. This is easiest to see using a mouse. If you click the mouse button on the canvas and hold the button without moving the mouse, a dab now gets painted when you click, while without the patch it would only get painted when you start moving the mouse. I don't think this causes any problems, but I could be overlooking something.

That actually sounds like an advantage, so I'm fine with the workaround.

There is also the issue of whether it is better, from a UI perspective, to have a Rate curve that gets ignored when it goes below a certain minimum rate, or to have an Interval curve that could be slightly confusing since higher means slower. I have implemented the former in the new version of the patch, but I am not really sure which is better.

This sounds good to me as well.

dkazakov added a comment.May 30 2017, 11:02 AM

Hi, @allenmarshall!

I have tested your patch. From the user's point of view it works almost perfect now.

If you click the mouse button on the canvas and hold the button without moving the mouse, a dab now gets painted when you click, while without the patch it would only get painted when you start moving the mouse. I don't think this causes any problems, but I could be overlooking something.

Yes, you suspicion is correct. It causes a problem when one connects rotation of the brush to the "Drawing Angle" sensor. Skipping of the first event was used to calculate drawing angle/drawing speed correctly. Now, with the patch applied it doesn't work anymore. The line always contains 'zero-rotated' dab in at the start position.

You could probably try to disable this extra paintAt() when airbrushing is disabled at least? Could it help?

For the rest, there are only three small code-related issues. It looks like we will merge the patch quite soon! :)

libs/image/kis_distance_information.cpp
293

[nitpickingmode=on]

Qt supports simplified version :)

timedSpacingInterval = qBound(MIN_TIMED_INTERVAL, m_d->spacing.timedSpacingInterval(), MAX_TIMED_INTERVAL)

[nitpickingmode=off]

libs/ui/tool/kis_tool_freehand_helper.cpp
806

Critical

Please recover the check for !m_d->painterInfos.isEmpty(). It is needed for the case when the timer event comes after the stroke has already been finished. That can happen in the following events flow:

  1. Timer posted an event to the events queue
  2. Tablet Release even has been processed by the helper and the stroke has been finished
  3. Timer even has been delivered and leads to a crash, because the stroke has already been finished.

See line 585 for the comment.

plugins/paintops/libpaintop/kis_paintop_plugin_utils.h
54

Critical

Passing string-based properties is a bit against our Option design. The basic idea of options is:

  1. In the c-tor of the paint op, it initializes the option by calling option.readOptionSetting(settings)
  2. The option reads the settings and saves them as parsed values (parsing strings 1000+ times per second is not a good idea)
  3. On every paintAt() the paintop calls option.apply(pi) or something like that to fetch the needed value

In this particular case you can move airbrushEnabled(), airbrushIgnoreSpacing() and airbrushRate() either into rateOption or into something separate.

The general rule of thumb here: "the paintop should not store/use the settings object while painting"

allenmarshall added a comment.May 30 2017, 11:51 AM

Thanks for the feedback. I will try to get the issues fixed soon.

dkazakov added a comment.May 30 2017, 12:13 PM

Just wanted to tell that I really appreciate your work that to get this patch finished! We had this problem with airbrushing for years and we were always afraid of touching the code because of its complexity. And you have almost fixed it already! I'm looking forward to seeing this patch in master :)

rempt added a comment.May 30 2017, 12:24 PM

Huh, and the patch also applies cleanly to the krita/3.2 branch, meaning that with a bit of luck we can give this to our users really soon.

kamathraghavendra added a comment.Jun 4 2017, 12:49 PM

After this landed in master, I finally got time to test this (Although I am still evaluating this) . I must say this is insanely cool. It allows you to mimic the way an aerosol spray paint works. This is awesome and gives another dimension to the airbrush :)
I am testing the brushes with setting lower Rate, and I notice that you guys know the problems when setting Rate to a lower number, I'll report any problems while testing this. I also notice the edges of the brush (if they are soft) get harder with lower Rate number.

And @allenmarshall Thank you very much for the cool work.

allenmarshall updated this revision to Diff 15314.Jun 9 2017, 7:57 PM
allenmarshall edited the test plan for this revision. (Show Details)

@dkazakov: I think this new diff addresses the issues you pointed out in the previous diff. I have added back the safety check in KisToolFreehandHelper::doAirbrushing, removed the reliance on parsing string options during painting, and added the call to qBound you suggested. I have also changed the KisToolFreehandHelper::paintAt workaround so that the initial paintAt call only happens when airbrushing, which should avoid the zero-rotation dab at the beginning of a stroke when angle-dependent brushes are used without airbrushing. For the case where an angle-dependent brush is used with airbrushing, I added the ability for KisToolFreehandHelper to supply an initial drawing angle based on the most recent cursor movement, so the initial dab's rotation will at least make some sense. I also changed the angle-computing code so that, if two consecutive dabs are at the same position, the drawing angle from the previous dab is reused. Airbrushing and angle-dependent brushes still don't work very well together, since it can be hard to avoid small movements that change the angle when trying to airbrush at a single point, but the behavior seems at least somewhat reasonable now.

I haven't been able to reproduce the problem @kamathraghavendra had with lower rates making the brush edges harder. Maybe it only happens with specific brush settings?

dkazakov added a comment.Jun 11 2017, 8:31 AM

Hi, @allenmarshall!

I have built your branch and tested it a bit: the last problem seem to be gone. I don't have time to check the patch from the code point of view right now, I will do it tonight. I hope we will be able to merge it after that :)

dkazakov accepted this revision.Jun 13 2017, 4:19 PM

Hi, @allenmarshall!

I have checked the patch and tested with a few more affected features (rotation lock, canvas rotation and filtering). The patch works fine now! And the code looks almost perfect as well. So please merge your branch into master! =)

The only comment I had during review was about KIS_ASSERT_RECOVER. If you think that an error is not too severe and Krita can "safely" continue, you can use KIS_SAFE_ASSERT_RECOVER instead. In release builds this version of an assert will not show an emergency dialog, but will just write a warning to the terminal. Just consider using this type of an assert instead.

This revision is now accepted and ready to land.Jun 13 2017, 4:19 PM
allenmarshall added a comment.Jun 13 2017, 5:09 PM

Okay, great! I will change those asserts then merge into master. I still want to do something about the issue of having to wait 1 second if the pressure curve goes to zero, but I will work on that in a separate revision.

dkazakov closed this revision.Jun 16 2017, 12:27 PM

The revision was merged into master by @allenmarshall. Just close it :)

Revision Contents
Changeset List

PathPackages
Mlibs/brush/tests/CMakeLists.txt (1 line)
AMlibs/brush/tests/kis_auto_brush_factory_test.h (13 lines)
AMlibs/brush/tests/kis_auto_brush_factory_test.cpp (51 lines)
Mlibs/global/kis_algebra_2d.h (13 lines)
Mlibs/global/kis_algebra_2d.cpp (16 lines)
Mlibs/image/brushengine/kis_paint_information.h (13 lines)
Mlibs/image/brushengine/kis_paint_information.cc (109 lines)
Mlibs/image/brushengine/kis_paintop_settings.h (41 lines)
Mlibs/image/brushengine/kis_paintop_settings.cpp (18 lines)
Mlibs/image/brushengine/kis_paintop_utils.h (14 lines)
Mlibs/image/kis_distance_information.h (140 lines)
Mlibs/image/kis_distance_information.cpp (169 lines)
Mlibs/ui/tool/kis_resources_snapshot.h (2 lines)
Mlibs/ui/tool/kis_resources_snapshot.cpp (4 lines)
Mlibs/ui/tool/kis_tool_freehand.h (1 line)
Mlibs/ui/tool/kis_tool_freehand.cc (14 lines)
Mlibs/ui/tool/kis_tool_freehand_helper.h (18 lines)
Mlibs/ui/tool/kis_tool_freehand_helper.cpp (133 lines)
Mlibs/ui/tool/kis_tool_multihand_helper.h (3 lines)
Mlibs/ui/tool/kis_tool_multihand_helper.cpp (5 lines)
Mlibs/ui/tool/strokes/kis_painter_based_stroke_strategy.h (2 lines)
Mlibs/ui/tool/strokes/kis_painter_based_stroke_strategy.cpp (5 lines)
Mlibs/ui/widgets/kis_scratch_pad.cpp (6 lines)
Mplugins/paintops/chalk/kis_chalk_paintop.h (4 lines)
Mplugins/paintops/chalk/kis_chalk_paintop.cpp (8 lines)
Mplugins/paintops/chalk/kis_chalk_paintop_settings.h (3 lines)
Mplugins/paintops/chalk/kis_chalk_paintop_settings.cpp (10 lines)
Mplugins/paintops/chalk/kis_chalk_paintop_settings_widget.cpp (2 lines)
Mplugins/paintops/defaultpaintops/brush/kis_brushop.h (4 lines)
Mplugins/paintops/defaultpaintops/brush/kis_brushop.cpp (8 lines)
Mplugins/paintops/defaultpaintops/brush/kis_brushop_settings_widget.cpp (2 lines)
Mplugins/paintops/deform/kis_deform_paintop.h (5 lines)
Mplugins/paintops/deform/kis_deform_paintop.cpp (10 lines)
Mplugins/paintops/deform/kis_deform_paintop_settings.h (1 line)
Mplugins/paintops/deform/kis_deform_paintop_settings.cpp (10 lines)
Mplugins/paintops/deform/kis_deform_paintop_settings_widget.cpp (4 lines)
Mplugins/paintops/dynadraw/kis_dyna_paintop.h (7 lines)
Mplugins/paintops/dynadraw/kis_dyna_paintop.cpp (30 lines)
Mplugins/paintops/dynadraw/kis_dyna_paintop_settings.h (2 lines)
Mplugins/paintops/dynadraw/kis_dyna_paintop_settings.cpp (10 lines)
Mplugins/paintops/dynadraw/kis_dyna_paintop_settings_widget.cpp (6 lines)
Mplugins/paintops/libpaintop/CMakeLists.txt (1 line)
Mplugins/paintops/libpaintop/forms/wdgairbrush.ui (29 lines)
Mplugins/paintops/libpaintop/kis_airbrush_option.h (33 lines)
Mplugins/paintops/libpaintop/kis_airbrush_option.cpp (97 lines)
Mplugins/paintops/libpaintop/kis_brush_based_paintop.h (13 lines)
Mplugins/paintops/libpaintop/kis_brush_based_paintop.cpp (43 lines)
Mplugins/paintops/libpaintop/kis_brush_based_paintop_settings.h (6 lines)
Mplugins/paintops/libpaintop/kis_brush_based_paintop_settings.cpp (11 lines)
Mplugins/paintops/libpaintop/kis_compositeop_option.h (3 lines)
AMplugins/paintops/libpaintop/kis_paintop_plugin_utils.h (87 lines)
AMplugins/paintops/libpaintop/kis_pressure_rate_option.h (15 lines)
AMplugins/paintops/libpaintop/kis_pressure_rate_option.cpp (11 lines)
Mplugins/paintops/particle/kis_particle_paintop.h (7 lines)
Mplugins/paintops/particle/kis_particle_paintop.cpp (29 lines)
Mplugins/paintops/particle/kis_particle_paintop_settings.h (2 lines)
Mplugins/paintops/particle/kis_particle_paintop_settings.cpp (10 lines)
Mplugins/paintops/particle/kis_particle_paintop_settings_widget.cpp (6 lines)
Mplugins/paintops/roundmarker/kis_roundmarkerop.cpp (4 lines)
Mplugins/paintops/roundmarker/kis_roundmarkerop_settings.h (10 lines)
Mplugins/paintops/sketch/kis_sketch_paintop.h (6 lines)
Mplugins/paintops/sketch/kis_sketch_paintop.cpp (27 lines)
Mplugins/paintops/sketch/kis_sketch_paintop_settings.h (2 lines)
Mplugins/paintops/sketch/kis_sketch_paintop_settings.cpp (10 lines)
Mplugins/paintops/sketch/kis_sketch_paintop_settings_widget.cpp (4 lines)
Mplugins/paintops/spray/kis_spray_paintop.h (6 lines)
Mplugins/paintops/spray/kis_spray_paintop.cpp (11 lines)
Mplugins/paintops/spray/kis_spray_paintop_settings.h (2 lines)
Mplugins/paintops/spray/kis_spray_paintop_settings.cpp (10 lines)
Mplugins/paintops/spray/kis_spray_paintop_settings_widget.cpp (4 lines)
Mplugins/paintops/tangentnormal/kis_tangent_normal_paintop.h (4 lines)
Mplugins/paintops/tangentnormal/kis_tangent_normal_paintop.cpp (7 lines)
Mplugins/paintops/tangentnormal/kis_tangent_normal_paintop_settings_widget.cpp (4 lines)
Mplugins/tools/basictools/kis_tool_line_helper.cpp (17 lines)
Mplugins/tools/tool_transform2/kis_liquify_paint_helper.cpp (7 lines)

Diff 15314

View Options

libs/brush/tests/CMakeLists.txt

Loading...
View Options

libs/brush/tests/kis_auto_brush_factory_test.h

Loading...
View Options

libs/brush/tests/kis_auto_brush_factory_test.cpp

Loading...
View Options

libs/global/kis_algebra_2d.h

Loading...
View Options

libs/global/kis_algebra_2d.cpp

Loading...
View Options

libs/image/brushengine/kis_paint_information.h

Loading...
View Options

libs/image/brushengine/kis_paint_information.cc

Loading...
View Options

libs/image/brushengine/kis_paintop_settings.h

Loading...
View Options

libs/image/brushengine/kis_paintop_settings.cpp

Loading...
View Options

libs/image/brushengine/kis_paintop_utils.h

Loading...
View Options

libs/image/kis_distance_information.h

Loading...
View Options

libs/image/kis_distance_information.cpp

Loading...
View Options

libs/ui/tool/kis_resources_snapshot.h

Loading...
View Options

libs/ui/tool/kis_resources_snapshot.cpp

Loading...
View Options

libs/ui/tool/kis_tool_freehand.h

Loading...
View Options

libs/ui/tool/kis_tool_freehand.cc

Loading...
View Options

libs/ui/tool/kis_tool_freehand_helper.h

Loading...
View Options

libs/ui/tool/kis_tool_freehand_helper.cpp

Loading...
View Options

libs/ui/tool/kis_tool_multihand_helper.h

Loading...
View Options

libs/ui/tool/kis_tool_multihand_helper.cpp

Loading...
View Options

libs/ui/tool/strokes/kis_painter_based_stroke_strategy.h

Loading...
View Options

libs/ui/tool/strokes/kis_painter_based_stroke_strategy.cpp

Loading...
View Options

libs/ui/widgets/kis_scratch_pad.cpp

Loading...
View Options

plugins/paintops/chalk/kis_chalk_paintop.h

Loading...
View Options

plugins/paintops/chalk/kis_chalk_paintop.cpp

Loading...
View Options

plugins/paintops/chalk/kis_chalk_paintop_settings.h

Loading...
View Options

plugins/paintops/chalk/kis_chalk_paintop_settings.cpp

Loading...
View Options

plugins/paintops/chalk/kis_chalk_paintop_settings_widget.cpp

Loading...
View Options

plugins/paintops/defaultpaintops/brush/kis_brushop.h

Loading...
View Options

plugins/paintops/defaultpaintops/brush/kis_brushop.cpp

Loading...
View Options

plugins/paintops/defaultpaintops/brush/kis_brushop_settings_widget.cpp

Loading...
View Options

plugins/paintops/deform/kis_deform_paintop.h

Loading...
View Options

plugins/paintops/deform/kis_deform_paintop.cpp

Loading...
View Options

plugins/paintops/deform/kis_deform_paintop_settings.h

Loading...
View Options

plugins/paintops/deform/kis_deform_paintop_settings.cpp

Loading...
View Options

plugins/paintops/deform/kis_deform_paintop_settings_widget.cpp

Loading...
View Options

plugins/paintops/dynadraw/kis_dyna_paintop.h

Loading...
View Options

plugins/paintops/dynadraw/kis_dyna_paintop.cpp

Loading...
View Options

plugins/paintops/dynadraw/kis_dyna_paintop_settings.h

Loading...
View Options

plugins/paintops/dynadraw/kis_dyna_paintop_settings.cpp

Loading...
View Options

plugins/paintops/dynadraw/kis_dyna_paintop_settings_widget.cpp

Loading...
View Options

plugins/paintops/libpaintop/CMakeLists.txt

Loading...
View Options

plugins/paintops/libpaintop/forms/wdgairbrush.ui

Loading...
View Options

plugins/paintops/libpaintop/kis_airbrush_option.h

Loading...
View Options

plugins/paintops/libpaintop/kis_airbrush_option.cpp

Loading...
View Options

plugins/paintops/libpaintop/kis_brush_based_paintop.h

Loading...
View Options

plugins/paintops/libpaintop/kis_brush_based_paintop.cpp

Loading...
View Options

plugins/paintops/libpaintop/kis_brush_based_paintop_settings.h

Loading...
View Options

plugins/paintops/libpaintop/kis_brush_based_paintop_settings.cpp

Loading...
View Options

plugins/paintops/libpaintop/kis_compositeop_option.h

Loading...
View Options

plugins/paintops/libpaintop/kis_paintop_plugin_utils.h

Loading...
View Options

plugins/paintops/libpaintop/kis_pressure_rate_option.h

Loading...
View Options

plugins/paintops/libpaintop/kis_pressure_rate_option.cpp

Loading...
View Options

plugins/paintops/particle/kis_particle_paintop.h

Loading...
View Options

plugins/paintops/particle/kis_particle_paintop.cpp

Loading...
View Options

plugins/paintops/particle/kis_particle_paintop_settings.h

Loading...
View Options

plugins/paintops/particle/kis_particle_paintop_settings.cpp

Loading...
View Options

plugins/paintops/particle/kis_particle_paintop_settings_widget.cpp

Loading...
View Options

plugins/paintops/roundmarker/kis_roundmarkerop.cpp

Loading...
View Options

plugins/paintops/roundmarker/kis_roundmarkerop_settings.h

Loading...
View Options

plugins/paintops/sketch/kis_sketch_paintop.h

Loading...
View Options

plugins/paintops/sketch/kis_sketch_paintop.cpp

Loading...
View Options

plugins/paintops/sketch/kis_sketch_paintop_settings.h

Loading...
View Options

plugins/paintops/sketch/kis_sketch_paintop_settings.cpp

Loading...
View Options

plugins/paintops/sketch/kis_sketch_paintop_settings_widget.cpp

Loading...
View Options

plugins/paintops/spray/kis_spray_paintop.h

Loading...
View Options

plugins/paintops/spray/kis_spray_paintop.cpp

Loading...
View Options

plugins/paintops/spray/kis_spray_paintop_settings.h

Loading...
View Options

plugins/paintops/spray/kis_spray_paintop_settings.cpp

Loading...
View Options

plugins/paintops/spray/kis_spray_paintop_settings_widget.cpp

Loading...
View Options

plugins/paintops/tangentnormal/kis_tangent_normal_paintop.h

Loading...
View Options

plugins/paintops/tangentnormal/kis_tangent_normal_paintop.cpp

Loading...
View Options

plugins/paintops/tangentnormal/kis_tangent_normal_paintop_settings_widget.cpp

Loading...
View Options

plugins/tools/basictools/kis_tool_line_helper.cpp

Loading...
View Options

plugins/tools/tool_transform2/kis_liquify_paint_helper.cpp

Loading...