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

Issues with invalidation when multiple windows are open #1593

Open
cmyr opened this issue Feb 16, 2021 · 0 comments
Open

Issues with invalidation when multiple windows are open #1593

cmyr opened this issue Feb 16, 2021 · 0 comments
Labels
bug does not behave the way it is supposed to

Comments

@cmyr
Copy link
Member

cmyr commented Feb 16, 2021

This issue was me attempting to talk through something I'm still confused by

Edit: if anyone is curious to try and reproduce this / play around, it manifests for me in linebender/runebender#223

This is a strange issue that I do not yet fully understand. I ran into it while testing some recent runebender work on my laptop (running macOS 10.15) and I did not encounter it on my desktop, running macOS 10.14. I haven't had the chance to test elsewhere.

The problem itself is fairly simple: with multiple windows open, certain views are failing to redraw correctly; they either do not redraw at all, or they redraw partially, leaving visual artifacts:

glitch.mov

There is nothing obviously wrong; the correct paint methods are being called, for instance.

(edit: after more experimenting, it looks like invalidation is behaving differently when there are multiple windows open?)

After a bunch of experimenting, I've tracked this down to being some consequence of moving AnimFrame to be an event, and how that interacts with the normal event/update cycle. Basically: when a window receives prepare_paint, it dispatches (if needed) the AnimFrame event, and then (as with all events) does an update pass (essentially does the Widget::update method) with all windows.

There are two things I've tried that at least make the symptoms go away: the first thing is to only do update on the current window after doing prepare_paint, and the second is to move to doing our layout pass during prepare_paint, instead of during paint.

My general understanding of the problem is that update is being called on windows at an unexpected time, and this is causing weird side-effects. I think there are multiple bugs being triggered here, and in trying to track this down I've come to suspect that the current mechanism for handling animation is broken.

Some things that feel wrong:

  • by sending update to other windows after prepare_paint, those windows may request additional animation frames, because they only clear their own anim_frame request flag in their own prepare_paint methods.
  • we call request_anim_frame in two places: in post_event_processing (which is itself called after layout, lifecycle, and update) as well as at the end of paint.

Looking through window.rs, there are a number of other things that seem a bit iffy; it's odd that we do post_event_processing in lifecycle, for instance.

Okay, I still don't know exactly what's going on. I've thought I did, several different times. I think there are various moving parts.

My best guess, so far, is that the extra update call means that invalidate_and_finalize is always invalidating everything (because we haven't done layout yet, and needs_layout) is true) which means that we aren't correctly invalidating regions, and then hand wave somehow we aren't repainting everything?

This at least explains why moving layout into prepare_paint (where it should be anyway) fixes this; it means we skip that branch and go back to correctly handling the invalid regions.

Longer term:

This whole situation has clarified for me something I've felt for a while, which is that animation needs to be special-cased, and I will push for this change during the next major re-architecture.

What this would mean in a general sense is that the Widget should have a method that is only for handling animation ticks.

In addition to this, I suspect we should also add API in druid-shell that will let us receive regular events on each potential frame painting opportunity. This would be the mechanism that would drive animations, and we could also use this to provide a (maybe opt-in, per-window?) FrameTick event in druid; this would be useful for applications that want to do custom animation stuff, or otherwise update some state in a way that's tied to the paint timing.

@cmyr cmyr added the bug does not behave the way it is supposed to label Feb 16, 2021
cmyr added a commit that referenced this issue Feb 16, 2021
It has taken me a large number of hours to decide on this particular
one-line fix, and I'm still not 100% sure what's going on.

That said, the logic now feels more correct: if we're invalidating
everything then that necessarily means we're invalidating whatever
rects were in the list, and we can ignore them.

More concretely, I think this was a bug; by leaving the invalid
rects in place, a subsequent call to `invalidate_and_finalize` would
see the left over rects and invalidate again, unnecessarily.

I do *not* know why this bug caused (or was a factor in) #1593.

My best guess is that there was a change to how macOS handles
setNeedsDisplayInRect: when it is called from viewWillDraw:; my
guess is that if you call this method after having previously called
setNeedsDisplay: it will assume that the new rect is more accurate
than the previous blanket invalidation, and will only redraw the
new rect.

I'm not convinced of this explanation, though. In particular,
it looks like our drawRect call is still being passed the expected
rect, the pixels just end up not the ones we expect.

Okay, here's my *revised* theory; the additional invalidation
is triggering a second paint call before the frame deadline, and
so the initial paint work, which covered the entire window, is
discarded; the second paint call does not update the regions
that weren't explicitly invalidated, and so we end up failing
to repaint portions of the view.

I think that might be it.
cmyr added a commit that referenced this issue Feb 18, 2021
It has taken me a large number of hours to decide on this particular
one-line fix, and I'm still not 100% sure what's going on.

That said, the logic now feels more correct: if we're invalidating
everything then that necessarily means we're invalidating whatever
rects were in the list, and we can ignore them.

More concretely, I think this was a bug; by leaving the invalid
rects in place, a subsequent call to `invalidate_and_finalize` would
see the left over rects and invalidate again, unnecessarily.

I do *not* know why this bug caused (or was a factor in) #1593.

My best guess is that there was a change to how macOS handles
setNeedsDisplayInRect: when it is called from viewWillDraw:; my
guess is that if you call this method after having previously called
setNeedsDisplay: it will assume that the new rect is more accurate
than the previous blanket invalidation, and will only redraw the
new rect.

I'm not convinced of this explanation, though. In particular,
it looks like our drawRect call is still being passed the expected
rect, the pixels just end up not the ones we expect.

Okay, here's my *revised* theory; the additional invalidation
is triggering a second paint call before the frame deadline, and
so the initial paint work, which covered the entire window, is
discarded; the second paint call does not update the regions
that weren't explicitly invalidated, and so we end up failing
to repaint portions of the view.

I think that might be it.
richard-uk1 pushed a commit to richard-uk1/druid that referenced this issue Apr 6, 2021
It has taken me a large number of hours to decide on this particular
one-line fix, and I'm still not 100% sure what's going on.

That said, the logic now feels more correct: if we're invalidating
everything then that necessarily means we're invalidating whatever
rects were in the list, and we can ignore them.

More concretely, I think this was a bug; by leaving the invalid
rects in place, a subsequent call to `invalidate_and_finalize` would
see the left over rects and invalidate again, unnecessarily.

I do *not* know why this bug caused (or was a factor in) linebender#1593.

My best guess is that there was a change to how macOS handles
setNeedsDisplayInRect: when it is called from viewWillDraw:; my
guess is that if you call this method after having previously called
setNeedsDisplay: it will assume that the new rect is more accurate
than the previous blanket invalidation, and will only redraw the
new rect.

I'm not convinced of this explanation, though. In particular,
it looks like our drawRect call is still being passed the expected
rect, the pixels just end up not the ones we expect.

Okay, here's my *revised* theory; the additional invalidation
is triggering a second paint call before the frame deadline, and
so the initial paint work, which covered the entire window, is
discarded; the second paint call does not update the regions
that weren't explicitly invalidated, and so we end up failing
to repaint portions of the view.

I think that might be it.
xarvic pushed a commit to xarvic/druid that referenced this issue Apr 7, 2021
It has taken me a large number of hours to decide on this particular
one-line fix, and I'm still not 100% sure what's going on.

That said, the logic now feels more correct: if we're invalidating
everything then that necessarily means we're invalidating whatever
rects were in the list, and we can ignore them.

More concretely, I think this was a bug; by leaving the invalid
rects in place, a subsequent call to `invalidate_and_finalize` would
see the left over rects and invalidate again, unnecessarily.

I do *not* know why this bug caused (or was a factor in) linebender#1593.

My best guess is that there was a change to how macOS handles
setNeedsDisplayInRect: when it is called from viewWillDraw:; my
guess is that if you call this method after having previously called
setNeedsDisplay: it will assume that the new rect is more accurate
than the previous blanket invalidation, and will only redraw the
new rect.

I'm not convinced of this explanation, though. In particular,
it looks like our drawRect call is still being passed the expected
rect, the pixels just end up not the ones we expect.

Okay, here's my *revised* theory; the additional invalidation
is triggering a second paint call before the frame deadline, and
so the initial paint work, which covered the entire window, is
discarded; the second paint call does not update the regions
that weren't explicitly invalidated, and so we end up failing
to repaint portions of the view.

I think that might be it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug does not behave the way it is supposed to
Projects
None yet
Development

No branches or pull requests

1 participant