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] - Update mouse.rs docs in bevy_input #4518

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Update mouse.rs docs in bevy_input #4518

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 18, 2022

Objective

Solution

  • Document mouse.rs inside of bevy_input.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 18, 2022
@ghost ghost added C-Docs An addition or correction to our documentation A-Input Player input via keyboard, mouse, gamepad, and more and removed S-Needs-Triage This issue needs to be labelled labels Apr 18, 2022
@ghost ghost changed the title Update mouse.rs docs Update mouse.rs docs in bevy_input Apr 18, 2022
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Finally some doc! 👍

Though there are some awkward bits that I think could be left out.

Comment on lines 27 to 28
/// resource. The resource stores the data of the buttons of a mouse and can be accessed
/// inside of a system.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this bit: "The resource stores the data of the buttons of a mouse"

Suggested change
/// resource. The resource stores the data of the buttons of a mouse and can be accessed
/// inside of a system.
/// resource. It can be accessed inside of a system with a `Res<Input<MouseButton>>`
/// parameter.

(Second half is just a suggestion. It's only the "data of a button" that I think should be really fixed)

Comment on lines 75 to 77
///
/// This event is the translated version of the `WindowEvent::MouseWheel` from the `winit` crate.
/// It is available to the end user and can be used for game logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
///
/// This event is the translated version of the `WindowEvent::MouseWheel` from the `winit` crate.
/// It is available to the end user and can be used for game logic.

Comment on lines 47 to 49
///
/// This event is the translated version of the `DeviceEvent::MouseMotion` from the `winit` crate.
/// It is available to the end user and can be used for game logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
///
/// This event is the translated version of the `DeviceEvent::MouseMotion` from the `winit` crate.
/// It is available to the end user and can be used for game logic.

Comment on lines 6 to 8
///
/// This event is the translated version of the `WindowEvent::MouseInput` from the `winit` crate.
/// It is available to the end user and can be used for game logic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
///
/// This event is the translated version of the `WindowEvent::MouseInput` from the `winit` crate.
/// It is available to the end user and can be used for game logic.

Is it relevant to the end user to say "this comes from the winit crate"? I don't feel like so.

Regardless, I think "It is available to the end user and can be used for game logic." doesn't add much value and should be removed.

KDecay and others added 2 commits April 25, 2022 19:59
Co-authored-by: Troels Jessen <kairyuka@gmail.com>
Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>
@ghost
Copy link
Author

ghost commented Apr 25, 2022

I don't understand this bit: "The resource stores the data of the buttons of a mouse"

@nicopap I can see how it could be confusing. Do you like "The resource stores the state of the mouse buttons" more?

Is it relevant to the end user to say "this comes from the winit crate"? I don't feel like so. Regardless, I think "It is available to the end user and can be used for game logic." doesn't add much value and should be removed.

I wouldn't say so. The winit information might be a little much, but I think it doesn't hurt to leave it in there. The sentence that it is available to the end user and that it can be used for game logic seems like an important notice if you are just skimming through the input docs.

The addition of the actual resource type you would have to use (Res<Input<MouseButton>>) was something I had inside of there. I removed it, because @alice-i-cecile noticed that it is too much and I definitely agree with that. It just adds a lot of boilerplate.

@nicopap
Copy link
Contributor

nicopap commented Apr 25, 2022

I think the winit line is fine. It's actually quite useful for the user. If they already know winit, they know they can re-use their knowledge in bevy thanks to that doc line. And if they don't know winit, but need more in depth understanding of bevy's internal, it's great that they have some hint of where to go. I've reconsidered my position.

I mean that "It is available to the end user" is a bit redundant on a doc string. As a user, I don't really need to know that a type is available to me in a documentation page. If I am reading the documentation for that type, it means that necessarily it is available to me, so no need to tell it. Maybe just keeping "this can be used in game logic"?

"Store the data of a button" is a bit of an awkward sentence. And all things considered, doesn't add much that is not already said, I find the doc is improved with the sentence removed.

@alice-i-cecile
Copy link
Member

Happy to merge this once @nicopap approves :)

@nicopap
Copy link
Contributor

nicopap commented Apr 26, 2022

Ah, this github ui confuses me. It's all good for me now

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Apr 26, 2022
# Objective

- Part of the splitting process of #3692.

## Solution

- Document `mouse.rs` inside of `bevy_input`.

Co-authored-by: KDecay <KDecayMusic@protonmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 26, 2022

Timed out.

@mockersf mockersf 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 Apr 26, 2022
@mockersf
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Apr 26, 2022
# Objective

- Part of the splitting process of #3692.

## Solution

- Document `mouse.rs` inside of `bevy_input`.

Co-authored-by: KDecay <KDecayMusic@protonmail.com>
@bors bors bot changed the title Update mouse.rs docs in bevy_input [Merged by Bors] - Update mouse.rs docs in bevy_input Apr 26, 2022
@bors bors bot closed this Apr 26, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
# Objective

- Part of the splitting process of bevyengine#3692.

## Solution

- Document `mouse.rs` inside of `bevy_input`.

Co-authored-by: KDecay <KDecayMusic@protonmail.com>
@ghost ghost deleted the update_mouse_docs branch August 7, 2022 08:27
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Part of the splitting process of bevyengine#3692.

## Solution

- Document `mouse.rs` inside of `bevy_input`.

Co-authored-by: KDecay <KDecayMusic@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more 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.

4 participants