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".

alice-i-cecile

This is quite nice overall, but as this is a breaking change it needs a migration guide before merging.

alice-i-cecile · 2026-09-12T21:56:40Z

LGTM once CI is green :)

DoubleThoughtTheProgrammer · 2026-09-13T08:39:59Z

I'm gonna still need to delay this a bit since I need to rename the file. There's no way to rename files on the github app.

DoubleThoughtTheProgrammer · 2026-09-13T08:45:35Z

The website has file renaming capabilities. So no need to delay

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

Comment thread crates/bevy_settings/src/lib.rs

Comment thread crates/bevy_settings/src/lib.rs Outdated

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

alice-i-cecile requested changes Sep 7, 2026

View reviewed changes

DoubleThoughtTheProgrammer added 3 commits September 12, 2026 13:35

Add migration guide for bevy-settings

2975403

Technical writing improvement

f9437a7

Error message improvement

47c8a4a

alice-i-cecile removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 12, 2026

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 12, 2026

alice-i-cecile approved these changes Sep 12, 2026

View reviewed changes

DoubleThoughtTheProgrammer added 2 commits September 13, 2026 14:22

Fix markdown error

459a683

Remove trailing space

e207bd2

Rename migration guide file

6a0093a

	//! 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.

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!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Sep 12, 2026

Uh oh!

DoubleThoughtTheProgrammer commented Sep 13, 2026

Uh oh!

DoubleThoughtTheProgrammer commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mnmaita Aug 26, 2026 •

edited

Loading