[Merged by Bors] - Add ExactSizeIterator implementation for QueryCombinatonIter #5148

nicopap · 2022-06-30T11:09:24Z

Following #5124 I decided to add the ExactSizeIterator impl for QueryCombinationIter.

Also:

Clean up the tests for size_hint and len for both the normal QueryIter and QueryCombinationIter.
Add tests to QueryCombinationIter when it shouldn't be ExactSizeIterator

Changelog

Added ExactSizeIterator implementation for QueryCombinatonIter

nicopap · 2022-06-30T11:26:05Z

crates/bevy_ecs/src/query/iter.rs

+        // binomial coefficient: (n ; k) = n! / k!(n-k)! = (n*n-1*...*n-k+1) / k!
+        // See https://en.wikipedia.org/wiki/Binomial_coefficient
+        // See https://blog.plover.com/math/choose.html for implementation
+        // It was chosen to reduce overflow potential.


Note: It reduces overflow potential at least compared to the previous solution which computed n! and k!(n-k)!. This still can overflow even when the result is not an overflow. An example is provided in https://blog.plover.com/math/choose-2.html

It's also probably slower than the previous solution, because it does k div and k checked_mul, while the previous one did 1 div, k mul and k checked_mul.

nicopap · 2022-06-30T11:26:50Z

crates/bevy_ecs/src/query/mod.rs

-        assert_eq!(a_query.iter_combinations::<5>(w).count(), 0);
-        assert_eq!(a_query.iter_combinations::<5>(w).size_hint().1, Some(0));
-        assert_eq!(a_query.iter_combinations::<128>(w).count(), 0);
-        assert_eq!(a_query.iter_combinations::<128>(w).size_hint().1, Some(0));


I moved those tests to query_filtered_exactsizeiterator_len, so that they can be deduplicated and tested on more stuff.

alice-i-cecile

Nice work; this LGTM.

alice-i-cecile · 2022-06-30T15:39:06Z

Check the timings for your tests; they may be making miri take excessively long.

bors retry

alice-i-cecile · 2022-06-30T16:14:23Z

Despite this behavior being incorrect per #5149, I'd prefer to leave this behavior intact for consistency. That issue should be fixed all at once.

Instead of returning the total count of elements in the `QueryIter` in `size_hint`, we return the count of remaining elements in it. This Fixes bevyengine#5149 (although, if bevyengine#5148 is merged, bevyengine#5149 will re-open) - bevyengine#5149 - bevyengine#5148

alice-i-cecile · 2022-07-05T14:58:07Z

@bevyengine/ecs-team can I get a review please? I'd like to merge this, then fix it all together in #5214.

Instead of returning the total count of elements in the `QueryIter` in `size_hint`, we return the count of remaining elements in it. This Fixes bevyengine#5149 (although, if bevyengine#5148 is merged, bevyengine#5149 will re-open) - bevyengine#5149 - bevyengine#5148

Instead of returning the total count of elements in the `QueryIter` in `size_hint`, we return the count of remaining elements in it. This Fixes bevyengine#5149. This is also true of `QueryCombinationIter`. - bevyengine#5149 - bevyengine#5148

hymm

Code looks good. My only question would be is #5214 guaranteed to land or what is the plan if we don't fix #5149?

alice-i-cecile · 2022-07-13T14:43:49Z

IMO #5214 is basically guaranteed to land; the performance overhead doesn't look nearly as bad as I expected. Merging now for consistency.

bors r+

Following #5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

bors · 2022-07-13T15:05:43Z

Build failed (retrying...):

check-compiles

Following #5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

bors · 2022-07-13T15:10:35Z

Build failed (retrying...):

check-compiles

Following #5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

bors · 2022-07-13T15:37:23Z

Build failed:

check-compiles

alice-i-cecile · 2022-07-13T15:52:34Z

bors retry

Following #5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

bors · 2022-07-13T16:01:01Z

Canceled.

alice-i-cecile · 2022-07-13T16:08:33Z

bors retry

Following #5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

bors · 2022-07-13T16:31:28Z

Pull request successfully merged into main.

Build succeeded:

Instead of returning the total count of elements in the `QueryIter` in `size_hint`, we return the count of remaining elements in it. This Fixes bevyengine#5149. This is also true of `QueryCombinationIter`. - bevyengine#5149 - bevyengine#5148

…ine#5148) Following bevyengine#5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

Instead of returning the total count of elements in the `QueryIter` in `size_hint`, we return the count of remaining elements in it. This Fixes bevyengine#5149. This is also true of `QueryCombinationIter`. - bevyengine#5149 - bevyengine#5148

…ine#5148) Following bevyengine#5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

Instead of returning the total count of elements in the `QueryIter` in `size_hint`, we return the count of remaining elements in it. This Fixes bevyengine#5149. This is also true of `QueryCombinationIter`. - bevyengine#5149 - bevyengine#5148

…er (#5214) # Objective Fix #5149 ## Solution Instead of returning the **total count** of elements in the `QueryIter` in `size_hint`, we return the **count of remaining elements**. This Fixes #5149 even when #5148 gets merged. - #5149 - #5148 --- ## Changelog - Fix partially consumed `QueryIter` and `QueryCombinationIter` having invalid `size_hint` Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

…ine#5148) Following bevyengine#5124 I decided to add the `ExactSizeIterator` impl for `QueryCombinationIter`. Also: - Clean up the tests for `size_hint` and `len` for both the normal `QueryIter` and `QueryCombinationIter`. - Add tests to `QueryCombinationIter` when it shouldn't be `ExactSizeIterator` --- ## Changelog - Added `ExactSizeIterator` implementation for `QueryCombinatonIter`

…er (bevyengine#5214) # Objective Fix bevyengine#5149 ## Solution Instead of returning the **total count** of elements in the `QueryIter` in `size_hint`, we return the **count of remaining elements**. This Fixes bevyengine#5149 even when bevyengine#5148 gets merged. - bevyengine#5149 - bevyengine#5148 --- ## Changelog - Fix partially consumed `QueryIter` and `QueryCombinationIter` having invalid `size_hint` Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

nicopap force-pushed the exact-size-query-combination branch from 4674c1c to 9e73a7e Compare June 30, 2022 11:19

nicopap commented Jun 30, 2022

View reviewed changes

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jun 30, 2022

alice-i-cecile approved these changes Jun 30, 2022

View reviewed changes

alice-i-cecile added this to the Bevy 0.8 milestone Jun 30, 2022

nicopap force-pushed the exact-size-query-combination branch from 9e73a7e to 8460fc4 Compare June 30, 2022 16:00

nicopap mentioned this pull request Jul 5, 2022

[Merged by Bors] - Fix size_hint for partially consumed QueryIter and QueryCombinationIter #5214

Closed

hymm approved these changes Jul 13, 2022

View reviewed changes

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 Jul 13, 2022

nicopap force-pushed the exact-size-query-combination branch from 8460fc4 to 2927014 Compare July 13, 2022 15:49

Add ExactSizeIterator impl for QueryCombinatonIter

865c34e

nicopap force-pushed the exact-size-query-combination branch from 2927014 to 865c34e Compare July 13, 2022 16:00

bors bot changed the title ~~Add ExactSizeIterator implementation for QueryCombinatonIter~~ [Merged by Bors] - Add ExactSizeIterator implementation for QueryCombinatonIter Jul 13, 2022

bors bot closed this Jul 13, 2022

nicopap deleted the exact-size-query-combination branch September 5, 2023 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add ExactSizeIterator implementation for QueryCombinatonIter #5148

[Merged by Bors] - Add ExactSizeIterator implementation for QueryCombinatonIter #5148

nicopap commented Jun 30, 2022

nicopap Jun 30, 2022

nicopap Jun 30, 2022

alice-i-cecile left a comment

alice-i-cecile commented Jun 30, 2022

alice-i-cecile commented Jun 30, 2022

alice-i-cecile commented Jul 5, 2022

hymm left a comment

alice-i-cecile commented Jul 13, 2022

bors bot commented Jul 13, 2022

bors bot commented Jul 13, 2022

bors bot commented Jul 13, 2022

alice-i-cecile commented Jul 13, 2022

bors bot commented Jul 13, 2022

alice-i-cecile commented Jul 13, 2022

bors bot commented Jul 13, 2022

[Merged by Bors] - Add ExactSizeIterator implementation for QueryCombinatonIter #5148

[Merged by Bors] - Add ExactSizeIterator implementation for QueryCombinatonIter #5148

Conversation

nicopap commented Jun 30, 2022

Changelog

nicopap Jun 30, 2022

Choose a reason for hiding this comment

nicopap Jun 30, 2022

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jun 30, 2022

alice-i-cecile commented Jun 30, 2022

alice-i-cecile commented Jul 5, 2022

hymm left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Jul 13, 2022

bors bot commented Jul 13, 2022

bors bot commented Jul 13, 2022

bors bot commented Jul 13, 2022

alice-i-cecile commented Jul 13, 2022

bors bot commented Jul 13, 2022

alice-i-cecile commented Jul 13, 2022

bors bot commented Jul 13, 2022