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

[Merged by Bors] - Fix panic in multiple_windows example #1737

Closed
wants to merge 3 commits into from

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Mar 23, 2021

Fixes #1736

There seemed to be an obvious transcription error where setup_pipeline was being called during on_update of AppState::CreateWindow instead of AppState::Setup.

However, the example still panicked after fixing that.

I'm not sure if there's some intended or unintended change in event processing during state transitions or something, but this PR make the example work again.

Cleaned up an unused Stage label while I was in there.

Please feel free to close this in favor of another approach.

@rparrett rparrett changed the title Fix #1736 Fix panic in multiple_windows example Mar 23, 2021
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples help wanted A-Windowing Platform-agnostic interface layer to run your app in labels Mar 23, 2021
@alice-i-cecile alice-i-cecile added this to the Bevy 0.5 milestone Mar 23, 2021
@alice-i-cecile
Copy link
Member

@TheRawMeatball you might want to take a look at this; I can't tell if it's reflective of an underlying State bug.

@TheRawMeatball
Copy link
Member

Why the extra Done state? You should change the on_updates to on_enters instead.

@mockersf
Copy link
Member

mockersf commented Mar 23, 2021

the setup_pipeline system is executed several times, until it gets a window_id that's not None

@jakobhellermann
Copy link
Contributor

Also fixes #1709. I looking into fixing that and added a Done state as well.

Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>
@rparrett
Copy link
Contributor Author

rparrett commented Mar 23, 2021

Yeah, 1709 (oops, missed that) explains a little better how I came to add the new Done state.

The issue is that state transitions seem to be behaving slightly differently since #1424 and the window no longer exists when CreateWindow's on_enter happens. Hence the on_update and checking for the existence of the new window and the new state.

It's not clear to me if this new behavior is expected, though.

Although setup_window could definitely be on_enter. (But it wasn't prior to or after 1424)

@cart
Copy link
Member

cart commented Mar 24, 2021

I think the previous impl worked with two states because we made a guarantee that only one "on_update" gets called per frame. This ensured that the stage exited after on_update and allowed the queued window to be created on the next frame.

The new state impl allows for multiple on_update calls per frame if a state change is queued up in on_update. This allows the entire state graph traversal (and associated events) to execute within the same frame.

I think the changes in this pr are the right call. The only way to improve this would be to somehow allow mid-frame window creation.

@cart
Copy link
Member

cart commented Mar 24, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 24, 2021
Fixes #1736

There seemed to be an obvious transcription error where `setup_pipeline` was being called during `on_update` of `AppState::CreateWindow` instead of `AppState::Setup`.

However, the example still panicked after fixing that.

I'm not sure if there's some intended or unintended change in event processing during state transitions or something, but this PR make the example work again.

Cleaned up an unused `Stage` label while I was in there.

Please feel free to close this in favor of another approach.
@bors bors bot changed the title Fix panic in multiple_windows example [Merged by Bors] - Fix panic in multiple_windows example Mar 24, 2021
@bors bors bot closed this Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

multiple_windows example panics
6 participants