Conversation
|
You added a new example but didn't add metadata for it. Please update the root Cargo.toml file. |
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
Okay, high-level design feedback time! Automatic preference registration and
|
|
@alice-i-cecile To respond to your first point, I'm going to need to unpack the issues around plugins and preferences. The question is: how much control should plugins have over their own settings? For the majority of cases, I think that plugins should be unaware of preferences; instead it's the responsibility of the application to manage the settings for the plugin, and communicate that to the plugin. For example, the "debug" flag for picking should be entirely under control of the app; if it wants to expose that as a preference, it can, but that's for the app to decide. However, I can also see cases where it might make sense for a plugin to take charge of it's own preferences. An example is the bevy inspector, which is a fairly independent subsystem from the rest of the app. I can see maybe wanting to save the inspector state (search filters, popup position and so on) without needing the app to get too much involved. That is, the app can decide whether or not it wants to enable preferences at all, but after that, the plugin can decide which plugin-specific resources it wants preserved between runs. The problem with this second scenario, however, is that the plugin needs a way to signal its intent before preferences are loaded, because once they are loaded, it's too late: we only load once, we don't keep the TOML data around in memory once it's been copied to the resources, so any resources which are discovered afterward miss the boat. But plugins don't get a chance to run any code until This would be different if plugins had a more complex lifecycle: if there were multiple As regards the issue of loading early: this isn't just about window position; this is coming from @cart's ancient comment on the original preferences ticket: #13312 (comment) :
Part of the difficulty here is that we don't know how late is "too late" because nobody knows. There's no way to predict, for any given third-party plugin existing today, at what point in the startup sequence that plugin becomes committed to the initial settings values. The best we can do is establish a policy recommendation for future plugins, telling them that settings will be fully loaded at initialization stage X. One other clarification: when we talk about users confirming saving of preferences, this means the app developers, not the end users. Whether or not end users need to confirm settings is an app developer choice - lots of apps save preferences automatically, there's no "save" button on the settings page, just a "close" button. Most settings also update in real time - dragging the music volume slider affects the music volume immediately, you can hear the changes. It's up to the UI designer for the app to decide this. |
examples/app/prefs_counter.rs
Outdated
| .run(); | ||
| } | ||
|
|
||
| #[derive(Resource, Reflect, Default, Serialize, Deserialize)] |
There was a problem hiding this comment.
I think relying on Serialize / Deserialize is reasonable for an initial implementation, but I think we should prioritize implementing serialization and deserialization logic on top of Reflect to cut down on boilerplate, compile times, and binary sizes:
#[derive(Resource, Reflect, Default)]
#[reflect(Resource, Default, @PreferencesGroup("counter"))]
struct Counter {
count: i32,
}See the current bevy_scene serialization impl for an idea of what that would look like.
There was a problem hiding this comment.
I've had some frustrations trying to deserialize using the reflection APIs. In particular, conversion from TOML to resource types requires type coercion (this is true of most other text-based formats like JSON), which bevy_reflect doesn't provide. Attempting to convert TOML into a DynamicStruct doesn't work because there's not enough type information to guide the deserialization.
Note that serde is used twice during both serialization and deserialization: once to convert from Rust types to TOML values, and then again to convert from TOML values to a byte string. The TOML crate exports both a Serialize and Serializer impl (and the corresponding deserializers). toml::Value is both a serialization source and a serialization target. I think it's quite clever that serde can be used for arbitrary object transformation and not just conversion into a byte buffer, but the implementation is quite complex and not something I would want to repeat.
I also wonder if bevy reflect has feature parity with serde when it comes to specifying things like property defaults and other custom serialization options. I've complained before that, although the docs for bevy reflect are extensive, they actually need to be about 10x larger because there are so many varied use cases, and I spend a lot of time scratching my head trying to figure out how to do something with reflection. Basically we need an entire book on reflection.
There was a problem hiding this comment.
Reflect deserialization is definitely a bit challenging, and I don't know exactly what it would look like here. But I know enough to know it could meet our needs here. Worst case scenario we write fully custom toml serialization logic. But I strongly suspect we can use the bevy_scene middle ground approach, which farms out to serde. There isn't much question in my mind when it comes to Bevy Reflect being the endgame here. The wins are too big to ignore relative to the engineering price we'd pay to build it.
There was a problem hiding this comment.
I'd like to ask this: what do you envision is the MVP here? I've deliberately left some things out of the initial PR (like the autosave / debouncing logic) to keep the size manageable for reviewers. And, having iterated on this a number of times I'm feeling a bit of burn-out and would like to get some collaborators piling on in stone soup fashion, which will be easier to do if there's something committed to main.
| @@ -0,0 +1,336 @@ | |||
| //! Framework for saving and loading user preferences in Bevy applications. | |||
There was a problem hiding this comment.
I've tried to consistently use the term "preferences" rather than "settings" or "config" because those are broader terms.
I do believe "preferences" are a more specialized term. Preferences (often known as "user preferences") are "user config". They also, notably, have the vibe of "please do your best to accommodate my needs, expressed here".
This is in contrast to "settings", which are much more "functional". You "set" them, and thats what they are. I believe the system here should be able to express the whole range ("preferences", "settings", "config", etc), based on the context. If we choose to distinguish them, I think we should do that within the system (ex: preferences.window.size vs settings.window.size, or perhaps engine_defaults.window.size or app.window.size or window_plugin.default_size, etc)
I think "settings" is the better "functional", general purpose term, and I think that is the "space" we should be aiming for here.
I think these "categories of settings", should be expressed as their own "settings groups" (or alternatively, a new "settings root" type/concept), as I expressed here. I quite like this API:
#[derive(SettingsGroup, Reflect)]
#[settings_group(source = OsConfig)]
#[reflect(SettingsGroup)]
struct AppSettings;
#[derive(SettingsGroup, Reflect)]
#[settings_group(parent = AppSettings)]
#[reflect(SettingsGroup)]
struct WindowSettings {
size: Vec2,
}
impl SettingsSource for OsConfig {
// API for retrieving settings text here
// Could read from a file, make a web request, call a platform api
// Probably async?
}There was a problem hiding this comment.
I have some concerns with this approach, although they aren't deal breakers.
The first is that I'm not actually sure how to implement this; I went with reflection annotations because it was easy and what I understood. I guess SettingsGroup is a trait with a reflection impl?
Secondly, I wonder if trying to bring all possible settings files under a single framework isn't a bit hubristic: because we are dealing with preferences only, we can impose opinions (items are only resources, etc.) which might not be tenable for others kind of configuration sources. It's hard to say for sure since "settings" is an open category.
There was a problem hiding this comment.
which might not be tenable for others kind of configuration sources
This isn't a particularly opinionated API. Really just hierarchies of global values (with default values) overridden by data read from a source. I see this system and think "this is exactly what I would want for plugin settings, editor settings, app preferences, etc". Of those areas, can you think of a reason why they wouldn't fit?
The first is that I'm not actually sure how to implement this; I went with reflection annotations because it was easy and what I understood. I guess SettingsGroup is a trait with a reflection impl?
Yup! Something like:
pub trait SettingsGroup: Resource {
type Parent;
fn settings_group_name() -> &'static str;
}
pub struct ReflectSettingsGroup {
settings_group_name: &'static str,
parent: TypeId,
}
impl<T: SettingsGroup> FromType<T> for ReflectSettingsGroup {
/* impl here */
} Plus a derive to generate the SettingsGroup impl, which would also farm out to bevy_ecs_macros::derive_resource.
There was a problem hiding this comment.
Note that the FromType impl would have FromType::insert_dependencies, which would insert whatever we want to be "implied" (ex: ReflectResource).
There was a problem hiding this comment.
There are a couple of other relevant "opinions":
"Preferences", unlike "settings", have no need for hot reloading. This is discussed in more detail in my hackmd design doc: https://hackmd.io/@dreamertalin/rkhljFM7R - but the tl;dr is that while "config" files may be written by a different app, "preferences" are only ever written by the consumer app - and the app has no need to use the filesystem to notify itself. For settings, OTOH, while hot reloading isn't mandatory, there's likely to be intense lobbying to support it, and this will have an impact on the architecture and lifecycle.
Also, a config file might be written in some platform-specific language (webgl? It's hard to say because we are in an area of deep speculation here), whereas my framework leans heavily on the semantics and API of the toml crate.
As you know, I tend to push back hard against technical requirements driven by speculation, as this tends to be a major source of bloat. I would feel differently if there was an actual concrete use case that we could point to.
All that being said, if we do go down this road, what would you want the crate name to be? If we go with bevy_settings we'll have to make it clear in the README that this is about end user settings, not build settings or some other kind of Bevy-related configuration.
|
|
||
| #[derive(Resource, Reflect, Default, Serialize, Deserialize)] | ||
| #[reflect(Resource, Serialize, Deserialize, Default, @PreferencesGroup("counter"))] | ||
| struct Counter { |
There was a problem hiding this comment.
I think reflection data is neat, but I think I prefer a more "typed" approach here:
#[derive(PreferencesGroup, Reflect, Default))]
#[reflect(PreferencesGroup, Default)]
struct Counter { /* */ }Where PreferencesGroup: Resource. This could internally do the Resource derive logic, and registering reflect(PreferencesGroup) can also register reflect(Resource) automatically.
This also allows us to automatically make the Counter resource/component immutable, which makes On<Insert, Counter> observers reliable to consume.
Additionally, this allows us to infer the name of the preferences group by the type name (by adding logic to the PreferencesGroup derive), and it makes that data available to the type system (ex: <Counter as PreferencesGroup>::preferences_group_name()). Developers could still override the default if they want by doing #[preferences_group(name = "custom_name")].
This approach allows us to constrain any relevant preference APIs to actual preferences (and not just all resources), serves as a form of self-documentation, and cuts down on boilerplate.
There was a problem hiding this comment.
Up to this point I've been avoiding relying on the type name because, in order to product idiomatic TOML (which is an unstated goal of mine), we'd have to do a PascalCase to snake_case conversion.
I also am a bit concerned about potential name collisions, although that could be avoided by not assuming a 1:1 mapping between TOML sections and struct fields.
There was a problem hiding this comment.
Yeah the conversion is definitely a chore, but its pretty straightforward. There are crates that can do this, but its just a few blocks of code really: https://github.com/serde-rs/serde/blob/6b1a17851ea3d86a56aa116ca1cbf428f8d5f22d/serde_derive/src/internals/case.rs#L21
There was a problem hiding this comment.
I also am a bit concerned about potential name collisions
This is something we can and should detect. If two registered preference group names conflict (regardless of manual vs inferred / converted-from-type-name), that should be an error on startup.
There was a problem hiding this comment.
If we decide to support "merged" groups (that is, pref groups made up of properties from more than one resource) then collisions between group names are not an issue, only collisions between individual property names. However, the latter are more tedious and expensive to detect.
There was a problem hiding this comment.
Oh and another thing: are we supporting newtype structs, tuple structs, and enums as preferences items? This could get rather involved, annotation-wise. Basically these all need to map to toml somehow.
In my bevy_basic_prefs (incarnation 1) I did support newtype structs, with an option to either make them a top-level property (not in a section) or in a section (adding an extra namespace layer).
There was a problem hiding this comment.
If we decide to support "merged" groups (that is, pref groups made up of properties from more than one resource) then collisions between group names are not an issue
Yup I think we should.
However, the latter are more tedious and expensive to detect.
Yup this would require reflection. This is the type of thing we could detect during serialization / deserialization if it was reflection-driven.
Oh and another thing: are we supporting newtype structs, tuple structs, and enums as preferences items? This could get rather involved, annotation-wise. Basically these all need to map to toml somehow.
In general I think we should adopt a single convention for each type and how it maps, such that the default unannotated type "just works" when it is mapped to toml, even if that means that we need to constraint what is supported in "value position" (ex: serialization "leafs").
examples/app/prefs_counter.rs
Outdated
| } | ||
|
|
||
| #[derive(Resource, Reflect, Default, Serialize, Deserialize)] | ||
| #[reflect(Resource, Serialize, Deserialize, Default, @PreferencesGroup("counter"))] |
There was a problem hiding this comment.
The big "downside" of allowing developers to define preferences inside of a "preferences group" is that we lose granularity when they change. Ex: to write logic that reacts to the WindowSettings::size preference changing, we need to do this:
app.add_observer(|window_settings: On<Insert, WindowSettings>, old_settings: Local<Option<WindowSettings>>| {
if let Some(old_settings) = &**old_settings && old_settings.size != window_settings.size {
/* do window resize logic here */
}
old_settings = Some(window_settings.clone());
})Whereas with individual "setting resources" we can just do:
app.add_observer(|window_settings: On<Insert, WindowSizeSetting>| {
/* do window resize logic here */
})Of course, defining them all together does make it all much cleaner for everything but observers. Fewer types, less boilerplate, easier discovery (as they are grouped together under a single type), etc. Perhaps optimizing the observer case isn't worth the price of admission.
There was a problem hiding this comment.
So, in my first incarnation of preferences (this PR is the fourth), I had something like this: a PreferencesKey annotation that let you override the property name. That version allowed multiple different resources to merge into the same logical TOML "section"; conversely on deserialization, it would draw matching properties from the TOML struct. But the code was a lot more complicated, as it had to traverse through all the reflected properties.
| let last_save = world.read_change_tick(); | ||
|
|
||
| // Get the type registry and clone the Arc so we don't have to worry about borrowing. | ||
| let Some(app_types) = world.get_resource::<AppTypeRegistry>() else { |
There was a problem hiding this comment.
@alice-i-cecile I'm moving the "should we use reflection derives to auto-register preferences" conversation to a thread to keep things organized. I strongly believe that using the Reflect registry is the right approach here. In addition to @viridia's points (which I of course agree with, because I also made most of them), I think Preferences fall into the same category as Scenes / Inspectors. They're all things that benefit from early registration, feed on reflection data, serialize/deserialize, and feed the "bevy data model". I think using the same general pattern here makes a lot of sense.
There was a problem hiding this comment.
I can see the argument. If you're convinced that the added complexity / weirdness is worth it here, I can live with that. I'll do my best to help with docs to explain this model!
There was a problem hiding this comment.
BTW, one option I did not mention was calling load_preferences() automatically if the prefs feature flag is enabled. So far I have avoided that because it requires an inverted dependency between preferences and App.
|
BTW, there's one other thing to say about preferences: they represent, for the game developer, a degree of optimistic faith in the future: a confident belief that users will run their app more than once :) |
|
I have made a bunch of changes based on the discussion:
However, a bunch of stuff is not done:
|
|
I added a new example which demonstrates saving the window size and position. However, in the process of doing this, I realized that I had some misconceptions about the way Bevy plugins work which have an impact on the design. Specifically, I now understand that plugins initialize (i.e. run In the example, I have a "window settings" plugin that copies the window size and position from the window settings to the window entity. In order for this to work, two things have to happen first:
This means adding the plugin after I now realize that I could eliminate the However, it's a little bit footgunny: most Bevy plugins are independent, and don't much care about order; users who use preferences will need to start thinking about plugin ordering. You'll likely want to add the preferences plugin before most other plugins. Another thing I want to point out is that the "glue" plugin - that is, the plugin that copies the window settings from the resource to the window - is not the same plugin that creates the window in the first place. I expect this pattern to be common. So the typical order will be something like:
Why do we need two separate plugins, one to create A and one to configure it? There are several reasons:
|
12dae00 to
203b6a5
Compare
|
The generated |
Yet another attempt at implementing
bevy_preferences. This version uses bevy_reflectserializationto convert resources from toml values into Rust types and vice versa. This is based on the feedback that I got from the earlier attempt in #22770To indicate that a resource type should be loaded as preferences, you'll need to add the
PreferenceGroupannotation:This will produce a TOML file that looks like this:
Theory of Operation
The
App::load_preferences()extension method scans the type registry for all resource types that implSerialize,Deserialize,Default, and have thePreferenceGroupannotation. An optionalPreferenceFileannotation can be used to write the resource to a different file (or different key in browser local storage).load_preferences()is meant to be called beforeApp::runwhich means it is called before.build(). This ensures that any other plugins can have access to the settings data during initialization.load_preferences()checks to see if the resource already exists; if so, it uses that resource instance and patches the toml values into it, preserving any defaults that have been set. If the resource does not exist, it constructs a new one viaReflectDefaultbefore applying the toml properties.(There was a suggestion of using
FromWorldinstead ofDefault. This is worth considering, although there may be issues with callingFromWorldso early in the app initialization lifecycle, before most resources have been created.)On
wasmplatforms, this uses browser local storage rather than the filesystem to store preferences. On platforms which have neither, preferences are not supported (although it's possible that some platform-specific settings storage could be implemented).Note on terminology
I've tried to consistently use the term "preferences" rather than "settings" or "config" because those are broader terms. For example, the
xorg.conffile, commonly used to configure an XWindows display, is technically a "settings" file, but it is not "preferences". However, for end users it's perfectly permissible to use the word "Settings" in menus and navigation elements since that is the term most commonly used in software today.Open Issues
Syncing with non-resources
Some important settings are not stored in resources: one of the most common things that users will want to preserve is the window position and size, which exist on the window entity. It's not possible, under my design, to store arbitrary entities as preferences, so in order for the window properties to be saved they will have to be copied to a resource before being serialized. We probably don't want to be continually copying the window size every time the window is dragged or moved, so we'll need some way to know when serialization is about to happen. I'm thinking that possibly some global event could be triggered just before serialization, and the handlers could use this event to make last-minute patches to resources.
Saving If Changed
Because saving involves i/o, we want to only save when preferences have actually changed. This involves two discrete checks:
The reason for these two steps is that even checking which files need to be saved is non-trivial and probably should not be done every frame.
Rather than check the
is_changed()field of every preference resource every frame, the code currently relies on the user to issue an explicitCommandwhenever they change a preferences property. This gets especially tricky if the settings to be saved aren't actually in a resource, like the aforementioned window position.There are two forms of the command:
SavePreferencesandSavePreferencesSync. The former, which uses an i/o task, is the preferred approach, unless the app is about to exit, in which case the sync version is preferred.Once we know that a save will take place, a second pass can be used to check the timestamp on every resource: if any resource has a tick value later than the last time the file was either loaded or saved, then we know that file is out of date.
Also some properties can change at high frequency - for example, dragging the master volume slider changes the volume every frame. For this reason, we will generally want to put in a delay / debounce logic to batch updated together (not present in this PR). However, this delay means that if the user adjusts the setting and then immediately terminates the app, the setting won't be recorded. (There is no chance of the file being corrupted, as it uses standard practices for ensuring file integrity.)
Unfortunately, on some platforms, depending on how the user chooses to quit (Command-Q on Mac) there's no opportunity to listen for the
AppExitevent. For this reason, it's best to use a "belt and suspenders" approach which listens for bothAppExitand autosave timer events.