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

Add simple preferences API #13312

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/bevy_app/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ mod main_schedule;
mod panic_handler;
mod plugin;
mod plugin_group;
mod preferences;
mod schedule_runner;
mod sub_app;

Expand All @@ -21,6 +22,7 @@ pub use main_schedule::*;
pub use panic_handler::*;
pub use plugin::*;
pub use plugin_group::*;
pub use preferences::*;
pub use schedule_runner::*;
pub use sub_app::*;

Expand Down
173 changes: 173 additions & 0 deletions crates/bevy_app/src/preferences.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
use bevy_ecs::system::Resource;
use bevy_reflect::{Map, Reflect, TypePath};

use crate::Plugin;

/// Adds application [`Preferences`] functionality.
pub struct PreferencesPlugin;

impl Plugin for PreferencesPlugin {
Copy link
Member

Choose a reason for hiding this comment

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

From my perspective, Plugins already have a preferences API:

pub struct AssetPlugin {
    pub file_path: String,
    pub processed_file_path: String,
    pub watch_for_changes_override: Option<bool>,
    pub mode: AssetMode,
}

Notably, these preferences are stored on Apps (and accessible) at runtime:

let asset_plugin = app.get_added_plugins::<AssetPlugin>()[0];

Not the ideal runtime API (not accessible on World, returns a Vec), but thats a solvable problem.

Also, notably, the vast majority of these plugin settings could be serializable.

This is the current "plugin settings" API, so any proposal should describe how it relates to it / justify its existence relative to it. What does Preferences enable that the current plugin settings approach could not support / evolve into? Can (and should) we unify the two concepts? Should we remove plugin settings as they are currently defined?

Note that we moved away from "plugin settings as resources" in this PR in favor of the current approach. This Preferences proposal has a lot in common with that pattern (just using a centralized resource instead of individual resources). Can you read through the rationale in that PR for moving away and give your thoughts?

Copy link
Member Author

@aevyrie aevyrie May 10, 2024

Choose a reason for hiding this comment

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

The (unclear) distinction here is that using Preferences gives you a place to declare state that you, as the plugin author want to be globally persistent.

Notably, these preferences are stored on Apps (and accessible) at runtime:

I had no idea this was the case, and I don't know if the ecosystem does either. I was under the impression that plugin state was set on init, and never updatable at runtime. Glad to be made aware of that, but I suppose that highlights a problem if I didn't even know this was a blessed pattern.

Stepping back from the current implementation questions, the user need here is for some idiomatic, easy-to-use place to declare some plugin state is persistent and global to the application. This is distinct from, say, serializing a game save or project file, which is not global.

From that point of view, Plugin is a place to hold in memory state tied to a specific plugin, but there is no distinction or affordances to declare that some part of that state is global and (generally-speaking) hot reloadable. Additionally, there is no way to turn all of that global persistent state into a single, serializable structure, e.g. settings.ron or preference.json. I think that's one of the hardest things to accomplish: gathering all global state into a single place, turning it into trait soup, and being able to serde it to a dynamic format is not easy for most users.

To me, the benefit of something like Preferences, is that I can have my in memory representation of all plugin settings, like

pub struct AssetPlugin {
    pub file_path: String,
    pub processed_file_path: String,
    pub watch_for_changes_override: Option<bool>,
    pub mode: AssetMode,
}

and additionally, I can declare my public-facing preferences, which might look completely different, and require extra processing or runtime steps to apply to my application - or even writing back to the preferences file because a setting is invalid:

pub enum AssetPreferences {
    Default,
    TurboMode,
    Custom(/* ... /*)
}

If I squint, it also seems trivial to add callbacks, so I could update my plugin settings when my preferences entry changes, or vice versa.

Can you read through the rationale in that PR for moving away and give your thoughts?

I don't think this conflicts with that, but it is at least clear that the distinction needs to be made much more obvious to developers.

  • "I have state specific to a plugin": great, add it as part of your plugin struct
  • "I want some of that state to persist in a global preferences file": define that, and add it to the Preferences.

If this approach is acceptable, we might want to integrate this more closely with plugins, so that adding a preferences entry requires implementing a trait that allows conversion to and from the preference and plugin types. Something vaguely along the lines of:

pub trait PluginPreferences {
    type Plugin: Plugin;
    fn from_plugin_state(plugin: &Self::Plugin) -> Self;
    fn into_plugin_state(&self) -> Self::Plugin;
}

This also aids discoverability, and guides users into these blessed patterns. If they want to add preferences for their plugin, they will immediately see that preferences tie directly to the plugin state, which maybe they (like me) didn't realize was the canonical place to store runtime plugin state.

Copy link
Contributor

@NthTensor NthTensor May 11, 2024

Choose a reason for hiding this comment

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

What does Preferences enable that the current plugin settings approach could not support / evolve into?

Plugin settings, as they are used currently, allow the developer to define the specific behavior they want. Stuff like "should the window exit when requested" or "how should we initialize wgpu".

These are distinct from user preferences. We don't want users tweaking them. We shouldn't mix developer settings and user-configurable preferences in the same type, that would be crazy!

Since we were just using the render plugin as an example, let's stick with it. The render plugin absolutely needs to expose preferences for visual fidelity if we want to target low end hardware. San Miguel runs at 10fps on my laptop, and I currently have no way to specify a different performance/fidelity trade off.

Implementing those options is a different problem, but it starts with having a place to put them that can be exposed to the user in a preferences screen or a file, and which isn't contaminated with settings not intended for user consumption. That's what we are missing currently.

Copy link
Member

@cart cart May 10, 2024

Choose a reason for hiding this comment

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

Why a centralized Preferences store instead of using the ECS?

#[derive(Resource, Reflect)]
struct MySettings {
  foo: Foo,
  bar: Bar,
}

app.add_plugin(SettingsPlugin::<MySettings>()::with_path("path/to/my_settings.ron"))

// later
fn system(my_settings: Res<MySettings>) {
}

Can we / should we use the asset system for this?

#[derive(Asset, Reflect)]
struct MySettings {
  foo: Foo,
  bar: Bar,
} 

// this kicks off an asset load
app.add_plugin(SettingsPlugin::<MySettings>()::with_path("path/to/my_settings.ron"))

Copy link
Contributor

Choose a reason for hiding this comment

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

That's interesting, but neither your proposal or this PR really solves the problem I was hoping to solve. What I was hoping for is something that would abstract away platform differences - browser local storage vs ${HOME}/.config vs ${HOME}/Libraries/Application Support vs C:\\Program Files\etc..

  • For some platforms, the game installation dir might not be writable (I think?).
  • For some platforms (browser), there might not even be a filesystem.

My specific use case is that I have a color picker that wants to save a list of "recent colors" as a user preference. Because this is part of a library, I have no knowledge of what platforms it will eventually run on, that's up to the game author that uses my crate. So I need some way to read/write my preferences, without knowing any of the details of how they will be stored.

Copy link
Member

@cart cart May 10, 2024

Choose a reason for hiding this comment

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

Using the asset system could play into solving this problem, via a new config AssetSource that writes to/reads from whatever config location makes sense for a given platform. I've proposed this exact API very recently on discord:

// ex: loads from user `AppData` on windows, `.config` on linux, etc
asset_server.load("config://my_settings.ron")

Copy link
Member

@cart cart May 10, 2024

Choose a reason for hiding this comment

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

(you could build this AssetSource today as a 3rd party plugin in Bevy Asset V2)

Copy link
Member

Choose a reason for hiding this comment

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

For some platforms, the game installation dir might not be writable (I think?).

Thats fine, the asset source writer APIs return results / write errors could be handled cleanly (either via an AssetWriter that fails, or by not registering an AssetWriter at all on platforms that dont support it)

For some platforms (browser), there might not even be a filesystem.

Thats fine. AssetSources can do arbitrary logic on arbitrary platforms.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very possibly, yes. I think this would be fruitful to add as part of the discussion in your other comment. I definitely heard my gut saying "use assets", but I responded "idk how the heck to use them".

In my mind, this could be provided by the user or plugins, but built on top of this centralized map. Want to save preferences to json? Use bevy_json_preferences, which might have other features like splitting into multiple files, or filtering before serialization.

To me this is orthogonal to providing a preferences API. The problem this PR solves is "how do we get the ecosystem to agree on a central store for globally persistent state with a dynamic format". What to use for serialization seems like a whole new can of worms.

Copy link
Member

Choose a reason for hiding this comment

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

What to use for serialization seems like a whole new can of worms.

I agree that it can (and should) be a separate discussion.

To me this is orthogonal to providing a preferences API. The problem this PR solves is "how do we get the ecosystem to agree on a central store for globally persistent state with a dynamic format".

To this point, can you explain why a centralized Preferences store is necessary at all relative to storing each preference in individual Resources (as I discussed above)? Using resources seems preferential from a "consume from code" perspective.

Copy link
Member Author

@aevyrie aevyrie May 12, 2024

Choose a reason for hiding this comment

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

You could implement this API in a way where the in-memory state lives in Resources; I don't think the two are mutually exclusive.

The discussion that spawned this came from a desire to have some blessed solution that allows us to read/write a single user-preferences file that can be made persistent. I don't really care where that data lives in memory, the only bit of this design that actually matters to the user is that it provides a singular place for plugin devs to declare "I want this data to be globally persistent and user configurable".

Using resources is an implementation detail - if we want to turn this into something that needs "gather" and "distribute" phases, that's fine, but it will be more code and implementation complexity. The end goal is a single file, so spreading the in-memory representation across the ECS in multiple Resources is possible, but annoying.

So, to answer your question in the most long-winded way possible, the backing in-memory storage for Preferences could go into Resources for all I care, but I do think there is value in having a centralized API surface for plugins to declare that they have persistent, user/dev-configurable settings. It might be that this is not the right place to put that API! In this comment, I suggest it might make more sense for it to be a trait extension on Plugin.

fn build(&self, app: &mut crate::App) {
app.init_resource::<Preferences>();
}
}

/// A map that stores all application preferences.
///
/// Preferences are strongly typed, and defined independently by any `Plugin` that needs persistent
/// settings. Choice of serialization format and behavior is up to the application developer. The
/// preferences resource simply provides a common API surface to consolidate preferences for all
/// plugins in one location.
///
/// ### Usage
///
/// Preferences only require that the type implements [`Reflect`].
///
/// ```
/// # use bevy_reflect::Reflect;
/// #[derive(Reflect)]
/// struct MyPluginPreferences {
/// do_things: bool,
/// fizz_buzz_count: usize
/// }
/// ```
/// You can [`Self::get`] or [`Self::set`] preferences by accessing this type as a [`Resource`]
/// ```
/// # use bevy_ecs::prelude::*;
/// # use bevy_app::Preferences;
/// # use bevy_reflect::Reflect;
/// #
/// # #[derive(Reflect)]
/// # struct MyPluginPreferences {
/// # do_things: bool,
/// # fizz_buzz_count: usize
/// # }
/// #
/// fn update(mut prefs: ResMut<Preferences>) {
/// let settings = MyPluginPreferences {
/// do_things: false,
/// fizz_buzz_count: 9000,
/// };
/// prefs.set(settings);
///
/// // Accessing preferences only requires the type:
/// let mut new_settings = prefs.get::<MyPluginPreferences>().unwrap();
///
/// // If you are updating an existing struct, all type information can be inferred:
/// new_settings = prefs.get().unwrap();
/// }
/// ```
#[derive(Resource, Default, Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe Preferences needs some kind of de/serialisation support to really be useful. Ideally, a generic FilePreferencesStorePlugin could store/load the entire preferences resource to disk, as an example.

Copy link
Member Author

@aevyrie aevyrie May 10, 2024

Choose a reason for hiding this comment

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

That's why this is based on reflect. You can serialize the map however you'd like, in any format you'd like. I can't implement reflect on Preferences as-is, but it possible with some elbow grease for the internal map.

This is also discussed in the OP.

Copy link
Member Author

@aevyrie aevyrie May 10, 2024

Choose a reason for hiding this comment

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

I proved this out in a test:

#[derive(Reflect)]
struct Foo(usize);

#[derive(Reflect)]
struct Bar(String);

fn get_registry() -> bevy_reflect::TypeRegistry {
    let mut registry = bevy_reflect::TypeRegistry::default();
    registry.register::<Foo>();
    registry.register::<Bar>();
    registry
}

#[test]
fn serialization() {
    let mut preferences = Preferences::default();
    preferences.set(Foo(42));
    preferences.set(Bar("Bevy".into()));

    for (_k, value) in preferences.map.iter() {
        let registry = get_registry();
        let serializer = bevy_reflect::serde::ReflectSerializer::new(value, &registry);
        let mut buf = Vec::new();
        let format = serde_json::ser::PrettyFormatter::with_indent(b"    ");
        let mut ser = serde_json::Serializer::with_formatter(&mut buf, format);
        use serde::Serialize;
        serializer.serialize(&mut ser).unwrap();

        let output = std::str::from_utf8(&buf).unwrap();
        println!("{:#}", output);
    }
}

which prints

{
    "bevy_app::preferences::tests::Foo": [
        42
    ]
}
{
    "bevy_app::preferences::tests::Bar": [
        "Bevy"
    ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, thanks for the clarification! For now, could you mark the inner map as pub? That way if we don't get a follow-up PR we can still get utility out of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added this test and included fully roundtripping a file. The test uses JSON, but you could use whatever format you'd like.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've provided iter_reflect, which gives all the data needed to serialize the data, and set_dyn to deserialize the data, without needing to make the inner map pub.

Copy link
Member Author

Choose a reason for hiding this comment

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

Someone smarter than me can figure out how to implement reflect, because it will require an assembly step to convert between the list of trait objects in the file, to a map in memory.

Copy link
Member

Choose a reason for hiding this comment

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

I can't implement reflect on Preferences as-is, but it possible with some elbow grease for the internal map.

Have you tried this?

Suggested change
#[derive(Resource, Default, Debug)]
#[derive(Resource, Reflect, Default, Debug)]
#[reflect(from_reflect = false)]

DynamicMap doesn't implement FromReflect, so you'll have to opt out of FromReflect for Preferences, but it should still be possible.

Note that when deserializing Preferences, you'll get back a DynamicStruct. Since you can't use FromReflect to convert it back to Preferences, you might need to use Default/ReflectDefault and patch with Reflect::apply.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is pretty outside of my comfort zone with Reflect. I had a sense that I needed to do something along those lines, but I have no idea what they do or what tools to reach for. If you can find a way to do that, I would appreciate a PR against this one!

pub struct Preferences {
map: bevy_reflect::DynamicMap,
}

impl Preferences {
/// Set preferences of type `P`.
pub fn set<P: Reflect + TypePath>(&mut self, value: P) {
self.map.insert(P::short_type_path(), value);
}

/// Get preferences of type `P`.
pub fn get<P: Reflect + TypePath>(&self) -> Option<&P> {
let key = P::short_type_path();
self.map
.get(key.as_reflect())
.and_then(|val| val.downcast_ref())
}

/// Get a mutable reference to preferences of type `P`.
pub fn get_mut<P: Reflect + TypePath>(&mut self) -> Option<&mut P> {
let key = P::short_type_path();
self.map
.get_mut(key.as_reflect())
.and_then(|val| val.downcast_mut())
}
}

#[cfg(test)]
mod tests {
use bevy_ecs::system::ResMut;
use bevy_reflect::{Map, Reflect};

use crate::{App, PreferencesPlugin, Startup};

use super::Preferences;

#[test]
fn typed_get() {
#[derive(Reflect, Clone, PartialEq, Debug)]
struct FooPrefsV1 {
name: String,
}

#[derive(Reflect, Clone, PartialEq, Debug)]
struct FooPrefsV2 {
name: String,
age: usize,
}

let mut preferences = Preferences::default();

let v1 = FooPrefsV1 {
name: "Bevy".into(),
};

let v2 = FooPrefsV2 {
name: "Boovy".into(),
age: 42,
};

preferences.set(v1.clone());
preferences.set(v2.clone());
assert_eq!(preferences.get::<FooPrefsV1>(), Some(&v1));
assert_eq!(preferences.get::<FooPrefsV2>(), Some(&v2));
}

#[test]
fn overwrite() {
#[derive(Reflect, Clone, PartialEq, Debug)]
struct FooPrefs(String);

let mut preferences = Preferences::default();

let bevy = FooPrefs("Bevy".into());
let boovy = FooPrefs("Boovy".into());

preferences.set(bevy.clone());
preferences.set(boovy.clone());
assert_eq!(preferences.get::<FooPrefs>(), Some(&boovy));
}

#[test]
fn init_exists() {
#[derive(Reflect, Clone, PartialEq, Debug)]
struct FooPrefs(String);

let mut app = App::new();
app.add_plugins(PreferencesPlugin);
app.update();
assert!(app.world().resource::<Preferences>().map.is_empty());
}

#[test]
fn startup_sets_value() {
#[derive(Reflect, Clone, PartialEq, Debug)]
struct FooPrefs(String);

let mut app = App::new();
app.add_plugins(PreferencesPlugin);
app.add_systems(Startup, |mut prefs: ResMut<Preferences>| {
prefs.set(FooPrefs("Initial".into()));
});
app.update();
assert_eq!(
app.world()
.resource::<Preferences>()
.get::<FooPrefs>()
.unwrap()
.0,
"Initial"
);
}
}