Refactor/rethink how duality handles settings

[Barsonax]

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?

[Barsonax]

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?

[Barsonax]

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.

[Barsonax]

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 😄

[ilexp]

Note: As far as I can see from #856, the file extension change to .xml is done only for the new EditorAppData.xml, while the others still use .dat.

@Barsonax Depending on your plans for that, this could either be done as part of #860, or with a separate PR. Where are we on this overall, and where can others (including me) step in and contribute towards the implementation of this issue?

[Barsonax]

Extensions are currently changed to xml in #860 actually. Didn't changed them all in #856 due to conflicts.

The PR is still WIP though. Will probably split it up since it has grown quite a bit.

[ilexp]

Progress

• Updated the core to use Duality Serializers and settings container object for all user and app data via Abstract away settings logic using composition #856
• Updated the editor to use Duality Serializers and settings container objects for all user and app data via Use duality serializer for editoruserdata #865

ToDo

• Port a very simple editor plugin to use the new user data handling to ensure the new API is complete and usable, extend and tweak as needed. LogView is probably a good candidate: updated logviewsettings #868
• Port the existing editor plugin user data code to use the new Duality Serializers / settings container based API.
  • Make sure to use sensible defaults in the settings objects, reflecting the formerly embedded EditorUserData.xml file, which no longer exists.
  • Updated editorbase settings #866, update camview settings #867, Updated tilemaps settings #869, update sceneViewsettings #870, updated object inspector settings #871, updated projectviewsettings #872
• Consider merging PluginSettings class into DualityEditorUserData directly, since it's essentially just a wrapper around a List<object> with a utility Get (or create) method, and we do pay for it with a little readability.
• Remove legacy API for user data serialization.
  • EditorPlugin Load / Save methods accepting XElement, as well as GetDefaultUserData method.
  • EditorPluginManager legacy support code blocks (would turn up as compiler errors anyway).
  • PluginSettings field and property for old-style data

[ilexp]

Worked on the API a bit while looking into #868 and I think we should keep in mind a slight design trap we might fall into:

• Even though the EditorPlugin API still suggests some sort of transformation / serialization in Load / Save, we should treat the new settings objects for each plugin as first-class storage where possible, avoiding the need for field assignments entirely in some cases.
• UI still needs their update after load, but that should be it.
• State retrieval from UI should be avoided for saving, since doing it only on save means that our first-class data object is outdated all the time until then. Instead, state retrieval from the UI into the settings object needs to happen directly when detecting the UI change.
• Settings access in plugins should always use the settings object directly, not some UI state.

[Barsonax]

Worked on the API a bit while looking into #868 and I think we should keep in mind a slight design trap we might fall into:

• Even though the EditorPlugin API still suggests some sort of transformation / serialization in Load / Save, we should treat the new settings objects for each plugin as first-class storage where possible, avoiding the need for field assignments entirely in some cases.
• UI still needs their update after load, but that should be it.
• State retrieval from UI should be avoided for saving, since doing it only on save means that our first-class data object is outdated all the time until then. Instead, state retrieval from the UI into the settings object needs to happen directly when detecting the UI change.
• Settings access in plugins should always use the settings object directly, not some UI state.

Agree this will make things cleaner with less steps in between. I just didn't went that far for a first itteration.

[ilexp]

Fixed some behavior that was broken during the SettingsContainer refactoring in commits ea5acb3 and 1531037. Unfortunately, this invalidates some of the benefits we got through the SettingsContainer abstraction:

• The "save-on-change" code now needs to check specific instances again, since there is no way to know which container to save based on just a changed instance.
• To re-establish the special case behavior for UserData.xml and DefaultUserData.xml there are now essentially two custom delegates overwriting load and save behavior, similar to how a virtual method overload would.

Both of them can be simplified away in an "abstract settings base class" scenario as opposed to a container design, which we didn't implement before due to the serializers inability to update an existing instance. However, I think we should reconsider this as soon as #877 is addressed.

Edit: Actually, #877 has a workaround that could be applied right now - could be enough to do a quick base class design prototype. Will see if / when I get around to it.

[ilexp]

Progress

• Fixed editor settings menus selecting the container object instead of the settings instances directly (see comment above).
• Fixed Default/UserData.xml behavior in core and editor (see comment above).
• Ported LogViewPlugin settings to the new model (updated logviewsettings #868) and established "settings object ground truth" design for this and following port PRs (see three comments above).
• Ported EditorBasePlugin settings to the new model (Updated editorbase settings #866)
• Ported ObjectInspectorPlugin settings to the new model (updated object inspector settings #871)

ToDo

• Port ProjectViewPlugin settings to the new model (updated projectviewsettings #872)
• Port TilemapsPlugin settings to the new model (Updated tilemaps settings #869)
• Consider revisiting CamViewPlugin settings and porting it to the new model as well instead of relying on legacy settings (revisit update camview settings #867)
• Consider merging PluginSettings class into DualityEditorUserData directly, since it's essentially just a wrapper around a List<object> with a utility Get (or create) method, and we do pay for it with a little readability.
• Consider removing SettingsContainer<T> design and replacing it with a DualitySettings base class.
  • Move change events, path info and load / save methods to base class.
  • Allows to get rid of all .Instance accesses in the codebase and re-generalize the "save-on-change" code in the editor.
  • Update existing instance on load via workaround mentioned in Duality Serializers should support updating an existing instance when loading #877.
• Remove legacy API for user data serialization (when made possible by porting all plugins)
  • EditorPlugin Load / Save methods accepting XElement, as well as GetDefaultUserData method.
  • EditorPluginManager legacy support code blocks (would turn up as compiler errors anyway).
  • PluginSettings field and property for old-style data
• Reconsider settings change handling in the editor.
  • Changes should not be saved in every single PropertyChanged event, but in the next "save all" action.
  • Changes should probably (?) be applied either after every PropertyChanged event, because it would be inconsistent if the value is already there, but no system relying on it got a chance to update, so only those not needing one will use the current value.
    • Need to be careful as to where Apply might trigger a lot of work. It's probably fine, but should be tested thoroughly, with a game in the sandbox and a real project to test. Maybe the DualStickSpaceShooter sample.
• The now-merged DesignTimeObjectDataManager should probably use its custom serialization to store its data in a byte[] to avoid giant EditorUserData XMLs when lots of objects are hidden or locked. As far as I know, the custom per-object user data was never used, ever, and could be removed for now to greatly simplify this.

[CodingMadness]

Progress

• Fixed editor settings menus selecting the container object instead of the settings instances directly (see comment above).
• Fixed Default/UserData.xml behavior in core and editor (see comment above).
• Ported LogViewPlugin settings to the new model (updated logviewsettings #868) and established "settings object ground truth" design for this and following port PRs (see three comments above).
• Ported EditorBasePlugin settings to the new model (Updated editorbase settings #866)
• Ported ObjectInspectorPlugin settings to the new model (updated object inspector settings #871)

ToDo

• Port ProjectViewPlugin settings to the new model (updated projectviewsettings #872)

• Port TilemapsPlugin settings to the new model (Updated tilemaps settings #869)

• Consider revisiting CamViewPlugin settings and porting it to the new model as well instead of relying on legacy settings (revisit update camview settings #867)

• Consider merging PluginSettings class into DualityEditorUserData directly, since it's essentially just a wrapper around a List<object> with a utility Get (or create) method, and we do pay for it with a little readability.

• Consider removing SettingsContainer<T> design and replacing it with a DualitySettings base class.

  • Move change events, path info and load / save methods to base class.
  • Allows to get rid of all .Instance accesses in the codebase and re-generalize the "save-on-change" code in the editor.
  • Update existing instance on load via workaround mentioned in Duality Serializers should support updating an existing instance when loading #877.

• Remove legacy API for user data serialization (when made possible by porting all plugins)

  • EditorPlugin Load / Save methods accepting XElement, as well as GetDefaultUserData method.
  • EditorPluginManager legacy support code blocks (would turn up as compiler errors anyway).
  • PluginSettings field and property for old-style data

• Reconsider settings change handling in the editor.

  • Changes should not be saved in every single PropertyChanged event, but in the next "save all" action.

  • Changes should probably (?) be applied either after every PropertyChanged event, because it would be inconsistent if the value is already there, but no system relying on it got a chance to update, so only those not needing one will use the current value.

    • Need to be careful as to where Apply might trigger a lot of work. It's probably fine, but should be tested thoroughly, with a game in the sandbox and a real project to test. Maybe the DualStickSpaceShooter sample.

• The now-merged DesignTimeObjectDataManager should probably use its custom serialization to store its data in a byte[] to avoid giant EditorUserData XMLs when lots of objects are hidden or locked. As far as I know, the custom per-object user data was never used, ever, and could be removed for now to greatly simplify this.

Hey, im interessted how far you guys pushed Duality to bring it closer to 4.0 version, Well done, how far did you come with the settings change tho?

[ilexp]

I'm currently more or less in hibernation mode due to daily workload in non-OSS domains, but current progress and ToDo as documented here should be up-to-date. @Barsonax might know more, in case there are more recent changes, if any.