-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Disjoint QueryData
access
#15880
base: main
Are you sure you want to change the base?
Disjoint QueryData
access
#15880
Conversation
crates/bevy_ecs/macros/src/lib.rs
Outdated
// SAFETY: each item in set is read only | ||
unsafe impl<'__w, #(#data: ReadOnlyQueryData,)*> ReadOnlyQueryData for DataSet<'__w, (#(#data,)*)> {} | ||
|
||
// SAFETY: deferes to soundness of `#data: WorldQuery` impl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// SAFETY: deferes to soundness of `#data: WorldQuery` impl | |
// SAFETY: defers to soundness of `#data: WorldQuery` impl |
crates/bevy_ecs/macros/src/lib.rs
Outdated
let mut current_access; | ||
#( | ||
// Updating empty [`FilteredAccess`] and then extending passed access. | ||
// This is done to avoid conflicts with other members of the set. | ||
current_access = FilteredAccess::default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current_access
is separate for each #data
, right? That might be more clear if you use a separate variable for each one instead of sharing the same variable.
let mut current_access; | |
#( | |
// Updating empty [`FilteredAccess`] and then extending passed access. | |
// This is done to avoid conflicts with other members of the set. | |
current_access = FilteredAccess::default(); | |
#( | |
// Updating empty [`FilteredAccess`] and then extending passed access. | |
// This is done to avoid conflicts with other members of the set. | |
let mut current_access = FilteredAccess::default(); |
It's definitely more niche than I believe it was sound when it was written, since it ensures only one subquery is alive at a time and declares all the access from each. It will need an update in light of #16843, though. It's now valid for a Also, this uses a proc macro, which was consistent with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had real variadics, I would say just merge this. As it is though this adds a fair amount of codegen for a niche feature, so I'm a little hesitant. This is probably only really useful in generic contexts. Overall I do lean towards merging this, since the alternatives are a lot more awkward to write.
We could limit this to 4 items like ParamSet was originally until a user asks for more.
|
||
#[test] | ||
#[should_panic] | ||
fn conflicting_query_with_data_set_system() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a should_panic test with the Query<DataSet>
as the first system parameter too. The conflict checking can be order dependent.
let mut world = World::default(); | ||
run_system(&mut world, sys); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a should_panic test with the conflict in the second item of the DataSet?
@@ -1716,6 +1717,154 @@ unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { | |||
type ReadOnly = Ref<'__w, T>; | |||
} | |||
|
|||
/// A collection of potentially conflicting [`QueryData`]s allowed by disjoint access. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs should make it clear that the query will fetch entitieis that match the union of all the QueryData's in the DataSet.
Objective
Fixes #14518
Solution
Added
QueryData
implementer similarly to howParamSet
is implementedTesting
2 doc tests and 3 tests that are adapted forms of the same tests for
ParamSet
Showcase
Adds
DataSet
that imlementsQueryData
and allows disjoint access to it's members, similarly to howParamSet
works.Mainly would be useful with in generic contexts where you need
QueryData
, but can't guarantee it won't conflict with otherQueryData
. Or with complex customQueryData
implementers that conflict with each other.This will compile even if A and B conflict: