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:
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- paint-step-queue (branched from master)
- Build Status
Buildable 4770 Build 4770: arc lint + arc unit
Event Timeline
Comment Actions
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
Comment Actions
@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.
Comment Actions
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.
Comment Actions
@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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
@Brecht Van Lommel (brecht) @Sergey Sharybin (sergey) I rebased the patch and I enabled it for sculpt mode. Maybe you have something else in mind that goes on top of this patch, but I still see this more like an UX improvement than a performance improvement. The difference this makes for users is that instead of freezing the viewport you have a delayed preview of the stroke being processed behind the pen, but (at least with how it is implemented now), this does not make the strokes more responsive.
Also, there is a function that disables the queue for certain types of brushes, but there may still be cases that I'm overlooking were this is not compatible. If we commit this, probably we want to commit it to experimental as this is the kind of patch that can produce a lot of bugs.
| source/blender/editors/sculpt_paint/paint_stroke.c | ||
|---|---|---|
| 73 | Brief comment explaining why this is needed STROKE_TIME_SCALE_FACTOR. | |
| 92 | Is this only used for samples? If so it could be called sample_update_timer. | |
| 137 | Prefer grouped terms sample_* (so prefix auto-completes usefully), also reads better in this case IMHO. Could be called sample_queue or samples_pending. | |
| 138 | Add a short description of how this is used. | |
| 293 | Shouldn't this be called elapsed_time ? | |
| 295 | Term fmax is vague, event_deadline makes more sense? | |
| 649–650 | This struct could be initialized as part of it's declaration (slight preference for this as it means there isn't any uncertainty about struct members being initialized or not). | |
| 934–937 | While not a huge amount of overhead, it seems odd to add a queue & timer which is never used. Why not check the tool on initialization, only adding the samples & the queue structure when they're used. | |
| 971–1006 | Making the done function return OPERATOR_RUNNING_MODAL is unnecessarily confusing. Since the checks for returning modal happen early, this can be refactored into a separate function, suggest:
This way it's clear whats intended by the caller. Currently it's not obvious when this function might return modal or not (without reading into the functions logic). | |
| 999–1000 | paint_stroke_free does this too. | |
| 1033–1045 | Why are these free calls added here? (rng, stroke_cursor & lines) as paint_stroke_free frees these. Since it's still being called, can't freeing be kept there? (free the stepsamples there too). Note, if there is some reason both stroke_done & stroke_free need to make free calls, that could be extracted into a 3rd function they both call. | |