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

Modified repaint system on Mac OS to use the OS-mediated system instead of doing it entirely in winit to better support non-GPU-accelerated rendering #2189

Closed
wants to merge 3 commits into from

Conversation

john01dav
Copy link

  • Tested on all platforms changed
  • 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

There has been prior discussion of this issue and how I found out that this is the needed fix:

The short version of the context for this change is that in working on softbuffer, my new crate to allow GPU-less 2D display to a raw window handle window I encountered a bug where once something was shown to a Mac OS window (during a resize) then nothing more could be shown until the window resized. After much digging, it turned out that the cause for this is that winit is not properly notifying Mac OS of the repaints in order to cause it to allow it to push these repaints to the screen. This is because on Mac OS winit handles repaints entirely within itself and does not notify the OS or allow it to mediate them. This pull request changes that by making request_repaint() notify the OS instead of simply tracking it internally. It also removes the internal tracking system as it is no longer used.

My biggest concern with this pull request is that it might break GPU-accelerated rendering. I can't test this myself as the only Mac computer that I have access to is too old to support Metal and this Wgpu. I tried to find some opengl crates with usable examples for testing, but what I found didn't work properly before the change so the results from after the change are not meaningful. As such, before this is merged it is vital that someone who has access to a more modern Mac computer test this with wgpu and other GPU-accelerated rendering systems to ensure that they are not broken.

@tinaun
Copy link
Contributor

tinaun commented Feb 10, 2022

this currently does break gpu rendering in my wgpu project, unfortunately

there's a not in the documentation for setNeedsDisplay that it has no effect in OpenGl views, which probably also means it has no effect in Metal views as well

@john01dav
Copy link
Author

this currently does break gpu rendering in my wgpu project, unfortunately

there's a not in the documentation for setNeedsDisplay that it has no effect in OpenGl views, which probably also means it has no effect in Metal views as well

Sorry for the delay, I got access to a slightly newer mac that can run Metal to iterate with and I tried to fix this and that took some time to do cheaply. Unfortunately, it did not go ideally. What I did was enable both the old non-OS-mediated system and the new OS-mediated system, which did allow both GPU and non-GPU drawing to take place. Unfortunately, in the non-GPU case it fired RedrawRequested twice for each repaint request, thus causing duplication of CPU time and possibly misbehaving according to winit's documentation. I see several ways to move forward, but none are ideal. I'm willing to try to implement these, although I'd likely have much more success implementing those that don't require changing other platforms to support new APIs.

  1. Change the public API and other platforms' implementations to have a parameter passed to request_redraw to specify whether hardware-accelerated rendering is being used.
  2. Let both OS-mediated and non-os-mediated events get through, but have add a flag on the public API's RedrawRequested enum that specifies which type of event is taking place so that handlers can respond appropriately.
  3. Some sort of complicated and probably not that reliable system to guess if a GL/Metal context is present based on whether or not OS-mediated repaints are getting through. This has the advantage of no API changes and thus not needing to touch other platforms to implement said API changes.
  4. Modify Softbuffer to have its own repaint request system. This is especially non-ideal because then other 2D-based libraries must do the same, there's either a non-uniform redrawing API on Mac OS versus other platforms or a clunky API that always includs both (and it might not even be compatible with non-winit window managers, the clunky API option also includes ), .
  5. Do nothing, letting both OS-mediated and non-OS-mediated repaints go through, but duplicating work in the RedrawRequested function. This is the simplest, and it can be dealt with by not doing that much work in this function in non-accelerated contexts (probably via generous caching). It would need some documentation changes to explain how to do this, I think.

@kchibisov
Copy link
Member

Change the public API and other platforms' implementations to have a parameter passed to request_redraw to specify whether hardware-accelerated rendering is being used.

In general you either use window in hardware acceleration context or not. So as a solution if macOS requires something else in software acceleration context you could add a window builder option for macOS like with_hardware_acceleration(bool). Enabled by default or could_hardware_accelerate(bool).

@john01dav
Copy link
Author

Change the public API and other platforms' implementations to have a parameter passed to request_redraw to specify whether hardware-accelerated rendering is being used.

In general you either use window in hardware acceleration context or not. So as a solution if macOS requires something else in software acceleration context you could add a window builder option for macOS like with_hardware_acceleration(bool). Enabled by default or could_hardware_accelerate(bool).

Having a separate API on different platforms seems questionable at best. Ideally, users of softbuffer could just see that the platforms that they care about are supported and then not care about any platform-specific concerns any further than that.

@john01dav
Copy link
Author

Is there any consensus or feedback from the winit maintainers over the best way to handle this? I'm thinking option 5 from my (not immediately) previous comment, as it's the only option that doesn't rely on heuristics and includes no new APIs. It might be a good idea to open an issue after so someone who is looking for issues to work on later can go and add such an API, or at least to make note of the non-ideal resolution, but by implementing option 5 now everything works with Softbuffer (and I can add a warning to its documentation about efficiency — it might be prudent to add it to the winit rustdoc on request_redraw as well) and the behavior of GPU-accelerated rendering is unchanged.

@john01dav
Copy link
Author

john01dav commented Feb 28, 2022

In order to get softbuffer fully working on mac os as quickly as possible, iI've re-added the non-OS-mediated repaint system, and tested against both softbuffer and wgpu and both work. After this is merged, I'll push a softbuffer update warning about not doing anything heavy in RedrawRequested's handler and to cache, due to the double call when not using GPU-accelerated rendering on Mac OS.

EDIT: It has been brought to my attention that I should clarify this: only non-GPU-accelerated applications (which presently means only softbuffer to the best of my knowledge) receive two RedrawRequested events. GPU-accelerated applications receive only one.

@kpreid
Copy link
Contributor

kpreid commented Feb 28, 2022

Requesting repaint twice sounds like a bad idea especially for GPU-using games, because it halves their possible render performance (due to duplicate drawing) unless they have some way of picking out only one of the two events, and most such programs don't have any concept of “nothing needs redraw, so don't” since they're doing continuous animation.

So, making that change would complicate (what I assume is) the typical case of winit's usage. Considering the issues, it seems to me that a better option would be to, somewhat as mentioned above but in reverse, add a builder option to opt-in to cooperating-with-the-system repaints (which might also be useful for other platforms in more complex cases).

(Or perhaps use cases like softbuffer should actually be creating their own custom views (in macOS terminology)/widgets with normal-for-the-platform redraw callbacks.)

@john01dav
Copy link
Author

john01dav commented Feb 28, 2022

@kpreid The OS-mediated system is a no-op when using GPU-based games, so it won't affect that use case. That's a huge part of why I think that this is the best solution for now. As far as I am aware, there are no non-GPU-mediated uses of winit currently, as all I'm aware of after my work on Softbuffer is some other people who attempted the same thing but didn't get nearly as far, and thus it was never suitable for production use.

@kpreid
Copy link
Contributor

kpreid commented Feb 28, 2022

I understood your description as that all applications, GPU-accelerated or not, would receive two RedrawRequested events per frame or per request. If that's not the case, then I have no concerns.

@john01dav
Copy link
Author

That is indeed not the case. To confirm, only non-GPU-accelerated applications (which presently means only softbuffer to the best of my knowledge) receive two RedrawRequested events. GPU-accelerated applications receive only one.

I just re-read my description and I see now that is very unclear. I'll edit it to make it more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - macos
Development

Successfully merging this pull request may close these issues.

5 participants