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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - ParamSet for conflicting SystemParam:s #2765

Closed
wants to merge 42 commits into from
Closed

[Merged by Bors] - ParamSet for conflicting SystemParam:s #2765

wants to merge 42 commits into from

Conversation

bilsen
Copy link
Contributor

@bilsen bilsen commented Sep 2, 2021

Objective

Add a system parameter ParamSet to be used as container for conflicting parameters.

Solution

Added two methods to the SystemParamState trait, which gives the access used by the parameter. Did the implementation. Added some convenience methods to FilteredAccessSet. Changed get_conflicts to return every conflicting component instead of breaking on the first conflicting FilteredAccess.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 2, 2021
@bilsen
Copy link
Contributor Author

bilsen commented Sep 2, 2021

will have to fix this bug first...

@bilsen bilsen closed this Sep 2, 2021
@bilsen bilsen reopened this Sep 2, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Sep 2, 2021
@alice-i-cecile alice-i-cecile self-requested a review September 2, 2021 14:12
@alice-i-cecile
Copy link
Member

This looks great! Thanks a ton for getting this working.

I'm pretty firmly of the opinion that we should merge this ASAP for 0.6 (ping @cart), and fully remove QuerySet in this PR as well. The API is clearer, more general, and strictly superior to QuerySet, and has the excellent bonus of avoiding the nasty new QueryState requirements introduced in #2605.

@alice-i-cecile
Copy link
Member

One last thought: if and when this is merged, we should make an issue to include an example for event reading and writing in the same system using this. I wouldn't bother adding it to this PR; it's an excellent good-first-issue and those are nice to put up for the broader community.

@bilsen
Copy link
Contributor Author

bilsen commented Sep 3, 2021

I removed QuerySet in the latest commit, yay or nay?

@TheRawMeatball TheRawMeatball added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 7, 2021
@CGMossa
Copy link
Contributor

CGMossa commented Oct 2, 2021

I'd like to say that I'm looking forward to this pr being merged more than anything.

If I may, I'd like to ping @cart just this once, as I truly need this. And fixing query set seems very important.

@TheRawMeatball TheRawMeatball mentioned this pull request Oct 6, 2021
Closed
@MinerSebas
Copy link
Contributor

I think this might not be sound when used with multiple Queries.
Before #2605 QuerySet was just:

QuerySet<
	Query<&Transform>,
	Query<&mut Transform>,
>

But the changes in #2605 made it necessary to now write it as:

QuerySet<
	QueryState<&Transform>,
	QueryState<&mtu Transform>,
>

The changes of this PR would then reopen that soundness hole:

ParamSet<
	Query<&Transform>,
	Query<&mut Transform>,
>

@alice-i-cecile
Copy link
Member

@MinerSebas It looks like the tests added in #2605 have been converted and pass successfully (i.e. fail to compile).

Was there some other concern that wasn't covered by the tests?

@MinerSebas
Copy link
Contributor

@MinerSebas It looks like the tests added in #2605 have been converted and pass successfully (i.e. fail to compile).

Was there some other concern that wasn't covered by the tests?

I was just going by PR descriptions and didn't check for Tests.
I don't think there is anything else.

@cart cart added this to the Bevy 0.6 milestone Oct 12, 2021
@HackerFoo
Copy link
Contributor

I've just merged this PR into my fork so I could try it out.

I'm in the process of deriving SystemParams to organize the parameters of my systems better, and abstract common operations over queries and resources. I was worried about getting stuck trying to keep them disjoint, so I asked on the Discord server, and @DJMcNab mentioned this PR.

I haven't looked over the PR yet, but this works on my codebase so far, so that's something. Maybe this is obvious, but it also allows SystemParams to reference overlapping resources (Res/ResMut), so it's safe to use arbitrary SystemParams together as long as their use is disjoint.

So I'm also depending on this feature, thanks.

@alice-i-cecile
Copy link
Member

@bilsen, I'd like to prioritize getting this PR merged. Do you have time to merge in Cart's changes, or would you like me to help in some way?

@bilsen
Copy link
Contributor Author

bilsen commented Mar 18, 2022

@alice-i-cecile I will be able to merge it in tomorrow, exams are done now.

@bilsen
Copy link
Contributor Author

bilsen commented Mar 19, 2022

I did a merge, It was difficult to get the better error reporting without knowledge of the access of specific parameters, so I left it as it were in Cart's branch

@bilsen
Copy link
Contributor Author

bilsen commented Mar 19, 2022

ah it fails because there is no ParamSet in docs.rs. Should I add that link when a new release is made?

@alice-i-cecile
Copy link
Member

Should I add that link when a new release is made

Yes please. For now, we could link to the new dev-docs site though; check #dev-announcements for the link.

@bilsen
Copy link
Contributor Author

bilsen commented Mar 19, 2022

But it still won't work until this is merged into main right?

@alice-i-cecile
Copy link
Member

Ugh right. Okay, don't worry about it for now then.

@alice-i-cecile
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Mar 25, 2022

try

Merge conflict.

@alice-i-cecile
Copy link
Member

@bilsen can you rebase this? I want to see how it behaves under MIRI, following #4310.

@bilsen
Copy link
Contributor Author

bilsen commented Mar 25, 2022

Seems to work

@cart
Copy link
Member

cart commented Mar 29, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 29, 2022
@bilsen
ParamSet for conflicting SystemParam:s (#2765)
63fee25 
# Objective
Add a system parameter `ParamSet` to be used as container for conflicting parameters.

## Solution
Added two methods to the SystemParamState trait, which gives the access used by the parameter. Did the implementation. Added some convenience methods to FilteredAccessSet. Changed `get_conflicts` to return every conflicting component instead of breaking on the first conflicting `FilteredAccess`.


Co-authored-by: bilsen <[email protected]>
@bors
Copy link
Contributor

bors bot commented Mar 29, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title ParamSet for conflicting SystemParam:s [Merged by Bors] - ParamSet for conflicting SystemParam:s Mar 29, 2022
@bors bors bot closed this Mar 29, 2022
@alice-i-cecile alice-i-cecile mentioned this pull request Apr 7, 2022
Closed
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@bilsen @aevyrie
ParamSet for conflicting SystemParam:s (bevyengine#2765)
dc51519 
# Objective
Add a system parameter `ParamSet` to be used as container for conflicting parameters.

## Solution
Added two methods to the SystemParamState trait, which gives the access used by the parameter. Did the implementation. Added some convenience methods to FilteredAccessSet. Changed `get_conflicts` to return every conflicting component instead of breaking on the first conflicting `FilteredAccess`.


Co-authored-by: bilsen <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@bilsen @ItsDoot
ParamSet for conflicting SystemParam:s (bevyengine#2765)
45d8326 
# Objective
Add a system parameter `ParamSet` to be used as container for conflicting parameters.

## Solution
Added two methods to the SystemParamState trait, which gives the access used by the parameter. Did the implementation. Added some convenience methods to FilteredAccessSet. Changed `get_conflicts` to return every conflicting component instead of breaking on the first conflicting `FilteredAccess`.


Co-authored-by: bilsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MinerSebas MinerSebas MinerSebas left review comments

@mockersf mockersf mockersf left review comments

@TheRawMeatball TheRawMeatball TheRawMeatball approved these changes

@cart cart cart left review comments

@lafi-99 lafi-99 lafi-99 approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@HackerFoo HackerFoo HackerFoo approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
@cart's attention
Archived in project
Milestone
Bevy 0.7
Development

Successfully merging this pull request may close these issues.
10 participants
@bilsen @alice-i-cecile @CGMossa @MinerSebas @HackerFoo @jamescarterbell @cart @TheRawMeatball @mockersf @lafi-99