-
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
Refactor remainder of Policies
tab groups
#1455
Conversation
Policies
tab groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works in game without any noticeable issues 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questioning why Propagate*To
can fail
=> SteamHelper.IsDLCOwned(SteamHelper.DLC.NaturalDisastersDLC); | ||
|
||
internal static void AddUI(UIHelperBase tab) { | ||
if (!HasNaturalDisastersDLC) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea to show the group caption at all times, but then if DLC is not owned, add a text label explaining that a DLC is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm thinking of doing that in later iteration once I add some more UI components.
EvacBussesMayIgnoreRules.AddUI(group); | ||
} | ||
|
||
private static bool NaturalDisastersDlcValidator(bool desired, out bool result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this exists? Confusing function: updates some out
variable and returns true? Move the logic to the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a guard in central location at the moment. I'll probably change that in 11.6.5.2 branch but depends on how things go with converting Options
to instance-based.
private static UIDropDown _vehicleRestrictionsAggressionDropDown; | ||
|
||
static PoliciesTab_OnRoadsGroup() { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this can fail? It should never fail in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Krzychu made same comment recently. The reason I do it is because if anything goes wrong in a static constructor it causes cascading failure in C:SL which does not guard against that eventuality. That in turn results in user's game config file being wiped, all mods are then disabled, paradox account reset, etc.
Errors are most likely to creep in during dev and test phase - while they will obviously get fixed it's a complete PITA to have to re-enable 140 mods, espeically when some mods (Ambient Sounds Tuner in particular) need enabling last to prevent further settings loss. So the try..catch is really just to protect myself from my own stupid bugs during dev & test :)
@@ -121,14 +121,14 @@ public class RoundaboutMassEdit { | |||
} | |||
|
|||
private static void FixRulesRoundabout(ushort segmentId, bool startNode) { | |||
if (OptionsMassEditTab.RoundAboutQuickFix_PrioritySigns) { | |||
if (Options.RoundAboutQuickFix_PrioritySigns) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, but just as an interim measure (eg. as you mentioned in another PR, harmony could be used to remove the reflection bottleneck).
For now my main goal is just to get everything in options as consistent as possible. In next stage I'm seeking to make the options fields instance-based; currently they are static. Once instance-based, it will be appropriate to define defaults at the field level (currently defaults are being set in OptionsManager.LoadData()
as an interim workaround for static fields in Options.cs
), and there will be a whole bunch of other changes.
Good catch! Looking at Line 30 in 23ee4b2
new[] {
Translation.Options.Get("Gameplay.Dropdown.Option:Path Of Evil (10%)"), // 0
Translation.Options.Get("Gameplay.Dropdown.Option:Rush Hour (5%)"), // 1
Translation.Options.Get("Gameplay.Dropdown.Option:Minor Complaints (2%)"), // 2
Translation.Options.Get("Gameplay.Dropdown.Option:Holy City (0%)"), // 3
}, So yeah, should be
Another good catch! Should be off by default, will update in next commit. |
@kianzarrin Fixed bugs you spotted. |
@kvakvs Is there any changes you specifically want in this PR or can comments above be left until next phase of updates to options stuff? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the code diff earlier. Its fine
urg, 1 sec, will send PR to fix |
Yeah, that was causing constant trickle of support so it was changed in this PR. Can change it back to off by default if desired. @krzychu124 what do you think? |
Compiled mod for testing: TMPE.zip
Part of ongoing work on #1356 phase 2.
This completes phase 2 refactor of all mod options groups.
Closes: #62