TMPE icon indicating copy to clipboard operation
TMPE copied to clipboard

Use XML to save/load options.

Open kianzarrin opened this issue 5 years ago • 12 comments

I think it would be more organised to save/load data not by index but rather by dictionary keys (eg use XML). We should also have a class for backward compatibility to load from old saves.

Also It would be nice if we add a button to 'use current values for new games' like the real time mode (#363): Screenshot (87)

kianzarrin avatar Nov 22 '19 04:11 kianzarrin

The fact that there is a lot of repeated code around is not helping the cause. Encapsulation can reduce a lot of such redundancies.

kianzarrin avatar Nov 22 '19 04:11 kianzarrin

It would be nice if we add a button to 'use current values for new games' like the real time mode

Yes, I would love this, and it's something users have asked for many times over the years.

The fact that there is a lot of repeated code around is not helping the cause. Encapsulation can reduce a lot of such redundancies.

If we adopt something similar to what Real Time does, it has a central method that deals with config version compatibility so it's really clear what changes from version to version.

I guess the big chore will be making it backwards compatible (ie. load old versions of TM:PE settings that would be completely different format, and there are several old versions of settings).

@kvakvs recently split the mod options in to separate files for the various tabs as part of multiple massive code clean-ups, so he probably knows most about the current system if you need infos on that.

originalfoo avatar Nov 22 '19 05:11 originalfoo

@aubergine10

@kvakvs recently split the mod options in to separate files for the various tabs as part of multiple massive code clean-ups, so he probably knows most about the current system if you need infos on that.

is there any issue regarding this. I didn't manage to find any. I'd like to @kvakvs code.

EDIT: Wait are you taking about the code that is already in master/origin ?

kianzarrin avatar Nov 23 '19 01:11 kianzarrin

Yes, the code that is already in master branch is the latest which includes the huge cleanup that kvakvs did some months ago.

I was meaning that if you have any questions on any of the config stuff, kvakvs is probably best person to ask as he had to pick through a lot of that stuff during the code cleanup.

originalfoo avatar Nov 23 '19 05:11 originalfoo

@aubergine10 lets move the discussion #1267 to here:

Example: Go look at GlobalConfig.Instance.Main.ScanForKnownIncompatibleModsAtStartup vs. Options.scanForKnownIncompatibleModsEnabled and how they both interact.

that sounds unreasonable. Is this a bug?

Maybe we should move them to options csv file for consistency. First I'd need to know why it's done that way currently.

Providing support for alternate translation is no big deal. I can give the option for user to enter the translated label or provide translation delegate. but first we need to see if we really need it.

Notable example is the GlobalConfig.Instance.Main.DisplaySpeedLimitsMph option which has locale Translation.SpeedLimits.Get("Checkbox:Display speed limits mph").

@krzychu124 @kvakvs @DaEgi01 Do you think we should put all option translations under options.csv or do you think some options should be under speed limit csv?

kianzarrin avatar Jan 07 '22 14:01 kianzarrin

@aubergine10 some stuff about the CheckboxOption that you might be already aware of but I say it anyway just in case:

  • you can add your own events to run after the default event. eg: Handler = UpdateDedicatedTurningLanePolicyHandler,
    • you can also remove drop the default handler AFTER instantiation if you want (but I don't think if there is any use case.)
  • this is TRANSITIONAL code. ultimately I want to drop the Options class and use the SerializableUIOptionBase as single source of truth. I want to use xml both for in game stuff and global stuff. (basically what this issue says we should do)

As for global VS in-game: some options are global only. In future if we change in game options from main menu it should be used as basis for new game.

kianzarrin avatar Jan 07 '22 14:01 kianzarrin

@krzychu124 @kvakvs @DaEgi01 Do you think we should put all option translations under options.csv or do you think some options should be under speed limit csv?

The speed limits option was used in both Options tab, and in Speed limits UI. This might be not true anymore. But the hassle of moving string with all translations... doing that via CSV editor instead of Crowdin...

If its still used in both places the only solution that would fix your concern, would be duplicating string in both files. If its now only used in options, then you can safely move it, no problem.

kvakvs avatar Jan 07 '22 14:01 kvakvs

. But the hassle of moving string with all translations... doing that via CSV editor instead of Crowdin...

is it really that hard?

I mean if there is a valid reason for this then that is OK but if this is just a bug then I think its better to solve it rather than to find work around.

kianzarrin avatar Jan 07 '22 16:01 kianzarrin

IMO, all translations relating to options should be in Options.csv - but we can sort that out as subsequent task.

I'm currently working on porting all the options to the new UI format (eg. CheckboxOption) and will send that in as subsequent PR after #1267.

For persistency, there's 3 tangible situations:

  • None (eg. debug options which could be set in-game via a UI)
  • Global-only (eg. Language setting)
  • Global and Savegame (most settings)

(A fourth would be Savegame-only but I can't see any situation where that would be required)

Tagging: #1268 , #1262 , #1240 , #813 , #689 , #659 , #363 , #281 , #190 , #69 , #62

originalfoo avatar Jan 07 '22 23:01 originalfoo

So, I've been chipping away at a mock of how things might look if we approched settings the same way as Real Time mod, and I don't think that's going to work for us:

  1. Many of our settings need to invoke code when changed; the attribute-driven UI approach isn't particularly good for that
    • I know we can use a naming convention to automagically invoke change handlers, but that's still icky imo
  2. We have vastly more settings - not just those shown on options tab, but also all the little microadjustment settings in global config, and the debug settings too
  3. We could potentially end up with 3 settings screens:
    • Mod options
    • Advanced stuff - a UI for micro settings in global config
    • Debug - eg. setting debug switches via in-game UI without need to fiddle with config xml
  4. We have some settings that don't persist (debug stuff), some that should always be global (eg. language), some that should be game/global depending on where user changes them

My suggestion would be an attribute that specifies persistence type (no attribute = no persistance), for example:

[OptionPersistance(Persist.GlobalAndSavegame)]
public bool someSetting { get; set; }

If UI updates that option via in-game pause menu, the change only gets stored in save game on next save / autosave. If they change it via main menu, it would immediately persist to global config.

If the setting were Persist.GlobalOnly then whenever it changes (via UI) it should immediately persist to global config, regardless of whether user is in-game or not.

Persistance would generally be triggered by UI which handles the change. The UI should be separate, eg. each tab in mod options has it's own file using the new UI stuff such as CheckboxOption.

Does that match more or less what you were thinking?

originalfoo avatar Jan 09 '22 00:01 originalfoo

I don't think our code should look like real-time mod. just that we should have the functionality to set options for new game.

kianzarrin avatar Jan 09 '22 01:01 kianzarrin

Yup, like I said in previous comment, I don't think the Real Time approach will work for us.

originalfoo avatar Jan 09 '22 01:01 originalfoo