bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Improve query API docs

Open Nilirad opened this issue 2 years ago • 4 comments

Objective

  • Query docs need a refresh and they look quite inconsistent.
  • WorldQuery docs had some info about the derive macro that should not belong there.
  • Closes #4742.

Solution

Documentation has been changed for the following items:

  • Query (item level and methods)
  • QueryCombinationIter
  • WorldQuery trait
  • WorldQuery macro

Query

  • Introduction has been redone.
  • “Renamed” type parameters to reflect their purpose.
  • Examples for declaration idioms have been simplified and reordered.
  • “Query result” terminology has been changed to “query item”, which is more in line with the underlying types and with general terminology.
  • Query methods: structure has been made consistent, old notes about query being able to access certain methods based on mutability have been updated and centralized to the item level.
  • A tables has been added to navigate the safe methods.
  • A performance section has been added, notably with a table with big O computational complexity.

QueryCombinationIter

  • Moved stuff about QueryCombinationIter from Query methods to the item itself.

WorldQuery trait

  • Introduction has been tidied up
  • Macro usage moved to macro item, with a link in the original place.

WorldQuery macro

  • Removed foobar terminology in favor of more recognizable identifiers (ComponentA, my_system). Examples with concrete identifiers are used where appropriate.
  • Removed “Ignored fields” section on WorldQuery macro.
  • Examples split by use case.

Additional notes

  • ~~I had to add bevy_ecs as a dev-dependency of bevy_ecs_macros to make doctests compile. Discussion on this is appreciated. I'm putting S-Controversial for now until a collective decision can be made.~~ Solved.
  • Query and WorldQuery macro idioms should be in the book, but in the meantime they can stay here.
  • ~~I don't know what the computational complexity for many_iter and many_for_each_mut is, so I added a placeholder for now.~~ Solved.
  • Consider removing the #[world_query(ignore)] feature, as it is no longer needed.
  • Please review by checking the rendered docs (cargo doc -p bevy_ecs --no-deps --open).
  • Suggestions on improving examples by making them simpler or more concrete (where applicable) are appreciated.

Todo list:

  • [x] Resolve the dev-dependency dilemma
  • [x] Write the computational complexity for many_iter and many_for_each_mut

  • Fixes #5506

Nilirad avatar Jun 11 '22 16:06 Nilirad

many_iter and many_for_each_mut essentially amount to a bunch of Query::get's with slightly reduced overhead. Since Query::get is O(1) the complexity should be O(n) where n is the number of entities provided.

irate-devil avatar Jun 11 '22 20:06 irate-devil

I moved back the macro docs to the WorldQuery trait and restored changes to bevy_ecs_macros from the main branch.

I also removed the S-Controversial label.

Nilirad avatar Jun 12 '22 10:06 Nilirad

Squashed because rebasing in multiple steps is a headache.

I preserved co-authors in the commit message.

Nilirad avatar Jun 16 '22 08:06 Nilirad

Rebased. I also pushed new commits. The first of the new commits is 5b064a2.

Nilirad avatar Jul 31 '22 15:07 Nilirad

Curious about the motivation behind

Moved stuff about QueryCombinationIter from Query methods to the item itself.

The general pattern in rust docs seems to be for the Iteratorish type to point to docs in the methods that create them. QueryIter and QueryManyIter seem to follow that pattern as well.

rparrett avatar Aug 04 '22 20:08 rparrett

Curious about the motivation behind

Moved stuff about QueryCombinationIter from Query methods to the item itself.

The general pattern in rust docs seems to be for the Iteratorish type to point to docs in the methods that create them. QueryIter and QueryManyIter seem to follow that pattern as well.

I did that because having put that on methods caused duplication of information, showing that the description is mislocated. This poses docs at a serious risk of remaining outdated in case of changes. This is probably not a piece of information that is going to change, but it's always better to deduplicate documentation and to describe at the nearest point to the implementation.

If you think it could reduce it's visibility, it's much better to link to the iterator instead of reverting the change.

Nilirad avatar Aug 08 '22 17:08 Nilirad

I had to rebase after #5205 got merged, but I don't have enough expertise about WorldQuery to fully understand what happened here. So the docs of the WorldQuery needs some expert eyes, especially because there is no more FetchState and now docs fail to build without warnings. I tried my best but I'm not able to go further alone.

More info on the commit notes.

Nilirad avatar Aug 08 '22 17:08 Nilirad

Ping @BoxyUwU for docs review following your WorldQuery changes.

alice-i-cecile avatar Aug 08 '22 17:08 alice-i-cecile

Generally what happend in that PR is that all of the traits were merged into WorldQuery. So anything talking about Fetch::foo or FetchState::foo now should talk about WorldQuery::foo

BoxyUwU avatar Aug 08 '22 18:08 BoxyUwU

Thanks!!

Nilirad avatar Aug 08 '22 19:08 Nilirad

I split this into multiple PRs, since it's difficult to find people to review it. Hopefully they are more digestible.

  • #5739
  • #5740
  • #5741
  • #5742

Nilirad avatar Aug 19 '22 12:08 Nilirad