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

Keep hot state consistent with mouse position. #841

Merged
merged 5 commits into from
Apr 15, 2020

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented Apr 13, 2020

Background

The genesis of this PR is a discussion in zulip where @kindlychung was having trouble with the hot state acting bizarrely.

In the original example (code available in zulip, as well as a video of it in action) there are three basic circles that you can drag around. When you really fling them around they keep flickering between black and white, representing their hot state. The reason is the order of how things are being done in druid.

How hot state is determined

When a MouseMove event gets sent by the platform, with a position that is no longer on top of the circle, then druid acts as follows:

  1. Calculates the hot state to be false and sends a LifeCycle::HotChanged(false) event to the circle widget.
  2. Sends the Event::MouseMove event based on which the container widget will calculate the new position for the circle widget and request layout.
  3. layout happens and the circle widget gets a new origin.
  4. paint happens and the circle gets painted black because the hot state is false.

Indeed druid never detects the hot state to be true because the hot state calculation happens during MouseMove. No more mouse movement is guaranteed, this might have been the last one. Even if there's a new mouse movement, if the mouse is moving fast enough this new MouseMove position will be once again too far away for hot to be true. In practice the situation seems to vary, which is why the circle flickers.

Tricky situation

One solution might be to re-send the MouseMove event with the same position at the beginning of the next wave of event/update/paint. If the event isn't flagged for immediate execution then this might take multiple seconds. However even if the new cycle is immediate this will still result in a single frame of incorrect hot state rendering. During a dragging operation like the example above, this will mean a lot of incorrect frames and so at best we've reduced the flickering but not removed it.

It gets worse

While thinking about this problem I realized another hole in the current system. What if the mouse doesn't move at all? What if the widget moves instead, e.g. with a timer? Well in that case the current system will never apply the hot state to the widget.

Easy demonstration

I updated the timer example to have a moving box which changes color based on hot state. This example can be used to create the mouse-not-moving challange. Just predict where the box is going to move to next, move your cursor in front of its path, and wait. With this PR the box changes color correctly, with a previous druid version it won't change color.

Solution

Thus it seemed clear to me that the solution is to keep track of the mouse position and re-calculate hot state and its associated LifeCycle::HotChanged events after layout but before paint.

This approach means zero incorrect hot state frames. Works perfectly if you're really speeding with your mouse or even with the mouse disconnected and the widget moving.

Details

LayoutCtx needs to get fatter, there's no obvious way around it. It's not just about having the mouse position, we also need to have everything required to synthesize the LifeCycle::HotChanged event and we need to transport back any BaseState changes, especially the hot state changes.

Breaking change

// Previously
fn set_layout_rect(&mut self, layout_rect: Rect) {}
// New signature
fn set_layout_rect(&mut self, ctx: &mut LayoutCtx, data: &T, env: &Env, layout_rect: Rect)

Additional changes

  • Unified event/lifecycle/update/layout post-processing in the window. Previously this was missing for lifecycle, although I did find a comment for the invalidate_and_finalize method saying it should happen for lifecycle.
  • Renamed a bunch of legacy layout_ctx variables to ctx.

@xStrom xStrom added breaking change pull request breaks user code bug does not behave the way it is supposed to S-needs-review waits for review labels Apr 14, 2020
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Interesting, thank you for the detailed explanation. I haven't thought about it has hard as you have, but I don't see another reasonable solution, and this is quite clean, all things considered.

I have a few little comments, nothing really blocking; I think it makes sense to merge this.

druid/src/window.rs Outdated Show resolved Hide resolved
@@ -73,7 +73,7 @@ pub(crate) struct BaseState {
/// The insets applied to the layout rect to generate the paint rect.
/// In general, these will be zero; the exception is for things like
/// drop shadows or overflowing text.
paint_insets: Insets,
pub(crate) paint_insets: Insets,
Copy link
Member

Choose a reason for hiding this comment

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

I think it might make sense to have the 'last mouse position' also be in base state? I could imagine it being useful for lots of things, like calculating deltas when you see MouseMoved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having it in base state wouldn't help with this particular PR, because the base state value is going to be stale in layout. This could be observed in the timer example but it's not limited to that.

If we end up wanting the last position in base state, then this layout pass would just be another update source for the base state's cached value.

Given this, I think it's best to go with YAGNI here. When we eventually need it in base state, then that's the moment to add it.

@@ -182,8 +182,25 @@ impl<T, W: Widget<T>> WidgetPod<T, W> {
///
/// Intended to be called on child widget in container's `layout`
/// implementation.
pub fn set_layout_rect(&mut self, layout_rect: Rect) {
pub fn set_layout_rect(&mut self, ctx: &mut LayoutCtx, data: &T, env: &Env, layout_rect: Rect) {
Copy link
Member

Choose a reason for hiding this comment

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

I wish there were an alternative, but I can't think of one.

@raphlinus
Copy link
Contributor

I haven't reviewed this in detail, and am mostly just lurking, but want to say: thank you for doing a such a detailed analysis and coming up with a fix.

@xStrom xStrom removed the S-needs-review waits for review label Apr 15, 2020
@xStrom xStrom merged commit ee55405 into linebender:master Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change pull request breaks user code bug does not behave the way it is supposed to
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants