Skip to content

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

Closed
maniwani wants to merge 1 commit into
bevyengine:mainfrom
maniwani:compare-filtered-access-sets
Closed

[Merged by Bors] - Add comparison methods to FilteredAccessSet #4211
maniwani wants to merge 1 commit into
bevyengine:mainfrom
maniwani:compare-filtered-access-sets

Conversation

@maniwani

@maniwani maniwani commented Mar 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 reviewed Mar 14, 2022
View reviewed changes
Comment thread crates/bevy_ecs/src/query/access.rs
alice-i-cecile
alice-i-cecile reviewed Mar 14, 2022
View reviewed changes
Comment thread crates/bevy_ecs/src/query/access.rs
alice-i-cecile
alice-i-cecile requested changes Mar 14, 2022
View reviewed changes

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

Copy link
Copy Markdown
Member

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

maniwani commented Mar 14, 2022

Copy link
Copy Markdown
Contributor Author

bors try

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

bors Bot commented Mar 14, 2022

Copy link
Copy Markdown

try

Build succeeded:

@alice-i-cecile

alice-i-cecile commented Mar 14, 2022

Copy link
Copy Markdown
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

mockersf commented Mar 14, 2022

Copy link
Copy Markdown
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

alice-i-cecile commented Mar 14, 2022

Copy link
Copy Markdown
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

alice-i-cecile commented Apr 25, 2022

Copy link
Copy Markdown
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

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

Copy link
Copy Markdown
Member

@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

maniwani commented May 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

alice-i-cecile commented May 9, 2022

Copy link
Copy Markdown
Member

bors r+

@alice-i-cecile

alice-i-cecile commented May 9, 2022

Copy link
Copy Markdown
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

bors Bot commented May 9, 2022

Copy link
Copy Markdown

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

@james7132 james7132 james7132 approved these changes
@alice-i-cecile alice-i-cecile alice-i-cecile 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