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

Rework glutin example #1706

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

Conversation

mickvangelderen
Copy link
Contributor

@mickvangelderen mickvangelderen commented Oct 2, 2024

In my opinion the main application implementation of the example is hard to follow due to the combinatory state machine. This refactor eliminates many possible combinations from the state space through a simple explicit state machine.

Additionally, the error handling was improved. Many of the unwrap calls are now properly forwarded out of the event loop.

  • 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

@kchibisov
Copy link
Member

I'm not sure about this one, since it gets more involved.

I think I'd tune the glutin API a bit for 0.33 to explicitly state that it owns the display and that way I can remove a lot of code from the example.

Comment on lines +97 to +108
WindowEvent::RedrawRequested => {
self.transition(event_loop, |state| {
match &state {
AppState::Uninitialized { .. } => return Err("invalid transition".into()),
AppState::Resumed(state) => {
state.redraw()?;
},
AppState::Suspended { .. } => {},
}
Ok(state)
});
},
Copy link
Contributor Author

@mickvangelderen mickvangelderen Oct 2, 2024

Choose a reason for hiding this comment

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

I moved the redraw code from about_to_wait to WindowEvent::RedrawRequested. This is recommended by winit. If there is a reason not to make this change specific to glutin, we should revert this change and document it.

Comment on lines 157 to 168
impl<T: GetGlDisplay<Target = Display>> Drop for TerminateDisplayOnDrop<T> {
fn drop(&mut self) {
#[cfg(egl_backend)]
#[allow(irrefutable_let_patterns)]
if let glutin::display::Display::Egl(display) = _gl_display {
if let glutin::display::Display::Egl(display) = self.0.display() {
unsafe {
display.terminate();
}
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kchibisov do you mean that this egl specific display terminate code should be handled by glutin by default?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like the terminate call is only necessary if the reference counting extensions are not available: #1588.

The DisplayInner implementation will terminate the display only if reference counting is supported and the reference is the last one.

The terminate call in the example is only correct if reference counting is not supported AND we guarantee that there are no uses of the display after termination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only solution I can think of is to emulate reference counting (through a global static lookup table) if the extension is not available.

Copy link
Member

Choose a reason for hiding this comment

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

it's already ref-counted internally, so it's as simple as calling terminate. We don't do so, because we don't state that we own a display and we'd need a from_raw functions, etc, etc.

I'll work on that myself, so don't worry.

Copy link
Contributor Author

@mickvangelderen mickvangelderen Oct 3, 2024

Choose a reason for hiding this comment

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

I think we need unsafe from_raw_assume_ownership(raw_window_handle::RawDisplayHandle) for this to work. In my example we can control the type of DisplayId, but it seems we do not actually control the type of the raw display representation.

If we have the "assume ownership" variant, then we can just use a bool in the inner display to terminate on drop. No lookup tables.

Copy link
Member

Choose a reason for hiding this comment

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

if you want like that, sure, but generally building a display from raw pointer means that some other lib created it as well.

Copy link
Contributor Author

@mickvangelderen mickvangelderen Oct 3, 2024

Choose a reason for hiding this comment

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

I just learned that there is a "native display" and a "platform display" and the two aren't the same (maybe)? I've never done anything with EGL, excuse my ignorance.

I'll try to read a bit more into what these things are and think about it. Some of the things I said above may be non-sensical.

Copy link
Contributor Author

@mickvangelderen mickvangelderen Oct 3, 2024

Choose a reason for hiding this comment

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

How many EGLDisplays can exist per "native display" (NativeDisplayType for eglGetDisplay and void * for eglGetPlatformDisplay)?

Copy link
Member

Choose a reason for hiding this comment

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

It's generally one, EGLDisplay is a global singletone.

Comment on lines +88 to +90
// TODO: Should we call resize while suspended or should we not do
// anything here?
renderer.resize(size.width as i32, size.height as i32);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we call resize while suspended or should we not do anything here?

Copy link
Member

Choose a reason for hiding this comment

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

Theoretically there should:

  • Not be a resized event after Suspended (or before Resumed);
  • Not be a render Surface to resize to begin with (but perhaps user code keeps persistent backing buffers around to reuse on a Resumed).

Copy link
Member

Choose a reason for hiding this comment

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

All the buffers are in the context, so during suspend -> resume there's no surface to issue a resize.

Copy link
Contributor Author

@mickvangelderen mickvangelderen Oct 3, 2024

Choose a reason for hiding this comment

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

So in conclusion, the current implementation could call glViewport while the context is definitely not current, but allegedly the resized event is not triggered after suspend and so the problem does not actually occur.

The proposed implementation can be updated to mark the transition as invalid.

Comment on lines 123 to 171
// TODO: Should this be done by glutin by default?
struct ContextTerminateDisplay<T: GetGlDisplay<Target = Display>>(T);

impl<T: GetGlDisplay<Target = Display>> Deref for ContextTerminateDisplay<T> {
type Target = T;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl<T: GetGlDisplay<Target = Display>> ContextTerminateDisplay<T> {
unsafe fn into_inner_without_drop(self) -> T {
std::ptr::read(&std::mem::ManuallyDrop::new(self).0)
}
}

impl ContextTerminateDisplay<PossiblyCurrentContext> {
fn make_not_current(
self,
) -> Result<ContextTerminateDisplay<NotCurrentContext>, glutin::error::Error> {
unsafe { self.into_inner_without_drop().make_not_current().map(ContextTerminateDisplay) }
}
}

impl ContextTerminateDisplay<NotCurrentContext> {
fn make_current<T: glutin::surface::SurfaceTypeTrait>(
self,
surface: &glutin::surface::Surface<T>,
) -> Result<ContextTerminateDisplay<PossiblyCurrentContext>, glutin::error::Error> {
unsafe { self.into_inner_without_drop().make_current(surface).map(ContextTerminateDisplay) }
}
}

impl<T: GetGlDisplay<Target = Display>> Drop for ContextTerminateDisplay<T> {
fn drop(&mut self) {
// NOTE: The handling below is only needed due to nvidia on Wayland to not crash
// on exit due to nvidia driver touching the Wayland display from on
// `exit` hook.
let _gl_display = self.gl_context.take().unwrap().display();

// Clear the window.
self.state = None;
#[cfg(egl_backend)]
#[allow(irrefutable_let_patterns)]
if let glutin::display::Display::Egl(display) = _gl_display {
if let glutin::display::Display::Egl(display) = self.0.display() {
unsafe {
display.terminate();
}
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 50 lines can be removed if glutin can just do this for the egl backend. I am not sure about when display.terminate() can be called though, does the window have to be dropped before? After?

Comment on lines +293 to +301
struct AppStateResumed {
// NOTE: Window should be dropped after all resources created using its raw-window-handle.
gl_surface: Surface<WindowSurface>,
// NOTE: Window should be dropped after all resources created using its
// raw-window-handle.
window: Window,
// TODO: Does the context need to be dropped after the window? Specifically, the egl display
// terminate code, when should it run?
gl_context: ContextTerminateDisplay<PossiblyCurrentContext>,
renderer: Renderer,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the correct drop order for all of this?

Comment on lines +315 to +318
// TODO: Can we create the context and the surface in any order?
let gl_context = create_gl_context(&window, &gl_config);
let gl_surface = create_gl_surface(&window, &gl_config)?;
let gl_context = gl_context.make_current(&gl_surface)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can the context and surface be created in any order?

@mickvangelderen
Copy link
Contributor Author

I'm not sure about this one, since it gets more involved.

@kchibisov I understand. I do think that there are some things in this PR that could lead to improvements in glutin or the examples.

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