bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add methods for joining queries with lists.

Open irate-devil opened this issue 2 years ago • 3 comments

Objective

This is similar in nature to #4879.

Add new new methods iter_join_map(_mut) to reduce the need to use Query::get.

These methods, compared to Query::get:

  1. Have reduced overhead.
  2. Do not require a match; Reduced rightward drift.
  3. Improved ergonomics (for the immutable variants, at least).

I also took the liberty to rename iter_many to iter_list, plus some other changes listed further below.

The name iter_join_map was chosen as the operation resembles an inner join on Entity between a Query and an arbitrary list that is mapped to Entity via the supplied fn.

Explaining is hard. Do take a look at the examples.

Examples

#[derive(Component)]
struct Health {
    value: f32
}

#[derive(Component)]
struct DamageEvent {
    target: Entity,
    damage: f32,
}

fn system(
    mut damage_events: EventReader<DamageEvent>,
    health_query: Query<&Health>,
) {
    for (health, event) in
        health_query.iter_join_map(damage_events.iter(), |event| event.target)
    {
        println!("Entity has {} health and will take {} damage!", health.value, event.damage);
    }
}

fn system(
    mut damage_events: EventReader<DamageEvent>,
    mut health_query: Query<&mut Health>,
) {
    let mut join = health_query.iter_join_map_mut(damage_events.iter(), |event| event.target);
    while let Some((mut health, event)) = join.fetch_next() {
        health.value -= event.damage;
    }
}
The same systems without this PR.
fn system(
    child_query: Query<(&Parent, &Health)>,
    parent_query: Query<&Health>,
) {
    for event in damage_events.iter() {
        if let Ok(health) = health_query.get(event.target) {
            println!("Entity has {} health and will take {} damage!", health.value, event.damage);
        }
    }
}

fn system(
    mut damage_events: EventReader<DamageEvent>,
    mut health_query: Query<&mut Health>,
) {
    for event in damage_events.iter() {
        if let Ok(mut health) = health_query.get_mut(event.target) {
            health.value -= event.damage;
        }
    }
}
More examples.
fn system(
    child_query: Query<(&Parent, &Health)>,
    parent_query: Query<&Health>,
) {
    for (parent_health, (_, health)) in
        parent_query.iter_join_map(&child_query, |(parent, _)| parent.0)
    {
        println!(
            "This entity's health is {}, and its parent's health is {}.",
            health.0, parent_health.0
        );
    }
}

fn system(parent_query: Query<(&Children, &Health)>, child_query: Query<&Health>) {
    let parent_iter = parent_query
        .iter()
        .flat_map(|(children, health)| children.iter().map(move |c| (*c, health)));
    
    for (child_health, (_, parent_health)) in
        child_query.iter_join_map(parent_iter, |(parent, _)| *parent)
    {
        println!(
            "This child entity's health is {}, and its parent's health is {}.",
            child_health.0, parent_health.0
        );
    }
}

If we had lending iterators :'( the mutable variant would simply look like this:

for (mut health, event) in
    health_query.iter_join_map_mut(damage_events.iter(), |event| event.target)
{
    health.value -= event.damage;
}

Changes

I tried seperating this into a couple commits to make it easier to review.

  • https://github.com/bevyengine/bevy/commit/d4b825edf2a4e8734321bb4bbd33c029a5d7f7ed Add iter_join_map and iter_join_map_mut to Query.
  • https://github.com/bevyengine/bevy/commit/65494091b53f22009fd90bf27a09ec9f562ab908 Remove many_for_each_mut, rename iter_many to iter_list and add iter_list_mut.
  • https://github.com/bevyengine/bevy/commit/7b08d5f93d0daa8c496765fae8f87ac8ff551ca1 Fix soundness regression on main for iter_combinations. It was capable of returning mutable data.
  • https://github.com/bevyengine/bevy/commit/5b3dfdcc032bc34d2b5ed266ec2a01115ff3128f EventWriter::send_batch now takes IntoIterator instead of just Iterator.
  • Moved tests to a more appropriate spot.

Instead of a for_each_mut variant I decide to re-use the same tricks iter_combinations uses to avoid aliased mutability. iter_list was updated to use this method as well.

Some Questions

You may have noticed the QueryJoinMapIter and QueryEntityListIter are practically identical. I tried to re-use QueryJoinMapIter with a hard-coded closure for map_f for iter_list, but in the return type it needs a concrete type for MapFn. It seems impossible to get a concrete type from a function, or at least I don't know how.

I added #[inline(always)] to the new iterator next methods, mimicking the other iterators. Is that the right call?

Follow up

  • It would be easy to add a right join as well. Outer join and left join will be trickier.
  • More tests to prevent this kind of soundness regression.
  • Add benchmarks for iter_list and iter_join_map.

irate-devil avatar Jun 22 '22 22:06 irate-devil

Does this fully solve #1658?

alice-i-cecile avatar Jun 22 '22 22:06 alice-i-cecile

Does this fully solve #1658?

Not quite, #1658 mentions a join of two queries that produces a new Query which I do think is possible, but not for me haha. I do think this PR makes iterating over 'relations' nicer, so it improves the situation a bit.

irate-devil avatar Jun 23 '22 19:06 irate-devil

Converting to draft because I'd like to base this on #5170 which will fix the unsoundness instead. I'll rework this and split out the changes to iter_many.

irate-devil avatar Jul 03 '22 18:07 irate-devil