-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
base: main
Are you sure you want to change the base?
Conversation
Trying this, I feel like there is some additional work to be done.
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. |
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? |
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) ? 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. |
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. |
Look at f20775c#diff-4dd2da25347b4ca65437059e5072856b3085e9c029be3a91a1a6b140fabaf47aR493 to make it compile. |
FIxed it, thanks! |
Sorry, it's been a while since I've looked at the PR. 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. |
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); |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing
The bug was caused by the eraser bounds still using the estimated speed instead of the added_width. 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) |
rnote/crates/rnote-engine/src/pens/eraser.rs Lines 190 to 191 in f7adb2a
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 Down(Element, EraserMotion) Doing that means we don't have to do a 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; |
There was a problem hiding this comment.
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)), |
Maybe a threshold at 4 ms would be good.
This will fix this issue for good
Eraser size now scales linearly with pen speed. Implements #819.