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

Register missing types in bevy_window #7993

Merged
merged 1 commit into from Mar 27, 2023
Merged

Conversation

lewiszlw
Copy link
Member

@lewiszlw lewiszlw commented Mar 9, 2023

Objective

Solution

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

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

@alice-i-cecile alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Mar 9, 2023
@alice-i-cecile alice-i-cecile modified the milestones: 0.11, 0.10.1 Mar 9, 2023
@james7132
Copy link
Member

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

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Mar 9, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 9, 2023

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

@MrGVSV
Copy link
Member

MrGVSV commented Mar 9, 2023

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

@cart
Copy link
Member

cart commented Mar 21, 2023

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.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 27, 2023

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.

@cart
Copy link
Member

cart commented Mar 27, 2023

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
Copy link
Member

cart commented Mar 27, 2023

This is a crash in a common use case

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

@alice-i-cecile
Copy link
Member

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

@cart cart added this pull request to the merge queue Mar 27, 2023
Merged via the queue into bevyengine:main with commit 8ff7e0d Mar 27, 2023
25 checks passed
mockersf pushed a commit that referenced this pull request Mar 27, 2023
# Objective

-  Fixes #7990.

## Solution

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

Do I need to register more `Option<T>` types?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types A-Windowing Platform-agnostic interface layer to run your app in C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DynamicScene Serialization broken by Windows-as-Entities
5 participants