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

correctly setup everything in the default run_once runner #8740

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Jun 1, 2023

Objective

  • Fix without_winit example panics #8658
  • without_winit example panics thread 'Compute Task Pool (2)' panicked at 'called Option::unwrap()on aNone value', crates/bevy_render/src/pipelined_rendering.rs:134:84

Solution

  • In the default runner method run_once, correctly finish the initialisation of the plugins. run_once can't be called twice so it's ok to do it there

@mockersf mockersf added the A-App Bevy apps and plugins label Jun 1, 2023
@mockersf mockersf changed the title correctly setup everything in the default run_once method correctly setup everything in the default run_once runner Jun 1, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Can you add simple doc strings to run_once, and note the limitation of "should only be called once" there?

@mockersf
Copy link
Member Author

mockersf commented Jun 1, 2023

I could, but it's not a new limitation that run_once can't be called twice. It's also not public.

To run it twice, one would have to do something like

    let mut app = App::new();
    app.add_plugins(DefaultPlugins.build().disable::<WinitPlugin>());
    app.run();
    app.run();

which doesn't work anyway because the run() method replaces the app with an empty one.

It feels a bit silly to put a comment "don't run this twice" on something called run_once to me.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I still think that's useful context for future engine devs, but I won't block on it :)

@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Jun 2, 2023
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

We should probably add some documentation somewhere that the runner is responsible for calling these functions, but that should probably be a separate pr.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 5, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 5, 2023
Merged via the queue into bevyengine:main with commit 4e25008 Jun 5, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

without_winit example panics
3 participants