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

Disjoint QueryData access #15880

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

vil-mo
Copy link
Contributor

@vil-mo vil-mo commented Oct 13, 2024

Objective

Fixes #14518

Solution

Added QueryData implementer similarly to how ParamSet is implemented

Testing

2 doc tests and 3 tests that are adapted forms of the same tests for ParamSet

Showcase

Adds DataSet that imlements QueryData and allows disjoint access to it's members, similarly to how ParamSet works.
Mainly would be useful with in generic contexts where you need QueryData, but can't guarantee it won't conflict with other QueryData. Or with complex custom QueryData implementers that conflict with each other.
This will compile even if A and B conflict:

fn iterate_generic_data<A: QueryData, B: QueryData>(mut query: Query<DataSet<(A, B)>>) {
    for mut set in query.iter_mut() {
        let a = set.d0();
        let b = set.d1();
    }
}

@IQuick143 IQuick143 added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 13, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 13, 2024
@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Oct 13, 2024
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// SAFETY: deferes to soundness of `#data: WorldQuery` impl
// SAFETY: defers to soundness of `#data: WorldQuery` impl

Comment on lines 536 to 540
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();
Copy link
Contributor

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.

Suggested change
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();

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 31, 2024
@alice-i-cecile
Copy link
Member

@chescock @hymm can I get your opinions on this?

@chescock
Copy link
Contributor

It's definitely more niche than ParamSet, but I don't see a simpler way to solve the use case in the linked issue. There could theoretically be cases with overlapping #[derive(QueryData)] queries where it could help, as well.

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 QueryData to request mutable access to a resource in init_fetch, and it would be unsound if multiple subqueries did that. I think that can be resolved by adding the resource access from each subquery to the main access before checking the next one.

Also, this uses a proc macro, which was consistent with ParamSet when written, but #16847 changed that to be a macro_rules macro. It would be nice to change this to macro_rules as well, but that doesn't need to block it.

@chescock chescock self-requested a review December 31, 2024 15:50
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 31, 2024
Copy link
Contributor

@hymm hymm left a 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() {
Copy link
Contributor

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);
}

Copy link
Contributor

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

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.

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-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disjoint QueryData access
6 participants