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] - Improve bevy_ecs and bevy_app API docs where referenced by the new Bevy Book #2365

Closed
wants to merge 151 commits into from

Conversation

Nilirad
Copy link
Contributor

@Nilirad Nilirad commented Jun 20, 2021

Objective

The upcoming Bevy Book makes many references to the API documentation of bevy.

Most references belong to the first two chapters of the Bevy Book:

This PR attempts to improve the documentation of bevy_ecs and bevy_app in order to help readers of the Book who want to delve deeper into technical details.

Solution

  • Add crate and level module documentation
  • Document the most important items (basically those included in the preludes), with the following style, where applicable:
    • Summary. Short description of the item.
    • Second paragraph. Detailed description of the item, without going too much in the implementation.
    • Code example(s).
    • Safety or panic notes.

Collaboration

Any kind of collaboration is welcome, especially corrections, wording, new ideas and guidelines on where the focus should be put in.


Related issues

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 20, 2021
@TheRawMeatball TheRawMeatball added C-Docs An addition or correction to our documentation C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jun 20, 2021
@alice-i-cecile alice-i-cecile self-assigned this Jun 20, 2021
@alice-i-cecile
Copy link
Member

The appbuilder methods are where a lot of the interesting content is. I kind of want those documented, but then migrate them over to the new API.

@Nilirad
Copy link
Contributor Author

Nilirad commented Jun 20, 2021

Ok. I may be able to do that in a couple of days.

@Nilirad
Copy link
Contributor Author

Nilirad commented Jun 22, 2021

Status update

I added stub documentation for all the AppBuilder methods.

Design decisions to be taken

  1. All public items of the app_builder module have now documentation. Should we add a #![warn(missing_docs)] at the top of the file to ensure it remains fully covered?
  2. Most methods are just a wrapper around World and Schedule methods. How do we manage to avoid too much doc duplication between bevy_app and bevy_ecs? Since bevy_app depends on bevy_ecs, i think the meat should be on the latter, and bevy_app adds a link to bevy_ecs for more details. I did something like that for the AppBuilder::stage method.

Further work

  • Add an example for each method.
  • Add a second paragraph where applicable to describe the item more deeply.

crates/bevy_app/src/app_builder.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
@Nilirad
Copy link
Contributor Author

Nilirad commented Jun 25, 2021

@alice-i-cecile I added in the last commit a link stub for the book. I think this may make a good addition for people who want to have a better insight into Bevy's design philosophy, if you find it appropriate.

This would likely go into chapter 2. I thought about expanding the scope of this PR to cover chapters 1 and 2, since they extensively touch topics that span over bevy_ecs.

@Nilirad
Copy link
Contributor Author

Nilirad commented Aug 18, 2021

@alice-i-cecile, @mockersf Merge conflicts resolved. I also deleted references in documentation to the removed WithBundle query filter.

@Ratysz I also documented part of the schedule API. Probably you can spot potential inaccuracies here better than many others.

Switching from draft to regular PR.

@Nilirad Nilirad marked this pull request as ready for review August 18, 2021 09:56
@alice-i-cecile 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 Sep 12, 2021
@Nilirad
Copy link
Contributor Author

Nilirad commented Sep 14, 2021

I fixed all rustdoc warnings when generating with command cargo doc -p bevy_ecs --no-deps and cargo doc -p bevy_app --no-deps

@alice-i-cecile
Copy link
Member

@cart, when you get a chance could you do a quick skim and then merge this? I think this is critical to get in for 0.6 and the sooner we merge it the less of a merge-conflict risk it poses (and the better the developer experience on main).

@cart
Copy link
Member

cart commented Sep 16, 2021

Sure thing! I'll throw it on the review stack :)

@alice-i-cecile alice-i-cecile added this to the Bevy 0.6 milestone Sep 16, 2021
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Lots of solid improvements here. Great work!

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/query/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
///
/// # Resources
///
/// The world can store information not associated to entity data, called *resources*. They
Copy link
Member

Choose a reason for hiding this comment

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

I prefer a slight rewording of this: "Worlds can also store Resources, which are unique instances of a given type that don't belong to a specific Entity. There are also "non send resources", which can only be accessed on the main thread."

I wouldn't call Local a resource. It isn't stored the same way / World doesn't store them. Therefore we don't need to distinguish between "global" and "local" resources. Resources are "global" by definition.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. They tend to behave like resources from the end-user's perspective in other ways, but I see your point. The main problems from a pedagogical perspective are a) "local" is an adjective, not a noun so it begs the "local what" question b) where should they be discussed from a teaching perspective.

Would you just call them "local unstructured data storages" or something? Do we start describing the ECS as having 3 places it can store data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alice-i-cecile maybe we can call it "system local persistent data". It's persistent in the sense that it lasts across system calls but I hope it doesn't get confused with the more general "persistent storage" concept, opposed to "volatile storage".

Copy link
Member

Choose a reason for hiding this comment

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

Right 🤔 I think I like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or "system local state". This time it can be confused with states though 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "system local persistent data" is fine if it's followed by the explanation that it's only persistent between frames

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think its enough to say that Local is "a persistent value stored locally in the system".

@Nilirad
Copy link
Contributor Author

Nilirad commented Sep 17, 2021

@cart Thanks for the review. I think I've applied all the suggestions.

@cart
Copy link
Member

cart commented Sep 17, 2021

Looks good to me!

@cart
Copy link
Member

cart commented Sep 17, 2021

bors r+

bors bot pushed a commit that referenced this pull request Sep 17, 2021
…vy Book (#2365)

## Objective

The upcoming Bevy Book makes many references to the API documentation of bevy.

Most references belong to the first two chapters of the Bevy Book:

- bevyengine/bevy-website#176
- bevyengine/bevy-website#182

This PR attempts to improve the documentation of `bevy_ecs` and `bevy_app` in order to help readers of the Book who want to delve deeper into technical details.

## Solution

- Add crate and level module documentation
- Document the most important items (basically those included in the preludes), with the following style, where applicable:
    - **Summary.** Short description of the item.
    - **Second paragraph.** Detailed description of the item, without going too much in the implementation.
    - **Code example(s).**
    - **Safety or panic notes.**

## Collaboration

Any kind of collaboration is welcome, especially corrections, wording, new ideas and guidelines on where the focus should be put in.

---

### Related issues

- Fixes #2246
@bors bors bot changed the title Improve bevy_ecs and bevy_app API docs where referenced by the new Bevy Book [Merged by Bors] - Improve bevy_ecs and bevy_app API docs where referenced by the new Bevy Book Sep 17, 2021
@bors bors bot closed this Sep 17, 2021
@Nilirad Nilirad deleted the api-docs-ch1 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
C-Docs An addition or correction to our documentation C-Feature A new feature, making something new possible 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.

add_startup_system_to_stage is broken
8 participants