Skip to content

Optimize spawn_batch + relations #19932

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+41 −23
Open

Optimize spawn_batch + relations #19932

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e63d60
Defer the whole operation of the on_insert hook
gwafotapa Jul 3, 2025
a1df086
Update the comment explaining why we defer
gwafotapa Jul 4, 2025
106e8a2
Shorten a call
gwafotapa Jul 4, 2025
370698c
Refine the comment
gwafotapa Jul 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 41 additions & 23 deletions crates/bevy_ecs/src/relationship/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::{
entity::{ComponentCloneCtx, Entity, SourceComponent},
error::CommandWithEntity,
lifecycle::HookContext,
world::{DeferredWorld, EntityWorldMut},
world::{DeferredWorld, EntityWorldMut, World},
};
use log::warn;

Expand Down Expand Up @@ -115,7 +115,7 @@ pub trait Relationship: Component + Sized {
}
}
}
let target_entity = world.entity(entity).get::<Self>().unwrap().get();
let target_entity = world.get::<Self>(entity).unwrap().get();
if target_entity == entity {
warn!(
"{}The {}({target_entity:?}) relationship on entity {entity:?} points to itself. The invalid {} relationship has been removed.",
Expand All @@ -126,27 +126,45 @@ pub trait Relationship: Component + Sized {
world.commands().entity(entity).remove::<Self>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was expecting to pull in the code starting from let target_entity = ... into the closure, as well, since it also ends with a command. But I can't convince myself it matters one way or the other, so keeping the change small seems better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I preferred having the target_entity locked in before deferring. If we get the target_entity from the command, our Relationship component could have been modified and we could end up applying the command to the wrong target. With that, it made sense to run the next block right away as well.

return;
}
if let Ok(mut entity_commands) = world.commands().get_entity(target_entity) {
// Deferring is necessary for batch mode
entity_commands
.entry::<Self::RelationshipTarget>()
.and_modify(move |mut relationship_target| {
relationship_target.collection_mut_risky().add(entity);
})
.or_insert_with(|| {
let mut target = Self::RelationshipTarget::with_capacity(1);
target.collection_mut_risky().add(entity);
target
});
} else {
warn!(
"{}The {}({target_entity:?}) relationship on entity {entity:?} relates to an entity that does not exist. The invalid {} relationship has been removed.",
caller.map(|location|format!("{location}: ")).unwrap_or_default(),
DebugName::type_name::<Self>(),
DebugName::type_name::<Self>()
);
world.commands().entity(entity).remove::<Self>();
}

// We defer the decision of inserting or modifying RelationshipTarget. Deciding now to defer
// either an insertion or a modification causes an issue when inserting in batch if the
// target doesn't have the component yet. In that case, since the hooks run with no flush in
// between, we end up deferring a batch of commands all inserting RelationshipTarget with a
// single entity, each command overwriting the previous one when applied, resulting in the
// target having an only child instead of the whole batch.
world.commands().queue(move |world: &mut World| {
if let Ok(mut target_entity_world) = world.get_entity_mut(target_entity) {
target_entity_world
.entry::<Self::RelationshipTarget>()
.and_modify(move |mut relationship_target| {
relationship_target.collection_mut_risky().add(entity);
})
.or_insert_with(|| {
let mut target = Self::RelationshipTarget::with_capacity(1);
target.collection_mut_risky().add(entity);
target
});
} else if let Ok(mut entity_world) = world.get_entity_mut(entity) {
warn!(
"{}The {}({target_entity:?}) relationship on entity {entity:?} relates to an entity that does not exist. The invalid {} relationship has been removed.",
caller
.map(|location| format!("{location}: "))
.unwrap_or_default(),
DebugName::type_name::<Self>(),
DebugName::type_name::<Self>()
);
entity_world.remove::<Self>();
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure we want this branch but in the case where the original entity doesn't exist either, I thought that information could be useful to our user.

Copy link
Contributor

Choose a reason for hiding this comment

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

My first thought was that warning here was consistent with the old code, but now I'm leaning towards not logging anything.

If we hit this branch, then the entity was despawned in between the observer and the command. And the despawn hook would have tried to remove entity from the Self::RelationshipTarget when that happened. So it's okay that we can't add entity to Self::RelationshipTarget, because we want it removed anyway.

Looking at it another way: The warnings here are when you try to add a relation and the hook prevents it from being added. In this case, the hook didn't prevent it; some other hook or observer did by despawning the entity.

But mostly I don't expect that branch to ever be hit, so I don't think it matters much either way!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that branch is rarely ever hit and it hurts readability a bit. On the other hand, When it is hit, it means our user got some debugging to do! And even though the culprit is elsewhere, it helps to know that the hook failed instead of leaving our user in the dark.

warn!(
"{}The {} relationship between entity {entity:?} and its target {target_entity:?} could not be set up because these entities don't exist.",
caller
.map(|location| format!("{location}: "))
.unwrap_or_default(),
DebugName::type_name::<Self>(),
);
}
});
}

/// The `on_replace` component hook that maintains the [`Relationship`] / [`RelationshipTarget`] connection.
Expand Down