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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add some docs about lowspec rendering #5091

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Add some docs about lowspec rendering #5091

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
734c134
Add some docs about lowspec rendering
inact1v1ty Jun 24, 2022
fd37ed0
Apply suggestions from code review
inact1v1ty Jun 24, 2022
79ed137
Apply suggestions from code review
inact1v1ty Jun 25, 2022
def85aa
Backtick rendering API names and fix link
inact1v1ty Jun 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/bevy_render/src/settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ pub enum WgpuSettingsPriority {
/// Provides configuration for renderer initialization. Use [`RenderDevice::features`](crate::renderer::RenderDevice::features),
/// [`RenderDevice::limits`](crate::renderer::RenderDevice::limits), and the [`WgpuAdapterInfo`](crate::render_resource::WgpuAdapterInfo)
/// resource to get runtime information about the actual adapter, backend, features, and limits.
/// NOTE: [`Backends::GL`](Backends::GL) is disabled by default.
/// You can use [`Backends::GL`](Backends::GL) on Windows only with the help of [`ANGLE`](https://github.com/gfx-rs/wgpu#angle).
Copy link
Member

@alice-i-cecile alice-i-cecile Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
/// You can use [`Backends::GL`](Backends::GL) on Windows only with the help of [`ANGLE`](https://github.com/gfx-rs/wgpu#angle).
/// On Windows, this requires [`ANGLE`](https://github.com/gfx-rs/wgpu#angle).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I committed this suggestion too, but now I think that it can be misunderstood as if all WgpuSettings require ANGLE on Windows 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it should be something like "GL is disabled by default and requires ANGL on Windows"?

Copy link
Member

Choose a reason for hiding this comment

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

I think if we add a line break above the NOTE this will be clear by grouping.

Copy link
Contributor Author

@inact1v1ty inact1v1ty Jun 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't add a line break intentionally cause on docs.rs everything after first empty line is shown only on page of the item itself, not the module containing it, and docs.rs doesn't give any hints whether there are additional docs on an item, so I thought users can miss this rather important stuff. Dk if this is true and maybe I should add a line break anyway, your thoughts @alice-i-cecile? 🤔

Also from a fresh point of view I think that changing "this" to "it" solves ambiguity.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn’t see this discussion and made my own suggestions. Let me know what you both think of them as to clarity / too much information.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with your suggestion :)

#[derive(Clone)]
pub struct WgpuSettings {
pub device_label: Option<Cow<'static, str>>,
Expand Down
10 changes: 10 additions & 0 deletions crates/bevy_render/src/view/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,16 @@ pub struct WindowSurfaces {
configured_windows: HashSet<WindowId>,
}

/// Creates or updates window surfaces for rendering.
inact1v1ty marked this conversation as resolved.
Show resolved Hide resolved
///
/// NOTE: `prepare_windows` can take a long time (and this can be seen using profiling)
/// if the GPU is a serious bottleneck and doesn't manage to keep up with CPU.
/// This can be caused by many reasons, but several of them are:
/// - Just too much load for current spec
/// - Unusual high workload (and maybe errouneusly written custom shaders causing it)
inact1v1ty marked this conversation as resolved.
Show resolved Hide resolved
/// - Bad drivers or mismatch with chosen [`Backends`](crate::settings::Backends) /
/// [`WgpuLimits`](crate::settings::WgpuLimits) / [`WgpuFeatures`](crate::settings::WgpuFeatures)
/// causing `wgpu` to pick up software renderer adapter which is usually not what you want.
inact1v1ty marked this conversation as resolved.
Show resolved Hide resolved
pub fn prepare_windows(
// By accessing a NonSend resource, we tell the scheduler to put this system on the main thread,
// which is necessary for some OS s
Expand Down