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

Query::get and Query::get_component return different references #6623

Closed
greytdepression opened this issue Nov 14, 2022 · 4 comments
Closed
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior

Comments

@greytdepression
Copy link
Contributor

Bevy version

0.9
308e092

What you did

I ran the following snippet of code

use bevy::prelude::*;

fn main() {
    App::new()

        // We need a game loop for the problem to occur.
        .add_plugins(DefaultPlugins)

        // spawn some entities
        .insert_resource(MyEntities::default())
        .add_startup_system(spawn_entities)
        .add_system(query_system)
        .run();
}

#[derive(Default, Resource)]
struct MyEntities(Vec<Entity>);

#[derive(Debug, Component, PartialEq, Eq)]
struct IndexComponent(i32); // we use this component to check that we have the correct component

#[derive(Component)]
#[component(storage = "SparseSet")]
struct MySparseComponent;


fn spawn_entities(mut commands: Commands, mut my_entities: ResMut<MyEntities>) {
    (0..10)
        .for_each(|i|
            // spawn an entity, give it an `IndexComponent` with its number, and push it into `MyEntities`
            my_entities.0.push(commands.spawn(IndexComponent(i)).id())
        );
}


fn query_system(
    mut commands: Commands,
    my_entities: Res<MyEntities>,
    query: Query<&IndexComponent>,
    mut frame_counter: Local<usize>,
) {

    let mut last: Option<Entity> = None;

    for &entity in my_entities.0.iter() {
        let get = query.get(entity).unwrap();
        let get_comp = query.get_component::<IndexComponent>(entity).unwrap();

        // give the entity the sparse component
        commands.entity(entity)
            .insert(MySparseComponent);

        // remove the sparse component from the last entity
        if let Some(last) = last {
            commands.entity(last)
                .remove::<MySparseComponent>();
        }

        // this is where things go wrong
        if get != get_comp {
            dbg!(entity, get, get_comp);
            dbg!(*frame_counter);
            panic!("AAAA");
        }

        last = Some(entity);
    }

    // count what frame we are on
    *frame_counter += 1;
}

What went wrong

Here, get and get_comp should have the same value, which they do on the first frame, but not on the second.

Additional information

In this example MyEntities and IndexcComponent are just some scaffolding in order to demonstrate the problem. What is important is MySparseComponent and the fact that it is of storage type SparseSet.

I made a git repo containing this snippet for convenience.

@greytdepression greytdepression added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 14, 2022
@greytdepression
Copy link
Contributor Author

I should add that this bug was introduced with bevy 0.9. I noticed it, because I have a similar system in my game which ran fine on bevy 0.8.1, but broke when I updated to 0.9.

@james7132 james7132 added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Nov 14, 2022
@greytdepression
Copy link
Contributor Author

One more thing, I just saw #6622 about duplicate components, and yes, my code would insert a duplicate component onto the last entity on the second frame. But the code panics before that and it didn't make a difference with or without, so I decided to go with the shorter code snippet and not remove explicitly remove MySparseComponent from the last entity.

@rparrett
Copy link
Contributor

bisected to fe7ebd4

@james7132
Copy link
Member

james7132 commented Nov 14, 2022

Was able to reproduce the bug.

Made the following changes to the example code:

        // this is where things go wrong
        dbg!(entity, get as *const IndexComponent, get_comp as *const IndexComponent);
        dbg!(*frame_counter);
        if get != get_comp {
            panic!("AAAA");
        }

... and got the following output:

[src/main.rs:60] entity = 0v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae500
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae500
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 1v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae504
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae504
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 2v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae508
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae508
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 3v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae50c
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae50c
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 4v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae510
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae510
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 5v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae514
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae514
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 6v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae518
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae518
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 7v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae51c
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae51c
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 8v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae520
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae520
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 9v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae524
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae524
[src/main.rs:61] *frame_counter = 0
[src/main.rs:60] entity = 0v0
[src/main.rs:60] get as *const IndexComponent = 0x00000285555ae508
[src/main.rs:60] get_comp as *const IndexComponent = 0x00000285555ae500
[src/main.rs:61] *frame_counter = 1

The returned pointer from Query::get seems to be incorrect.

@james7132 james7132 added P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior labels Nov 14, 2022
@bors bors bot closed this as completed in 688f13c Nov 15, 2022
cart pushed a commit that referenced this issue Nov 30, 2022
…6625)

# Objective
Fix #6623.

## Solution
Use the right table row instead of the `EntityLocation` archetype index.
bors bot pushed a commit that referenced this issue Dec 5, 2022
# Objective
Prevent future unsoundness that was seen in #6623.

## Solution
Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level.

---

## Changelog
Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
bors bot pushed a commit that referenced this issue Dec 6, 2022
# Objective
Prevent future unsoundness that was seen in #6623.

## Solution
Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level.

---

## Changelog
Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
ickshonpe pushed a commit to ickshonpe/bevy that referenced this issue Dec 6, 2022
# Objective
Prevent future unsoundness that was seen in bevyengine#6623.

## Solution
Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level.

---

## Changelog
Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
# Objective
Prevent future unsoundness that was seen in bevyengine#6623.

## Solution
Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level.

---

## Changelog
Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…evyengine#6625)

# Objective
Fix bevyengine#6623.

## Solution
Use the right table row instead of the `EntityLocation` archetype index.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective
Prevent future unsoundness that was seen in bevyengine#6623.

## Solution
Newtype both indexes in `Archetype` and `Table` as `ArchetypeRow` and `TableRow`. This avoids weird numerical manipulation on the indices, and can be stored and treated opaquely. Also enforces the source and destination of where these indices at a type level.

---

## Changelog
Changed: `Archetype` indices and `Table` rows have been newtyped as `ArchetypeRow` and `TableRow`.
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-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
3 participants