-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Why do option tabs OWN the options. They shouldn't. #1372
Comments
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. |
The option fields in With the updates I'm working on to the options tabs, particularly conversion to using
|
Another issue with putting interwiring/validation in to the With the way it's currently working, particulary after transition to 100% |
@aubergine10 |
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. |
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 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.
Yes I noticed that. Shouldn't be too difficult. Is there an issue?
we can. but should we? maybe we should change the title of this issue: In any case now that I think about it, its not a requirement for XML migration. |
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. |
@kvakvs That would also work. All i am saying is that the |
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
We could just replace the bool fields in BTW, I think we should also be ensuring that managers do their own refresh/update in their If the default val gets added we could probably do with a We should also be thinking about stuff like:
But first, before any of that, let's get a UI component for dropdowns done then at least everything is consistent with the |
I have experience with generic drop downs. @aubergine10 Do you want me to do it? |
I created #1510 |
I don't fully understand this issue. @aubergine10 would you do the honors? |
There's quite a lot mentioned in this #1372 - which bit specifically needs elaboration? |
I don't understand this at all: #1372 (comment)
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 |
Ah, that's from #1372 (comment) 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:
EDIT: Oh, and then there's the global xml options which themselves are split across several classes.
If we further devleop the The group classes would still exist but would be limited to just a bunch of
I assume you meant to type If it were just primitive value fields and we wanted to define the defaults in 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
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. |
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 :) |
@kvakvs currently all TMPE code references the primitive values from fields in Only exception to that is some stuff on |
Originally posted by @kvakvs in #1370 (review)
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.The text was updated successfully, but these errors were encountered: