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

Add support for 'or' in ECS querying #218

Closed
wants to merge 5 commits into from

Conversation

BimDav
Copy link
Contributor

@BimDav BimDav commented Aug 17, 2020

Add Either query for a to do a logical or on a pair of Mutated or Changed queries (using should_skip method).

#162

One problem with this solution is that Query<Either<Mutated, Mutated>, C> yields a hierarchical tuple ((a,b), c) and not (a, b, c)

@cart
Copy link
Member

cart commented Aug 18, 2020

This is definitely welcome functionality. At first glance this feels like the right approach, but I'll need a bit of time to think this one over. Thanks!

@lachlansneff
Copy link
Contributor

This could also be called Or, which is very clear whether it's Xor or Or.

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Aug 23, 2020
@cart
Copy link
Member

cart commented Aug 26, 2020

Yeah I prefer Or here too, both for its terseness and resolving Or vs XOr. Although I do think Either is a bit more grammatically correct / "reads" better.

This is good to go as soon as:

  1. merge conflicts are resolved
  2. we add one more test: check that Or<&A, &B> returns A matches and B matches.

@BimDav
Copy link
Contributor Author

BimDav commented Aug 26, 2020

@cart I renamed Either to Or and completed the test to confirm that Or<&A, &B> returns A matches, B matches, and A and B matches.
I was wondering if maybe I could add support for Or<&A, &B, &C> and for any reasonable number of arguments ? Are macros the only way to implement this ? If so, I would like to try as it seems pretty straightforward.

@cart
Copy link
Member

cart commented Aug 26, 2020

Yeah macros (or a bunch of manual implementations) are the only way to accomplish this. They would require Or to be the type and then we would impl the relevant traits for each Or<(&A, &B)>, Or<(&A, &B, &C)>, etc via macro. Given that nesting should already work (ex: Or<&A, Or<&B, &C>>), we probably don't need the macro. This also feels a bit more consistent because rust Or operators also take two operands.

For now, maybe just add a test that proves that nesting works and then we can merge this?

@BimDav
Copy link
Contributor Author

BimDav commented Aug 26, 2020

I had fun implementing the aforementioned macros in this PR #358 !

I added the test for nested queries; the destructuring of nested tuples is a bit of a mouthful, but it works as intended 👍

@cart
Copy link
Member

cart commented Aug 26, 2020

Nice! that was fast. Now i guess the only question is which one we pick. I think the tuple actually reads better, but Or<&A, &B> is simpler and has parity with rust or semantics. I'll give this a little bit of thought and let you know what i think. I'm also curious what your thoughts are!

@BimDav
Copy link
Contributor Author

BimDav commented Aug 26, 2020

To me the clarity cost of the nested approach is exponential in the tuple size, so if we have to consider large "Mutated queries", the tuple approach may be the way to go. I agree with you on the cons, though

@cart
Copy link
Member

cart commented Aug 30, 2020

Yeah I think the tuple approach is much more legible. Lets roll with that.

@cart
Copy link
Member

cart commented Aug 30, 2020

Closing in favor of #358

@cart cart closed this Aug 30, 2020
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants