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 documentation to QueryCombinationIter #5739

Closed
wants to merge 25 commits into from
Closed
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1f81535
Squash and rebase
Nilirad Jun 16, 2022
a04c319
State that query creation has a time-constant cost
Nilirad Jun 20, 2022
e403f9e
Table vs sparse set query iteratino performance
Nilirad Jun 20, 2022
b0304e4
Reference `Query` from `WorldQuery`
Nilirad Jun 20, 2022
7be57c9
Remove `WorldQuery` repetition in second paragraph
Nilirad Jun 20, 2022
8195f83
Add note about `for_each` vs `iter` performance
Nilirad Jun 21, 2022
c7b2343
Remove implementation detail from performance section
Nilirad Jul 1, 2022
2a37c0e
Reword `for_each` performance
Nilirad Jul 1, 2022
91c0b9d
Remove `for_each` performance remarks from method descriptions
Nilirad Jul 1, 2022
8ffe098
Add cross-links between `iter` and `for_each` methods
Nilirad Jul 1, 2022
9166cd1
Apply @james7132 suggestion on `for_each` performance
Nilirad Jul 2, 2022
ea313b8
Break doc comments at clause or sentence boundaries
Nilirad Jul 2, 2022
bbf8a1e
Split long line
Nilirad Jul 31, 2022
890624c
Migrate `many_for_each_mut` → `iter_many_mut`
Nilirad Jul 31, 2022
68df415
Fix `iter_many_mut` description
Nilirad Jul 31, 2022
4aa7e20
Update `to_readonly` description
Nilirad Jul 31, 2022
d5372e9
Ref-style link revision
Nilirad Jul 31, 2022
74cf66c
Move `WorldQuery safety section up
Nilirad Jul 31, 2022
01ff486
Apply suggestions from code review
Nilirad Aug 4, 2022
fa78f7c
Apply diff-based suggestions
Nilirad Aug 8, 2022
1ad2328
Remove duplicated and outdated “Safety” section
Nilirad Aug 8, 2022
6a9c560
Add link to `State` associated type
Nilirad Aug 8, 2022
684dff6
Make clippy happy
Nilirad Aug 8, 2022
cdab909
Checkout other files from `main`
Nilirad Aug 19, 2022
69da34a
Revert old comment above `impl` block
Nilirad Aug 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions crates/bevy_ecs/src/query/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,59 @@ where
{
}

/// An iterator over `K`-sized combinations of query items without repetition.
///
/// In this context, a combination is an unordered subset of `K` elements.
/// The number of combinations depend on how `K` relates to the number of entities matching the [`Query`] (called `N`):
/// - if `K = N`, only one combination exists,
/// - if `K < N`, there are <sub>N</sub>C<sub>K</sub> combinations (see the [performance section] of `Query`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a screenshot of how this looks in the docs? Having <sub> first looks wrong to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should render the same as here: NCK

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this notation. What does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a common notation of the binomial coefficient. You can also see the rendered docs by checking out the branch and doing the cargo doc thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, if it's common enough I guess it's fine. Otherwise I think it would be good to switch to another notation or to describe it textually. I guess the problem is that we are constrained by the Markdown formatting so we can't use the "classical" notation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still better than to write a fraction with factorials in it.

/// - if `K > N`, there are no combinations.
Comment on lines +217 to +223
Copy link
Contributor

@Weibye Weibye Aug 27, 2022

Choose a reason for hiding this comment

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

This is very technically precise but might be a bit abstract or unapproachable for new users. Is there any value in changing the wording here to be more approachable, or is this not the place to do that?

I think I'm leaning towards the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk, I just moved the documentation here from the methods, with minimal changes. Anyway, I think it's not too difficult to understand. You just need to have a basic understanding of combinatorics.

///
/// # Usage
///
/// This type is returned by calling [`Query::iter_combinations`] or [`Query::iter_combinations_mut`].
///
/// It implements [`Iterator`] only if it iterates over read-only query items ([learn more]).
///
/// In the case of mutable query items, it can be iterated by calling [`fetch_next`] in a `while let` loop.
///
/// # Examples
///
/// The following example shows how to traverse the iterator when the query items are read-only.
///
/// ```
/// # use bevy_ecs::prelude::*;
/// # #[derive(Component)]
/// # struct ComponentA;
/// #
/// fn some_system(query: Query<&ComponentA>) {
/// for [a1, a2] in query.iter_combinations() {
/// // ...
/// }
/// }
/// ```
///
/// The following example shows how `fetch_next` should be called with a `while let` loop to traverse the iterator when the query items are mutable.
///
/// ```
/// # use bevy_ecs::prelude::*;
/// # #[derive(Component)]
/// # struct ComponentA;
/// #
/// fn some_system(mut query: Query<&mut ComponentA>) {
/// let mut combinations = query.iter_combinations_mut();
/// while let Some([a1, a2]) = combinations.fetch_next() {
/// // ...
/// }
/// }
/// ```
///
/// [`fetch_next`]: Self::fetch_next
/// [learn more]: Self#impl-Iterator
/// [performance section]: crate::system::Query#performance
/// [`Query`]: crate::system::Query
/// [`Query::iter_combinations`]: crate::system::Query::iter_combinations
/// [`Query::iter_combinations_mut`]: crate::system::Query::iter_combinations_mut
pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> {
tables: &'w Tables,
archetypes: &'w Archetypes,
Expand Down