Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Erase faster to increase eraser size #1235

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

PerfectlyInternal
Copy link

Eraser size now scales linearly with pen speed. Implements #819.

@PerfectlyInternal PerfectlyInternal marked this pull request as ready for review September 25, 2024 15:33
@Doublonmousse
Copy link
Collaborator

Doublonmousse commented Sep 29, 2024

Trying this, I feel like there is some additional work to be done.

There is no upper limit to the eraser size. When switching from the pen to the eraser, the initial size can sometimes be too large, even exceeding the visible canvas size (and delete a whole document instead of staying as a small eraser). It might be worse on some devices (an initial small displacement over a very small timestep will give a velocity estimation with very large error bounds, and that timestep might be 1/60 s on some pens down to 1/480 s. Hence the potential very large overestimate even with the initial smoothing). This should be fixed.
An upper bound on speed or eraser size should also be here.
Maybe we can try to re use the ink stroke modeler for this (but output the velocity estimation instead) or do some kalman based filtering (where we can model the error on the speed estimation and act on it if necessary).

Edit : issue from below. However setting an upper bound might not be a bad idea anyway.

The other thing I'm wondering is that on other software that scales the eraser size depending on speed, the actual behavior is not doing a speed estimation and scaling the eraser size with respect to that. There is some inertia to it, or some way to lock or decrease smoothly or in steps the eraser size when the speed decreases.
It might be more akin to a "erase faster means getting the next eraser size and locking it to that size until you stop or slow down" situation.
Realistically, we're not obliged to follow suit on that, but we should come up with a behavior that makes sense and will satisfy the original feature request.

@PerfectlyInternal
Copy link
Author

In the video shown in the issue, the eraser seems to step in size as the user moves quickly for a while, then gradually step down once the user has stopped moving. This could be implemented either as smoothing over a very long period of time, so the eraser needs to be moved quickly for a while in order to get to its full size, or by checking if the eraser has been moving fast for at least X ms, then stepping up the size. Which one do you think would be better?

@Doublonmousse
Copy link
Collaborator

I'm still getting the same issue with the start size being sometimes too large then going down. I will try to spend some times logging things to check where this comes from.

Doing things when the user stops moving is also a little harder to do. With pens, you always have some jitter hence events continue happening (but events stop happening if you stop with a mouse). This is the same issue I had with #1175 (needing a separate thread to call changes in the future and being cautious not to have more than one event fire at the same time because of it). But maybe in that case that's okay not to do that (as you trigger the change of size with movement, not stopping the pen)

Maybe we can use a better designed speed to eraser size function (somewhat of a stepped function so you get the same size for some range of speed) ?
The other idea I had was to add a size to the EraserMotion structure and have it be updated by some well chosen function like size = f(size, speed) so that we get some hysteresis-like behavior (lock/converge to some size levels and change value significantly only for large/slow speed).

Honestly I don't have a perfect answer, though I want to check whether if we can get what we want with the current strategy (smoothing). I don't know what's the better approach here.

@PerfectlyInternal
Copy link
Author

The eraser now needs to exceed some minimum speed in order to start scaling up, and scaling is now a stepped function with hysteresis. The exact values to trigger stepping up/down can be tweaked, so we'll want to play around with those to get something that feels good to use. It might even be a good idea to expose them as config options for the user, since different users will probably have different speeds at which they'll want to trigger eraser scaling.

We still need some way of detecting a lack of motion, since events stop firing when you hold still, even on a drawing tablet. This will need to be faster than the long press detector you already have, but the underlying logic should be similar.

@Doublonmousse
Copy link
Collaborator

@PerfectlyInternal
Copy link
Author

FIxed it, thanks!

@Doublonmousse
Copy link
Collaborator

Sorry, it's been a while since I've looked at the PR.
I still get the bug of the eraser being very large when the pen touches the screen (do we need or can we estimate correctly the speed when the pen is hovering ? maybe there is something happening here ?)

I think I need to take the time to debug this properly (and maybe extract some representative raw input sequences that we can use to fine tune the program. I wonder if it's of interest to add something like that to the dev menu for future uses as well : record raw/smoothed inputs for pen and eraser and save to a file/json for analysis). To be clear, I'm not asking you for this, I'll probably do a separate PR for this.

@PerfectlyInternal
Copy link
Author

I think what might help is calculating pen speed as part of input handling, so we can filter the input more, and possibly use it for other features later down the line.

let event_result = match (&mut self.state, event) {
(EraserState::Up | EraserState::Proximity { .. }, PenEvent::Down { element, .. }) => {
widget_flags |= erase(element, engine_view);
self.motion.update(element, now);
widget_flags |= erase(element, self.motion.speed, engine_view);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speed instead of added_width here

widget_flags |=
erase(element, engine_view) | engine_view.store.record(Instant::now());
self.motion.reset();
widget_flags |= erase(element, self.motion.speed, engine_view)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing, speed instead of added_width

@@ -167,7 +230,7 @@ impl DrawableOnDoc for Eraser {
engine_view
.pens_config
.eraser_config
.eraser_bounds(*current_element),
.eraser_bounds(*current_element, self.motion.speed),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing

@Doublonmousse
Copy link
Collaborator

The bug was caused by the eraser bounds still using the estimated speed instead of the added_width.
It now works correctly. Maybe we can fine tune parameters a little bit?

As far as calculating speed higher in the input handling code, it may not be worth estimating speed for nothing. I think it's the right place for it. We may mutualize some code between here and the ink stroke modeler in the future if we are to implement a finer velocity estimation method (though I think this already works pretty well)

@Doublonmousse
Copy link
Collaborator

Doublonmousse commented Nov 10, 2024

(EraserState::Proximity { .. } | EraserState::Down { .. }, PenEvent::Cancel) => {
self.state = EraserState::Up;

Missing a self.motion.reset().

I'm not convinced we need to estimate speed while the eraser is in proximity mode. If we don't, what we can do is change the EraserState to have

   Down(Element, EraserMotion)

Doing that means we don't have to do a reset as the EraserMotion will only be there if we are in the Down mode.

Edit : The jump in speed when the pen touches the screen seems to come from a weird timing of events on my device, when my pen touches the screen an event is sent with a very short time delta compared to the rest

fn update(&mut self, element: Element, time: Instant) {
if let Some((last_element, last_element_time)) = self.last_element {
let delta = element.pos - last_element.pos;
let delta_time = time - last_element_time;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, after looking at my issue (with large speed jumps), it's not impossible to get a delta time that's either too small or negative (and in that case the - clamps the value to zero).

We should only update the speed if the delta_time > 0.0 or superior to an expected minimum time delta.
From

PenStyle::Eraser => BacklogPolicy::Limit(Duration::from_millis(33)),
(for each screen frame, gtk send all the events in one go and we only take events so that they are spread apart by at least the value here), this allows only one update per screen frame, so something like 8ms at 120 Hz.

Maybe a threshold at 4 ms would be good.

This will fix this issue for good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants