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

Use raw-window-handle #1279

Closed
wants to merge 1 commit into from
Closed

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Jan 21, 2021

Closes #256

This depends on rust-windowing/raw-window-handle#52, which means that we need a release containing it (rust-windowing/raw-window-handle#58). That however is going to be a churny release, because rust-windowing/raw-window-handle#55 is massively breaking.

@msiglreith what you have said in rust-windowing/raw-window-handle#54 suggests that this approach is not suitable for android, if I'm understanding that correctly? Would we need to update the TrustedWindowHandle every frame on android?

@msiglreith
Copy link

Here is a minimal example for wgpu on android I did some time ago: msiglreith/wgpu-rs@991c081

winit in particular only gives you a valid native window handle, therefore, raw window handle inbetween resume and suspend cycles. For different cycles the window handle may change so it needs to be handled appropriately. This means before running the event loop there will also be no handle available, resulting in a crash currently.

@DJMcNab
Copy link
Member Author

DJMcNab commented Jan 21, 2021

I want to test this pr and compare it to master on android, to see if this introduces breakage. To be clear, it would surprise me massively if it did. My current understanding is that both should fail (crash?) if the application gets suspended and resumed.

@enfipy I don't have an android sdk installed, so if you could check this in the next couple of days that would be fantastic, but if not I might get around to it at some point

@DJMcNab DJMcNab marked this pull request as ready for review January 21, 2021 17:06
@DJMcNab DJMcNab marked this pull request as draft January 21, 2021 17:06
render = ["bevy_internal/bevy_pbr", "bevy_internal/bevy_render", "bevy_internal/bevy_sprite", "bevy_internal/bevy_text", "bevy_internal/bevy_ui"]
render = [
"bevy_internal/bevy_pbr",
"bevy_internal/bevy_render",
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a drive by automatic formatting, although I think this is nicer anyway

@DJMcNab
Copy link
Member Author

DJMcNab commented Jan 21, 2021

This is still a draft since it depends on unreleased features of raw-window-handle

This is not high priority, but if anyone needs this functionality (@aclysma ?) urgently, we can recreate the internal api of TrustedWindowHandle in bevy. I'd prefer to wait for it to land upstream though.

@blaind
Copy link
Contributor

blaind commented Jan 23, 2021

I also worked on the android activity lifecycle problem before noticing this ticket. Got a functional PoC-solution. This would require upstream commit to winit, though.

Please check out:

@DJMcNab
Copy link
Member Author

DJMcNab commented Jan 23, 2021

Your changes look like they wouldn't conflict with mine at all in that case, which is positive?

So basically, this change would still be an improvement, and so can be safely merged once the raw-window-handle upstream catches up?

@blaind
Copy link
Contributor

blaind commented Jan 23, 2021

Your changes look like they wouldn't conflict with mine at all in that case, which is positive?

So basically, this change would still be an improvement, and so can be safely merged once the raw-window-handle upstream catches up?

Allright, sounds good! I've opened a pull request #1293 to continue discussion/development of the lifecycle part.

@DJMcNab
Copy link
Member Author

DJMcNab commented Jan 23, 2021

Cool, that makes sense.

@cart
Copy link
Member

cart commented Jan 23, 2021

I'm on board for these changes when upstream winit is ready for us.

@DJMcNab
Copy link
Member Author

DJMcNab commented Jan 23, 2021

I've also opened rust-windowing/raw-window-handle#66, which we hopefully be able to use.

@alice-i-cecile alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in S-Blocked This cannot move forward until something else changes labels Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@alice-i-cecile
Copy link
Member

This appears to no longer be blocked, per the linked issues :) Removing the label; hopefully we can get this polished and merged finally.

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Jul 9, 2021
@DJMcNab
Copy link
Member Author

DJMcNab commented Jul 9, 2021

V0.4 still isn't released - see https://crates.io/crates/raw-window-handle/versions

@cart
Copy link
Member

cart commented Jul 9, 2021

It is worth pointing out that the new renderer is already using raw window handles:
https://github.com/cart/bevy/blob/305ff4c06322b934c3b307c60b1a571af615cb85/crates/bevy_window/src/window.rs#L129

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@DJMcNab
Copy link
Member Author

DJMcNab commented Aug 6, 2021

It's a simple enough change, that it can be recreated if needed.

@DJMcNab DJMcNab closed this Aug 6, 2021
@DJMcNab DJMcNab deleted the raw_window_handle branch August 6, 2021 18:57
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants