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

Entity Relations #1627

Closed
wants to merge 2 commits into from
Closed

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Mar 12, 2021

RFC

warning: this PR does not implement all of the RFC and some of the RFC is out of date

cc #1527 #1592 #142

@alice-i-cecile alice-i-cecile added the A-ECS Entities, components, systems, and events label Mar 12, 2021
@alice-i-cecile alice-i-cecile mentioned this pull request Mar 24, 2021
9 tasks
crates/bevy_ecs/Cargo.toml Outdated Show resolved Hide resolved
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Apr 7, 2021

Future work after this PR: think about how relations should work with scenes

@BoxyUwU BoxyUwU changed the title Relationship ECS refactoring Implement min_relations RFC Apr 8, 2021
@BoxyUwU BoxyUwU mentioned this pull request Apr 8, 2021
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Apr 9, 2021

With the last commit this test now passes:

#[test]
fn query() {
    struct ChildOf;

    let mut world = World::new();

    let parent1 = world.spawn().id();
    let child1 = world.spawn().insert_relation(ChildOf, parent1).id();
    let parent2 = world.spawn().id();
    let child2 = world.spawn().insert_relation(ChildOf, parent2).id();

    let mut query = world.query::<(Entity, &Relation<ChildOf>)>();
    let mut iter = query.iter_mut(&mut world);
    assert!(iter.next() == Some((child1, ())));
    assert!(iter.next() == Some((child2, ())));
    assert!(iter.next() == None);

    query.set_relation_filter(
        &world,
        QueryRelationFilter::new().add_target_filter::<ChildOf, _>(parent1),
    );
    let mut iter = query.iter_mut(&mut world);
    assert!(iter.next() == Some((child1, ())));
    assert!(iter.next() == None);

    query.set_relation_filter(
        &world,
        QueryRelationFilter::new().add_target_filter::<ChildOf, _>(parent2),
    );
    let mut iter = query.iter_mut(&mut world);
    assert!(iter.next() == Some((child2, ())));
    assert!(iter.next() == None);
}

Still a lot of work to do but it's nice to see something working :O

@NathanSWard
Copy link
Contributor

Would you have an abstract for the overall approach to your implementation?
E.g. talking about the move from ComponentId -> RelationKindId as well as the additional [ComponentID] -> [(RelationKindId, Option<Entity>)].
Having a high level overview of the implementation would make it a bit easier for some of us (me) to review the code 😄

@alice-i-cecile
Copy link
Member

Would you have an abstract for the overall approach to your implementation?

These sort of design details are important to communicate clearly, and Boxy and I are aiming to write up the RFC mentioned in this PR's title Soon:tm: <3

This will be hard to review until then (and is deliberately a draft PR).

sparse_set_components: Cow<'static, [ComponentId]>,
pub(crate) unique_components: SparseSet<ComponentId, Column>,
pub(crate) components: SparseSet<ComponentId, ArchetypeComponentInfo>,
table_components: Cow<'static, [(RelationKindId, Option<Entity>)]>,
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 Entity should get a niche by for example making generation a NonZeroU32. This would make Option<Entity> just as big as Entity.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually want to remove all the Option<Entity> everywhere and just have a dedicated entity for the None case as it'll make a lot of the API nicer. It's kind of annoying to have to return or take Option<Entity> in places where 99% of the time it will be Some(..)

Copy link
Member Author

@BoxyUwU BoxyUwU Apr 17, 2021

Choose a reason for hiding this comment

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

For example when setting the target filters on a Relation query param we currently just take an Entity but really we ought to be taking an Option<Entity>. Problem then is 99% of use cases have to wrap it in Some just because someone might want to query for the Nonecase...

I could take some T: Into<Option<Entity>> for that specific function but there are other places where this kind of pattern doesnt work, like the return type of the RelationAccess<T> iterator that returns the target entity- except the target Entity should actually be Option<Entity> but if I did that you'd either be unwrapping or doing a bunch of boilerplate to handle the None case :S

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 20, 2021

component_info and resource_info now contain an unnecessary bound check. It should never go out of bounds and many callers actually only need the ComponentId and not ComponentInfo.

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@BoxyUwU BoxyUwU force-pushed the relationship-prereqs branch 6 times, most recently from db347a5 to 5c6f5fe Compare July 28, 2021 22:03
@BoxyUwU BoxyUwU changed the title Partial impl of min relations RFC Entity Relations Jul 28, 2021
@BoxyUwU BoxyUwU force-pushed the relationship-prereqs branch 4 times, most recently from 8ea24cc to b7172d7 Compare July 29, 2021 23:58
Co-authored-by: Patrik Buhring <patrikbuhring@gmail.com>
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
@BoxyUwU BoxyUwU closed this Sep 6, 2021
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 S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants