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

Add PossiblyCurrentContext::make_not_current_as_possibly_current #1705

Conversation

mickvangelderen
Copy link
Contributor

@mickvangelderen mickvangelderen commented Oct 2, 2024

Fixes #1704

Allow making the context not current by reference.

  • 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

@mickvangelderen mickvangelderen marked this pull request as ready for review October 2, 2024 13:47
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I feel like in_place is a bit better, since it's just shorter...

Other than that should be good.

Allow making the context not current by reference.
@mickvangelderen mickvangelderen force-pushed the add-make-not-current-as-possibly-current branch from 1497b4f to 6701890 Compare October 2, 2024 14:29
@mickvangelderen
Copy link
Contributor Author

I rebased to redo the commit message after the name-change

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I completely forgot, could you change the src/lib.rs in examples to also use this function and not Option?

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Though, it has reasons to use Option since it's needed during drop, so maybe it's fine as is.

Up to you, but I won't block on it, since it won't save much, just by looking more into it.

@mickvangelderen
Copy link
Contributor Author

mickvangelderen commented Oct 2, 2024

I think the example code can be reworked to match the diagram I sketched in #1704. There are a number of Option<T>'s that together define a state machine. One thing I noticed is that it is possible that the renderer will be asked to resize while the application is suspended and the context is not current. I am not sure this is a problem, but the fact that this is not obvious is a result of the current type-level design.

If you are curious what I mean, I'm happy to take a shot at improving the example code and make a proposal. For this PR however, I think this is good enough.

The proposal would look something like this (taken from the engine I'm working on):

#[derive(Default)]
enum EngineState<TGame, TRenderContext: craft_engine_core::RenderContext> {
    #[default]
    Uninitialized,
    Initialized {
        game: TGame,
        render_context: TRenderContext,
        window: winit::window::Window,
    },
    Suspended {
        game: TGame,
        render_context: TRenderContext::Suspended,
    },
    Exited,
}

Except that there would not be a generic game state or render context, and the render context can be inlined.

@kchibisov
Copy link
Member

Nah, the only issue is because we need to drop the context from inside the resume without moving it out, which is not possible, since it's all RAII...

Like the example is already quite complex, so I'd probably won't add anthying more to it.

glutin/src/context.rs Outdated Show resolved Hide resolved
@kchibisov kchibisov merged commit 5a7783c into rust-windowing:master Oct 2, 2024
43 checks passed
@mickvangelderen mickvangelderen deleted the add-make-not-current-as-possibly-current branch October 2, 2024 21:55
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.

Expose non-moving PossiblyCurrentContext::make_not_current
2 participants