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

Throttle RedrawRequested #2896

Merged

Conversation

kchibisov
Copy link
Member

@kchibisov kchibisov commented Jun 22, 2023

Fixes #2412.

  • macOS (broken drawRect, DisplayLink?)
  • Windows (WM_PAINT?)
  • Wayland (frame callbacks)
  • web (requestAnimationFrame)
  • fallback (X11, redox) (timer based with global timer seed)

The docs are also not updated and it would be nice to have software example showing FPS it's rendering it right now.

--

Wayland done and it works.

@kchibisov
Copy link
Member Author

That's basically what I'm using for testing now https://github.com/kchibisov/glutin/tree/test-winit.

Similar changes could be done to example/window(it'll need a way to request_redraw.

@kchibisov kchibisov force-pushed the redraw-requested-throttle branch from 9091f46 to a21b4ea Compare June 22, 2023 09:15
@madsmtm madsmtm mentioned this pull request Jun 22, 2023
3 tasks
@rib
Copy link
Contributor

rib commented Jun 22, 2023

I think it could be good to focus on one platform at a time with updating how RedrawRequested is throttled.

So maybe just look at the Wayland changes.

I have essentially implemented some of this for Windows in my pump_events branch, because I found it overly convoluted to try and maintain the old logic that was buffering redraw requests to be dispatched after MainEventsCleared.

I think it could also be good to add the present_notification API via a completely separate PR.

Conceptually I would see the present_notification API as way for Winit to get feedback about when the application renders which might be useful for a variety of use cases but in the short term it happens it provides a solution for letting the Wayland backend use frame callbacks.

So it's only indirectly related to throttling - it's needed for throttling on Wayland because otherwise it can't use frame callbacks but that feels like an implementation detail for the Wayland backend.

The stubbed macos change here also e.g. doesn't consider that setNeedsDisplayInRect can only be called from the main thread I'm fairly sure, so there still needs to be a queuing mechanism that goes via the main thread that could potentially call setNeedsDisplayInRect.

@kchibisov
Copy link
Member Author

I think it could also be good to add the present_notification API via a completely separate PR.

I'll do 2 commits in this PR, because otherwise we'll add an API without a real use.

The stubbed macos change here also e.g. doesn't consider that setNeedsDisplayInRect can only be called from the main thread I'm fairly sure, so there still needs to be a queuing mechanism that goes via the main thread that could potentially call setNeedsDisplayInRect.

I just tested whether they work at all, but they don't, I'll revert them all together, because CVDisplayLink should be used.

I think I'll focus mostly on docs in this PR.

@rib
Copy link
Contributor

rib commented Jun 22, 2023

I think it could also be good to add the present_notification API via a completely separate PR.

I'll do 2 commits in this PR, because otherwise we'll add an API without a real use.

yeah, makes sense

I think I'll focus mostly on docs in this PR.

Just to note that I at least made some updates to docs in the pump_events branch.

Maybe you could base on top of my pump_events branch to avoid conflicts?

@kchibisov kchibisov force-pushed the redraw-requested-throttle branch from a21b4ea to 12eb090 Compare June 22, 2023 18:56
@kchibisov kchibisov marked this pull request as ready for review June 22, 2023 18:56
@kchibisov
Copy link
Member Author

Maybe you could base on top of my pump_events branch to avoid conflicts?

I've added just some basic docs, we could improve them with your work ofc, they shouldn't overlap from what I could say.

@kchibisov
Copy link
Member Author

This is ready for review wrt docs.

@daxpedda if you want to send requestAnimationFrame you could do it after that patch, should be simply with your previous patch.

@kchibisov kchibisov force-pushed the redraw-requested-throttle branch from 12eb090 to b6c34d9 Compare June 22, 2023 19:16
@rib
Copy link
Contributor

rib commented Jun 22, 2023

I think it would be best to rename on_present_notify to something like pre_present_notify because it's important to be very clear that that this callback needs to happen before the application actually presents.

Just going by the name I think I would expect to call on_present_notify after presenting.

Especially for the initial Wayland use case where the backend wants an opportunity to insert a frame callback before the surface.commit() - this ordering is important.

@kchibisov kchibisov force-pushed the redraw-requested-throttle branch 2 times, most recently from 1e669fa to ae6e85a Compare June 23, 2023 08:28
@kchibisov
Copy link
Member Author

I've renamed the method. @rib is this good for now?

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Web backend implemented and approved.
Small question about the documentation.

src/window.rs Outdated Show resolved Hide resolved
@kchibisov kchibisov force-pushed the redraw-requested-throttle branch 2 times, most recently from 8558d09 to 301686f Compare June 23, 2023 19:06
@kchibisov
Copy link
Member Author

@daxpedda could you recheck that I've rebased correctly web?

@rib
Copy link
Contributor

rib commented Jul 31, 2023

One thing I was wanted to double check here @kchibisov is whether you have tested how apps behave if they omit the pre_present_notify call.

It would be good to be clear on whether this is now a hard requirement or whether the backend can reliably only enable throttling once it has seen that the application has called pre_present_notify at least once.

@kchibisov
Copy link
Member Author

It would be good to be clear on whether this is now a hard requirement or whether the backend can reliably only enable throttling once it has seen that the application has called pre_present_notify at least once.

They will get instant redraw, so sounds fine to me, that's what they get before.

kchibisov and others added 3 commits August 3, 2023 20:42
That's a way to communicate to winit that you'll present to the window.
While it's a no-op for now, it'll be used to throttle drawing.
Throttle RedrawRequested events by the frame callbacks, so the users
could render at the display refresh rate.
@kchibisov kchibisov force-pushed the redraw-requested-throttle branch from 5f4c1b9 to f971acd Compare August 3, 2023 16:50
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Web is fine, thanks @kchibisov.

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.

Implement VSync source
3 participants