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

macos: Handle resizes synchronously #1911

Closed
wants to merge 1 commit into from

Conversation

scoopr
Copy link
Contributor

@scoopr scoopr commented Apr 20, 2021

Allows for a lot better resize behaviour on macos, when allowed
to redraw the frame within the resize event callback.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Allows for a lot better resize behaviour on macos, when allowed
to redraw the frame within the resize event callback.
@scoopr
Copy link
Contributor Author

scoopr commented Apr 20, 2021

I thought #1901 would of fixed this, but it doesn't do anything when the view has a metal or opengl CALayer attached, as display won't be called, as far as I could understand.

@scoopr
Copy link
Contributor Author

scoopr commented Apr 20, 2021

Hmm, on further testing, this fixes especially the case when I actually render in the Resized event, in conjunction with drawing on MainEventsCleared.

So I'm not sure, but is the OS resizing the window, winit giving MainEventsCleared, and after that the Resized event?

But in any case, I believe rendering within the Resized event handler is somewhat required if I want to enable and use presentsWithTransaction with the CAMetalLayer

I'm curious is all the queuing of the events all that necessary overall though.

@ArturKovacs
Copy link
Contributor

I think that the biggest problem -that #1901 did not address- is that now you get the redraw events before the resized events, which is almost certainly unexpected by developers. Note that it's advised to render in the RedrawRequested event; in which case #1901 does fix the flickering/blank screen issue.

However I do believe that there's merit in solving the problem of "getting the redraw events before the resized events" and this PR does solve that. Although I am somewhat uncomfortable with synchronously dispatching some events while others are queued. I have to admit that I don't really understand why we queue events in the first place rather than just immediately dispatching all of them (I tried getting to the bottom of this but all I found was #853 which didn't help enlightening me).

Maybe @francesca64 knows why events are queued and only dispatched in the AppState::cleared function. That would probably help in deciding if it's okay to synchronously dispatch the resized event from the "windowDidResize".

@ArturKovacs ArturKovacs self-requested a review April 21, 2021 08:23
@scoopr
Copy link
Contributor Author

scoopr commented Apr 23, 2021

I'm totally fine with changing all the callbacks to non-queueing, thats just changing the emit_event to not queue but handle it directly.

it seems Francesca did set it to send resizes directly in francesca64@b75073a but then later in #1173 it was then deferred again. Unfortunately it seems Aleksi has removed his github account.

In there he both defers the event and change a runloop mode.

I guess I didn't double-check that the other events come through, but with a quick check, key events are not coming through while resizing, but thats not too unexpected. But I did get continuous rendering of the app still (and with maineventscleared), so should be fine on that front.

@ArturKovacs
Copy link
Contributor

It would be to the very least surprising if there wouldn't be a good reason why events are queued instead of dispatched immediately. Unfortunately I don't know what that reason is. As another attempt I'm tagging @goddessfreya to help here. Otherwise I can't think of a better way of finding the reason, than to try to make every event dispatched immediately.

@francesca64
Copy link
Member

@scoopr thanks for doing the archaeology on that!

@ArturKovacs neither Osspial nor me have been able to communicate with Freya over either email or Discord in the past year or so, and her website's been gone for just as long, so all we can do is hope that she's alright. On the bright side, she likely wouldn't know about this, since #853 was her politely rebasing my macOS EL2 work that I'd sorta kinda abandoned.

For more historical background, I based the macOS AppState on the struct of the same name in the iOS backend. This is because the latter was written by my work teammate @mtak-. He queued events, so I did too, since it made sense to me at the time.

As for resize in particular... we apparently discussed issues I found with queuing resizes, but since 99% of the discussion happened in calls, that's all the info left now. It's possible he remembers why it was important to queue events in the first place, but he's presently on leave, so I'm not sure when I'd be able to ask him.

@ArturKovacs
Copy link
Contributor

First of all thank you @francesca64 for clarifying the situation with Freya. I wish her the best.

And thanks for describing the history of how we came to queue events, it is definitely helpful.

I'll probably spend some time with this in the near future as I think it's an important issue, but in any case I'm looking forward to hearing from @mtak- because even if the immediate dispatch would seem to work, I'd have the feeling that we are missing something that @mtak- knew about.

@madsmtm
Copy link
Member

madsmtm commented Sep 2, 2024

See #2640 (comment), I plan to fix this myself, but it'll take a while.

@madsmtm madsmtm closed this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants