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

Conversation

Nilirad
Copy link
Contributor

@Nilirad Nilirad commented Aug 19, 2022

Objective

  • Document QueryCombinationIter

Solution

Additional notes

Nilirad and others added 25 commits August 19, 2022 12:33
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: James Liu <contact@jamessliu.com>
This will reflect the added context in this PR
that is aware of what query items are
and that they can be read-only or not.
Moved all links to the bottom of the doc comment.
Links have been alphabetically ordered.

Rebase notes:

- After bevyengine#5205 got merged,
  the “Safety” section of `WorldQuery` has changed.
  I listed the mentioned methods for a better layout.
  I also removed the `WorldQuery::` prefix for readability.
- I don't have enough knowledge of the previous and current state of `WorldQuery` types,
  so the documentation may be outdated somewhere.
  `WorldQuery` needs another review round.
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Co-authored-by: Boxy <supbscripter@gmail.com>
@Nilirad Nilirad mentioned this pull request Aug 19, 2022
2 tasks
@Nilirad Nilirad added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Aug 19, 2022
/// 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.

Copy link
Contributor

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +217 to +223
/// 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`),
/// - if `K > N`, there are no combinations.
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.

@Weibye Weibye 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 Aug 27, 2022
@cart
Copy link
Member

cart commented Aug 30, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 30, 2022
# Objective

- Document `QueryCombinationIter`

## Solution

- Describe the item, add usage and examples
- Copy notes about the number of query items generated from the corresponding query methods (they will be removed in #5742 ([motivation]))

## Additional notes

- Derived from #4989 

[motivation]: #4989 (comment)
@bors bors bot changed the title Add documentation to QueryCombinationIter [Merged by Bors] - Add documentation to QueryCombinationIter Aug 30, 2022
@bors bors bot closed this Aug 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Document `QueryCombinationIter`

## Solution

- Describe the item, add usage and examples
- Copy notes about the number of query items generated from the corresponding query methods (they will be removed in bevyengine#5742 ([motivation]))

## Additional notes

- Derived from bevyengine#4989 

[motivation]: bevyengine#4989 (comment)
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Document `QueryCombinationIter`

## Solution

- Describe the item, add usage and examples
- Copy notes about the number of query items generated from the corresponding query methods (they will be removed in bevyengine#5742 ([motivation]))

## Additional notes

- Derived from bevyengine#4989 

[motivation]: bevyengine#4989 (comment)
@Nilirad Nilirad deleted the QueryCombinationIter branch December 5, 2023 18:05
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-Docs An addition or correction to our documentation 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.

6 participants