Skip to content

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 targeted 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
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 targeted 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