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

Revert "merge matches_archetype and matches_table (#4807)" #5006

Closed
wants to merge 1 commit into from

Conversation

alice-i-cecile
Copy link
Member

Reverts #4807.

@cart raised some reasonable concerns about the implications of what appeared to be a simple code-deduplication PR.

  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).

I'm broadly in agreement, especially with point 1, and so I've made this PR to revert those changes.

@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 X-Controversial There is active debate or serious implications around merging this PR labels Jun 13, 2022
@alice-i-cecile
Copy link
Member Author

I'd like to add some more comments as part of this PR; this area of the code base is clearly not well documented enough. But that can wait until after we make a call on reversion or not.

@alice-i-cecile
Copy link
Member Author

This now has merge conflicts; given the lack of prioritization that this has seen + the complexity of this code I'm very reluctant to attempt to resolve those rather than attempting to fix the concerns in a separate PR.

@cart I think we should close this out.

@cart
Copy link
Member

cart commented Jul 9, 2022

Yup makes sense to me!

@cart cart closed this Jul 9, 2022
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-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants