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

Added documentation for ParamSet. #4854

Closed
wants to merge 9 commits into from
Closed

Conversation

Nanox19435
Copy link
Contributor

Objective

Fixes #4729.
Adds documentation for ParamSet and an example of its usage.

Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Congratulations on your first PR here! Documenting ParamSet is immensely useful.

I added my comments below about some details that can be improved. On the general side, I would advise you to follow some of the tips in #4754, especially breaking lines at sentence level.

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels May 30, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this is a marked improvement over nothing and the technical side of the explanation is spot-on.

@Nilirad's comments are excellent; try your best to address those. Other than that, there are a couple of formatting nits.

  1. Use spaces after comments.
  2. Try to split your doc comments at the sentence level so they're cleaner in version control.

Nanox19435 and others added 6 commits June 4, 2022 00:16
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
… illustrate which problem System Param solves.
@Nanox19435
Copy link
Contributor Author

Thanks for the feedback everyone, and apologies for the delay in implementing it. I hope this is good enough!

@alice-i-cecile
Copy link
Member

Not a problem! Could you press "Resolve comment" on the feedback you've addressed so this is easier to review?

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Looking much better! A couple small changes; this is ready to go once those suggestions are accepted :)

Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

I would like to address some possible high level improvements that can be made:

  • The panicking example should not be in the “Panics” section, since it's not the ParamSet that is panicking. It can be moved under the “Example” section, before the good example. Also, a little bit of text before and after the bad example to connect the two does not hurt.
  • As you can see by rendering the docs, some comments in the examples are too long, and you need to scroll horizontally to read it. This is unconvenient. You can fix that for the majority of screens by manually wrapping the comments before the 100th column (still make sure to render the docs to check for it).
  • I would move the introductory text that you include as comments at the beginning of the code block. Putting it just before the code block increases readability and focuses the reader's attention to the code, since it's no longer cluttered by long comments.

Tip: I don't know if you can do it, but you can render the docs to check how they looks with the command:

cargo doc -p bevy_ecs --no-deps --open

This will open the docs in your browser. Then you can search for ParamSet to see the docs you made 😄

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
Nanox19435 and others added 2 commits June 7, 2022 18:57
Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@Nanox19435
Copy link
Contributor Author

I think It should be done for now. Thanks for all the tips and for reviewing!

/// # struct Ally;
/// #
/// // This is an example of a system that panics because of the existence of the mutually exclusive system params.
/// // An entity may have both the Enemy and Ally components, causing two mutable references to the same Health component to be active at once.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// // An entity may have both the Enemy and Ally components, causing two mutable references to the same Health component to be active at once.
/// // An entity may have both the Enemy and Ally components, causing two mutable references to the same Health component to be active at once.
/// // ParamSet lets you avoid this by only handing out mutable references to one conflicting query at a time.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This looks good now! Personally I would structure the example as:

# Example

[EXPLANATION OF MUTUALLY INCOMPATIBLE DATA]

[FAILING SYSTEM BLOCK]

[EXPLANATION OF HOW PARAMSET ONLY HANDS OUT ONE PIECE OF DATA AT A TIME]

[FIXED CODE USING A PARAMSET]

But this is really just a matter of taste, so I'll leave this call to you.

@Nilirad
Copy link
Contributor

Nilirad commented Jun 10, 2022

I personally think that the failing example should stay in the “Example” section, since the “Panic” section is misleading because ParamSet does not panic. I particularly like the way @alice-i-cecile laid out the example, to guide the progression from a wrong usage of system parameters to a correct one using ParamSet.

@CGMossa
Copy link
Contributor

CGMossa commented Jun 30, 2022

Maybe this is a bit of a stretch, but I would like an example added here that uses a system-param that isn't Query, and perhaps an example with ParamSet<(EventReader<Spawn>, EventWriter<Spawn>)> where Spawn is some event..
Reason for this is that this sort of encapsulates the issue in one thing..

@JoJoJet
Copy link
Member

JoJoJet commented Dec 15, 2022

@Nanox19435 hey, do you plan on finishing this PR? If not, we can put it up for adoption and let someone else take care of it :)

@JoJoJet
Copy link
Member

JoJoJet commented Dec 21, 2022

Closing in favor of #6998. You'll still get credit in the new PR, of course.

@JoJoJet JoJoJet closed this Dec 21, 2022
bors bot pushed a commit that referenced this pull request Dec 25, 2022
# Objective

Fixes #4729.
Continuation of #4854.

## Solution

Add documentation to `ParamSet` and its methods. Includes examples suggested by community members in the original PR.


Co-authored-by: Nanox19435 <50684926+Nanox19435@users.noreply.github.com>
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

Fixes bevyengine#4729.
Continuation of bevyengine#4854.

## Solution

Add documentation to `ParamSet` and its methods. Includes examples suggested by community members in the original PR.


Co-authored-by: Nanox19435 <50684926+Nanox19435@users.noreply.github.com>
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Fixes bevyengine#4729.
Continuation of bevyengine#4854.

## Solution

Add documentation to `ParamSet` and its methods. Includes examples suggested by community members in the original PR.


Co-authored-by: Nanox19435 <50684926+Nanox19435@users.noreply.github.com>
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add docs and doc examples to ParamSet
6 participants