Remove window descriptor resource once used #2886

mockersf · 2021-09-29T00:22:06Z

Objective

Fixes Can't change title at runtime. #2879. Somewhat?

Solution

Remove resource WindowDescriptor once it's used
Add doc explaining what to do and linking to example

crates/bevy_window/src/lib.rs

tigregalis · 2021-09-30T11:06:08Z

The objection I have to this is that it makes WindowDescriptor more surprising than it already is, because now if I have a system that manipulates the WindowDescriptor it crashes the application saying it doesn't exist, even though I as a user know I specifically added the resource, and the error message I get isn't exactly enlightening:

thread 'Compute Task Pool (1)' panicked at 'Resource requested by window_settings::change_window_title does not exist: bevy_window::window::WindowDescriptor', D:\Dev\bevy\crates\bevy_ecs\src\system\system_param.rs:436:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'task has failed', C:\Users\Harimau\.cargo\registry\src\github.com-1ecc6299db9ec823\async-task-4.0.3\src\task.rs:368:45
error: process didn't exit successfully: `target\debug\examples\window_settings.exe` (exit code: 101)

use bevy::prelude::*;

fn main() {
    App::new()
        // I added this resource
        .insert_resource(WindowDescriptor {
            title: "I am a window!".to_string(),
            width: 500.,
            height: 300.,
            vsync: true,
            ..Default::default()
        })
        // Plugins after (crashes)
        .add_plugins(DefaultPlugins)
        .add_system(change_window_title)
        .run();
}

fn change_window_title(mut frame_count: Local<u64>, mut window: ResMut<WindowDescriptor>) {
    window.title = format!("I am a window! Frames since start: {}", *frame_count);
    *frame_count += 1;
}

And that's if I did the right thing and put the WindowDescriptor before the DefaultPlugins. If I put the plugins before the resource (which is a common enough mistake), it doesn't crash anymore, but now I have two things that fail silently - the initial window settings AND the system that's trying to change the window settings.

use bevy::prelude::*;

fn main() {
    App::new()
        // Plugins before (fails silently)
        .add_plugins(DefaultPlugins)
        // I added this resource
        .insert_resource(WindowDescriptor {
            title: "I am a window!".to_string(),
            width: 500.,
            height: 300.,
            vsync: true,
            ..Default::default()
        })
        .add_system(change_window_title)
        .run();
}

fn change_window_title(mut frame_count: Local<u64>, mut window: ResMut<WindowDescriptor>) {
    window.title = format!("I am a window! Frames since start: {}", *frame_count);
    *frame_count += 1;
}

mockersf · 2021-10-02T02:10:43Z

If resources used only for setup are something that we want to promote in the future, we could create a "special" kind of resource, SetupResource, and each plugin would remove it after using it. The app could then list all SetupResource still available after setup is finished and issue a warning. Having a separate function from insert_resource dedicated to those SetupResource would help show that those resources are not to be used during runtime.

Co-Authored-By: Daniel McNab <[email protected]>

inodentry · 2021-10-02T18:32:10Z

I agree with the points above, that this change turns something that already has somewhat bad UX, into something with even worse UX.

@mockersf I like your idea about setup resources.

tigregalis · 2021-10-04T14:48:20Z

If resources used only for setup are something that we want to promote in the future, we could create a "special" kind of resource, SetupResource, and each plugin would remove it after using it. The app could then list all SetupResource still available after setup is finished and issue a warning. Having a separate function from insert_resource dedicated to those SetupResource would help show that those resources are not to be used during runtime.

I disagree with the basis that this particular resource should not be used during runtime, but perhaps this concept is applicable elsewhere:

Are there other examples of "setup resources" in the bevy engine?
Are there examples of "setup resources" in libraries in the bevy ecosystem?

In each case, what is the reason for these resources to only be used for setup and not during runtime (other than being unimplemented)?

mockersf · 2021-10-04T18:23:16Z

I disagree with the basis that this particular resource should not be used during runtime

My issue is that I don't like having two ways to change the window, and doing it through the Windows resource is more flexible and would probably be how it would be implemented for changes by WindowDescriptor

IceSentry · 2021-10-06T12:55:40Z

crates/bevy_window/src/window.rs

+/// This resource will only be used as initialization to setup the window.
+/// To change those settings during runtime, the [`Windows`](super::windows::Windows)
+/// resource should be used. See example
+/// [window_settings](https://github.com/bevyengine/bevy/blob/latest/examples/window/window_settings.rs)
 #[derive(Debug, Clone)]
 pub struct WindowDescriptor {


Since this PR makes this initialization only, what do you think about renaming this to WindowInitializationDescriptor? It's a bit long, but it makes it more obvious that it's only used for initialization.

I actually think this is a very good idea

mockersf · 2021-11-27T01:23:12Z

closed in favour of #2988

# Objective Fixes #5884 #2879 Alternative to #2988 #5885 #2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to #2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~#2988 doesn't solve (1)~~ #2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints.

# Objective Fixes #5884 #2879 Alternative to #2988 #5885 #2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to #2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~#2988 doesn't solve (1)~~ #2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints. ## Changelog - PluginGroups can now be configured directly using the builder pattern. Individual plugin values can be overridden by using `plugin_group.set(SomePlugin {})`, which enables overriding default plugin values. - `WindowDescriptor` plugin settings have been moved to `WindowPlugin` and `AssetServerSettings` have been moved to `AssetPlugin` - `app.add_plugins_with` has been replaced by using `add_plugins` with the builder pattern. ## Migration Guide The `WindowDescriptor` settings have been moved from a resource to `WindowPlugin::window`: ```rust // Old (Bevy 0.8) app .insert_resource(WindowDescriptor { width: 400.0, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(WindowPlugin { window: WindowDescriptor { width: 400.0, ..default() }, ..default() })) ``` The `AssetServerSettings` resource has been removed in favor of direct `AssetPlugin` configuration: ```rust // Old (Bevy 0.8) app .insert_resource(AssetServerSettings { watch_for_changes: true, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(AssetPlugin { watch_for_changes: true, ..default() })) ``` `add_plugins_with` has been replaced by `add_plugins` in combination with the builder pattern: ```rust // Old (Bevy 0.8) app.add_plugins_with(DefaultPlugins, |group| group.disable::<AssetPlugin>()); // New (Bevy 0.9) app.add_plugins(DefaultPlugins.build().disable::<AssetPlugin>()); ```

# Objective Fixes bevyengine#5884 bevyengine#2879 Alternative to bevyengine#2988 bevyengine#5885 bevyengine#2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to bevyengine#2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~bevyengine#2988 doesn't solve (1)~~ bevyengine#2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints. ## Changelog - PluginGroups can now be configured directly using the builder pattern. Individual plugin values can be overridden by using `plugin_group.set(SomePlugin {})`, which enables overriding default plugin values. - `WindowDescriptor` plugin settings have been moved to `WindowPlugin` and `AssetServerSettings` have been moved to `AssetPlugin` - `app.add_plugins_with` has been replaced by using `add_plugins` with the builder pattern. ## Migration Guide The `WindowDescriptor` settings have been moved from a resource to `WindowPlugin::window`: ```rust // Old (Bevy 0.8) app .insert_resource(WindowDescriptor { width: 400.0, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(WindowPlugin { window: WindowDescriptor { width: 400.0, ..default() }, ..default() })) ``` The `AssetServerSettings` resource has been removed in favor of direct `AssetPlugin` configuration: ```rust // Old (Bevy 0.8) app .insert_resource(AssetServerSettings { watch_for_changes: true, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(AssetPlugin { watch_for_changes: true, ..default() })) ``` `add_plugins_with` has been replaced by `add_plugins` in combination with the builder pattern: ```rust // Old (Bevy 0.8) app.add_plugins_with(DefaultPlugins, |group| group.disable::<AssetPlugin>()); // New (Bevy 0.9) app.add_plugins(DefaultPlugins.build().disable::<AssetPlugin>()); ```

mockersf added 2 commits September 29, 2021 02:17

remove WindowDescriptor resource after use

efa93ef

add doc comment explaining how to update window settings

a905b40

mockersf changed the title ~~Remove window descriptor~~ Remove window descriptor resource once used Sep 29, 2021

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 29, 2021

mockersf mentioned this pull request Sep 29, 2021

Can't change title at runtime. #2879

Closed

DJMcNab approved these changes Sep 29, 2021

View reviewed changes

crates/bevy_window/src/lib.rs Outdated Show resolved Hide resolved

use value from remove if available

285739b

Co-Authored-By: Daniel McNab <[email protected]>

NiklasEi added A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Oct 2, 2021

IceSentry reviewed Oct 6, 2021

View reviewed changes

mockersf mentioned this pull request Oct 18, 2021

Plugin - setup resource #2988

Closed

mockersf closed this Nov 27, 2021

cart mentioned this pull request Oct 22, 2022

[Merged by Bors] - Plugins own their settings. Rework PluginGroup trait. #6336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Remove window descriptor resource once used #2886

Remove window descriptor resource once used #2886

Uh oh!

mockersf commented Sep 29, 2021

Uh oh!

Uh oh!

tigregalis commented Sep 30, 2021 •

edited

Loading

Uh oh!

mockersf commented Oct 2, 2021

Uh oh!

inodentry commented Oct 2, 2021

Uh oh!

tigregalis commented Oct 4, 2021 •

edited

Loading

Uh oh!

mockersf commented Oct 4, 2021 •

edited

Loading

Uh oh!

IceSentry Oct 6, 2021

Uh oh!

wilk10 Oct 14, 2021

Uh oh!

mockersf commented Nov 27, 2021

Uh oh!

Uh oh!

Uh oh!

Remove window descriptor resource once used #2886

Remove window descriptor resource once used #2886

Uh oh!

Conversation

mockersf commented Sep 29, 2021

Objective

Solution

Uh oh!

Uh oh!

tigregalis commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockersf commented Oct 2, 2021

Uh oh!

inodentry commented Oct 2, 2021

Uh oh!

tigregalis commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockersf commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IceSentry Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

wilk10 Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

mockersf commented Nov 27, 2021

Uh oh!

Uh oh!

tigregalis commented Sep 30, 2021 •

edited

Loading

tigregalis commented Oct 4, 2021 •

edited

Loading

mockersf commented Oct 4, 2021 •

edited

Loading