-
-
Notifications
You must be signed in to change notification settings - Fork 477
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
Add Android example #1417
Add Android example #1417
Conversation
fa3b1d2
to
5e9cd0e
Compare
5e9cd0e
to
3fb7531
Compare
3fb7531
to
7ec432a
Compare
This would need update to the latest Api. |
7ec432a
to
da28d56
Compare
@kchibisov Sure, I've just returned from holiday and am slowly getting back into things, here's hoping we can use this Android example to finally vet the API to be compatible/sensible in this regard. Pieced together something that compiles at least, but running into |
You should ask for GLES I guess in the template builder. In general we should likely remove that requirement... In a config builder try |
da28d56
to
8c79710
Compare
@kchibisov Right, yes, I also had to add |
@MarijnS95 could we use the same example as for desktop, but for android as well? I'll look into adjusting api wrt gles picking in that regard. |
@kchibisov That'd be nice, I copy-pasted the example from yours so it's almost identical, |
Yeah, if we can have a single example for all platforms it could be better? But I'm not entirely sure? Maybe having a separate example for android works as well... |
@kchibisov I think we can, we already have the |
8c79710
to
e5cd726
Compare
You should rebase and fix the CI. Be aware that formatting is on nightly. |
I'm aware, and was just waiting for the dependent PR to be merged before reevaluating where to take this. |
e5cd726
to
d1890b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest you to create context out side of run
. We should advertise the best we can here. The problem is symbol loading, I guess? I wonder if we can make context current, but without any surface? Make add Api for that or something to help here.
Ah, we can, but it's not guaranteed to work. The issue I was trying to solve is gl functions loading, since I was confused why we need context to be current for that. And it turned out that WGL needs that to perform ICD loading. In general we don't need that if we use Though, we can provide a method to get proc address from not current context on supported platforms. Do you have any idea how to approach the symbol loading here and WGL situation? |
It seems you figured out how to clean this up on the glutin API side in #1460 now, I'll rebase and see where we end up. |
d1890b6
to
00751b3
Compare
Now you should be able to reuse your context properly. |
@kchibisov Having a little trouble with that, I can create an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you not recreate the renderer on Resume
? There's no need for that at all and it just slows you down. You also don't need to drop it.
@kchibisov It'd be nice if you actually read my questions before shooting this down immediately. |
I mean, I've read it. Nothing stops you from using hand rolled |
Just asking what your design decision was here, and why the selected comment is incomplete.
Yawn.
Pls halp I can't Rust /s |
5f2da6a
to
a2ee665
Compare
You may base it on your recently merged PR now. |
d088baf
to
e2c3cc3
Compare
@kchibisov Done, can you recheck that this still works on WGL, especially since WGL needs a window before creating a display according to your comments? |
// Make it current and load symbols. | ||
let gl_context = gl_context.make_current(&gl_window.surface).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kchibisov If I understood things correctly make_current()
now doesn't load any functions anymore, that's done by Renderer::new()
right? Or does it load a limited set of functions?
Make the shared event loop suitable for creating and destroying windows in accordance with `winit`s `Resumed` and `Suspended` events.
e2c3cc3
to
15cbd62
Compare
1ed3163
to
d377c60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be good to go once CI passes. I've updated the handling of X11 to be more robust given that it was proven that it's not really working.
Not sure if I want to update internal handling of X11 config picking, but might alter it to not consider raw-window-handle at all.
d377c60
to
de8557a
Compare
Thanks. |
@kchibisov Thank you for patching up the X11 situation and getting this in! |
As mentioned in #1398/#1411 we should have an example that showcases how to deal with the Android activity lifecycle. Ultimately this turns into a helper function on
ContextWrapper
that has access to the internal surface for recreation, as did the broken implementation prior to #1411. At the same time I hope to replacenew_windowed()
with an implementation soon™ that takes aRawWindowHandle
, making the code more generic,winit
-agnostic, and allows users to use AndroidSurface
s for specific views in their Java/Kotlin/Flutter apps without needing a fullscreenNativeActivity
.This PR is based on top of #1412 so that my editor doesn't go bonkers on all the lint warnings/errors :)