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] - merge matches_archetype and matches_table #4807

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - merge matches_archetype and matches_table #4807

wants to merge 3 commits into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented May 20, 2022

Objective

the code in these fns are always identical so stop having two functions

Solution

make them the same function

Changelog

change matches_archetype and matches_table to fn matches_component_set(&self, &SparseArray<ComponentId, usize>) -> bool then do extremely boring updating of all FetchState impls

Migration Guide

  • move logic of matches_archetype and matches_table into matches_component_set in any manual FetchState impls

@BoxyUwU BoxyUwU added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels May 20, 2022
@alice-i-cecile alice-i-cecile 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 20, 2022
@alice-i-cecile
Copy link
Member

PR description could provide a bit more context on what exactly these types are, but I'm happy with the changes themselves.

james7132
james7132 requested changes May 20, 2022
View reviewed changes
crates/bevy_ecs/src/archetype.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/sparse_set.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/table.rs Outdated Show resolved Hide resolved
@james7132 james7132 removed 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 20, 2022
@alice-i-cecile alice-i-cecile 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 20, 2022
@james7132 james7132 mentioned this pull request May 22, 2022
Closed
alice-i-cecile
alice-i-cecile approved these changes May 30, 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.

bors r+

bors bot pushed a commit that referenced this pull request May 30, 2022
@BoxyUwU
merge matches_archetype and matches_table (#4807)
e528b63 
# Objective

the code in these fns are always identical so stop having two functions

## Solution

make them the same function

---

## Changelog

change `matches_archetype` and `matches_table` to `fn matches_component_set(&self, &SparseArray<ComponentId, usize>) -> bool` then do extremely boring updating of all `FetchState` impls

## Migration Guide

- move logic of `matches_archetype` and `matches_table` into `matches_component_set` in any manual `FetchState` impls
@bors
Copy link
Contributor

bors bot commented May 30, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title merge matches_archetype and matches_table [Merged by Bors] - merge matches_archetype and matches_table May 30, 2022
@bors bors bot closed this May 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
@BoxyUwU @james7132
merge matches_archetype and matches_table (bevyengine#4807)
645e2d2 
# Objective

the code in these fns are always identical so stop having two functions

## Solution

make them the same function

---

## Changelog

change `matches_archetype` and `matches_table` to `fn matches_component_set(&self, &SparseArray<ComponentId, usize>) -> bool` then do extremely boring updating of all `FetchState` impls

## Migration Guide

- move logic of `matches_archetype` and `matches_table` into `matches_component_set` in any manual `FetchState` impls
@cart
Copy link
Member

cart commented Jun 13, 2022 •
edited
Loading

@BoxyUwU @alice-i-cecile

Came across this change while reviewing #4626: im not sold on this for a number of reasons:

  1. This forces each caller of matches_component_set to decide what it means for an archetype or table to match a component id (edit: and a Query generally). This should not be externalized, it is a core part of what it means for a query to match an archetype / table.
  2. It assumes that archetype matching will always hinge on ComponentIds. I think there is a reasonable chance that we will add things like "value based" archetype identity (ex: for things like indexing), which would split out our definitions of "matches table" vs "matches archetype".
  3. We've removed a "duplicate" method from each impl, but it comes at the cost of additional implementation complexity because it adds the abstraction of "calling an arbitrary input function". It is much harder to tell whats going on (and the purpose of the function).

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jun 13, 2022 •
edited
Loading

  1. I think this is an important consideration: I agree with your concerns here.
  2. I agree that we'll likely have some form of value based matching. I'm reluctant to call this an "archetype", because of how much that further muddies the waters around what an archetype is. By itself I don't think this is a good reason to block this change; those changes are a long way in the future.
  3. That's a fair criticism.

alice-i-cecile pushed a commit to alice-i-cecile/bevy that referenced this pull request Jun 13, 2022
@BoxyUwU @alice-i-cecile
Revert "merge matches_archetype and matches_table (bevyengine#4807)"
90e8f74 
This reverts commit e528b63.
@alice-i-cecile alice-i-cecile mentioned this pull request Jun 13, 2022
Closed
@alice-i-cecile
Copy link
Member

@BoxyUwU, you mentioned you had rebuttals to these concerns but never remembered to actually write them down :)

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@BoxyUwU @ItsDoot
merge matches_archetype and matches_table (bevyengine#4807)
d01c511 
# Objective

the code in these fns are always identical so stop having two functions

## Solution

make them the same function

---

## Changelog

change `matches_archetype` and `matches_table` to `fn matches_component_set(&self, &SparseArray<ComponentId, usize>) -> bool` then do extremely boring updating of all `FetchState` impls

## Migration Guide

- move logic of `matches_archetype` and `matches_table` into `matches_component_set` in any manual `FetchState` impls
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
@BoxyUwU @alice-i-cecile @cart @james7132