bevy-settings requires more validity by DoubleThoughtTheProgrammer · Pull Request #25548 · bevyengine/bevy

DoubleThoughtTheProgrammer · 2026-08-25T16:41:34Z

Objective

It took me half a day to get bevy-settings working. This is in part due to lacking documentation and in part due to it failing without any errors. In my first PR (#25547), I improved the situation by improving the documentation. However, even with the improved documentation, it's still possible to write a program that compiles and runs without any errors or warnings, but still is multiple lines annotations and trait implementations away from actually letting SettingsPlugin save and load the settings.

Solution

THIS IS A BREAKING CHANGE

This PR adds trait bounds so that types that implement the SettingsGroup trait must also implement Default and Reflect, which are both required for SettingsPlugin to save and load the settings. Furthermore, SettingsPlugin will now panic if a type is registered #[reflect(SettingsGroup)] but doesn't register #[reflect(Default)], since only types that are registered ReflectDefault and ReflectSettingsGroup will be saved and loaded by SettingsPlugin.

I justify these breaking changes by how only invalid code (that is, code that tries to use SettingsPlugin and SettingsGroup but does not manage to let SettingsPlugin save and load all the SettingsGroup types) can have compile errors.

Testing

Did you test these changes? Yes, the test suite passes like before.
Are there any parts that need more testing? Oh, absolutely. Since this is a breaking change, it should be determined how much code "in the wild" will be negatively affected by this change.
How can other people (reviewers) test your changes? Are there any applications of the SettingsGroup trait that the trait bounds are getting in the way of?
If relevant, what platforms did you test these changes on, and are there any important ones you can't test? I only tested this on Linux, as the changes were small enough and confined to cross-platform code so it shouldn't be affected by which platform the tests run on.

crate

`SettingsGroup`s need to be `Resource + Default + Reflect` to actually do anything with the `SettingsPlugin`, so they may as well be trait bounds. Also, since `ReflectSettingsGroup` also needs `ReflectDefault` to function, it may as well panic if that is missing.

mnmaita

I'm really thankful for changes in this crate, as I had some issues while trying to implement it in my projects. Leaving a few comments and suggestions.

mnmaita · 2026-08-26T14:17:53Z

+//!
+//! Settings are loaded into resources that implement [`SettingsGroup`](trait@SettingsGroup),
+//! which is best implemented using the derive macro [`SettingsGroup`](derive@SettingsGroup).
+//! In addition, the resource have the `#[reflect(SettingsGroup, Default)]` annotation.


Suggested change

//! In addition, the resource have the `#[reflect(SettingsGroup, Default)]` annotation.

//! In addition, the resource should have the `#[reflect(SettingsGroup, Default)]` annotation.

Edited! Thanks for catching that one

I'd prefer "must have", since otherwise the settings plugin won't load and save that type.

Makes sense!

mnmaita · 2026-08-26T14:19:30Z

+//! In addition, the resource have the `#[reflect(SettingsGroup, Default)]` annotation.
+//!
+//! Once all these conditions are met, and when [`SettingsPlugin`] is added, systems can query
+//! for settings using like any other resource.


Suggested change

//! for settings using like any other resource.

//! for settings as like any other resource.

Maybe a native can double check this, but I feel like this reads better.

Alternatively, "just like"

I originally meant to do "for settings using Res and ResMut, like any other resource".

mnmaita · 2026-08-26T14:20:46Z

 /// ```
 ///
+/// Note that it's possible to make multiple different types share the same group and file
+/// using this, and that case isn't well tested


I'd add tests instead of documenting the lack of them. What would it take to create them?

Also, it's Bevy house style not to editorialize in user-facing comments (I've tripped up on this myself - it's tempting to justify or hedge, but those discussions belong elsewhere.)

Honestly I don't think it's been decided what the expected behaviour is if multiple types have the same group or file - or even the same key, which (I checked) is in fact possible.

Multiple types with the same group and/or file make total sense to me. You could have a "per field" type of configuration where each resource represents a single field in the file. This pattern is useful for change detection for example.

On the other hand, same key for different resources sounds problematic and I don't know how we could prevent that.

Ideally, same key would produce a warning, but would be expensive to check since you'd have to keep a map of all keys that have been used up to this point. Whether it's worth it is a discussion to have.

Multiple types with the same group is an intentional feature: it's very common in Bevy that we have to split resources apart for technical reasons (change detection, etc.) that have nothing to do with categorization or logical grouping. Forcing each resource to be its own top-level TOML key would make the settings file less legible.

viridia

Looks good generally, just a few nits.

mnmaita

Last minor thing I spotted. Approving anyway, but it should be fixed for correctness in the docs. Thanks for the changes!

mnmaita · 2026-08-28T08:39:27Z

 ///
-/// Note that it's possible to make multiple different types share the same group and file
-/// using this, and that case isn't well tested
+/// Note that it's possible to make multiple different settings types share the same file,


I think "multiple different setting types" is correct here, instead of "settings types".

kfc35 · 2026-09-04T18:17:55Z

+/// When added to an app, `SettingsPlugin` will load settings from storage (either the filesystem
+/// or browser local storage) into resources that implement the [`SettingsGroup`](trait@SettingsGroup),
+/// [`Default`], and [`Reflect`] traits, and, in
+/// addition, are also annotated with `#[reflect(Default, SettingsGroup)]`. The plugin can also be used


Nit: if im not mistaken, the plugin isnt used directly to do the save. If you feel the need to mention that the plug in is required in order for save commands to be respected, id be more precise with the wording i.e. "Adding the plugin also enables settings save commands submitted to the commands queue to be respected" or something like that

kfc35 · 2026-09-04T18:22:10Z

+            // Collect all the errors into a single list so that a user can see all of them at once rather than chasing them
+            // down one by one as they fix the errors.
+            errors.push(format!(
+                "Type {} has #[reflect(SettingsGroup)], which requires #[reflect(Default)]. It will not be saved or loaded.",


Suggested change

"Type {} has #[reflect(SettingsGroup)], which requires #[reflect(Default)]. It will not be saved or loaded.",

"Type {} has #[reflect(SettingsGroup)], which requires #[reflect(Default)] in order for it to be saved or loaded.",

Imo leaving it as second sentence sounds unrelated / not enough reason for it to be a panic.

DoubleThoughtTheProgrammer added 7 commits August 25, 2026 15:22

Give usage guidelines for bevy::settings crate

cbb0100

Fix broken links

98b709f

Print useful errors when a SettingsGroup is not in a usable state

ea0dd11

Add descriptions of preconditions of settings types to the bevy_settings

4492c4f

crate

Fix typo

d33e859

Fix documentation for project requirements

40229fd

Zeophlite approved these changes Aug 26, 2026

View reviewed changes

Zeophlite added A-Reflection Runtime information about types D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Settings Related to the preferences and settings framework itself labels Aug 26, 2026

github-project-automation Bot added this to Reflection Aug 26, 2026

github-project-automation Bot moved this to Needs SME Triage in Reflection Aug 26, 2026

Zeophlite requested a review from viridia August 26, 2026 08:25

mnmaita reviewed Aug 26, 2026

View reviewed changes

viridia reviewed Aug 26, 2026

View reviewed changes

Fix docs nits

0bed35b

mnmaita approved these changes Aug 28, 2026

View reviewed changes

kfc35 approved these changes Sep 4, 2026

View reviewed changes

kfc35 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 4, 2026

	//! In addition, the resource have the `#[reflect(SettingsGroup, Default)]` annotation.
	//! In addition, the resource should have the `#[reflect(SettingsGroup, Default)]` annotation.

	//! for settings using like any other resource.
	//! for settings as like any other resource.

	"Type {} has #[reflect(SettingsGroup)], which requires #[reflect(Default)]. It will not be saved or loaded.",
	"Type {} has #[reflect(SettingsGroup)], which requires #[reflect(Default)] in order for it to be saved or loaded.",

Uh oh!

Uh oh!

Conversation

DoubleThoughtTheProgrammer commented Aug 25, 2026

Objective

Solution

Testing

Uh oh!

mnmaita left a comment

Choose a reason for hiding this comment

Uh oh!

mnmaita Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viridia left a comment

Choose a reason for hiding this comment

Uh oh!

mnmaita left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mnmaita Aug 26, 2026 •

edited

Loading