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

Update and improve Window Documentation #8858

Merged
merged 20 commits into from
Jun 22, 2023

Conversation

Selene-Amanita
Copy link
Member

@Selene-Amanita Selene-Amanita commented Jun 16, 2023

Objective

Improve the documentation relating to windows, and update the parts that have not been updated since version 0.8.

Version 0.9 introduced Window as a component, before that WindowDescriptor (which would become Window later) was used to store information about how a window will be created. Since version 0.9, from my understanding, this information will also be synchronised with the current state of the window, and can be used to modify this state.

However, some of the documentation has not been updated to reflect that, here is an example: https://docs.rs/bevy/0.8.0/bevy/window/enum.WindowMode.html / https://docs.rs/bevy/latest/bevy/window/enum.WindowMode.html (notice that the verb "Creates" is still there).

This PR aims at improving the documentation relating to windows.

Solution

  • Change "will" for "should" when relevant, "should" implies that the information should in both direction (from the window state to the Window component and vice-versa) and can be used to get and set, will implies it is only used to set a state.
  • Remove references to "creation" or be more clear about it.
  • Reference back the Window component for most of its sub-structs.
  • Clarify what needs to be clarified
  • A lot of other minor changes, including fixing the link to W3schools in bevy_winit

Warning

Please note that my knowledge about how winit and bevy_winit work is limited and some of the informations I added in the doc may be inaccurate. A person who knows better how it works should review some of my claims, in particular:

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Windowing Platform-agnostic interface layer to run your app in labels Jun 16, 2023
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Comment on lines 841 to 844
/// The window should take the full size of the screen by using the maximum supported size.
Fullscreen,
}

Copy link
Member

Choose a reason for hiding this comment

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

There's some subtle additional behavioral differences on most platforms: it may be worth distinguishing between the different fullscreen modes more clearly.

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jun 16, 2023

Choose a reason for hiding this comment

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

I 'm not sure what the difference between the different fullscreen variants are tbh, from my understanding (I just tested):

  • Borderless fullscreen takes the desktop's resolution and simply applies it to the window
  • SizedFullscreen changes the desktop's resolution to match as closely as possible the window's, then applies it to the window
  • Fullscreen changes the desktop's resolution to take the biggest one available, then applies it to the window

I would appreciate help from someone who understands it to confirm what I understood and write accurate documentation.

Copy link
Member

Choose a reason for hiding this comment

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

@inodentry this seems like the sort of thing you have a good handle on :) This article lines up with my understanding though: https://helpdeskgeek.com/help-desk/windowed-fullscreen-and-borderless-modes-which-one-is-best/

Basically, "true full screen" is an old-school approach that provides dedicated resource allocation but makes swapping programs more expensive. IIRC these days most OSes have other tools to determine priority though.

Copy link
Contributor

Choose a reason for hiding this comment

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

These days borderless window should be the recommended one. It's on par performance-wise will Fullscreen, but as @alice-i-cecile mentioned provides a much better app swap experience.

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jun 21, 2023

Choose a reason for hiding this comment

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

Tried to clarify fullscreen with fa0c745

Again I stayed "general" as some behaviours I encountered seemed more like bugs than features: #8921 .

@Selene-Amanita
Copy link
Member Author

Selene-Amanita commented Jun 16, 2023

Added documentation to link PrimaryWindow to WindowPlugin: d36b553

(every time I want to configure the primary window I look for the marker component in the doc and I have to think after that)

Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

Minor typos so Approved. Left some comments for discussion though, but are probably out of scope of this documentation work.

Thanks!

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Show resolved Hide resolved
crates/bevy_window/src/window.rs Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
scale_factor_override: Option<f64>,
/// Ratio for `physical_with` (or `physical_height`) over logical width (or height).
///
/// Set by the operating system depending on monitor pixel densities.
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 that sentence.

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jun 17, 2023

Choose a reason for hiding this comment

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

I should just rewrite it "Ratio of physical size to requested size" (respectively logical)

physical/requested/logical is explained above in the doc of WindowResolution

I didn't modify that explanation, mainly because I'm not sure I understand it, but I feel like it could be made more clear!

Copy link
Member Author

Choose a reason for hiding this comment

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

Partially resolved by a36128c (?) but WindowResolution's doc should probably be rewritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I think I understand now, nevermind.

Maybe something not involving the OS? It's kind of a detail; the user only cares that's it's automatically set, no?

Suggested change
/// Set by the operating system depending on monitor pixel densities.
/// Set automatically depending on the pixel density of the screen.

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jun 17, 2023

Choose a reason for hiding this comment

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

Committed your change but keeping the conversation open to clarify WindowResolution's doc

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

Copy link
Member Author

@Selene-Amanita Selene-Amanita Jun 21, 2023

Choose a reason for hiding this comment

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

After reading more of the code and pocking around with the scale_factor_override example, here's my attempt to clarify WindowResolution's documentation: 5a30319

I tried to stay somewhat general and focus more on the intent than the actual behaviour, because I feel like some behaviours are not necessarily intended, for example in the scale_factor_override example if the scale factor is too big to fit on the screen, the logical size will change to match the ratio of height to width* but the physical size will continue to increase despite the window not taking all of that space, I feel like this is not intended for example. See #8921 for more examples.

*(which is what I'm talking about in The requested size is not kept in memory, for example changing the scale factor one way and changing it back after might not get the original size back., I didn't stay general in that part)

Edit: (changed that last part to: The requested size is not kept in memory, for example requesting a size too big for the screen, making the logical size different from the requested size, then setting a scale factor that makes the previous requested size within the limits of the screen will not get back that previous requested size.)

Co-authored-by: Jerome Humbert <djeedai@gmail.com>
Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot for all the experimentations and efforts to make those docs correct. Couple of minor points but I wouldn't block any merge on them.

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
@@ -514,31 +595,31 @@ impl WindowResolution {
self
}

/// The window's client area width in logical pixels.
/// The window's area width in logical pixels.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think "client area" was the proper term, at least on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted that part. I deleted it in the WindowResolution's methods but not in the Window's ones anyway...

@djeedai djeedai 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 Jun 21, 2023
Selene-Amanita and others added 2 commits June 21, 2023 23:54
Co-authored-by: Jerome Humbert <djeedai@gmail.com>
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.

Very nice: these are a substantial improvement. The cross-linking is also nice!

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 22, 2023
Merged via the queue into bevyengine:main with commit f7ea93a Jun 22, 2023
@Selene-Amanita Selene-Amanita deleted the update-window-doc branch June 22, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in 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