bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Register missing types in bevy_window

Open lewiszlw opened this issue 2 years ago • 3 comments

Objective

  • Fixes https://github.com/bevyengine/bevy/issues/7990.

Solution

  • Register needed types, verified pasted code in issue works.

Do I need to register more Option<T> types?

lewiszlw avatar Mar 09 '23 03:03 lewiszlw

IMO we should not be serializing window state in scenes. This feels like a massive footgun waiting to happen.

james7132 avatar Mar 09 '23 03:03 james7132

I'm basically in agreement with that. I think we should instead exclude these components and entities from the default serialization.

alice-i-cecile avatar Mar 09 '23 03:03 alice-i-cecile

I'm basically in agreement with that. I think we should instead exclude these components and entities from the default serialization.

Should be possible once we merge #6793

MrGVSV avatar Mar 09 '23 04:03 MrGVSV

IMO we should not be serializing window state in scenes. This feels like a massive footgun waiting to happen.

As in, scenes should not be able to spawn windows? If so I strongly disagree. "Windows in scenes" seems like a useful feature and was one of the reasons used to justify windows as entities. Our Window type is designed to be a "high level user facing component" and I'm not seeing anything that would be particularly troublesome here.

cart avatar Mar 21 '23 21:03 cart

I think that serializing windows-as-entities is a valid use case, but not something that's going to be desired most of the time.

I think we should merge this, but not for 0.10.1, and not until #6793 is shipped to give users better control over what exactly is getting stored in the scene.

Dynamic scenes being broken really sucks, but in the current state the linked code will try to save the game, grabbing window state. When that scene is later spawned we'll have two windows, which will be very surprising (especially for a minor version bump), and will often panic user games which call query.single on windows.

alice-i-cecile avatar Mar 27 '23 21:03 alice-i-cecile

I guess thats reasonable. Hard to say which behavior is more broken: serialization being broken by default, or spawning serialized default worlds duplicating the window created by DefaultPlugins.

Definitely worth discussing more / resolving these interactions. I think I'm still in favor of fixing this now because:

  1. This is a crash in a common use case
  2. Supporting this scenario was intended
  3. The duplicate window problem is resolved by disabling default window creation.

But if you still feel like this should wait thats enough controversy to hold off until later.

cart avatar Mar 27 '23 21:03 cart

This is a crash in a common use case

I guess "common" is a pretty big stretch of the word

cart avatar Mar 27 '23 21:03 cart

You've convinced me: weak preference to merge now, as the created buggy behavior is easier for end users to work around :p

alice-i-cecile avatar Mar 27 '23 21:03 alice-i-cecile