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

[Merged by Bors] - Add a method iter_combinations on query to iterate over combinations of query results #1763

Closed
wants to merge 21 commits into from

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Mar 26, 2021

Related to discussion on discord

With const generics, it is now possible to write generic iterator over multiple entities at once.

This enables patterns of query iterations like

for [e1, e2, e3] in query.iter_combinations() {
   // do something with relation of all three entities
}

The compiler is able to infer the correct iterator for given size of array, so either of those work

for [e1, e2] in query.iter_combinations()  { ... }
for [e1, e2, e3] in query.iter_combinations()  { ... }

This feature can be very useful for systems like collision detection.

When you ask for permutations of size K of N entities:

  • if K == N, you get one result of all entities
  • if K < N, you get all possible subsets of N with size K, without repetition
  • if K > N, the result set is empty (no permutation of size K exist)

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 26, 2021
@mockersf
Copy link
Member

I'm guessing this would need Rust 1.51 - just a note to keep track of the minimum supported rust version

Also, this seems like a nice generalisation of #1263

Comment on lines 143 to 165
'outer: for i in (0..K).rev() {
match self.cursors[i].next(&self.tables, &self.archetypes, &self.query_state) {
Some(_) => {
// walk forward up to last element, propagating cursor state forward
for j in (i + 1)..K {
self.cursors[j] = self.cursors[j - 1].clone();
match self.cursors[j].next(
&self.tables,
&self.archetypes,
&self.query_state,
) {
Some(_) => {}
None if i > 0 => continue 'outer,
None => return None,
}
}
break;
}
None if i > 0 => continue,
None => return None,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a fan of this complex double loop, and I believe this can be written simpler. Just wasn't able to simplify it more for now. If you have any idea how to do that, I'm all ears.

@alice-i-cecile
Copy link
Member

I'd love to have a short user facing example for this. It's very useful non-obvious functionality that will be hard to discover otherwise.

@Frizi Frizi force-pushed the query-k-iter branch 3 times, most recently from 58b332a to fd71bef Compare March 26, 2021 22:31
@Frizi
Copy link
Contributor Author

Frizi commented Mar 26, 2021

Also, this seems like a nice generalisation of #1263

I don't believe it's a generalization, but those could work together.

The linked PR is about getting a singular entity from the query. The permutation iterator doesn't allow you to do so. But it does allow getting a (mutable) reference to multiple entities from a single query at once. You can do k_iter::<5>().next() to get references to all of them at once, but there is no guarantee there aren't more.

If we would generalize #1263 to support this, we would have something like this:

fn exactly_k::<const K: usize>(&self) -> Result<[<Q::Fetch as Fetch<'_>>::Item; K], QueryExactlyError>

Which would basically do k_iter::<K>().next() and check if there are no more or less entities than expected.

Is this worth adding to this PR? The implementation of that wouldn't be terribly complicated, given that k_iter is working. But it is a separate bit of functionality, and I would rather avoid another bikeshedding over the function name :) We can always add that separately.

@alice-i-cecile
Copy link
Member

I don't mind the idea of exactly_k, but a) don't see any common use case for it b) think it's definitely out of scope for this PR. We can consider it in the future if we get user requests for it.

@Frizi
Copy link
Contributor Author

Frizi commented Mar 27, 2021

added a simple example with n-body simulation
image

@Frizi Frizi force-pushed the query-k-iter branch 4 times, most recently from 40785c3 to 08f100c Compare March 27, 2021 16:36
@bjorn3
Copy link
Contributor

bjorn3 commented Mar 27, 2021

I think the name iter_permutations would be more descriptive than k_iter.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 27, 2021

The n_bodies example is cool! It definitely doesn't belong in the 3D folder though: that's for 3D rendering / assets mostly.

In its current form, it would be a good fit for the ecs folder, renamed to iter_permutations. My preference would be to flesh it out a little more (create some basic 3D meshes, a light source and add a flycam) and put it in the games folder though :) We could use a couple more complete examples, especially in 3D.

Wherever it ends up, make sure to update the examples/README.md to make sure it's discoverable.

@Frizi
Copy link
Contributor Author

Frizi commented Mar 27, 2021

I think the name iter_permutations would be more descriptive than k_iter.

Agree, will change it that way.

My preference would be to flesh it out a little more (create some basic 3D meshes, a light source and add a flycam) and put it in the games folder though :)

I don't exactly want to write the full example game just yet :) Maybe after this PR lands the example could be extended.

But I agree that in current form it fits into ECS. That was also my initial idea, but I changed my mind after seeing no other examples with 3d rendering in ecs folder. I will move it now.

@Frizi Frizi changed the title Add a method k_iter on query to iterate over permutations of query results Add a method iter_permutations on query to iterate over permutations of query results Mar 27, 2021
@TheRawMeatball
Copy link
Member

Hmm, should we use iter_combinations to have used the correct word mathematics-wise, or is permutations fine?

@alice-i-cecile
Copy link
Member

Hmm, should we use iter_combinations to have used the correct word mathematics-wise, or is permutations fine?

Very much agree with iter_combinations, since order doesn't matter. That's the correct behavior for 99% of the use cases.

Using the right name is much less confusing and lets us use iter_permutations for the order-matters behavior later if we need it.

@Frizi
Copy link
Contributor Author

Frizi commented Mar 27, 2021

Is there anything that could be done about that Android build failing? It is clearly caused by too old rust version. All other builders are already updated and passing.

@alice-i-cecile
Copy link
Member

For consistency with iter_tools, do we want this to be a method on QueryIter or the like? I think that's the more idiomatic design and would be easier to discover.

@Frizi
Copy link
Contributor Author

Frizi commented Mar 27, 2021

Hmm, should we use iter_combinations to have used the correct word mathematics-wise, or is permutations fine?

Agree again, iter_combinations is more accurate to what's happening here. Doc comments even talk about combinations.

@Frizi
Copy link
Contributor Author

Frizi commented Mar 27, 2021

For consistency with iter_tools, do we want this to be a method on QueryIter or the like? I think that's the more idiomatic design and would be easier to discover.

I'm not sure how itertools come to the picture here, but QueryIter is a separate type from QueryCombinationIter. Those share some common implementations details, but those are fundamentally separate iterator types. In theory QueryIter could be a special case of QueryCombinationIter with K = 1, but that is likely to cause unnecessary performance regression. It's easier to optimize the base case.

@Frizi Frizi changed the title Add a method iter_permutations on query to iterate over permutations of query results Add a method iter_combinations on query to iterate over combinations of query results Mar 27, 2021
@Frizi Frizi changed the title Add a method iter_combinations on query to iterate over combinations of query results Add a method iter_combinations on query to iterate over combinations of query results Mar 27, 2021
@alice-i-cecile
Copy link
Member

I'm not sure how itertools come to the picture here,

It's just the common ecosystem crate for this sort of thing.

QueryIter is a separate type from QueryCombinationIter. Those share some common implementations details, but those are fundamentally separate iterator types. In theory QueryIter could be a special case of QueryCombinationIter with K = 1, but that is likely to cause unnecessary performance regression. It's easier to optimize the base case.

Okay, good to know. Should be fine to leave as is then :)

@Frizi
Copy link
Contributor Author

Frizi commented Apr 24, 2021

Fixed the doc comments. Should be ready to go.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Alrighty this looks good to me! It definitely fills a painful functionality gap. I did my two regular benchmarks for ECS changes:

  • A clean build of Bevy looking for significant compile time regressions. Nothing measurable to report here, which is in line with my expectations for a change like this.
  • A run of my fork of ecs_bench_suite. QueryIter is notoriously subject to the whims of the compiler and sometimes even minor structural changes can cause regressions. Unfortunately this regresses a few benchmarks enough to merit some investigation:

image

This is surprising given the relatively superficial QueryIterationCursor refactor. Structurally and functionally it seems pretty much identical. I think its worth experimenting with adding/removing inlines here and there to see if we can regain whatever optimization magic the compiler is currently giving us.

crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/iter.rs Outdated Show resolved Hide resolved
}

#[inline]
pub fn iter_combinations_mut<'w, 's, const K: usize>(
Copy link
Member

Choose a reason for hiding this comment

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

I think this could use docs explaining the quirks of how to use it and the current rust limitations that force it to work that way (ex: how to write the while loop that consumes the iterator, why we can't safely implement Iterator for mutable combinations, and why for_each (which should be the favored implementation) can't work due to rustc errors).

I expect this to be a source of confusion, so we should try to nip it in the bud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is thoroughly documented on Query::iter_combinations_mut. I wonder if I should repeat those docs here as well. I specifically documented in on Query, as this is how other docs are done as well. All other QueryState methods are left undocumented except for safety, so I was just following an established pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yeah thats reasonable. QueryState is the api people doing direct World queries will use, so it should be documented, but its also unfair to hold you to a standard that I didn't hold myself to :)

I created a new issue to track this work #2090

@Frizi
Copy link
Contributor Author

Frizi commented May 3, 2021

I tried looking at the benchmarks and finding the right combinations of inlines, but it turns out for some reason the benchmarks are not very repeatable for me. After running literally the same benchmark multiple times on main branch, I'm getting regressions and improvements as high as 40%. Without any changes at all (not rebuild). Maybe my machine is particularily noisy at this time. I'm running Windows, who knows what kind of crap is being run in background. If that's the case and the benchmark is predictible for others, then I would need help with determining the cause of this regression (if it is indeed there).

For reference, here is one of the runs testing bevy main branch. It is comparing against previous run of the exact same code.
image

@cart
Copy link
Member

cart commented May 3, 2021

The "insert" benchmarks do vary wildly for me as well. I think this is because its largely a benchmark of the os memory allocator, which I would expect to swing around based on the current memory usage / cached allocations.

The iterator benchmarks (edit: don't) swing around for me in the same way. Usually they stay "within noise threshold" for me.

@alice-i-cecile
Copy link
Member

Is there a way to convince the benchmarking library to intersperse the two versions? That would help control for the random noise, especially when combined with increasing the sample size.

@cart
Copy link
Member

cart commented May 4, 2021

@alice-i-cecile : not that I know of. Criterion is a "compiled in" orchestrator, so it would need to be a post-processing step that somehow consolidates two sets of results.

Can someone that isn't me try to validate the regression here? Ex: run the benchmark suite once on main, then again on this branch (rebased on main to control for recent changes).

@cart
Copy link
Member

cart commented May 4, 2021

I can try to find time to play around with optimizations, but that won't be for at least a couple of weeks while we sort out rendering.

@cart
Copy link
Member

cart commented May 4, 2021

I think "worst case scenario" we can just use the old impl for normal iterators and just eat the code-duplication complexity from the added "cursor" impl.

@Frizi
Copy link
Contributor Author

Frizi commented May 11, 2021

I ended up focusing on a few benchmarks that I was able to reliably test against

fragmented_iter/bevy
fragmented_iter/bevy_foreach
sparse_fragmented_iter/bevy
sparse_fragmented_iter/bevy_foreach

The foreach variants don't actually excersize changed code, so I was kinda using them as a control group. I've only looked at benchmarks where those had no performance change detected. And I indeed have seen the regression at around 70% worse performance in fragmented_iter/bevy.

Unfortunately, no amount of adding or removing inline or inline(force) was able to remove that regression. I ended up just inlining it manually and it totally brought it back to previous performance. We are probably just on the verge of some very useful compiler heurestic right now, and this leaves me somewhat uneasy about that whole situation. I wonder if we can do better, to allow abstracting this code away and keep the performance. But for now, it is just manually duplicated.

@cart
Copy link
Member

cart commented May 17, 2021

I think its worth considering using a macro to cut down on code duplication here, but I don't see that as a prerequisite for merging. I added comments to remind devs to update iterators in multiple places (and a reference to this issue explaining why the code isn't unified).

std uses a macro for cases like this:
image

@cart
Copy link
Member

cart commented May 17, 2021

bors r+

@Frizi
Copy link
Contributor Author

Frizi commented May 17, 2021

I see the added comments about duplicated code, and while this is the only exact copy, those aren't really the only similar ones. There is also QueryState::for_each_unchecked_manual and QueryState::par_for_each_unchecked_manual which share a lot of the same logic.

@cart
Copy link
Member

cart commented May 17, 2021

bors r-

bors bot pushed a commit that referenced this pull request May 17, 2021
…s of query results (#1763)

Related to [discussion on discord](https://discord.com/channels/691052431525675048/742569353878437978/824731187724681289)

With const generics, it is now possible to write generic iterator over multiple entities at once.

This enables patterns of query iterations like

```rust
for [e1, e2, e3] in query.iter_combinations() {
   // do something with relation of all three entities
}
```

The compiler is able to infer the correct iterator for given size of array, so either of those work
```rust
for [e1, e2] in query.iter_combinations()  { ... }
for [e1, e2, e3] in query.iter_combinations()  { ... }
```

This feature can be very useful for systems like collision detection.

When you ask for permutations of size K of N entities:
- if K == N, you get one result of all entities
- if K < N, you get all possible subsets of N with size K, without repetition
- if K > N, the result set is empty (no permutation of size K exist)

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented May 17, 2021

Canceled.

@cart
Copy link
Member

cart commented May 17, 2021

Updated!

@cart
Copy link
Member

cart commented May 17, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 17, 2021
…s of query results (#1763)

Related to [discussion on discord](https://discord.com/channels/691052431525675048/742569353878437978/824731187724681289)

With const generics, it is now possible to write generic iterator over multiple entities at once.

This enables patterns of query iterations like

```rust
for [e1, e2, e3] in query.iter_combinations() {
   // do something with relation of all three entities
}
```

The compiler is able to infer the correct iterator for given size of array, so either of those work
```rust
for [e1, e2] in query.iter_combinations()  { ... }
for [e1, e2, e3] in query.iter_combinations()  { ... }
```

This feature can be very useful for systems like collision detection.

When you ask for permutations of size K of N entities:
- if K == N, you get one result of all entities
- if K < N, you get all possible subsets of N with size K, without repetition
- if K > N, the result set is empty (no permutation of size K exist)

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title Add a method iter_combinations on query to iterate over combinations of query results [Merged by Bors] - Add a method iter_combinations on query to iterate over combinations of query results May 18, 2021
@bors bors bot closed this May 18, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
…s of query results (bevyengine#1763)

Related to [discussion on discord](https://discord.com/channels/691052431525675048/742569353878437978/824731187724681289)

With const generics, it is now possible to write generic iterator over multiple entities at once.

This enables patterns of query iterations like

```rust
for [e1, e2, e3] in query.iter_combinations() {
   // do something with relation of all three entities
}
```

The compiler is able to infer the correct iterator for given size of array, so either of those work
```rust
for [e1, e2] in query.iter_combinations()  { ... }
for [e1, e2, e3] in query.iter_combinations()  { ... }
```

This feature can be very useful for systems like collision detection.

When you ask for permutations of size K of N entities:
- if K == N, you get one result of all entities
- if K < N, you get all possible subsets of N with size K, without repetition
- if K > N, the result set is empty (no permutation of size K exist)

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Development

Successfully merging this pull request may close these issues.

10 participants