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 option to ignore events when receving unknown WindowId #1072

Merged
merged 3 commits into from Dec 27, 2020

Conversation

refnil
Copy link
Contributor

@refnil refnil commented Dec 15, 2020

Bevy would panic when received window event for an unknown window id. This pull request add the option ignore_unknown_window_id to WinitConfig. When false, we keep the current behavior (panicking on unknown ID) which I think is fine for most regular bevy app. When true, a guard is added in the event loop and exit early if an unknown id is received.
This new behavior is useful when testing bevy app (see #1057).

@smokku
Copy link
Member

smokku commented Dec 15, 2020

When can Bevy receive window event for an unknown window id?

@refnil
Copy link
Contributor Author

refnil commented Dec 16, 2020

I've seen it happen in two situations. First, when running multiple bevy app simultaneously. I don't think this is something that bevy need to support. Second, when running two bevy app one after the other with return_after_run. Sometimes, the second app seem to receive some events from the previous app.

I'm personally running in the second case when running bevy app as integration tests sequentially.

One alternative to this PR would be to drain the event loop before or after running a bevy app. However, I don't really know how to do that.

@smokku
Copy link
Member

smokku commented Dec 16, 2020

If it can happen during normal run, we should make it default behavior (maybe with logging a warning), not an option.

@refnil
Copy link
Contributor Author

refnil commented Dec 16, 2020

I don't have an strong opinion for setting it as default behavior or an option. I'll change the PR accordingly if needed.
On one hand, I could see some value to the current behavior. Receiving unknown window id seems a bit fishy and maybe we want to tell the user to let him make his choices. On the other hand, making bevy "better" by default also seem like a good idea.

Who would decide on this? Should I open 2 PR and we vote on them?

@smokku
Copy link
Member

smokku commented Dec 16, 2020

Ultimately @cart has a last word.

@Moxinilian Moxinilian added C-Crash A sudden unexpected crash A-Windowing Platform-agnostic interface layer to run your app in labels Dec 17, 2020
@mockersf
Copy link
Member

@refnil I looked a little into that as I had a somewhat similar issue with event happening for a closed window. you can check #1082 (second crash) for some details. Just noticed you actually talking about that there

Could you list the events that you receive after in your case? Is it just a Focused(false) like me or are there other?

@refnil
Copy link
Contributor Author

refnil commented Dec 18, 2020

@mockersf I think I only received Focused events. Didn't really check for more detail at the time. I don't know if it is possible to receive other events unless you are running multiple winit app at the same time. In that case, events seem to be distributed randomly between the multiple event loops.

I'll follow up in your PR since I have some thought about it.

@mockersf
Copy link
Member

In that case, I think the two cases (Focused for one loop after another, and random events between multiple loops) should be handled differently...

how does it even work if events are random between the loops?

@refnil
Copy link
Contributor Author

refnil commented Dec 18, 2020

For the multiple loops thing, I don't think we should handle that a all. I could be wrong but I think there could be something in the windowing stack winit/window manager (x11, wayland) that would prevent that from working. I'd go as far as saying that they expect only on event loop by process.

@cart
Copy link
Member

cart commented Dec 18, 2020

(copying in some notes on this pr from #1082)

But I do like the approach in your PR to factor out the winit window fetch. It cuts down on duplicate boilerplate and ensures we ignore events the same way everywhere. If you can rebase your changes on master (after I merge this pr) and then remove the configuration option, I'd love to merge those changes (although i won't block the 0.4 release on it).

@refnil
Copy link
Contributor Author

refnil commented Dec 19, 2020

No problem. I'll rebase soon.

@refnil
Copy link
Contributor Author

refnil commented Dec 24, 2020

I've removed the option and reverted the WindowEvent::Focused part of #1082. Let me know if there is anything else to do.

@cart
Copy link
Member

cart commented Dec 26, 2020

just made a couple of minor tweaks. lets get this merged!

@cart cart merged commit 909b396 into bevyengine:master Dec 27, 2020
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-Crash A sudden unexpected crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants