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

Low power default option [#366] #397

Merged
merged 1 commit into from
Oct 20, 2020
Merged

Low power default option [#366] #397

merged 1 commit into from
Oct 20, 2020

Conversation

lee-orr
Copy link
Contributor

@lee-orr lee-orr commented Aug 30, 2020

Adding a low_power feature to bevy, that changes from using the High Performance graphics card to the default one. This is meant to provide a workable solution for #366 until such a time as the underlying issue is resolved, without requiring developers to clone the bevy repository and make changes to it.

I was unsure whether doing something like this would match the standards/expectations for bevy, but I figured it's worth creating a PR just in case.

The actual problem & solution used here were found by https://github.com/fopsdev & @MGlolenstine

@@ -19,9 +19,15 @@ pub struct WgpuRenderer {
impl WgpuRenderer {
pub async fn new() -> Self {
let instance = wgpu::Instance::new(wgpu::BackendBit::PRIMARY);

#[cfg(feature = "low_power")]
let power_preference = wgpu::PowerPreference::Default;
Copy link
Member

Choose a reason for hiding this comment

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

Naming a feature low_power and then mapping it to PowerPreference::Default instead of PowerPreference::LowPower may be confusing to users (it certainly would be to me).

Suggestion:

  • If you intend to force things into low power mode, then map the feature to PowerPreference::LowPower
  • If you want to switch to the more powerful GPU when main power is connected (which is what PowerPreference::Default does), then rename the feature so it more accurately describes what it does. Maybe adaptive_power or something 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.

Changed the option to "adaptive_power", added the "low_power" feature as PowerPreference::LowPower

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to switch to the more powerful GPU when main power is connected (which is what PowerPreference::Default does)

I don't think this is true any longer. The power module was removed. I believe PowerPreference::Default now always prefers an integrated GPU over discrete when both are available. This behavior is odd as nearly every other software behaves in the opposite way.

Copy link
Member

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

This seems functional and understandable to me. 👍

It might make more sense to put this into some builder or configuration for wgpu plugin, rather than in a feature, but I'll leave that up to the actual decision makers. 😉

The failing CI build can be fixed by running cargo +nightly fmt --all and then committing and pushing the formatting fix. Or, if you don't want to install nightly, you can manually make the changes that CI points out here.

@karroffel karroffel added A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on A-Build-System Related to build systems or continuous integration labels Aug 31, 2020
@zicklag
Copy link
Member

zicklag commented Aug 31, 2020

I would definitely lean in the direction of a builder or config option for this one. Adding a feature makes it unconfigurable at runtime and increases the number of features you have to test in CI, etc. @cart also expressed the desire to limit the features that we add ( comment ):

I think in general I would prefer to keep the features as minimal as possible.

@lee-orr
Copy link
Contributor Author

lee-orr commented Aug 31, 2020

I'll fix the formatting/ci - thanks @CleanCut for the guidance on how to do that.
@CleanCut & @zicklag - As for a builder/config option - I'm very new to rust (these are literally the first lines of code I wrote for rust), so I'm not quite sure how you would go about adding that. Is there a guide or convention for how to do that anywhere?

@MGlolenstine
Copy link
Contributor

MGlolenstine commented Aug 31, 2020

@lee-orr
I'm unsure how familiar you are with Builders, but in Rust you usually do a "constructor" (a new method) and then just add more functions, that alter default options, so that you don't have to specify all options during the initialisation. I'm adding a simple builder below.

struct Example{
    pub runnable: bool,
    pub path: String,
    pub index: u32,
}

impl Example{
    pub fn new() -> Example{
        // You set your defaults here
        Example{
            runnable: false,
            path: String::new(),
            index: 0u32
        }
    }
    
    // Basic setters
    pub fn set_runnable(self, runnable: bool) -> Self{
        self.runnable = runnable
        self
    }

    pub fn set_path(self, path: &str) -> Self{
        self.path = String::from(path);
        self
    }

    pub fn set_index(self, index: u32) -> Self{
        self.index = index;
        self
    }
}

fn main(){
    let example = Example::new()
        .set_runnable(true)
        .set_path("./hello_world")
        .set_index(1);
}

EDIT: There's also a lot of documentation on rust, I suggest you read Rust Book and the Rust by example for more clarity.

And welcome to Rust community!

@zicklag
Copy link
Member

zicklag commented Aug 31, 2020

@lee-orr Congratulations on your first Rust, then! 🎉

So there aren't any guides exactly on how to do it, but I know how we could do it so I can try to walk you through it if you want. 🙂 It's going to be a little involved, but not too bad so hold on. 😉

So the goal is for us to allow you to specify the power preference in a way very similar to how you can specify the max thread count in this example:

use bevy::prelude::*;
use bevy_app::DefaultTaskPoolOptions;

/// This example illustrates how to customize the thread pool used internally (e.g. to only use a
/// certain number of threads).
fn main() {
    App::build()
        .add_resource(DefaultTaskPoolOptions::with_num_threads(4))
        .add_default_plugins()
        .run();
}

So what we do is we do here is we include a Rust struct, DefaultTaskPoolOptions, at the top of the page with this line:

use bevy_app::DefaultTaskPoolOptions;

This struct contains the the options that are used to configure the task pool. In our case we want a struct that can be used to configure the WGPU power settings so we'll want to create our own struct in the bevy_wgpu crate ( probably in that crate's lib.rs would be a reasonable spot ) that is something like this:

/// The options that WGPU plugin accepts
#[derive(Clone)]
struct WgpuOptions {
      power_pref: WgpuPowerPreference
}

/// Enum represents possible power preferences
#[derive(Clone)]
enum WgpuPowerPreference {
      Default,
      HighPerformance,
      LowPower,
}

That covers the data we need for our options, now we also want to create a convenient way to create the options and we will want to use the builder pattern, like @MGlolenstine mention. So we'll add a new function for our struct along with a power_preference function:

impl WgpuOptions {
   // Create default WGPU options
    pub fn new() -> Example {
        // Create default options
        WgpuOptions {
            power_pref: false,
        }
    }
    
    /// Set the power preference
    pub fn power_preference(self, preference: WgpuPowerPreference) -> WgpuOptions {
        self.power_pref = preference
        self
    }
}

Now we can pass these in when we create our app like this:

App::build()
    .add_resource(
        WgpuOptions::new()
            .power_preference(WgpuPowerPreference::LowPower)
    )     
    .add_default_plugins()
    .run();

OK, so now we have our options and the ability to pass them in when we create the app, we just need to take those into account when setting up WGPU.

In the WGPU plugin we have the following code:

bevy_wgpu/src/lib.rs

pub fn wgpu_render_system(resources: &mut Resources) -> impl FnMut(&mut World, &mut Resources) {
    let mut wgpu_renderer = pollster::block_on(WgpuRenderer::new());
    // other stuff we don't care about...
}

Remember when we created out app the we used add_resource to add our WGPU options. This means that our resource will get added to the resources arg we get here and we can get our resource like so:

let wgpu_options = resources.get::<WgpuOptions>().cloned().unwrap_or_else(|| WgpuOptions::new());

What the resources.get functions does, is get the resource with the type that we can specify ( in this case WgpuOptions ) out of the resource storage. We also use cloned to get a clone of the options because the get function returns a reference of sorts and we want to have our own copy of the data. Finally, with unwrap_or_else we say that, if there were no WgpuOptions specified, just create some new ones.

Now that we have obtained our options, we want to pass them into the WgpuRenderer::new() function:

pub fn wgpu_render_system(resources: &mut Resources) -> impl FnMut(&mut World, &mut Resources) {
    let wgpu_options = resources.get::<WgpuOptions>().cloned();
    let mut wgpu_renderer = pollster::block_on(WgpuRenderer::new(wgpu_options));
    // other stuff we don't care about...
}

Now we need to go and edit the WgpuRenderer::new() method so that it can take our options and do stuff with it.

Over in the wgpu_renderer.rs file, we have the new() function like this currently:

    pub async fn new() -> Self {
        // Do other stuff we don't care about..
        let adapter = instance
            .request_adapter(&wgpu::RequestAdapterOptions {
                power_preference: wgpu::PowerPreference::HighPerformance,
                compatible_surface: None,
            })
        // Do other stuff we don't care about..
    }

We need to add an argument that takes WgpuOptions and then uses the the power_pref inside to set the WGPU power preference:

    pub async fn new(wgpu_options: WgpuOptions) -> Self {
        // Do other stuff we don't care about..
        let adapter = instance
            .request_adapter(&wgpu::RequestAdapterOptions {
                power_preference: match wgpu_options.power_pref {
                    WgpuPowerPreference::Default => wgpu::PowerPreference::Default,
                    WgpuPowerPreference::HighPerformance => wgpu::PowerPreference::HighPerformance,
                    WgpuPowerPreference::LowPower => wgpu::PowerPreference::LowPower,
                },
                compatible_surface: None,
            })
        // Do other stuff we don't care about..
    }

Wow! That turned out a little longer than I expected and maybe I overestimated the value of putting essentially the whole PR here in guide format, but if you are adventurous enough to follow, it hopefully will lead you to something that works. I may not have gotten everything perfect, and it touches quite a few aspects of the Rust language which might be confusing if you have never written it before.

If you need help on a point or something doesn't work, just ask. 😃

@lee-orr
Copy link
Contributor Author

lee-orr commented Aug 31, 2020

@zicklag & @MGlolenstine - Thanks for the detailed answers! I'll try to update the branch today.

@lee-orr
Copy link
Contributor Author

lee-orr commented Aug 31, 2020

created an options object & builder for the resource, as well as an example of how to make it work.

Copy link
Member

@zicklag zicklag 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. Good job!

Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Ah, there are a couple of clippy warnings that should be fixed. Then CI should be happy. The changes to be made are pretty much shown in the error messages:

https://github.com/bevyengine/bevy/runs/1053656852#step:6:246

Copy link
Member

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

Wow, this exceeded my expectations! Thanks, @zicklag, @lee-orr and @MGlolenstine!

I have a couple suggestions. The comment suggestions are just opinions, so you can take them or leave them. The code changes are necessary to get past clippy in CI.

examples/app/set_power_preference.rs Outdated Show resolved Hide resolved
crates/bevy_wgpu/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_wgpu/src/lib.rs Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
@lee-orr
Copy link
Contributor Author

lee-orr commented Sep 1, 2020

@CleanCut - I was just about to start working through those at @zicklag's suggestion when I saw your suggestion, so I just brought those in. Thanks!

@zicklag
Copy link
Member

zicklag commented Sep 1, 2020

Love it! Good ones @CleanCut.

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.

This is definitely a useful addition. Thanks!

@zicklag @CleanCut @MGlolenstine : I really appreciate the focus on getting @lee-orr ramped up here.

I just have one comment (on the usage of builder pattern)

LowPower,
}

impl WgpuOptions {
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 we should just cut this impl in favor of a direct Default impl. In general in Bevy I don't want to use the builder pattern over the Default pattern unless we have a specific reason to make an exception (ex: we need statefulness or special get/set logic).

@cart
Copy link
Member

cart commented Oct 20, 2020

I pushed some changes removing the builder pattern in favor of a default impl. I also removed the example as I think this feature is pretty niche / the need for it will decrease over time.

@cart cart merged commit 0dba0fe into bevyengine:master Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants