Improve Query's top-level documentation#18622
Improve Query's top-level documentation#18622alice-i-cecile merged 5 commits intobevyengine:mainfrom
Query's top-level documentation#18622Conversation
BD103
added
C-Docs
| /// |[`get`]\[[`_mut`][`get_mut`]\]|O(1)| | ||
| /// |[`get_many`]|O(k)| | ||
| /// |[`get_many_mut`]|O(k<sup>2</sup>)| | ||
| /// |Archetype-based filtering ([`With`], [`Without`], [`Or`])|O(a)| |
There was a problem hiding this comment.
I realize you didn't change this, but the O(a) here is a little bit misleading. Archetypal filters are evaluated when new archetypes are created, and during iteration they essentially have no cost.
There was a problem hiding this comment.
How would you change it to make it clearer? Would it be
There was a problem hiding this comment.
I'm not really sure how to improve it, and I'm fine leaving this alone if we can't think of anything better!
I think the main purpose of this section is to note that Added or Changed evaluate entities one-by-one at query time, while archetype filters don't. In some sense, the problem is that iter() isn't O(n) in the number of matching entities, it's O(n) in the number of entities in matched archetypes, even if those entities get filtered out by non-archetypal filters like Added or Changed.
But I don't know how to say that concisely, so I'm not sure what to suggest writing here.
There was a problem hiding this comment.
I'm going to leave this to a follow-up issue / PR, since I'm not sure either. (My knowledge of the ECS is limited to the user-facing side, I don't know much about the internals!)
Carter0
left a comment
Carter0
left a comment
There was a problem hiding this comment.
Minor thoughts and some questions, but it looks good!
JaySpruce
added
S-Ready-For-Final-Review
BD103
added
S-Waiting-on-Author
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
I didn't realize `Single` was defined in the same file! :)
BD103
added
S-Ready-For-Final-Review
# Objective - There's been several changes to `Query` for this release cycle, and `Query`'s top-level documentation has gotten slightly out-of-date. - Alternative to #18615. ## Solution - Edit `Query`'s docs for consistency, clarity, and correctness. - Make sure to group `get()` and `get_many()` together instead of `single()` and `get_many()`, to enforce the distinction from #18615 (comment). - Reformat doc tests so they would be readable if extracted into their own file. (Which mainly involves adding more spacing.) - Move link definitions to be nearer where they are used. - Fix the tables so they are up-to-date and correctly escape square brackets `\[ \]`. ## Testing I ran `cargo doc -p bevy_ecs --no-deps` to view the docs and `cargo test -p bevy_ecs --doc` to test the doc comments. ## Reviewing The diff is difficult to read, so I don't recommend _just_ looking at that. Instead, run `cargo doc -p bevy_ecs --no-deps` locally and read through the new version. It should theoretically read smoother with less super-technical jargon. :) ## Follow-up I want to go through some of `Query`'s methods, such as `single()`, `get()`, and `get_many()`, but I'll leave that for another PR. --------- Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
5 participants
Objective
Queryfor this release cycle, andQuery's top-level documentation has gotten slightly out-of-date.get_many()tomany(), making it consistent withsingle()#18615.Solution
Query's docs for consistency, clarity, and correctness.get()andget_many()together instead ofsingle()andget_many(), to enforce the distinction from Renameget_many()tomany(), making it consistent withsingle()#18615 (comment).\[ \].Testing
I ran
cargo doc -p bevy_ecs --no-depsto view the docs andcargo test -p bevy_ecs --docto test the doc comments.Reviewing
The diff is difficult to read, so I don't recommend just looking at that. Instead, run
cargo doc -p bevy_ecs --no-depslocally and read through the new version. It should theoretically read smoother with less super-technical jargon. :)Follow-up
I want to go through some of
Query's methods, such assingle(),get(), andget_many(), but I'll leave that for another PR.