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

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

Closed
wants to merge 1 commit into from

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Mar 29, 2021

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 alice-i-cecile added A-ECS Entities, components, systems, and events help wanted C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 29, 2021
@alice-i-cecile
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

zicklag commented Mar 29, 2021

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

@zicklag
Copy link
Member Author

zicklag commented Mar 29, 2021

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
Copy link
Member

@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
Copy link
Member Author

zicklag commented Mar 29, 2021

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
Copy link
Member

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
Copy link
Member Author

zicklag commented Apr 11, 2021

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.

@zicklag zicklag closed this Apr 11, 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 C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants