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

Rename map_entities and map_specific_entities #7570

Merged
merged 1 commit into from
May 1, 2023

Conversation

Testare
Copy link
Contributor

@Testare Testare commented Feb 8, 2023

Objective

After fixing dynamic scene to only map specific entities, we want map_entities to default to the less error prone behavior and have the previous behavior renamed to "map_all_entities." As this is a breaking change, it could not be pushed out with the bug fix.

Solution

Simple rename and refactor.

Changelog

Changed

  • map_entities now accepts a list of entities to apply to, with map_all_entities retaining previous behavior of applying to all entities in the map.

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_entities, but consider if passing the entities in specifically might be better for your use case to avoid bugs.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Scenes Serialized ECS data stored on the disk A-ECS Entities, components, systems, and events labels Feb 8, 2023
@Testare
Copy link
Contributor Author

Testare commented Feb 8, 2023

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

JMS55 commented Feb 8, 2023

bors try-

bors bot added a commit that referenced this pull request Feb 8, 2023
@Illiux
Copy link
Contributor

Illiux commented Feb 11, 2023

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
Copy link
Contributor Author

Testare commented Feb 11, 2023

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

Illiux commented Feb 11, 2023

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

Illiux commented Feb 11, 2023

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

@Testare
Copy link
Contributor Author

Testare commented Feb 12, 2023

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

@Testare
Copy link
Contributor Author

Testare commented Feb 16, 2023

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.

@MrGVSV MrGVSV self-requested a review February 16, 2023 06:46
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Seems like a simple fix for this. Thanks for the awesome "Visual Journey" explanation— it really helped to make this problem more digestible!

Just a few comments.

(self.map_entities)(world, entity_map, entity_map.values().collect())
}

pub fn map_specific_entities(
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Doc comments would be helpful for helping users understand why they would want to use this method over map_entities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Collection of entities that have references to entities in the scene,
// that need to be updated to entities in the world.
// Keyed by Component's TypeId.
let mut entity_mapped_entities = HashMap::default();
Copy link
Member

Choose a reason for hiding this comment

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

Minor Nit: Not super necessary, but I think explicitly specifying the type here can be helpful for review/readability. Otherwise, we either need to load up an editor or mentally track the key/value types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh definitely!

// Collection of entities that have references to entities in the scene,
// that need to be updated to entities in the world.
// Keyed by Component's TypeId.
let mut entity_mapped_entities = HashMap::default();
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed: Name could be clearer imo. Maybe scene_mappings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeaah, that makes sense. I updated the comment too to be a little more clear, hopefully.

fn components_not_defined_in_scene_should_not_be_effected_by_scene_entity_map() {
// Testing that scene reloading applies EntitiyMap correctly to MapEntities components.

// First, we create a simple world with a parent and a child relationship
Copy link
Member

Choose a reason for hiding this comment

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

Glad to see such a thorough test complete with helpful step-by-step comments that walk us through the intention behind the test. Thanks for this!

use crate::dynamic_scene_builder::DynamicSceneBuilder;

#[test]
fn components_not_defined_in_scene_should_not_be_effected_by_scene_entity_map() {
Copy link
Member

Choose a reason for hiding this comment

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

Typo:

Suggested change
fn components_not_defined_in_scene_should_not_be_effected_by_scene_entity_map() {
fn components_not_defined_in_scene_should_not_be_affected_by_scene_entity_map() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooh good catch!

.resource_mut::<AppTypeRegistry>()
.write()
.register::<Parent>();
let gen_0_entity = world.spawn_empty().id();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This test might be a bit easier to read if these names were a little more descriptive. Maybe like world_entity_a/scene_entity_a or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I did orginal_/from_scene_ prefixes, and then explicitly called them parent_entity/child_entity.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this is much clearer. Thanks!

@Testare
Copy link
Contributor Author

Testare commented Feb 20, 2023

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

Applied the feedback, things should be clearer now.

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Just a few comments on documentation stuff.

Happy belated birthday btw! 🥳

(self.map_entities)(world, entity_map, entity_map.values().collect())
}

/// This is like `map_entities`, but only applied to specific entities, not all values in the `EntityMap`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You can probably make map_entities and EntityMap links.


/// This is like `map_entities`, but only applied to specific entities, not all values in the `EntityMap`.
///
/// This is useful for when you only want to apply the `MapEntity` logic to specific entities. For example,
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
/// This is useful for when you only want to apply the `MapEntity` logic to specific entities. For example,
/// This is useful for when you only want to apply the [`MapEntities`] logic to specific entities. For example,

Comment on lines 427 to 429
/// a scene can be loaded with `Parent` components, but then a `Parent` component can be added to these entities
/// after they have been loaded. If you reload the scene and used plain `map_entities`, those `Parent` components
/// with already valid entity references could be updated to point at something else entirely.
Copy link
Member

Choose a reason for hiding this comment

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

This might be a little too specific of an example haha. Maybe we just say something along the lines of how it scopes the mapping to a specific pool of entities so as to preserve the proper relations? Or at least something a bit more general?


Hm, that actually makes me wonder. Do we ever want the standard map_entities function? Would it be better to just merge this function into that one? Then just require users who do want it to do map_entities(&mut world, &entity_map, entity_map.values().collect())?

I may be missing a glaringly obvious reason for keeping it haha but rn it sounds like leaving the door open to footguns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I left this here as a habit from work where if you have a public API that might be consumed by others, leave it alone. But I realize that's not TheBevyWay(tm) right now, so we can go ahead and replace it.

So I used ripgrep to find invocations, and I found out the only other place is in Scene::write_to_world_with (Not DynamicScene::write_to_world_with). This bug does not exist in this case because EntityMap is not passed to this code; it seems you can't do a "reload" with a Scene. I'll modify the code there to pass in the entity values though to use this interface.

/// This is like `map_entities`, but only applied to specific entities, not all values in the `EntityMap`.
///
/// This is useful for when you only want to apply the `MapEntity` logic to specific entities. For example,
/// a scene can be loaded with `Parent` components, but then a `Parent` component can be added to these entities
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I wonder if we should avoid mentioning "scenes" in bevy_ecs content like this since it's not actually a dependency of the crate. Perhaps we use the concept of "worlds" since scenes are basically a tiny and limited World?

@alice-i-cecile probably has more knowledge in this area

Copy link
Member

Choose a reason for hiding this comment

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

I think that this is probably net useful: humans can figure this out.

@Testare
Copy link
Contributor Author

Testare commented Feb 21, 2023

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?

@Shatur Shatur added this to the 0.10 milestone Feb 22, 2023
Copy link
Contributor

@Shatur Shatur left a comment

Choose a reason for hiding this comment

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

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.

@MrGVSV
Copy link
Member

MrGVSV commented Feb 22, 2023

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
Copy link
Contributor Author

Testare commented Feb 22, 2023

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?

if let Some(mut component) = world.get_mut::<C>(entity) {
component.map_entities(entity_map)?;
map_entities: |world, entity_map, entities_opt| {
if let Some(entities) = entities_opt {
Copy link
Contributor

@Shatur Shatur Feb 22, 2023

Choose a reason for hiding this comment

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

Oh, we can't use unwrap_or_else on entities_opt, right?
Then I would provide two separate methods. It's more explicit and we duplicate loops anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah my rustfu isn't strong enough to know how to unwrap an Option of dyn Iterator<Item=Entity>. I tried to make it prettier but i couldn't without collecting.

We do provide two public methods, but they both call one internal closure. Are you saying you'd prefer we store two closures?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do provide two public methods, but they both call one internal closure. Are you saying you'd prefer we store two closures?

Yep!

Copy link
Contributor Author

@Testare Testare Feb 22, 2023

Choose a reason for hiding this comment

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

Oh, or do you mean have a separate method for the inner loop that the closure calls?

EDIT: sorry, didn't see you already responded XP

Ok I'll get on that

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Just a doc request. But otherwise, I'd like to see this merged, it expands the range of things you can use DynamicScene for by quite a lot.

for registration in type_registry.iter() {
// Updates references to entities in the scene to entities in the world
for (type_id, entities) in scene_mappings.into_iter() {
let registration = type_registry.get(type_id).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like unwrapping here is fine, since by construction scene_mappings is populated by registrations that come from the type_registry. But it would be a good idea to make the expectation clear (through a comment), so that in the future, say if previous code changes, and the code panic, the poor soul dealing with the code has a chance of understanding why it happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better with expect.

@@ -115,7 +115,8 @@ impl DynamicScene {

// Updates references to entities in the scene to entities in the world
for (type_id, entities) in scene_mappings.into_iter() {
let registration = type_registry.get(type_id).unwrap();
let registration = type_registry.get(type_id)
.expect("This TypeRegistration was where we got the TypeId initially, should be safe to unwrap");
Copy link
Contributor

@Shatur Shatur Feb 22, 2023

Choose a reason for hiding this comment

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

One last minor nitpick. In Rust panic messages usually written differently.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM

@nicopap nicopap modified the milestones: 0.10.1, 0.11 Mar 7, 2023
@nicopap
Copy link
Contributor

nicopap commented Mar 7, 2023

## 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.

@nicopap nicopap added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

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
Copy link
Contributor Author

Testare commented Mar 7, 2023

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

cart pushed a commit that referenced this pull request Mar 27, 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

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

(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.
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.
@nicopap
Copy link
Contributor

nicopap commented Apr 22, 2023

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

@alice-i-cecile alice-i-cecile removed X-Controversial There is active debate or serious implications around merging this PR S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 1, 2023
@alice-i-cecile
Copy link
Member

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

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change and removed C-Bug An unexpected or incorrect behavior C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels May 1, 2023
@Testare Testare changed the title Bugfix: Scene reloading corruption Rename map_entities and map_specific_entities May 1, 2023
@Testare
Copy link
Contributor Author

Testare commented May 1, 2023

@alice-i-cecile Updated PR description =]

@alice-i-cecile alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label May 1, 2023
@alice-i-cecile
Copy link
Member

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

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 1, 2023
@Testare
Copy link
Contributor Author

Testare commented May 1, 2023

Awww, thanks =]

Merged via the queue into bevyengine:main with commit a29d328 May 1, 2023
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-ECS Entities, components, systems, and events A-Scenes Serialized ECS data stored on the disk C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants