Page MenuHome

Paint: Stroke step queue
Needs ReviewPublic

Authored by Pablo Dobarro (pablodp606) on Sep 4 2019, 5:34 PM.
Tags
None
Subscribers
None
Tokens
"100" token, awarded by Frozen_Death_Knight."Love" token, awarded by ChildishGiant."Love" token, awarded by johnsyed."Love" token, awarded by franMarz."Burninate" token, awarded by craig_jones."Like" token, awarded by Zino."Like" token, awarded by amonpaike."Like" token, awarded by billreynish.

Details

Summary

This patch adds a queue that stores brush step samples to process them at a constant rate. This avoids sending too many updates to the paint modes locking the brush preview and the UI during a stroke and affecting performance.
I disabled some stroke methods for using the step queue. They work fine, but I'm not sure if that is the best UX.
Before:

After:

Diff Detail

Repository
rB Blender
Branch
paint-step-queue (branched from master)
Build Status
Buildable 5765
Build 5765: arc lint + arc unit

Event Timeline

The risk with this change is that if redrawing / updating is slow and painting a single step is relatively fast, then completing the stroke can become quite slow.

For example if you quickly draw a long stroke with small brush radius, on a high res image or high poly mesh.

I think that could be mitigated by something like this:

  • max_time = max(STROKE_UPDATE_DELTA, redraw_time * factor)
  • redraw_time estimated as time elapsed since the last paint_step_sample_update
  • factor equals 1 or something not too far from it
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
  • Add STROKE_TIME_SCALE_FACTOR

@Brecht Van Lommel (brecht) I'm not sure if I implement your proposal correctly but I don't notice much difference changing the time factor
I think that even if this patch makes paint modes a bit slower, most people are going to prefer this behavior over blocking the cursor and UI just to gain a few milliseconds when a stroke is lagging. We can always add it as an option for the paint modes as it can easily be disabled.

I don't think it needs to be an option, with an automatic solution it should be fine.

The case I'm thinking of is not for a few milliseconds. For example:

  • paint_step_sample_update() takes 1/60s
  • redrawing, depsgraph update, ... takes 1/6s
  • draw stroke with 100 steps

Previously it would do about 5 redraws and complete that stroke in about 5/6 + 100/60 = 2.5s. Without the mitigation, the new code would do 100 redraws and take about 100/6 + 100/60 = 18.3s.

@Brecht Van Lommel (brecht) So you think we need something more than controlling the maximum time using the time from the last update iteration? Maybe we should ensure a fixed minimum amount of updates per redraw?

I don't known exactly what to do with the patch. Perhaps we should go a step back and describe a bit why this is useful and what requirements we expect (performance etc). Perhaps this will open the way to select the solution of this feature in more clarity.

Brecht Van Lommel (brecht) requested changes to this revision.Nov 24 2019, 1:28 PM

@Brecht Van Lommel (brecht) So you think we need something more than controlling the maximum time using the time from the last update iteration?

It's probably enough, but it needs to be tested.

Maybe we should ensure a fixed minimum amount of updates per redraw?

I don't think so, the correct measure is how long it takes to do those updates vs. redraw time, a fixed amount doesn't say much.

This revision now requires changes to proceed.Nov 24 2019, 1:28 PM

I don't known exactly what to do with the patch. Perhaps we should go a step back and describe a bit why this is useful and what requirements we expect (performance etc). Perhaps this will open the way to select the solution of this feature in more clarity.

It needs to correctly handle two extremes:

  • Update time for a single step is much higher than redraw time (ensuring it does not wait too long to provide some visual feedback to users)
  • Redraw time is much higher than update time for a single step (ensuring overall stroke painting time does not increase too much)

It should be possible to create an artificial test case for both and verify that they work as needed.

source/blender/editors/sculpt_paint/paint_stroke.c
291

This is measuring redraw + update time, instead of just redraw time.

1062

This logic can be moved into paint_step_queue_update, no need to duplicate it since the MAX2(DBL_MAX, ..) case will work fine.

Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Rebase
  • Review update