[Question/PR] Should We Increase the Max Query Set Size?

[zicklag]

Consider this a question with PR just because it was a one character change. 🙂

I just ran into a situation where I needed all four slots in a QuerySet because of conflicting queries and I could easily see there being a situation where I needed more, is there any reason not to push this up to allow more queries in a QuerySet?

Here's the query set I have:

mut queries: QuerySet<(
        // Query needed to propagate world positions ( think bevy transform propagation, but for 2D )
        Query<'a, (Entity, &'static mut Position, &'static mut WorldPosition)>,
        // The player read query. This is needed to loop through the players for detecting collisions
        Query<
            (
                &Handle<Image>,
                &Sprite,
                &Handle<SpriteSheet>,
                &WorldPosition,
            ),
            With<Player>,
        >,
        // The radish read query. This is needed to loop through radishes that the player needs
        // to detect collisions with
        Query<(Entity, &Handle<Image>, &Sprite, &WorldPosition), Without<Player>>,
        // The radish mutation query. This is needed to modify the image of any radishes the player
        // has collided with.
        Query<(Entity, &mut Handle<Image>), Without<Player>>,
    )>

All of these queries will conflict with each-other due to, at least, the borrow of WorldPosition. While I could split these different steps into a system chain to keep the borrows from conflicting, it is much less verbose just to use a QuerySet, and while I didn't run out of queries this time, it just feels like it's cutting it a little close to limit to 4 queries in a set.

I'm not super sold on the need to push it up, but it seems like it might help in some situations and it's no trouble to change as far as code goes.

Here's the entire system if you need more context:

fn collision_detection(
    // We will need to read and write to the radish entities at different stages of the collision
    // detection so we create a query set to enforce that don't borrow the reading and writing
    // queries at the same time.
    mut queries: QuerySet<(
        Query<'a, (Entity, &'static mut Position, &'static mut WorldPosition)>,
        // The player read query
        Query<
            (
                &Handle<Image>,
                &Sprite,
                &Handle<SpriteSheet>,
                &WorldPosition,
            ),
            With<Player>,
        >,
        // The radish read query
        Query<(Entity, &Handle<Image>, &Sprite, &WorldPosition), Without<Player>>,
        // The radish mutation query
        Query<(Entity, &mut Handle<Image>), Without<Player>>,
    )>,
    mut scene_graph: ResMut<SceneGraph>,
    image_assets: Res<Assets<Image>>,
    sprite_sheet_assets: Res<Assets<SpriteSheet>>,
    radish_images: Res<RadishImages>,
) {
    // Make sure collision positions are synchronized
    queries.q0_mut().sync_world_positions(&mut scene_graph);

    // Create list of colliding radishes
    let mut colliding_radishes = HashSet::default();

    // Loop over the players
    for (player_image, player_sprite, player_sprite_sheet, player_pos) in queries.q1().iter() {
        // Get the collision image for the player
        let player_image = if let Some(col) = image_assets.get(player_image) {
            col
        } else {
            continue;
        };
        let player_sprite_sheet = if let Some(col) = sprite_sheet_assets.get(player_sprite_sheet) {
            col
        } else {
            continue;
        };

        for (other_radish, other_radish_image, other_radish_sprite, other_radish_pos) in
            queries.q2().iter()
        {
            // Get collision image for the other radish
            let other_radish_image = if let Some(col) = image_assets.get(other_radish_image) {
                col
            } else {
                continue;
            };

            // If they are colliding
            if pixels_collide_with(
                PixelColliderInfo {
                    image: player_image,
                    sprite: player_sprite,
                    position: player_pos,
                    sprite_sheet: Some(player_sprite_sheet),
                },
                PixelColliderInfo {
                    image: other_radish_image,
                    sprite: other_radish_sprite,
                    position: other_radish_pos,
                    sprite_sheet: None,
                },
            ) {
                // Add it to the colliding radish list
                colliding_radishes.insert(other_radish);
            }
        }
    }

    // Make all colliding radishes red and non-colliding radishes blue
    for (radish, mut image) in queries.q3_mut().iter_mut() {
        if colliding_radishes.contains(&radish) {
            *image = radish_images.collided.clone();
        } else {
            *image = radish_images.uncollided.clone();
        }
    }
}

[alice-i-cecile]

Archetype invariants (#1481) should help reduce the pain of conflicting queries in general. But I see your point, and I'm curious about the costs of increasing this limit.

[MinerSebas]

Most instances where a QuerySet is necessary can also be rewritten using a Combination of Filters and Query.Get().

(Warning this is untested and could contain Typos/Bugs.)

Rewritten System

fn collision_detection(
    // We will need to read and write to the radish entities at different stages of the collision
    // detection so we create a query set to enforce that don't borrow the reading and writing
    // queries at the same time.
    mut PositionQuery: Query<'a, (Entity, &'static mut Position, &'static mut WorldPosition)>,
    // The player read query
    mut PlayerQuery: Query<(Entity, &Handle<Image>, &Sprite, &Handle<SpriteSheet>,), (With<Player>, With<WorldPosition>)>,

    mut queries: QuerySet<(
        // The radish read query
        Query<(Entity, &Handle<Image>, &Sprite), (Without<Player>, With<WorldPosition>)>,
        // The radish mutation query
        Query<(Entity, &mut Handle<Image>), Without<Player>>,
    )>,
    mut scene_graph: ResMut<SceneGraph>,
    image_assets: Res<Assets<Image>>,
    sprite_sheet_assets: Res<Assets<SpriteSheet>>,
    radish_images: Res<RadishImages>,
) {
    // Make sure collision positions are synchronized
    PositionQuery.sync_world_positions(&mut scene_graph);

    // Create list of colliding radishes
    let mut colliding_radishes = HashSet::default();

    // Loop over the players
    for (entity, player_image, player_sprite, player_sprite_sheet) in PlayerQuery.iter() {
        // Get the collision image for the player
        let player_image = if let Some(col) = image_assets.get(player_image) {
            col
        } else {
            continue;
        };
        let player_sprite_sheet = if let Some(col) = sprite_sheet_assets.get(player_sprite_sheet) {
            col
        } else {
            continue;
        };

        for (entity_radish, other_radish, other_radish_image, other_radish_sprite) in
            queries.q0().iter()
        {
            // Get collision image for the other radish
            let other_radish_image = if let Some(col) = image_assets.get(other_radish_image) {
                col
            } else {
                continue;
            };

            // If they are colliding
            if pixels_collide_with(
                PixelColliderInfo {
                    image: player_image,
                    sprite: player_sprite,
                    position: PositionQuery.get(entity).1,
                    sprite_sheet: Some(player_sprite_sheet),
                },
                PixelColliderInfo {
                    image: other_radish_image,
                    sprite: other_radish_sprite,
                    position: PositionQuery.get(entity_radish).1,
                    sprite_sheet: None,
                },
            ) {
                // Add it to the colliding radish list
                colliding_radishes.insert(other_radish);
            }
        }
    }

    // Make all colliding radishes red and non-colliding radishes blue
    for (radish, mut image) in queries.q1_mut().iter_mut() {
        if colliding_radishes.contains(&radish) {
            *image = radish_images.collided.clone();
        } else {
            *image = radish_images.uncollided.clone();
        }
    }
}

[zicklag]

Ah, that's an interesting strategy, I like it. I'll try that out and see how it works. Thanks!

[zicklag]

Yeah, I was able to make that work and get rid of the QuerySet completely in one of my use-cases. I this case I have to do an otherwise unnecessary clone because I need two accesses to the world position and the world_positions query takes only mutable access, but the clone is probably not too bad and it's worth avoiding the query set:

fn collision_detection(
    // We need mutable access to the world positions so that we can sync transforms
    mut world_positions: Query<'a, (Entity, &'static mut Position, &'static mut WorldPosition)>,
    mut players: Query<(Entity, &Handle<Image>, &Sprite, &Handle<SpriteSheet>), With<Player>>,
    mut radishes: Query<(Entity, &mut Handle<Image>, &Sprite), Without<Player>>,
    mut scene_graph: ResMut<SceneGraph>,
    image_assets: Res<Assets<Image>>,
    sprite_sheet_assets: Res<Assets<SpriteSheet>>,
    radish_images: Res<RadishImages>,
) {
    // Make sure collision positions are synchronized
    world_positions.sync_world_positions(&mut scene_graph);

    // Loop over the players
    for (player, player_image, player_sprite, player_sprite_sheet) in players.iter_mut() {
        // Get the collision image of the player
        let player_image = if let Some(col) = image_assets.get(player_image) {
            col
        } else {
            continue;
        };
        // Get the spritesheet of the player
        let player_sprite_sheet = if let Some(col) = sprite_sheet_assets.get(player_sprite_sheet) {
            col
        } else {
            continue;
        };

        for (radish, mut radish_image, radish_sprite) in radishes.iter_mut() {
            // Get collision image for the other radish
            let other_radish_image = if let Some(col) = image_assets.get(radish_image.clone()) {
                col
            } else {
                continue;
            };

            // Check for collisions
            if pixels_collide_with(
                PixelColliderInfo {
                    image: player_image,
                    sprite: player_sprite,
                    // We need to grab the world position of the player from the
                    // `WorldPositionsQuery` query. Also, because the `WorldPositionsQuery` takes
                    // mutable borrow to the world position, we have to clone it to avoid mutably
                    // borrowing it twice when we get the position of the radish immediately below.
                    position: &world_positions
                        .get_world_position_mut(player)
                        .unwrap()
                        .clone(),
                    sprite_sheet: Some(player_sprite_sheet),
                },
                PixelColliderInfo {
                    image: other_radish_image,
                    sprite: radish_sprite,
                    position: &world_positions
                        .get_world_position_mut(radish)
                        .unwrap()
                        .clone(),

                    sprite_sheet: None,
                },
            ) {
                // Set the radish image to the collided image if we are running into him
                *radish_image = radish_images.collided.clone();
            } else {
                // Set the radish image to the uncollided image if we are not running into him
                *radish_image = radish_images.uncollided.clone();
            }
        }
    }
}

[alice-i-cecile]

@zicklag have you seen #1763? I've found myself needing similar patterns in a few cases where I ended up with a QuerySet before.

[zicklag]

Oh, that's interesting. I hadn't seen that before. That's cool. That allows mutably accessing a component of two or more entities in a query at the same time, which would allow me to get rid of the QuerySet in my other use-case, so that does help.

[alice-i-cecile]

Sanity-checking: the cost of doing this is marginally increased compile times right?

I think we should shelve this PR for now, and bump the number up if people still find a need for it after the new tools have been introduced.

[zicklag]

I'm guessing that the compile time would be almost imperceptibly increased, but I'm not sure.

Either way I think it's reasonable to close this for now as it hasn't actually be required by anybody yet.