Why do option tabs OWN the options. They shouldn't.

[originalfoo]

Originally posted by @kvakvs in https://github.com/CitiesSkylinesMods/TMPE/pull/1370#pullrequestreview-874101969

The big question for a separate PR: Why do option tabs OWN the options. They shouldn't.

This is a good point. Additionally all code should reference options the same way - currently some code refers to fields in Options.cs while other code is querying checkboxes and other stuff in the various tabs files.

[kvakvs]

Group together the options and their setters/getters and allow hooking the listeners which need to observe value changes. I'd say no need to create a dynamic subscription event system, a single location in a setter for each option is enough, just to add callback calls as necessary.

[originalfoo]

The option fields in Options.cs are used in multiple hotpaths. Fields are faster than properties, so IMO there is actually merit to having the interwiring of options in the tabs.

With the updates I'm working on to the options tabs, particularly conversion to using CheckboxOption with its new properties (notably PropagatesTrueTo and its reciprocal PropagatesFalseTo properties) the interwiring between options is significantly cleaner and there's also a huge reduction (~90%) in number of On...Changed and Set... functions.

• Options.cs - read values from fast access fields
• Tabs - set values via UI components (which deals with all the interwiring of options neatly)

[originalfoo]

Another issue with putting interwiring/validation in to the Options.cs is that it will be disconnected from the mod options UI and thus require a load of extra infrastructure to keep UI in sync with option values. For example, if enabling an option also enables another option, and both checkboxes need updating, that would result in lots of crufty code if we tried plumbing that in to getter/setters in Options.cs.

With the way it's currently working, particulary after transition to 100% CheckboxOption for bool options, we're essentially putting the getter/setters in to the UI - unconventional, and probably breaks some design principles, but it's working really well so...

[kianzarrin]

@aubergine10 Can we get rid of the option fields in Options.cs and add a TVal _Value field in SerializableUIOptionBase?

[kianzarrin]

Then using XML for persistency it would be easier to choose which one's are for in-game (but also can be set for new games) and which one's are for global only.

[originalfoo]

Yes, that is straightforward to do. Still need to get drop-downs updated to similar mode of operation though. Also, there's some fields/methods in Options.cs that aren't covered by the options UI components.

How will defaults be set, for example if user opens asset editor then, in same app session, starts a new game to test their asset?

[kianzarrin]

How will defaults be set, for example if user opens asset editor then, in same app session, starts a new game to test their asset?

Yeah I was thinking about that too. Ill create issue. I think it should be all in one place. I don't think it should be duplicated in legacy and the new code.

Still need to get drop-downs updated to similar mode of operation though

Yes I noticed that. Shouldn't be too difficult. Is there an issue?

Yes, that is straightforward to do.

we can. but should we? maybe we should change the title of this issue: Why do option tabs OWN the options. They shouldn't. option values should be stored in option tabs.

In any case now that I think about it, its not a requirement for XML migration.

[kvakvs]

we can. but should we? maybe we should change the title of this issue: Why do option tabs OWN the options. They shouldn't. option values should be stored in option tabs.

There should be a separate Settings or Options or some other named container class, which will own options definitions and options values. Options tabs are UI, UI should not own data, UI displays the data and owns only its own UI state.

[kianzarrin]

@kvakvs That would also work. All i am saying is that the CheckboxOption fields should not only contain the UI but also should contain the value and default-value. All in one place. The fields could then be accessed from the tabs.

[originalfoo]

Still need to get drop-downs updated to similar mode of operation though

Yes I noticed that. Shouldn't be too difficult. Is there an issue?

I just got delayed with covid and then my knee started playing up again so my daily routine is looking like this rn:

The main thing I still haven't tackled with the drop-downs is that they are often different types (eg. enum, byte, etc). If you have any ideas on clean way to implement the DropDownOption class lmk :)

There should be a separate Settings or Options or some other named container class, which will own options definitions and options values. Options tabs are UI, UI should not own data, UI displays the data and owns only its own UI state.

We could just replace the bool fields in Options.cs with the definitions of the CheckboxOption etc. (this assuming #1508 is done first). Issue there is we might end up with a mass of "On Changed" event handlers in that file.

BTW, I think we should also be ensuring that managers do their own refresh/update in their OnAfterLoadData() - during deserialization when a game loads it's a bit weird for options to be prodding managers during deserialization process (ie. in options manager LoadData()). It just feels weird the way it works rn. Like, managers should probably not be starting up until after deserialization otherwise they're going to start with one set of values then need to refresh to any updated values.... just feels icky.

If the default val gets added we could probably do with a .Reset() method (reset to default value) that can be called when entering asset editor or game. Ideally rather than resetting each option individually there should be a centralised way to do it (ie. centralised .Reset() method)?

We should also be thinking about stuff like:

• Reset to default in-game
• Setting gloabl defaults from main menu
• Ability to save in-game settings as global defaults

But first, before any of that, let's get a UI component for dropdowns done then at least everything is consistent with the SerializableUIOptionBase.

[kianzarrin]

I have experience with generic drop downs. @aubergine10 Do you want me to do it? Is there an issue tracking drop downs?

[kianzarrin]

I created #1510

[kianzarrin]

I don't fully understand this issue. @aubergine10 would you do the honors?

[originalfoo]

There's quite a lot mentioned in this #1372 - which bit specifically needs elaboration?

[kianzarrin]

@aubergine10 There's quite a lot mentioned in this https://github.com/CitiesSkylinesMods/TMPE/issues/1372 - which bit specifically needs elaboration?

I don't understand this at all: https://github.com/CitiesSkylinesMods/TMPE/issues/1372#issuecomment-1030959090

There should be a separate Settings or Options or some other named container class, which will own options definitions and options values. Options tabs are UI, UI should not own data, UI displays the data and owns only its own UI state.

The fields defined in option tabs only handle the UI. The option values are all actually stored in Options.cs . except for the global ones. Why is it OK for global options to be in different places but not for save-game options ?

---

So what do we actually want to do? put a _value field in SerializableUIOptionBase (#1508) and then put them all in one class ? are the fields going to be instant or static? what about the global options?

[originalfoo]

There should be a separate Settings or Options or some other named container class, which will own options definitions and options values. Options tabs are UI, UI should not own data, UI displays the data and owns only its own UI state.

Ah, that's from https://github.com/CitiesSkylinesMods/TMPE/issues/1372#issuecomment-1086983794

I think @kvakvs was pointing out that the options components (checkboxes, etc) are defining a bunch of stuff about the options that are not purely UI-based, notably their event handlers.

We've currently got stuff spread across three main areas:

1. OptionsManager.cs - in addition to load/save, it's currently handling the default values too (which is a necessary short-term measure).
2. Options.cs - the fields that the rest of TM:PE refers to when it wants to know an option value.
3. {Some}Tab_{Some}Group.cs files - the components, event handlers, and AddUI() stuff.

EDIT: Oh, and then there's the global xml options which themselves are split across several classes.

So what do we actually want to do? put a _value field in SerializableUIOptionBase (https://github.com/CitiesSkylinesMods/TMPE/issues/1508) and then put them all in one class ?

If we further devleop the SerializableUIOptionBase to hold default and current value for options (#1508) then we could theoretically move most of the component definitions from the group classes to the Options.cs, assuming the encapsulation/abstraction of those values isn't going to cause performance issues with hotpath code that needs to know an option value.

The group classes would still exist but would be limited to just a bunch of .AddUI() calls.

are the fields going to be instant or static?

I assume you meant to type instance not instant?

If it were just primitive value fields and we wanted to define the defaults in Options.cs then I'd say yeah, probably make it instance-based so it's easy to fire up a new "default" set of options when entering asset editor or loading game etc. Would also make it easy to "reset to default" (as in the hard-coded defaults in the Options.cs).

However, if we put the UI component definitions in there then I suspect it would be cumbersome and even somewhat pointless to have it instance based; we could just add a .Reset() feature to SerializableUIOptionBase maybe?

what about the global options?

I don't think we'd want to start defining UI components in the global option classes (of which there are several) - it would be weird to have like 5 global options being different to all the others simply because they are exposed to mod options UI.

[kvakvs]

I did not check the recent state of the code, where stuff is moved recently. But all I wanted to say is that the option variables should not be in the UI classes, and the game code should not be accessing the UI files to know the option values. If this is already changing or changed, disregard my comment and keep up the good work :)

[originalfoo]

@kvakvs currently all TMPE code references the primitive values from fields in Options.cs.

Only exception to that is some stuff on OptionsManager.cs which interacts with the UI components (as the UI components currently manage the value change handlers).