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

Bugfix: Scene reload fix (nonbreaking) #7951

Merged
merged 13 commits into from
Mar 27, 2023

Conversation

Testare
Copy link
Contributor

@Testare Testare commented Mar 7, 2023

Objective

Fix a bug with scene reload.

(This is a copy of #7570 but without the breaking API change, in order to allow the bugfix to be introduced in 0.10.1)

When a scene was reloaded, it was corrupting components that weren't native to the scene itself. In particular, when a DynamicScene was created on Entity (A), all components in the scene without parents are automatically added as children of Entity (A). But if that scene was reloaded and the same ID of Entity (A) was a scene ID as well*, that parent component was corrupted, causing the hierarchy to become malformed and bevy to panic.

*For example, if Entity (A)'s ID was 3, and the scene contained an entity with ID 3

This issue could affect any components that:

  • Implemented MapEntities, basically components that contained references to other entities
  • Were added to entities from a scene file but weren't defined in the scene file

Solution

The solution was to keep track of entities+components that had MapEntities functionality during scene load, and only apply the entity update behavior to them. They were tracked with a HashMap from the component's TypeID to a vector of entity ID's. Then the ReflectMapEntities struct was updated to hold a function that took a list of entities to be applied to, instead of naively applying itself to all values in the EntityMap.

(See this PR comment #7570 (comment) for a story-based explanation of this bug and solution)

Changelog

Fixed

  • Components that implement MapEntities added to scene entities after load are not corrupted during scene reload.

.get::<Parent>()
.unwrap()
.get(),
"Something about reloading the scene is touching entities with the same scene Ids"
Copy link
Contributor

@Shatur Shatur Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor nitpick. Usually assert* and except messages in Rust starts with non-capital letters. See: https://doc.rust-lang.org/std/error/index.html#common-message-styles

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Useful knowledge =] Fixed it

@alice-i-cecile alice-i-cecile added this to the 0.10.1 milestone Mar 7, 2023
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior A-Scenes Serialized ECS data stored on the disk X-Controversial There is active debate or serious implications around merging this PR and removed A-Assets Load files from disk to use for things like images, models, and sounds labels Mar 7, 2023
When a scene was reloaded, it was corrupting components that
weren't native to the scene itself. In particular, when a DynamicScene
was created and Parent components added to entities without parents so
that they were children of the entity that spawne them, if that scene
was reloaded and the ID of the DyanmicSceneSpawner entity was in the
scene, that parent component was corrupted, causing the
hierarchy to become malformed and bevy to panic.
@Testare Testare force-pushed the scene_reload_fix_nonbreaking branch from c007d6e to 2a1043d Compare March 22, 2023 15:30
@cart cart added this pull request to the merge queue Mar 27, 2023
Merged via the queue into bevyengine:main with commit 3d8c768 Mar 27, 2023
mockersf pushed a commit that referenced this pull request Mar 27, 2023
Fix a bug with scene reload.

(This is a copy of #7570 but without the breaking API change, in order
to allow the bugfix to be introduced in 0.10.1)

When a scene was reloaded, it was corrupting components that weren't
native to the scene itself. In particular, when a DynamicScene was
created on Entity (A), all components in the scene without parents are
automatically added as children of Entity (A). But if that scene was
reloaded and the same ID of Entity (A) was a scene ID as well*, that
parent component was corrupted, causing the hierarchy to become
malformed and bevy to panic.

*For example, if Entity (A)'s ID was 3, and the scene contained an
entity with ID 3

This issue could affect any components that:
* Implemented `MapEntities`, basically components that contained
references to other entities
* Were added to entities from a scene file but weren't defined in the
scene file

- Fixes #7529

The solution was to keep track of entities+components that had
`MapEntities` functionality during scene load, and only apply the entity
update behavior to them. They were tracked with a HashMap from the
component's TypeID to a vector of entity ID's. Then the
`ReflectMapEntities` struct was updated to hold a function that took a
list of entities to be applied to, instead of naively applying itself to
all values in the EntityMap.

(See this PR comment
#7570 (comment) for
a story-based explanation of this bug and solution)

- Components that implement `MapEntities` added to scene entities after
load are not corrupted during scene reload.
UkoeHB pushed a commit to UkoeHB/bevy that referenced this pull request Sep 10, 2023
Fix a bug with scene reload.

(This is a copy of bevyengine#7570 but without the breaking API change, in order
to allow the bugfix to be introduced in 0.10.1)

When a scene was reloaded, it was corrupting components that weren't
native to the scene itself. In particular, when a DynamicScene was
created on Entity (A), all components in the scene without parents are
automatically added as children of Entity (A). But if that scene was
reloaded and the same ID of Entity (A) was a scene ID as well*, that
parent component was corrupted, causing the hierarchy to become
malformed and bevy to panic.

*For example, if Entity (A)'s ID was 3, and the scene contained an
entity with ID 3

This issue could affect any components that:
* Implemented `MapEntities`, basically components that contained
references to other entities
* Were added to entities from a scene file but weren't defined in the
scene file

- Fixes bevyengine#7529

The solution was to keep track of entities+components that had
`MapEntities` functionality during scene load, and only apply the entity
update behavior to them. They were tracked with a HashMap from the
component's TypeID to a vector of entity ID's. Then the
`ReflectMapEntities` struct was updated to hold a function that took a
list of entities to be applied to, instead of naively applying itself to
all values in the EntityMap.

(See this PR comment
bevyengine#7570 (comment) for
a story-based explanation of this bug and solution)

- Components that implement `MapEntities` added to scene entities after
load are not corrupted during scene reload.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior 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.

Bevy hot-reloading hierarchies cause panic (most of the time)
6 participants