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

examples/winit: Implement proper resumed() semantics #127

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
Member

On Android the backing buffer (NativeWindow) disappears when the application is not focussed and/or the screen is locked. Winit handles this by requiring apps to create their raw_window_handle() consumers after Event::Resumed and to clean it up before returning from Event::Suspended. For consistency Winit also sends Resumed on all other platforms during init.


Draft until this is transplanted to all the other examples, even though that doesn't magically make them run on Android yet.

@MarijnS95 MarijnS95 mentioned this pull request Jun 16, 2023
@MarijnS95 MarijnS95 force-pushed the winit-resumed branch 2 times, most recently from e3d086e to 6b0083a Compare November 8, 2023 23:55
@notgull
Copy link
Member

notgull commented May 7, 2024

This was done as a part of #214, so I'll close this PR

@notgull notgull closed this May 7, 2024
@MarijnS95 MarijnS95 deleted the winit-resumed branch May 7, 2024 22:21
@MarijnS95
Copy link
Member Author

@notgull I disagree that this was fixed in #214. While the new winit API forces you to handle suspend() and resume(), your PR is panicking quite violently when handling events outside of a pair of these, where no window/surface is active.

@notgull
Copy link
Member

notgull commented Jun 26, 2024

I see, thanks for letting me know.

@MarijnS95
Copy link
Member Author

MarijnS95 commented Jul 10, 2024

@notgull I've force-pushed a rework of this patch (based on top of #227), let me know what you think before I convert the remaining examples and make a PR.

Note that there is some confusion currently in the winit APIs that we're cleaning up upstream in rust-windowing/winit#3765 and rust-windowing/winit#3779.

@MarijnS95 MarijnS95 restored the winit-resumed branch July 10, 2024 12:28
@MarijnS95

This comment was marked as outdated.

@daxpedda daxpedda reopened this Jul 10, 2024
@MarijnS95 MarijnS95 force-pushed the winit-resumed branch 3 times, most recently from bc0a66a to 592c49e Compare July 14, 2024 19:51
@MarijnS95 MarijnS95 changed the title examples/winit: Implement proper Event::Resumed semantics examples/winit: Implement proper resumed() semantics Jul 14, 2024
@MarijnS95 MarijnS95 marked this pull request as ready for review July 14, 2024 19:52
@MarijnS95 MarijnS95 requested a review from john01dav as a code owner July 14, 2024 19:52
@MarijnS95
Copy link
Member Author

MarijnS95 commented Jul 14, 2024

Note that, to actually implement the Android backend, there are ideas to (temporarily?) provide the user a fake Vec without stride and the current fixed pixel format. This buffer will be "transformed/converted" into the Android buffer on "submit".

If we do that, we could also pretend to the user that their Surface "always" exist (and give them a buffer), and handle the lifetime internally.
EDIT: After, all we don't expect winit (:crossed_fingers:) to emit RedrawRequested when there is no surface for the window).

EDIT: Never mind this. The surface can't know without winit loop interaction; I was modifying the example utility code here.

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.

LGTM!
CI is broken because of the changes in WinitApp right?

examples/utils/winit_app.rs Outdated Show resolved Hide resolved
examples/winit_multithread.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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.

LGTM!
Would like to replace (e)print(ln) with tracing, but that's for a follow-up.

Documentation tests have to be fixed.

@daxpedda
Copy link
Member

@MarijnS95 rebasing on #228 should fix the remaining CI.

@MarijnS95 MarijnS95 force-pushed the winit-resumed branch 2 times, most recently from c1162bf to ae7f947 Compare August 3, 2024 10:34
@MarijnS95 MarijnS95 requested a review from daxpedda August 3, 2024 10:35
@MarijnS95
Copy link
Member Author

Only nightly seems to fail on #[path = ...] mod winit_app;, perhaps irrelevant to this PR?

After originally seeing complaints on `nightly`, we now see them on
`stable` (but no longer on `nightly`): either some regression slipped
in, or `rustc` understands that any relative path references inside
content from `include_str!()` are supposed to be relative to the file
that is being included, not the file that it is being included in
(i.e. in this case relative to `./README.md`, not to `src/lib.rs`
 which includes `../README.md`).
On Android the backing buffer (`NativeWindow`) disappears when the
application is not focussed and/or the screen is locked.  Winit handles
this by requiring apps to create their `raw_window_handle()` consumers
_after_ `Event::Resumed` and to clean it up _before_ returning from
`Event::Suspended`.  For consistency Winit also sends `Resumed` on all
other platforms during init.
@MarijnS95
Copy link
Member Author

Yikes, this relative path error, that we first only saw on nightly, is now only showing on stable:

---- src/../README.md - (line 69) stdout ----
error: couldn't read src/../../examples/utils/winit_app.rs: No such file or directory (os error 2)
Error:   --> src/../README.md:78:1
   |
11 | mod winit_app;
   | ^^^^^^^^^^^^^^

error: aborting due to 1 previous error

Couldn't compile the test.

failures:
    src/../README.md - (line 69)

test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

Did the fix (assuming it is a fix to how relative paths from content in include_str!() are resolved) get reverted, removed, or regressed on nightly again?

After all, I'd argue the path should be relative to README.md no matter what, not to wherever this file in #[doc = include_str!()]'d which could be at any arbitrary level in our directory tree...

Pushed a "fix", let's see if nightly would then start to break again...

@MarijnS95
Copy link
Member Author

It breaks both stable and our MSRV... How do you want to fix this, @ids1024 @notgull?

@madsmtm madsmtm mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants