Bugfix: Scene reloading corruption

[Testare]

Objective

Fix a bug with scene reload.

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

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.

I believe this change in struct definition to be safe since ReflectMapEntities is primarily with the FromType trait, which has been updated for this change. An additional method has been added to specify the entities applied, and the old method still exists using the EntityMap values as the entities to be applied to.

(See this PR comment https://github.com/bevyengine/bevy/pull/7570#issuecomment-1432302796 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.

Migration Guide

• in bevy_ecs, ReflectMapEntities::map_entites now requires an additional entities parameter to specify which entities it applies to. To keep the old behavior, use the new `ReflectMapEntities::map_all_entites

[Testare]

markdownlint failed because of github throttling

Error: fatal: unable to access 'https://github.com/bevyengine/bevy/': The requested URL returned error: 429

(429 means too many request)

Is there a way to rerun the check?

[JMS55]

bors try-

[bors[bot]]

try

Build succeeded:

• build (macos-latest)
• build-and-install-on-iOS
• build-without-default-features (bevy_reflect)
• check-doc
• build (windows-latest)
• build (ubuntu-latest)
• build-wasm
• build-android
• markdownlint
• run-examples
• run-examples-on-wasm
• check-missing-examples-in-docs
• ci
• check-compiles
• run-examples-on-windows-dx12
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• msrv

[Illiux]

I don't think this approach is robust. It's possible that an entity has both a DynamicScene-added Parent component and some other component that implements MapEntities. In that case, I believe, you'd have to somehow map every component on that entity except the Parent component. Your current changes would map every component on the entity including Parent and cause the same bugged behavior. Maybe the better approach is to remove all the Parent components before applying the scene then re-add them according to normal logic.

[Testare]

Parent is used as an example, since it is the most pertinent (Parent is added to entities automatically through scene load and causes panic when set improperly, hence how this bug came to my attention), but I did write the code to solve this issue for any MapEntities component.

Basically, the code works by making sure that when a dynamic scene is loaded, it keeps track of all the MapEntities components (by TypeID and Entity), and makes sure that only the MapEntities components that are loaded from the scene are invoked with the entities map.

So let's make a theoretical component struct StepFather(Entity) that implements MapEntities. If this component is added to a scene file, it will be affected by scene file changes. But if another system were to add this component to scene entities, unless somebody edits the scene file to add the component to the scene file, MapEntities won't be invoked (And in this case, we would WANT MapEntities invoked).

Importantly, this also works even if some components in the scene have StepFather components loaded from files, and others have StepFather components added through systems.

As for the approach of removing parent components and re-adding them, I /did/ try that and it seemed to work fine for Parents-only, though the UI seemed to do weird things (I think children order might not have been preserved). But the big issue was as you've said, that approach only fixes the problem for Parent components, other MapEntities components added systemically would suffer the same issues.

[Illiux]

So let's make a theoretical component struct StepFather(Entity) that implements MapEntities. If this component is added to a scene file, it will be affected by scene file changes. But if another system were to add this component to scene entities, unless somebody edits the scene file to add the component to the scene file, MapEntities won't be invoked (And in this case, we would WANT MapEntities invoked).

The issue would be if the scene has a StepFather component and then some other system adds a Parent component to the scene entities. MapEntities gets invoked on Parent when Parent isn't in the scene.

[Illiux]

Oh wait, nevermind I see. I was misreading the code and didn't properly notice that the entities list is per-component.

[Testare]

Oh hahaha yeah i can see how that would be easy to miss XP Do you think it's fine then?

[Testare]

Okay, I'm finding it hard to communicate technically what this bug is and what I did to solve it... So I wrote it out in story/table form! Hopefully it's more entertaining AND clear this way.

Note: The component P in this story is loosely based on Parent, so if it helps you visualize it you can think of P that way. But I wrote it as P because the bug affects all MapEntities components; P could just as easily stand for GankedLastBy, CrushOfTheWeek, or SecretSanta.

The Scene-Reload Bug In 3 Acts: A Visual Journey

Click to read the story

Act I: Setting Up

Let's say we have a component type P. P components contain references to other entities, and so P implements the MapEntities trait in order for these references to be maintained when loaded from scenes or across networks.

Now, you have a world with one entity, and the only component it has is a name. It does not have a P component. I'll illustrate this with a table

World

Entity	P	Name
0	X	Alice

Now you have a scene file that describes two named entities, and one of them has a P component referencing the other

Scene

Entity	P	Name
0	X	Bob
1	0	Charlie

This scene is then loaded into the world with a scene loader. After it has loaded the components, but not applied ReflectMapEntities logic, it looks like this:

World

Entity	P	Name
0	X	Alice
1	X	Bob
2	0	Charlie

Now you'll notice that the P Component is not actually referencing Bob, like the scene defined, since obviously Entity 0 was already taken in the world so the entities loaded from the scene must be given new ones. But while those components were loaded, an EntityMap was created that shows the relationship from the Entity defined in the Scene to the actual Entity in the world.

Entity Map

Scene Entity	World Entity
0	1
1	2

Since P implements MapEntities, it has logic to correct the entity reference using this map. This logic is invoked while the scene is being loaded, applying to all P components where their entity is defined in the EntityMap (1 and 2), so that it looks like this:

World

Entity	P	Name
0	X	Alice
1	X	Bob
2	1	Charlie

Now things look exactly as they should! Bob is Charlie's P, whatever that means.

Act II: The Bug Revealed

Now, things happen in the world that might not be exactly as described in the Scene. Alice, who the scene knew nothing about, becomes Bob's P.

World

Entity	P	Name
0	X	Alice
1	0	Bob
2	1	Charlie

These things happen. Then, we decide that Bob should really go by his full name Robert. We update the scene...

Scene

Entity	P	Name
0	X	Robert
1	0	Charlie

...Then the scene is reloaded! Before we apply MapEntities logic, the World looks like this:

World

Entity	P	Name
0	X	Alice
1	0	Robert
2	0	Charlie

Now a couple things to note. Fortunately for us, scene reloading does not remove or even affect components that aren't defined explicitly in the scene. That means that Alice is safe as Robert's P, even though his name has been changed. This is good, since a large part of the draw of hot-reloading is that state is otherwise unaffected by the reload.

However, Charlie isn't so lucky. Since his P is defined in the Scene, it gets updated to the value defined in the Scene, back to 0. But don't worry Charlie, we still have that EntityMap from before! So let's update P for our scene entities...

World

Entity	P	Name
0	X	Alice
1	1	Robert
2	1	Charlie

There we go Charlie! You're now back to having Robert as your P, all is right with the World-

Wait, no! Alice and Robert are mortified! Alice is no longer Robert's P. Instead Robert's P is... Robert himself?! What will he neighbors think?! There will be shouts of Malformed hierarchy!

It turns out that when we were going around correcting P for all the entities in the scene, we saw that Robert had a P component. Thinking it must still contain a scene entity, we applied the map to point at a real world Entity. But P isn't defined in the scene, so it already had a proper world Entity defined, and now we've gone and set the pointer to some other random entity defined in the scene, in this case the selfsame entity.

Act III: Happily ever After

So here's the solution: Let's turn back time. This time, when we're going to correct the P component with ReflectMapEntities, we'll give it a bit of extra information. We'll make sure that it'll only correct P when the P was actually loaded from the Scene. In this case that's a very short list, a list of only 2.

World

Entity	P	Name
0	X	Alice
1	0	Robert
2	1	Charlie

There we go. Life as we intended it.

Interview with the Author: Handling Multiple Component Types

Interviewer: People often ask, what if there were two components that implemented MapEntities? What if there was another Component, let's say C, and Bob/Robert DID have that component defined in the scene?

Author: Well let's say we defined the scene like this:

Scene

Entity	P	C	Name
0	X	1	Bob
1	0	X	Charlie

Then we skipped ahead in the story to the part where the bug first appeared, upon loading the scene but before updating the entities, it would look something like this:

World

Entity	P	C	Name
0	X	1	Alice
1	0	1	Robert
2	0	X	Charlie

I took the liberty of assuming Robert would be Alice's C, it seems to make sense for their characters. Now people could worry that if we only had the EntityMap applied to entity 2, Charlie, then we would have a similar situation to before, where Robert is his own C.

World

Entity	P	C	Name
0	X	1	Alice
1	0	1	Robert
2	1	X	Charlie

But really the solution to this is quite simple. The MapEntities logic is already called on a per-component-type basis. It goes down each column individually. So we just have to maintain a different entity list for each component, like a HashMap<ComponentId, Vec<Entity>>. So for the scene I wrote out earlier, the list for component P would only contain 2, Charlie, and the list for Component C would only contain 1, Robert. So we would only update those two specific components, those "cells" in the table, so to speak.

World

Entity	P	C	Name
0	X	1	Alice
1	0	2	Robert
2	1	X	Charlie

So the relationship between Robert and Charlie is still exactly as we defined in the scene, without affecting Robert's relationship with Alice.

Interviewer: Fascinating.

[Testare]

Awww, yesterday was my birthday! PR feedback is a great birthday present <3

Applied the feedback, things should be clearer now.

[Testare]

Sorry for the delay, yesterday was also abnormally busy.

As I said in reply to one of your comments, I think it makes sense just to combine the two methods and remove footguns. There was one other place it was used, in the Scene class, and so I made the adjustment there.

Now that the separate method has been removed, I guess that kinda resolves all the comments about those doc comments. Unless you think I should add a doc comment for the map_entities function itself?

[MrGVSV]

I like this change. But I would suggest to keep the original map_entities and rename it into map_all_entities and call your new method map_entities. It will allow to avoid extra Vec allocation. Also I would use slice instead of &Vec.

Could we just take in an impl Iterator<Item = Entity> instead of &Vec<Entity>/&[Entity]? That would mean we can pass entity_map.values() directly without collecting into a Vec. And it removes the need for keeping around a possibly-footgunny method.

[Testare]

Sadly, I don't think we can do that, because the way ReflectMapEntities is implemented is with an internal closure, and you can't make closures generic over traits. I think I can do it with slices though.

I liked what Shatur suggested though about making a map_entities and a map_all_entities function though. We can use an Option<&[Entity]> in the internal closure, and if the Option is None, we just iterate over the entity_map values in the closure itself. That way we don't need to allocate any Vec's that way either.

Thoughts?

[nicopap]

## Migration Guide

- in `bevy_ecs`, `ReflectMapEntities::map_entites` now
  requires an additional `entities` parameter to specify
  which entities it applies to. To keep the old behavior,
  use the new `ReflectMapEntities::map_all_entites` instead.

[github-actions[bot]]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy? It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[Testare]

Created a PR #7951 for this bugfix without the breaking changes

[nicopap]

Now that #7951 is merged, you need to resolve the conflicts, it should be ready for final review/merge.

[alice-i-cecile]

@Testare once you've updated your PR description I'm happy to merge this :) The rename still looks nice.

[Testare]

@alice-i-cecile Updated PR description =]

[alice-i-cecile]

Excellent PR description: really clearly motivates the change and provides a helpful migration guide. Thanks!

[Testare]

Awww, thanks =]