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] - Add comparison methods to FilteredAccessSet #4211

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Add comparison methods to FilteredAccessSet #4211

wants to merge 1 commit into from

Conversation

maniwani
Copy link
Contributor

@maniwani maniwani commented Mar 14, 2022 •
edited
Loading

Objective

  • (Eventually) reduce noise in reporting access conflicts between unordered systems.
    • SystemStage only looks at unfiltered ComponentId access, any conflicts reported are potentially false.
      • the systems could still be accessing disjoint archetypes
    • Comparing systems' filtered access sets can maybe avoid that (for statically known component types).

Solution

  • Modify SparseSetIndex trait to require PartialEq, Eq, and Hash (all internal types except BundleId already did).
  • Add is_compatible and get_conflicts methods to FilteredAccessSet<T>
    • (existing method renamed to get_conflicts_single)
  • Add docs for those and all the other methods while I'm at it.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Mar 14, 2022
alice-i-cecile
alice-i-cecile requested changes Mar 14, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Minor nits, but I like the direction this is going in.

@maniwani
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Mar 14, 2022
@bors
Try #4211:
11d0775
@bors
Copy link
Contributor

bors bot commented Mar 14, 2022

try

Build succeeded:

@alice-i-cecile
Copy link
Member

Looking good. IMO we should add some tests for this as part of this PR, just to verify that everything is and continues to work as expected.

@mockersf
Copy link
Member

Would it make sense to change get_conflicts to return an Option<Vec<T>> to allow for a None in the case of no conflicts? I think I would prefer it as a user, but not if it makes it more painful to manipulate in Bevy code

@alice-i-cecile
Copy link
Member

Would it make sense to change get_conflicts to return an Option<Vec> to allow for a None in the case of no conflicts? I think I would prefer it as a user, but not if it makes it more painful to manipulate in Bevy code

In general, I'm not a fan of the Option<Vec<T>> pattern. Returning an empty vector has a clear meaning in most cases, and reduces the type complexity nicely.

@maniwani maniwani mentioned this pull request Mar 31, 2022
Closed
4 tasks
@alice-i-cecile
Copy link
Member

@maniwani can you rebase?

@bevyengine/ecs-team this is a useful and simple improvement; can I get another couple reviews on this?

@james7132 james7132 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 May 1, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 1, 2022 •
edited
Loading

@maniwani I'd like to merge this tomorrow; can you rebase?

I'm also curious about your response to @mockersf's comment above.

@maniwani maniwani force-pushed the compare-filtered-access-sets branch from 0be6896 to c411d51 Compare May 9, 2022 14:10
@maniwani maniwani force-pushed the compare-filtered-access-sets branch from c411d51 to 08c6e56 Compare May 9, 2022 14:15
@maniwani
Copy link
Contributor Author

maniwani commented May 9, 2022 •
edited
Loading

I'm also curious about your response to @mockersf's comment above.

For Access<T> and FilteredAccess<T>, get_conflicts returns a Vec<T>. I just stuck to that. I don't know if users are making use of these methods. I made this PR to help with the ambiguity checking stuff (by providing a means to potentially remove false positives).

@alice-i-cecile
Copy link
Member

bors r+

@alice-i-cecile
Copy link
Member

For Access and FilteredAccess, get_conflicts returns a Vec. I just stuck to that.

Sounds good. We can clean this up further in a future PR if needed.

bors bot pushed a commit that referenced this pull request May 9, 2022
@maniwani
Add comparison methods to FilteredAccessSet (#4211)
4c878ef 
# Objective

- (Eventually) reduce noise in reporting access conflicts between unordered systems. 
	- `SystemStage` only looks at unfiltered `ComponentId` access, any conflicts reported are potentially `false`.
		- the systems could still be accessing disjoint archetypes
	- Comparing systems' filtered access sets can maybe avoid that (for statically known component types).
		- #4204

## Solution

- Modify `SparseSetIndex` trait to require `PartialEq`, `Eq`, and `Hash` (all internal types except `BundleId` already did).
- Add `is_compatible` and `get_conflicts` methods to `FilteredAccessSet<T>`
	- (existing method renamed to `get_conflicts_single`)
- Add docs for those and all the other methods while I'm at it.
@bors
Copy link
Contributor

bors bot commented May 9, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add comparison methods to FilteredAccessSet [Merged by Bors] - Add comparison methods to FilteredAccessSet May 9, 2022
@bors bors bot closed this May 9, 2022
robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
@maniwani @robtfm
Add comparison methods to FilteredAccessSet (bevyengine#4211)
eabbb3e 
# Objective

- (Eventually) reduce noise in reporting access conflicts between unordered systems. 
	- `SystemStage` only looks at unfiltered `ComponentId` access, any conflicts reported are potentially `false`.
		- the systems could still be accessing disjoint archetypes
	- Comparing systems' filtered access sets can maybe avoid that (for statically known component types).
		- bevyengine#4204

## Solution

- Modify `SparseSetIndex` trait to require `PartialEq`, `Eq`, and `Hash` (all internal types except `BundleId` already did).
- Add `is_compatible` and `get_conflicts` methods to `FilteredAccessSet<T>`
	- (existing method renamed to `get_conflicts_single`)
- Add docs for those and all the other methods while I'm at it.
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
@maniwani @exjam
Add comparison methods to FilteredAccessSet (bevyengine#4211)
771f8cd 
# Objective

- (Eventually) reduce noise in reporting access conflicts between unordered systems. 
	- `SystemStage` only looks at unfiltered `ComponentId` access, any conflicts reported are potentially `false`.
		- the systems could still be accessing disjoint archetypes
	- Comparing systems' filtered access sets can maybe avoid that (for statically known component types).
		- bevyengine#4204

## Solution

- Modify `SparseSetIndex` trait to require `PartialEq`, `Eq`, and `Hash` (all internal types except `BundleId` already did).
- Add `is_compatible` and `get_conflicts` methods to `FilteredAccessSet<T>`
	- (existing method renamed to `get_conflicts_single`)
- Add docs for those and all the other methods while I'm at it.
@maniwani maniwani deleted the compare-filtered-access-sets branch November 16, 2022 20:43
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@maniwani @ItsDoot
Add comparison methods to FilteredAccessSet (bevyengine#4211)
a9cadc3 
# Objective

- (Eventually) reduce noise in reporting access conflicts between unordered systems. 
	- `SystemStage` only looks at unfiltered `ComponentId` access, any conflicts reported are potentially `false`.
		- the systems could still be accessing disjoint archetypes
	- Comparing systems' filtered access sets can maybe avoid that (for statically known component types).
		- bevyengine#4204

## Solution

- Modify `SparseSetIndex` trait to require `PartialEq`, `Eq`, and `Hash` (all internal types except `BundleId` already did).
- Add `is_compatible` and `get_conflicts` methods to `FilteredAccessSet<T>`
	- (existing method renamed to `get_conflicts_single`)
- Add docs for those and all the other methods while I'm at it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maniwani @alice-i-cecile @mockersf @james7132