-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add simple preferences API #13312
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
Changes from all commits
020c681
19f8c0f
7cbba20
1f7131e
79c3a07
72a1dd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,275 @@ | ||||||||
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 { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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")) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 // ex: loads from user `AppData` on windows, `.config` on linux, etc
asset_server.load("config://my_settings.ron") There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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)
Thats fine. AssetSources can do arbitrary logic on arbitrary platforms. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree that it can (and should) be a separate discussion.
To this point, can you explain why a centralized There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||
fn build(&self, app: &mut crate::App) { | ||||||||
app.init_resource::<Preferences>(); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
/// A map storing 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 a type being added 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(); | ||||||||
/// } | ||||||||
/// ``` | ||||||||
/// | ||||||||
/// ### Serialization | ||||||||
/// | ||||||||
/// The preferences map is build on `bevy_reflect`. This makes it possible to serialize preferences | ||||||||
/// into a dynamic structure, and deserialize it back into this map, while retaining a | ||||||||
/// strongly-typed API. Because it uses `serde`, `Preferences` can be read ad written to any format. | ||||||||
/// | ||||||||
/// To build a storage backend, use [`Self::iter_reflect`] to get an iterator of `reflect`able trait | ||||||||
/// objects that can be serialized. To load serialized data into the preferences, use | ||||||||
/// `ReflectDeserializer` on each object to convert them into `Box<dyn Reflect>` trait objects, | ||||||||
/// which you can then load into this resource using [`Preferences::set_dyn`]. | ||||||||
#[derive(Resource, Default, Debug)] | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This is also discussed in the OP. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, ®istry);
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've provided There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Have you tried this?
Suggested change
Note that when deserializing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is pretty outside of my comfort zone with |
||||||||
pub struct Preferences { | ||||||||
// Note the key is only used while the struct is in memory so we can quickly look up a value. | ||||||||
// The key itself does not need to be dynamic. This `DynamicMap` could be replaced with a custom | ||||||||
// built data structure to (potentially) improve lookup performance, however it functions | ||||||||
// perfectly fine for now. | ||||||||
map: bevy_reflect::DynamicMap, | ||||||||
} | ||||||||
|
||||||||
impl Preferences { | ||||||||
/// Set preferences entry of type `P`, potentially overwriting an existing entry. | ||||||||
pub fn set<P: Reflect>(&mut self, value: P) { | ||||||||
let path = value.reflect_short_type_path().to_string(); | ||||||||
self.map.insert(path, value); | ||||||||
} | ||||||||
|
||||||||
/// Set preferences entry from a boxed trait object of unknown type. | ||||||||
pub fn set_dyn(&mut self, value: Box<dyn Reflect>) { | ||||||||
let path = value.reflect_short_type_path().to_string(); | ||||||||
self.map.insert_boxed(Box::new(path), value); | ||||||||
} | ||||||||
|
||||||||
/// Get preferences entry of type `P`. | ||||||||
pub fn get<P: Reflect + TypePath>(&self) -> Option<&P> { | ||||||||
let key = P::short_type_path().to_string(); | ||||||||
self.map | ||||||||
.get(key.as_reflect()) | ||||||||
.and_then(|val| val.downcast_ref()) | ||||||||
} | ||||||||
|
||||||||
/// Get a mutable reference to a preferences entry of type `P`. | ||||||||
pub fn get_mut<P: Reflect + TypePath>(&mut self) -> Option<&mut P> { | ||||||||
let key = P::short_type_path().to_string(); | ||||||||
self.map | ||||||||
.get_mut(key.as_reflect()) | ||||||||
.and_then(|val| val.downcast_mut()) | ||||||||
} | ||||||||
|
||||||||
/// Iterator over all preference entries as [`Reflect`] trait objects. | ||||||||
pub fn iter_reflect(&self) -> impl Iterator<Item = &dyn Reflect> { | ||||||||
self.map.iter().map(|(_k, v)| v) | ||||||||
} | ||||||||
|
||||||||
/// Remove and return an entry from preferences, if it exists. | ||||||||
pub fn remove<P: Reflect + TypePath>(&mut self) -> Option<Box<P>> { | ||||||||
let key = P::short_type_path().to_string(); | ||||||||
self.map | ||||||||
.remove(key.as_reflect()) | ||||||||
.and_then(|val| val.downcast().ok()) | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
#[cfg(test)] | ||||||||
mod tests { | ||||||||
use bevy_ecs::system::ResMut; | ||||||||
use bevy_reflect::{Map, Reflect}; | ||||||||
use serde_json::Value; | ||||||||
|
||||||||
use crate::{App, PreferencesPlugin, Startup}; | ||||||||
|
||||||||
use super::Preferences; | ||||||||
|
||||||||
#[derive(Reflect, PartialEq, Debug)] | ||||||||
struct Foo(usize); | ||||||||
|
||||||||
#[derive(Reflect, PartialEq, Debug)] | ||||||||
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 setters_and_getters() { | ||||||||
let mut preferences = Preferences::default(); | ||||||||
|
||||||||
// Set initial value | ||||||||
preferences.set(Foo(36)); | ||||||||
assert_eq!(preferences.get::<Foo>().unwrap().0, 36); | ||||||||
|
||||||||
// Overwrite with set | ||||||||
preferences.set(Foo(500)); | ||||||||
assert_eq!(preferences.get::<Foo>().unwrap().0, 500); | ||||||||
|
||||||||
// Overwrite with get_mut | ||||||||
*preferences.get_mut().unwrap() = Foo(12); | ||||||||
assert_eq!(preferences.get::<Foo>().unwrap().0, 12); | ||||||||
|
||||||||
// Add new type of preference | ||||||||
assert!(preferences.get::<Bar>().is_none()); | ||||||||
preferences.set(Bar("Bevy".into())); | ||||||||
assert_eq!(preferences.get::<Bar>().unwrap().0, "Bevy"); | ||||||||
|
||||||||
// Add trait object | ||||||||
preferences.set_dyn(Box::new(Bar("Boovy".into()))); | ||||||||
assert_eq!(preferences.get::<Bar>().unwrap().0, "Boovy"); | ||||||||
|
||||||||
// Remove a preference | ||||||||
assert_eq!(*preferences.remove::<Foo>().unwrap(), Foo(12)); | ||||||||
} | ||||||||
|
||||||||
#[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" | ||||||||
); | ||||||||
} | ||||||||
|
||||||||
#[test] | ||||||||
fn serialization_round_trip() { | ||||||||
use bevy_reflect::serde::ReflectDeserializer; | ||||||||
use serde::{de::DeserializeSeed, Serialize}; | ||||||||
|
||||||||
let registry = get_registry(); | ||||||||
let mut preferences = Preferences::default(); | ||||||||
preferences.set(Foo(42)); | ||||||||
preferences.set(Bar("Bevy".into())); | ||||||||
|
||||||||
// Manually turn this into a valid JSON map. There is almost certainly a better way to | ||||||||
// express this if we want to make this part of the `Preferences` API as a blessed way to | ||||||||
// assemble a preferences file, but this is enough to get the file round tripping. | ||||||||
let mut output = String::new(); | ||||||||
output.push('['); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused because the comment says "valid JSON map", but it's pushing a bracket, which starts a list. I believe it's more idiomatic in JSON to use a map type for type/value pairs, where the type name is the key, rather than a list. This doesn't allow duplicate types, but that's the case here anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible, by the way, to use serde to serialize the entire map. I have previously written a serde custom serializer that uses Bevy Reflect internally: https://github.com/viridia/panoply/blob/main/crates/panoply_exemplar/src/aspect_list.rs#L31 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is a list, the comment is wrong. This was a case of "it's 2AM and I want to build a serialization round tripping test to prove it is possible". As I noted in that section, there is definitely a better way to do it, but this was enough to get it running and working. :) |
||||||||
|
||||||||
for value in preferences.iter_reflect() { | ||||||||
let serializer = bevy_reflect::serde::ReflectSerializer::new(value, ®istry); | ||||||||
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); | ||||||||
serializer.serialize(&mut ser).unwrap(); | ||||||||
|
||||||||
let value_output = std::str::from_utf8(&buf).unwrap(); | ||||||||
output.push_str(value_output); | ||||||||
output.push(','); // Again, manual JSON map | ||||||||
} | ||||||||
output.pop(); // Remove trailing comma | ||||||||
output.push(']'); // Close manual JSON map | ||||||||
|
||||||||
let expected = r#"[{ | ||||||||
"bevy_app::preferences::tests::Foo": [ | ||||||||
42 | ||||||||
] | ||||||||
},{ | ||||||||
"bevy_app::preferences::tests::Bar": [ | ||||||||
"Bevy" | ||||||||
] | ||||||||
}]"#; | ||||||||
assert_eq!(expected, output); | ||||||||
|
||||||||
// Reset preferences and attempt to round-trip the data. | ||||||||
|
||||||||
let mut preferences = Preferences::default(); | ||||||||
assert!(preferences.map.is_empty()); | ||||||||
|
||||||||
let json: Value = serde_json::from_str(&output).unwrap(); | ||||||||
let entries = json.as_array().unwrap(); | ||||||||
|
||||||||
for entry in entries { | ||||||||
// Convert back to a string and re-deserialize. Is there an easier way? | ||||||||
let entry = entry.to_string(); | ||||||||
let mut deserializer = serde_json::Deserializer::from_str(&entry); | ||||||||
|
||||||||
let reflect_deserializer = ReflectDeserializer::new(®istry); | ||||||||
let output: Box<dyn Reflect> = | ||||||||
reflect_deserializer.deserialize(&mut deserializer).unwrap(); | ||||||||
let type_id = output.get_represented_type_info().unwrap().type_id(); | ||||||||
let reflect_from_reflect = registry | ||||||||
.get_type_data::<bevy_reflect::ReflectFromReflect>(type_id) | ||||||||
.unwrap(); | ||||||||
let value: Box<dyn Reflect> = reflect_from_reflect.from_reflect(&*output).unwrap(); | ||||||||
preferences.set_dyn(value); | ||||||||
} | ||||||||
|
||||||||
assert_eq!(preferences.get(), Some(&Foo(42))); | ||||||||
assert_eq!(preferences.get(), Some(&Bar("Bevy".into()))); | ||||||||
} | ||||||||
} |
There was a problem hiding this comment.
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:
Notably, these preferences are stored on Apps (and accessible) at runtime:
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?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.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
orpreference.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, likeand 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:
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.
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.
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:
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.