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

Added Surface::{width,height} implementations for CGL/WGL #1551

Merged

Conversation

Melchizedek6809
Copy link
Contributor

  • 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

Hello again,

I've looked into implementing these missing methods on CGL/WGL. For the CGL implementation I'm not sure whether the nil check is necessary, if not then the maybe the return type could be changed to u32 since all Platforms have an implementation now?

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Ideally, the != nil check in CGL should be moved to create_window_surface. Perhaps one could even change the type from ns_view: id to ns_view: NonNull<Object>.

@Melchizedek6809
Copy link
Contributor Author

@madsmtm Alright, I've moved the check into create_window_surface. Changing the type to ns_view: NonNull<Object> seems like a bigger change, I might tackle that in a separate PR.

Apart from that this newest change means that Surface::{width,height} will always return Some on all APIs, would changing the return type from Option<u32> to u32 be appropriate?

@kchibisov
Copy link
Member

@MarijnS95 could you look into the android failure? It looks like the ndk just doesn't build, given that traces don't go into glutin?

@MarijnS95
Copy link
Member

@kchibisov this was caused by num_enum 0.5.8 which has aptly been yanked. I've restarted the CI, should become green soon again.

This commit also fixes thread safety issues on macOS, before that
accessing context from thread other than main would simply result in
SIGILL.
@kchibisov
Copy link
Member

@madsmtm could you take a look, I think I've fixed all thread safety issues with macOS and were able to run stuff off the main thread, though, I've removed the example due to its complexity...

I've also clarified the docs and ensured that we return physical sizes from the width/height` functions.

@kchibisov
Copy link
Member

Also, @madsmtm do you know a way to somehow wakeup macOS's main thread from set_wait in winit? Without side channels and such, maybe there's a global method to magically wake-it-up?

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

@madsmtm could you take a look

Looks good to me now 👍

Also, @madsmtm do you know a way to somehow wakeup macOS's main thread from set_wait in winit? Without side channels and such, maybe there's a global method to magically wake-it-up?

I assume you mean after having called ControlFlow::set_wait? But yes, what we do in winit to do so is to send an empty NSEvent:

https://github.com/rust-windowing/winit/blob/5ba6bdef4931e66eb4c4be87c90a51fad7675ee5/src/platform_impl/macos/event_loop.rs#L260

@kchibisov
Copy link
Member

I assume you mean after having called ControlFlow::set_wait? But yes, what we do in winit to do so is to send an empty NSEvent:

I wonder if I can do so from inside glutin, I think I can do so, given that NSApplication is a singleton? Might be worth force waking-up the event loop like that?

@kchibisov kchibisov merged commit d4cd621 into rust-windowing:master Feb 7, 2023
@madsmtm
Copy link
Member

madsmtm commented Feb 7, 2023

You definitely could - though I don't think I understand why you need to?

@kchibisov
Copy link
Member

If winit is blocked on the main thread, and you call something on the off thread, you'll block on the off-thread as well. If we send dummy event we'll wake-up the main loop without any issue?

Otherwise the user must wake-up the main thread manually when calling functions in glutin which block on macOS?

@Melchizedek6809 Melchizedek6809 deleted the surface_width_height branch February 7, 2023 18:19
@madsmtm
Copy link
Member

madsmtm commented Feb 8, 2023

No, you needn't worry about that: Queue::main().exec_sync(|| ...) wakes up the main event loop for you

@kchibisov
Copy link
Member

Huh, ok.

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.

None yet

4 participants