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

Multiple Querys with mut acces and changed filters cant be used without QuerySet #1791

Closed
MinerSebas opened this issue Mar 31, 2021 · 4 comments
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@MinerSebas
Copy link
Contributor

Bevy version

d6bc414

Operating system & version

Windows 10

What you did

I wanted to remove a QuerySet that seemed unneeded, but then Bevy Panicked: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/camera/camera.rs#L70-L73

I then reduced it to this minimal example:

use bevy::prelude::*;

fn main() {
    App::build().add_system(test_system.system()).run();
}

struct TestStruct;

fn test_system(
    mut query1: Query<(Entity, &mut TestStruct)>,
    query2: Query<Entity, Added<TestStruct>>,
) {

}

What you expected to happen

The minimum example should not Panic.

What actually happened

It Panicked, while claiming the system would break Rust's mutability rules, which it cant:
thread 'main' panicked at 'Query<bevy_ecs::entity::Entity, bevy_ecs::query::filter::Added<hello_world::TestStruct>> in system &hello_world::test_system accesses component(s) hello_world::TestStruct in a way that conflicts with a previous system parameter. Allowing this would break Rust's mutability rules. Consider merging conflicting Queries into a QuerySet.', crates\bevy_ecs\src\system\system_param.rs:144:5

Additional information

Bisecting the Repo, the example Panics after #1525 was merged.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Mar 31, 2021
@MinerSebas
Copy link
Contributor Author

After remembering that before #1525 the conflict detection depended on the actually spawned entities, I expanded the Example:

use bevy::prelude::*;

fn main() {
    App::build().add_startup_system(test_startup.system()).add_system(test_system.system()).run();
}

struct TestStruct;

fn test_startup(
    commands: &mut Commands,
) {
    commands.spawn((TestStruct,));
}

fn test_system(
    mut query1: Query<(Entity, &mut TestStruct)>,
    query2: Query<Entity, Added<TestStruct>>,
) {

}

This causes a Panic even before #1525 with this message:
thread 'main' panicked at 'System &hello_world::test_system has conflicting queries. bevy_ecs::core::entities::Entity conflicts with the component access [hello_world::TestStruct] in this prior query: (bevy_ecs::core::entities::Entity, &mut hello_world::TestStruct).', crates\bevy_ecs\src\system\into_system.rs:75:13

This means that this Bug isn't actually a regression caused by #1525.

@cart
Copy link
Member

cart commented Mar 31, 2021

Supporting Added filters is technically safe here but would require special casing for it. We can't support Changed<T> filters in this context because they read state that &mut T queries set.

I'm also a little hesitant to support this case right now because we're hoping to eventually make immediate component add/removes possible from within a system, which might not be safe if we allow these queries to exist side by side (hard to tell before we have a design for that feature)

@alice-i-cecile alice-i-cecile added S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged labels Dec 12, 2021
@alice-i-cecile
Copy link
Member

bevyengine/rfcs#30 is useful prior art for anyone interested in tackling these issues. Like I laid out in that closed RFC, there are actually 4 access levels here, which we don't differentiate fully in the current design:

  • Forge: add and remove components
  • Write: modify component values
  • Read: read component values
  • Presence: check if a component exists

Added filters only need presence-level permissions, while Changed needs read-level permissions. We could implement this model independently of the more problematic elements of the linked RFC, which would fix this issue without blocking future instant-addition work.

@alice-i-cecile
Copy link
Member

This resolved! Use the SystemState API.

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 S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

3 participants