-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Minimal Many-to-Many Relationships #20377
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
base: main
Are you sure you want to change the base?
Conversation
8435040
to
d486991
Compare
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
the previous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just one question on performance; but it doesn't affect existing one-to-many relationships so I don't think it's a blocker.
.unwrap() | ||
.get(); | ||
for target_entity in target_entities.iter() { | ||
if target_entity == entity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe .filter(|target_entity| target_entity != entity)
(with the comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matter of taste; I prefer the if statement to give space inside the loop for the lengthy comment.
|
||
world | ||
.commands() | ||
if let Ok(target_entity_cell) = world_cell.get_entity(target_entity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if the source_entity has a Relationship with 10 entities and we add an 11th; we will first apply the remove on all 10 entities in on_replace
(potentially queuing up 10 remove commands), and then in on_insert
we queue up 11 commands to add the entity?
It's probably fine since relationship modifications are usually rare, but maybe this should be documented? (updating the relationship 1 by 1 is a O(n2) op)
Maybe in the future we could have an improved ON_REPLACE observer that gives you both the existing component value and the new component value. Theoretically it should be doable since both are accessible here:
bevy/crates/bevy_ecs/src/bundle/insert.rs
Line 175 in 45c997c
deferred_world.trigger_on_replace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could have a flag (like LINKED_SPAWN); if present we would use a diffing approach to compare the old and the new values.
The diffing would require changes to the ON_REPLACE observer; or adding another component like CachedRelationship
that holds the previous values of the Relationship component.
This could be nice in particular for RelationshipCollections like HashSet or IndexSet where you can efficiently get the diff.
Out of scope for this PR though
if let Some(relationship) = source_entity_mut.get::<Self::Relationship>() { | ||
// Only despawn if this source entity's target collection only contains the entity being despawned | ||
if relationship.len() == 1 && relationship.get().contains(entity) { | ||
source_entity_mut.despawn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is unrelated to your PR, but I don't understand this part of the code anymore.
The docs say that Relationship is the source of truth; so how come we despawn the source entity if it has no more targets? Shouldn't we at least check for LINKED_SPAWN?
let b = world.spawn(Likes(a)).id(); | ||
let c = world.spawn(Likes(a)).id(); | ||
let b = world.spawn(Likes(vec![a])).id(); | ||
let c = world.spawn(Likes(vec![a, b])).id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add
assert_eq!(world.entity(b).get::<LikedBy>().unwrap().0, &[c]);
assert!(world.entity(c).get::<LikedBy>().is_none());
|
||
assert_eq!((spawner, spawn_tick), *TRACKED.get().unwrap()); | ||
assert_eq!( | ||
despawner, | ||
spawner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we have to slightly change despawn behavior to support many-to-many relationships, but I'm not exactly sure why this test needed to be changed. Guidance welcome!
The
It doesn't function correctly by directly mutating, that's why the docs for the |
@ItsDoot so the expected way to mutate the |
Doesn't seem to be playing well with |
The path for n:m relationships that I have in my head is having a component storage type of multi map, that way you would be able to have multiple |
Sounds like Fragmenting Relations |
Maybe you've answered this elsewhere, but is there any mechanism that prevents you doing stuff like this? #[derive(Component)]
#[relationship(relationship_target = Friends)]
#[relationship_target(relationship = Friends)]
struct Friends(pub Vec<Entity>); |
Nope. We could make the attributes mutually exclusive in the derive, but I don't think we could prevent someone manually implementing |
Would this work? Is this a back-door into undirected many-to-many relationships? I'd be pretty happy if this was something we could support. I don't think we should try to prevent this, it seems great. |
|
Ok, now I have a related (pardon the pun) question: It seems like under this, many-to-one relationships can be implemented in two ways. Either the |
This seems possible yes, but I have not personally tested it. |
I'll have a go at it. That seems like it might present a potential problem. You can't write generic stuff for directed many to one relations if you're not sure which side is the many and which side is the one. At the very least, the |
Objective
Solution
RelationshipSourceCollection
toRelationshipCollection
(alternatives welcome), since it's also used for target entities now.Collection: RelationshipCollection
associated type toRelationship
, mirroring theRelationshipTarget
trait.The API and docs are probably due for some improvements; this is mostly intended as a "get it working" PR. Originally I had taken this a lot further API-wise with
RelationshipCollection
sub-traits so each collection could have its own default-specified hooks, but I think it's better to start with a minimal support PR first.Testing
Added many (heh) new tests for many-to-many relationships.
Showcase
The
Relationship
side of a relationship now allows more than justEntity
: