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

Can't change title at runtime. #2879

Closed
Gingeh opened this issue Sep 27, 2021 · 22 comments
Closed

Can't change title at runtime. #2879

Gingeh opened this issue Sep 27, 2021 · 22 comments
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Docs An addition or correction to our documentation C-Usability A simple quality-of-life change that makes Bevy easier to use

Comments

@Gingeh
Copy link
Contributor

Gingeh commented Sep 27, 2021

Bevy version

rev = "d158e0893ea88b6acf523a4264f076d6e0b540c6"

Operating system & version

Linux Mint 20.1 Ulyssa

What you did

I tried to change the window title:

fn update_title(score: Res<Score>, mut window: ResMut<WindowDescriptor>) {
    window.title = format!("Snake: {}", score.0);
}

What you expected to happen

I expected the title to change.

What actually happened

The title stayed the same.

Additional information

I am using a WindowDescriptor to set the title at startup.
Window::set_title works, but is more verbose.

@Gingeh Gingeh added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Sep 27, 2021
@alice-i-cecile alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in C-Docs An addition or correction to our documentation C-Usability A simple quality-of-life change that makes Bevy easier to use and removed C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Sep 27, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Sep 27, 2021

The WindowDescriptor resource is never used to update the primary window, except for when the WindowPlugin (or maybe the WinitPlugin reads it for the first time)

That is, you should use Window::set_title

This is not really a sensible default, but it's the current state of affairs.

@cart
Copy link
Member

cart commented Sep 28, 2021

Yup we already have an example that covers how to do this: https://github.com/bevyengine/bevy/blob/main/examples/window/window_settings.rs#L22

I agree that people are often confused by WindowDescriptor being "initialization settings" and not "runtime settings". If anyone has suggestions on ways to improve this im all ears. Any suggestions need to retain the behavior of the "default window" being created with "reasonable defaults", without any user action. These initial settings must be configurable prior to the window being created.

@mockersf
Copy link
Member

If anyone has suggestions on ways to improve this im all ears

I have an idea... why not remove the resource once used for initialisation? #2886

@tigregalis
Copy link
Contributor

I made comments on this topic earlier in the year #1255 (comment)

My view for the specific issue at hand: WindowDescriptor (if that's the resource used) ideally should be an "active" resource, in the same way as ClearColor. A user simply updates a resource, and things just work. This could be as simple as a system added to WindowPlugin that monitors changes to Res<WindowDescriptor>, and automatically applies the appropriate window.set_* methods. WindowDescriptor only applies to a single window, sure, but so does ClearColor.

I made a prototype of this a while ago (just updated to work with latest main)

mod window {
    use bevy::{prelude::*, window::WindowMode};

    #[derive(Default)]
    pub struct PrevWindow(WindowDescriptor);

    #[inline]
    fn compare_f32(a: f32, b: f32) -> bool {
        (a - b).abs() < std::f32::EPSILON
    }

    pub fn update_window(
        mut prev_window: ResMut<PrevWindow>,
        curr: Res<WindowDescriptor>,
        mut windows: ResMut<Windows>,
    ) {
        if curr.is_changed() {
            let window = windows.get_primary_mut().unwrap();
            let prev = &prev_window.0;

            if compare_f32(prev.width, curr.width) || compare_f32(prev.height, curr.height) {
                window.set_resolution(curr.width, curr.height);
            }
            if prev.scale_factor_override != curr.scale_factor_override {
                window.set_scale_factor_override(curr.scale_factor_override);
            }
            if prev.title != curr.title {
                window.set_title(curr.title.clone());
            }
            if prev.vsync != curr.vsync {
                window.set_vsync(curr.vsync);
            }
            if prev.resizable != curr.resizable {
                window.set_resizable(curr.resizable);
            }
            if prev.decorations != curr.decorations {
                window.set_decorations(curr.decorations);
            }
            if prev.cursor_visible != curr.cursor_visible {
                window.set_cursor_visibility(curr.cursor_visible);
            }
            if prev.cursor_locked != curr.cursor_locked {
                window.set_cursor_lock_mode(curr.cursor_locked);
            }
            match (prev.mode, curr.mode) {
                (WindowMode::Windowed, WindowMode::Windowed)
                | (WindowMode::BorderlessFullscreen, WindowMode::BorderlessFullscreen) => {}
                (
                    WindowMode::Fullscreen { use_size: p },
                    WindowMode::Fullscreen { use_size: c },
                ) => {
                    if p != c {
                        window.set_mode(curr.mode);
                    }
                }
                _ => {
                    window.set_mode(curr.mode);
                }
            }
            // there is no set_canvas method
            // #[cfg(target_arch = "wasm32")]
            // if prev.canvas != curr.canvas {
            //     window.set_canvas(curr.canvas);
            // }

            prev_window.0 = curr.clone();
        }
    }
}

use bevy::{prelude::*, window::WindowMode};

fn main() {
    App::new()
        .insert_resource(WindowDescriptor {
            title: "Press Escape to toggle full screen vs windowed. Hit a key to change the title."
                .into(),
            mode: WindowMode::Windowed,
            ..Default::default()
        })
        .add_plugins(DefaultPlugins)
        .init_resource::<window::PrevWindow>()
        .add_system(demo)
        .add_system(window::update_window)
        .run();
}

fn demo(input: Res<Input<KeyCode>>, mut window_descriptor: ResMut<WindowDescriptor>) {
    if input.just_pressed(KeyCode::Escape) {
        window_descriptor.mode = match window_descriptor.mode {
            WindowMode::Windowed => WindowMode::BorderlessFullscreen,
            WindowMode::BorderlessFullscreen => WindowMode::Windowed,
            _ => unreachable!(),
        }
    }
    for key in input.get_just_pressed() {
        window_descriptor.title = format!("Press Escape to toggle full screen vs windowed. Hit a key to change the title. You just pressed {:?}!", key);
    }
}

@DJMcNab
Copy link
Member

DJMcNab commented Sep 29, 2021

One challenge I put against that is that it encourages code written with only one window in mind. That might be fine, for your specific game code, for example.

But it might not be fine for a library, as that might make it misbehave e.g. for within the editor, with multiple windows

However on the other hand, a library shouldn't be poking at your window size anyway, so it probably would be fine.

@tigregalis
Copy link
Contributor

By the same token, you aren't creating multiple windows by inserting WindowDescriptor, so we don't have a multiple-windows-first API. I think we should double-down on WindowDescriptor, because it makes it less surprising. Perhaps it could be made more explicit by renaming WindowDescriptor to PrimaryWindowDescriptor.

It turns out that a side-benefit of my approach is that you can insert the WindowDescriptor after the DefaultPlugins, and it just seems to work, which makes WindowDescriptor even less surprising.

Alternatively, you could have an API that assumes multiple windows, i.e. something like

// resource
struct WindowDescriptors(Vec<WindowDescriptor>);

with the default being a vector of one. (Or each window is an entity.) This starts to get less ergonomic in the primary (usual) case of having one window.

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 30, 2021

Alternatively, you could have an API that assumes multiple windows, i.e. something like

We already have Windows for multiple windows.

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 30, 2021

I think we should double-down on WindowDescriptor, because it makes it less surprising.

Except that removing the WindowDescriptor doesn't close the window as you would expect. Winit doesn't allow programmatically closing windows at all. Only the user can close windows.

@tigregalis
Copy link
Contributor

I think we should double-down on WindowDescriptor, because it makes it less surprising.

Except that removing the WindowDescriptor doesn't close the window as you would expect. Winit doesn't allow programmatically closing windows at all. Only the user can close windows.

Interesting, not something I had considered as I wouldn't have thought to close the window that way. In the case of the prototype, removing the resource would crash the application, because the system depends on it being available, which is the opposite problem that #2886 has.

Alternatively, you could have an API that assumes multiple windows, i.e. something like

We already have Windows for multiple windows.

True! WindowDescriptor is there as a "sensible default" way of initialising the window settings for the primary window - we can extend its utility by tracking the resource and updating the window settings for the same primary window. Any multiple window requirements should use the Windows resource.

@Plecra
Copy link
Contributor

Plecra commented Oct 4, 2021

Winit doesn't allow programmatically closing windows at all.

Is something wrong with winit's Drop for Window implementation? It looks like it tries to close windows programmatically to me.

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 4, 2021

Oh, didn't know about that. My bad.

@DJMcNab
Copy link
Member

DJMcNab commented Oct 4, 2021

Indeed, we (I've) got a pr open (#2898) which integrates that properly, now that we know about it.

@IceSentry
Copy link
Contributor

Considering WindowDescriptor is only used for initialization, why not just rename it to WindowInitializationDescriptor to make it more obvious that it is only used for that?

@tigregalis
Copy link
Contributor

Considering WindowDescriptor is only used for initialization, why not just rename it to WindowInitializationDescriptor to make it more obvious that it is only used for that?

Is there a reason we're married to the idea that WindowDescriptor should only be used for initialisation?

@IceSentry
Copy link
Contributor

Not as far as I know, my point is that this change would be a somewhat temporary fix until a better API is found, because this isn't the first time someone got confused by the current API.

@mockersf
Copy link
Member

mockersf commented Oct 4, 2021

Is there a reason we're married to the idea that WindowDescriptor should only be used for initialisation?

There is one for me! It's to avoid having two ways to do the same thing

@tigregalis
Copy link
Contributor

tigregalis commented Oct 4, 2021

Is there a reason we're married to the idea that WindowDescriptor should only be used for initialisation?

There is one for me! It's to avoid having two ways to do the same thing

There are a couple points to this.

The first is that it represents an unintuitive way of using resources*, but we have considered a special app.insert_setup_resource() method or better docs or renaming the type.

*unintuitive because:

  1. It fails silently if you insert it after DefaultPlugins
  2. It fails silently when you try to change it (as per OP)
  3. With Remove window descriptor resource once used #2886:
    a. if you had inserted it before DefaultPlugins, it crashes when you try to change it
    b. if you had inserted it after DefaultPlugins, it fails silently when you try to change it
  4. With or without Remove window descriptor resource once used #2886, it is a "dead" resource that can't be interacted with

The second is that there are already several examples of having more than one way to do the same thing.
In the best cases, there is usually an easy way in the general case, and a harder (more verbose) way that's more flexible.

  • Text::with_section() vs Text { .. }
  • Res<Input<KeyCode>> vs EventReader<KeyboardInput>
  • input.any_pressed([a, b, c]) vs input.pressed(a) || input.pressed(b)) || input.pressed(c))
  • etc.

This:

fn update_title(score: Res<Score>, mut window: ResMut<WindowDescriptor>) {
    window.title = format!("Snake: {}", score.0);
}

represents an easy way in the general case (having one window). It's easier because it's intuitive: it matches the way that you initialise the window settings, and it matches the way you initialise and make changes to other resources (and components).

WindowDescriptor tracking would likely build on top of Windows sure, but I don't see there being a problem of building one abstraction on top of another: look at Input<KeyCode>, which is a stateful abstraction over the KeyboardInput event.

Do you gain much by having that ^ over this:

fn update_title(score: Res<Score>, mut windows: ResMut<Windows>) {
    let window = windows.get_primary_mut().unwrap();
    window.set_title(format!("Snake: {}", score.0));
}

One less line perhaps, but other than that, nothing more than what's already been discussed: I think the question is whether the clarity and usability win is enough.

@mockersf
Copy link
Member

opened a pr to check how a startup resource would work: #2988

@jmonasterio
Copy link

It turns out that a side-benefit of my approach is that you can insert the WindowDescriptor after the DefaultPlugins, and it just seems to work, which makes WindowDescriptor even less surprising.

Who knew. Moving window descriptor first is important.

@helltraitor
Copy link

Hi guys, what about using a window as an entity with components like Descriptor and other?
In that case, we could just change the descriptor of the concrete window instance.
Moreover, we can pass some special event that will indicate window update, or use some system with Changed Query (passing an event is not bad at all, but I think about efficient in that context). For the performance side of this question, we could cache window property and use them directly instead of often querying.

As for me, I wanted to implement fullscreen for my application with follow code:

pub fn fullscreen(keyboard: Res<Input<KeyCode>>, mut window: ResMut<WindowDescriptor>) {
}

Also, for some reason, I am unable to get WindowDescriptor if it wasn't set manually (LOL, where is default resource for current window?)

P.S. Too many related issues: #4386, #1255, #2988, #1144 (That one is closed but related) and so on. That is not funny. If you have chosen ECS may be the best way is to use it (It's not a critic, because seems like I don't quite understand bevy engine)?

Thanks for attention

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 26, 2022

Hi guys, what about using a window as an entity with components like Descriptor and other?

I believe that is exactly what #5589 does.

bors bot pushed a commit that referenced this issue Oct 24, 2022
# 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.
bors bot pushed a commit that referenced this issue Oct 24, 2022
# 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>());
```
bors bot pushed a commit that referenced this issue Oct 24, 2022
# 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>());
```
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
# 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>());
```
@rparrett
Copy link
Contributor

rparrett commented Nov 1, 2022

It looks like bors didn't close this for some reason. Fixed by #6336.

@rparrett rparrett closed this as completed Nov 1, 2022
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this issue Dec 17, 2022
# 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>());
```
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# 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>());
```
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 C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging a pull request may close this issue.