Refactor/rethink how duality handles settings

[Rick-van-Dam]

Summary

There are some things that might be improved in the settings department of duality:

First it seems that duality uses 2 approaches to storing settings.

• EditorUserData.xml seems to use a completely separate code flow for writing to the settings file in DualityEditorApp.SaveUserData(). The file itself also has 2 <?xml version="1.0" encoding="utf-8"?> which I have never seen before in a single file. Maybe some hack around dockpanel behavior?
• Other setting files are using the duality Serializer class to write to the files. They also use the .dat file extension instead of .xml

Second there's not a clear location for where saving and loading is implemented. Sometimes it happens in DualityApp or DualityEditorApp and other times it happens in the settings class itself. I think we might benefit from making a interface with Save and Load methods where these are implemented on the class itself as that would allow us to treat settings in a more generic way (such as here https://github.com/AdamsLair/duality/blob/master/Source/Editor/DualityEditor/DualityEditorApp.cs#L1012) and also clean up some code from the already big DualityApp and DualityEditorApp classes.

Thirdly because of how the serializer api works atm it always creates a new instance. This makes it impossible to put change events on the settings class itself because then you would also have to resubscribe to those events which kinda defeats the purpose. It might be beneficial if the serializer api could take a already existing instance and update that instance.

[ilexp]

Thirdly because of how the serializer api works atm it always creates a new instance. This makes it impossible to put change events on the settings class itself because then you would also have to resubscribe to those events which kinda defeats the purpose. It might be beneficial if the serializer api could take a already existing instance and update that instance.

Yeah, this is a design flaw in the serializer API, but a big one to fix properly. We should skip this for now and just assume the serializer API to be as-is for now. We should track this it in a separate issue though, because it is worth improving here.

Second there's not a clear location for where saving and loading is implemented. Sometimes it happens in DualityApp or DualityEditorApp and other times it happens in the settings class itself. I think we might benefit from making a interface with Save and Load methods where these are implemented on the class itself as that would allow us to treat settings in a more generic way (such as here https://github.com/AdamsLair/duality/blob/master/Source/Editor/DualityEditor/DualityEditorApp.cs#L1012) and also clean up some code from the already big DualityApp and DualityEditorApp classes.

If we use the regular serializer in all cases, we don't need an API or interface for this, but we could still provide one to avoid requiring that knowledge. Maybe have an abstract SettingsContainer base class?

• EditorUserData.xml seems to use a completely separate code flow for writing to the settings file in DualityEditorApp.SaveUserData(). The file itself also has 2 <?xml version="1.0" encoding="utf-8"?> which I have never seen before in a single file. Maybe some hack around dockpanel behavior?
• Other setting files are using the duality Serializer class to write to the files. They also use the .dat file extension instead of .xml

Yeah, let's switch this to use Duality serializers as well and, I don't know, put the DockPanel XML inside a string or byte[] field or something. Nobody is going to manually adjust this in XML form anyway.

That said, the custom XML approach currently has the advantage that every editor plugin can just read and write arbitrary custom stuff, so when we do switch to Duality serializers, we need to mirror this behavior in a different way.

One way would be to have an abstract SettingsSection base class with the class for EditorUserData storing a dictionary of them, and editor infrastructure asking each of the plugins for theirs (or null to skip). Essentially the same as it is now, only it's not the editor handing over an XElement, but the plugin handing over a SettingsSection. Would require every plugin with settings to have a class for them though, but maybe that's a good idea anyway?

Alternatively, the plugin classes could themselves be serialized, though that (a) requires the existing-instance thing to be implemented and (b) I'm not sure how I feel about the semantic breach we'd introduce by this. A separate class for containing a plugins settings that is serialized as a whole would be a clear separation.

---

Besides all of this, as briefly mentioned in #852, it could make sense to rename and / or regroup all existing Duality settings. It might make sense to do this separately of any internal API / serialization changes.

So right now we have:

• AppData.dat
• UserData.dat (and actually DefaultUserData.dat as well!)
• ProjectSettings.dat / EditorAppData.dat (depending on the outcome of Add project settings and move launcher path to it #852)
• EditorUserData.xml
• DesignTimeData.dat

I'd propose to rename them like this going forward, in order to provide better hints regarding their contents:

• SystemSettings.dat: Internal engine settings that are constant for this game, like rendering config, starting scene, physics settings, etc.
• GameOptions.dat: User-configurable settings of the game, like window size, audio volumes, etc.
• ProjectSettings.dat: Global project settings for editing purposes, like launcher path.
• EditorOptions.dat: User-specific settings of the editor, like window layout, panels configs, etc. - and I'd actually merge DesignTimeData.dat into this, since right now it only contains whether objects are hidden or locked, which is definitely a user-specific setting.

In this naming scheme, I've chosen "Options" to represent things users can adjust as they want, and "Settings" to represent something global, shared, that is not meant for everyone to adjust as they want. "System" also hints at internals not meant to be touched by players and suggests that it's the engine that is configured here, which is exactly what it does. "Project" sets the context of working on something and hints towards editor usage that way.

Or, in a different form:

	Constant	Choose your own
Game	SystemSettings	GameOptions
Editor	ProjectSettings	EditorOptions

Thoughts?

[Rick-van-Dam]

Thirdly because of how the serializer api works atm it always creates a new instance. This makes it impossible to put change events on the settings class itself because then you would also have to resubscribe to those events which kinda defeats the purpose. It might be beneficial if the serializer api could take a already existing instance and update that instance.

Yeah, this is a design flaw in the serializer API, but a big one to fix properly. We should skip this for now and just assume the serializer API to be as-is for now. We should track this it in a separate issue though, because it is worth improving here.

Agree.

Second there's not a clear location for where saving and loading is implemented. Sometimes it happens in DualityApp or DualityEditorApp and other times it happens in the settings class itself. I think we might benefit from making a interface with Save and Load methods where these are implemented on the class itself as that would allow us to treat settings in a more generic way (such as here https://github.com/AdamsLair/duality/blob/master/Source/Editor/DualityEditor/DualityEditorApp.cs#L1012) and also clean up some code from the already big DualityApp and DualityEditorApp classes.

If we use the regular serializer in all cases, we don't need an API or interface for this, but we could still provide one to avoid requiring that knowledge. Maybe have an abstract SettingsContainer base class?

We could also use composition:

	public class SettingsContainer<TSettings>
		where TSettings : class, new()
	{
		public event EventHandler Changed;
		public TSettings Value { get; private set; }
		private readonly string path;

		public SettingsContainer(string path)
		{
			this.path = path;
		}

		public void Load()
		{
			this.Value = Serializer.TryReadObject<TSettings>(this.path) ?? new TSettings();
			Changed?.Invoke(this, EventArgs.Empty); //OnLoaded/OnLoading migth be a better name though
		}

		public void Save()
		{
			Serializer.WriteObject(this.Value, this.path, typeof(XmlSerializer));
		}
	}

The settings class itself will just be a POCO and be wrapped by this class.

• EditorUserData.xml seems to use a completely separate code flow for writing to the settings file in DualityEditorApp.SaveUserData(). The file itself also has 2 <?xml version="1.0" encoding="utf-8"?> which I have never seen before in a single file. Maybe some hack around dockpanel behavior?
• Other setting files are using the duality Serializer class to write to the files. They also use the .dat file extension instead of .xml

Yeah, let's switch this to use Duality serializers as well and, I don't know, put the DockPanel XML inside a string or byte[] field or something. Nobody is going to manually adjust this in XML form anyway.

That said, the custom XML approach currently has the advantage that every editor plugin can just read and write arbitrary custom stuff, so when we do switch to Duality serializers, we need to mirror this behavior in a different way.

One way would be to have an abstract SettingsSection base class with the class for EditorUserData storing a dictionary of them, and editor infrastructure asking each of the plugins for theirs (or null to skip). Essentially the same as it is now, only it's not the editor handing over an XElement, but the plugin handing over a SettingsSection. Would require every plugin with settings to have a class for them though, but maybe that's a good idea anyway?

I think thats a good idea to always have. Weakly typed settings are just a pain to deal with. Steering users this way is only a good thing.

Alternatively, the plugin classes could themselves be serialized, though that (a) requires the existing-instance thing to be implemented and (b) I'm not sure how I feel about the semantic breach we'd introduce by this. A separate class for containing a plugins settings that is serialized as a whole would be a clear separation.

I would go for the separate class approach because thats easier to do but also like you already pointed out separating these would ensure a clear separation of concerns.

Besides all of this, as briefly mentioned in #852, it could make sense to rename and / or regroup all existing Duality settings. It might make sense to do this separately of any internal API / serialization changes.

So right now we have:

• AppData.dat
• UserData.dat (and actually DefaultUserData.dat as well!)
• ProjectSettings.dat / EditorAppData.dat (depending on the outcome of Add project settings and move launcher path to it #852)
• EditorUserData.xml
• DesignTimeData.dat

I'd propose to rename them like this going forward, in order to provide better hints regarding their contents:

• SystemSettings.dat: Internal engine settings that are constant for this game, like rendering config, starting scene, physics settings, etc.
• GameOptions.dat: User-configurable settings of the game, like window size, audio volumes, etc.
• ProjectSettings.dat: Global project settings for editing purposes, like launcher path.
• EditorOptions.dat: User-specific settings of the editor, like window layout, panels configs, etc. - and I'd actually merge DesignTimeData.dat into this, since right now it only contains whether objects are hidden or locked, which is definitely a user-specific setting.

In this naming scheme, I've chosen "Options" to represent things users can adjust as they want, and "Settings" to represent something global, shared, that is not meant for everyone to adjust as they want. "System" also hints at internals not meant to be touched by players and suggests that it's the engine that is configured here, which is exactly what it does. "Project" sets the context of working on something and hints towards editor usage that way.

Or, in a different form:

Constant Choose your own
Game SystemSettings GameOptions
Editor ProjectSettings EditorOptions
Thoughts?

One problem with this naming I see is that its not immediately clear where these settings are used. Is ProjectSettings used for other purposes as well than just the editor? Same with SystemSettings maybe the editor might use that as well? Its not clear from the name.

Constant	Choose your own
GameSettings	GameOptions
EditorSettings	EditorOptions

This makes it clear which one is used by the game and which one is used by the editor and just feels more consistent.

EDIT: Worked out the settingscontainer idea into a draft PR #856

[ilexp]

We could also use composition:

Could you give an example what the advantage of this design would be? Because it seems more complex / harder to understand than just deriving from a base class.

One problem with this naming I see is that its not immediately clear where these settings are used. Is ProjectSettings used for other purposes as well than just the editor? Same with SystemSettings maybe the editor might use that as well? Its not clear from the name.

Constant Choose your own
GameSettings GameOptions
EditorSettings EditorOptions
This makes it clear which one is used by the game and which one is used by the editor and just feels more consistent.

The same thought crossed my mind, but now we have the problem that options and settings are used pretty much synonymously - and I couldn't give an exact definition on when to use each myself. I think we need something else.

How about GameProjectSettings and EditorProjectSettings? Or something along those lines?

[Rick-van-Dam]

We could also use composition:

Could you give an example what the advantage of this design would be? Because it seems more complex / harder to understand than just deriving from a base class.

Some advantages:

• Setting class itself is just a POCO.
• Allows us to move the change events to the settingcontainer instead of keeping them in DualityApp and DualityEditorApp.
• Can use this as a reference to a setting instance, no need to modify the serializer for that.

Composition is not that hard of a pattern to grasp and widely used. It really is nothing more than wrapping a class with a class (doesn't necessarily need to be a class though) actually as you can see in the draft PR I made. Duality actually already uses composition with for instance ContentRef.

One problem with this naming I see is that its not immediately clear where these settings are used. Is ProjectSettings used for other purposes as well than just the editor? Same with SystemSettings maybe the editor might use that as well? Its not clear from the name.
Constant Choose your own
GameSettings GameOptions
EditorSettings EditorOptions
This makes it clear which one is used by the game and which one is used by the editor and just feels more consistent.

The same thought crossed my mind, but now we have the problem that options and settings are used pretty much synonymously - and I couldn't give an exact definition on when to use each myself. I think we need something else.

How about GameProjectSettings and EditorProjectSettings? Or something along those lines?

But the term 'Project' really only makes sense in a editor context (which is probably the reason for your first proposal?). Shipping a GameProjectSettings with the published game (without the editor) to a enduser feels a bit weird.

Maybe we should just stick to the AppData/UserAppData naming scheme. I mean that is a quite standard way of naming this.

[ilexp]

Some advantages:

• Setting class itself is just a POCO.
• Allows us to move the change events to the settingcontainer instead of keeping them in DualityApp and DualityEditorApp.
• Can use this as a reference to a setting instance, no need to modify the serializer for that.

Hmm.. honestly, the first two arguments don't sound super convincing to me 😄 It's a design among others, but I don't see an inherent advantage there? But you got me with the third one. Since we can't deserialize into an existing settings instance, this is a spot where it could really help.

Composition is not that hard of a pattern to grasp and widely used. It really is nothing more than wrapping a class with a class (doesn't necessarily need to be a class though) actually as you can see in the draft PR I made. Duality actually already uses composition with for instance ContentRef.

Yeah, though you gotta admit, DualityAppData is just a lot simpler to understand than SettingsContainer<DualityAppData> - now I need to know about two types and their interaction and purposes, and I start to wonder when to wrap and when not to, and so on. Even if widely used, we pay with added complexity, so we should be sure we're getting something nice in return.

Anyway, you're solving an actual issue with this, so I'll stop with the design philosophy right here. Can still revisit this at some point later on when other alternatives present themselves.

Maybe we should just stick to the AppData/UserAppData naming scheme. I mean that is a quite standard way of naming this.

Alright, let's establish this as a baseline then:

• AppData.dat: Internal engine and app settings that are constant for this game, like rendering config, starting scene, physics settings, etc.
• UserData.dat: User-configurable settings of the game, like window size, audio volumes, etc.
• EditorAppData.dat: Global project and editor settings, like launcher path.
• EditorUserData.dat: User-specific settings of the editor, like window layout, panels configs, etc. -with DesignTimeData.dat merged into this, since right now it only contains whether objects are hidden or locked, which is definitely a user-specific setting.

	Constant	Choose your own
Game	AppData	UserData
Editor	EditorAppData	EditorUserData

...and we can just all keep in mind that if anyone comes up with something better, we can reconsider this later.

[Rick-van-Dam]

For usability reasons I do think we should stick with .xml extensions though. Editors know what to do with .xml while with .dat you don't get any highlighting making it harder to read.

[ilexp]

In that case, the code that serializes any of these settings should choose the XmlSerializer specifically via preferredSerializer, avoiding that any future or project-specific change will cause them to use a different one. Having binary (or something else entirely) in a .xml is not fun 😄