Skip to content
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

Road sign themes reloadable + Canadian signs #1215

Merged
merged 80 commits into from
Dec 31, 2021

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Dec 5, 2021

Features

NOTE: This is based on Speed Limits branch so the change set may look very large.
⚠ REVIEW SHORT DIFF AFTER SPEED LIMITS ARE MERGED

  • Road sign themes are configured in a shared list for both MPH and KMPH. Themes can be marked as supporting MPH and/or KMPH. Themes also have sign size/aspect stored in them in a nicer way.
  • Selecting a theme will unload previous theme and load new (save texture memory and faster loading time)
  • Selecting a theme without MPH support will disable the MPH checkbox
  • Selecting a theme without KM support will enable the MPH checkbox
  • Clicking MPH checkbox on a theme which doesn't support the new state, will reset theme to German_Kmph (the default).
  • Canadian signs contributed by a Discord user (Coreybpa)
  • Translations (option name and sign theme names)

TODO

  • Sorting currently by the translation key, which puts America in the end because the key is MPH_US. Consider sorting by translation maybe or leave like this?

bild
bild

-#if DEBUG guards for release
Config version bump
Reverted DebugIf to accept a lambda instead of string
Copied and adapted UIScaler logic from Kian's ModTools
SpeedLimits manager uses intent structs for set speed limit action
…cessible if action type is Unlimited or SetOverride
… apply speed limit changes, its for preview only
@@ -146,11 +146,12 @@ or ItemClass.SubService.PublicTransportMetro
laneIndex++;
}

if (validLanes > 0) {
meanSpeedLimit = meanSpeedLimit.Scale(1.0f / validLanes);
switch (validLanes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not switch case if two cases only? :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converted to return with validLanes == 0 ? null : ...

private static UIDropDown _roadSignsMphThemeDropdown;
private static int _roadSignMphStyleInt;

// [Obsolete("To be removed")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh .. plz remove :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing

@@ -338,24 +371,57 @@ public static class OptionsGeneralTab {
}
}

private static void OnRoadSignsMphThemeChanged(int newRoadSignStyle) {
// private static void OnRoadSignsMphThemeChanged(int newRoadSignStyle) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meh, plz remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing


private string PathPrefix;

public bool SupportsKmph;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these public fields that can be changed from the outside?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to readonly, they are accessed from General options tab.

mip: true);
TexturesKmph.Add(speedLimit, resource ? resource : TexturesKmph[5]);
}
public static RoadSignTheme ActiveTheme {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lazy loading getter suck .. but ok .. same same before.

mip: true);
TexturesMphUK.Add(speedLimit, resourceUk ? resourceUk : TexturesMphUK[5]);
}
public const string GERMAN_KM_SIGNS = "Kmph_Germany";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this public and the rest not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used as default fallback theme in the Options General tab

break;
}
}
// TODO: Split loading here into dynamic sections, static enforces everything to stay in this ctor
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why static ctor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class is static.
The solution would be to make it a regular class and access via SpeedLimitsTextures.Instance a bit longer and slower.

@krzychu124
Copy link
Member

plus missing translation string (keybinds tab)
image

@krzychu124
Copy link
Member

Oh, and I noticed one more bug: cannot change language in main menu

NullReferenceException: Object reference not set to an instance of an object
  at TrafficManager.State.OptionsGeneralTab.OnLanguageChanged (Int32 newLanguageIndex) [0x00000] in <filename unknown>:0 
  at UIHelper+<AddDropdown>c__AnonStorey2.<>m__0 (ColossalFramework.UI.UIComponent c, Int32 sel) [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIDropDown.OnSelectedIndexChanged () [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIDropDown.set_selectedIndex (Int32 value) [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIDropDown.PopupSelectedIndexChanged (ColossalFramework.UI.UIComponent component, Int32 si) [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIListBox.OnSelectedIndexChanged () [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIListBox.set_selectedIndex (Int32 value) [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIListBox.OnMouseDown (ColossalFramework.UI.UIMouseEventParameter p) [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIInput+MouseHandler.ProcessInput (IInputTranslator translator, Ray ray, ColossalFramework.UI.UIComponent component, Boolean retainFocusSetting) [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIInput.ProcessMouseInput () [0x00000] in <filename unknown>:0 
  at ColossalFramework.UI.UIInput.Update () [0x00000] in <filename unknown>:0 

@krzychu124 krzychu124 added this to the 11.6.0 milestone Dec 12, 2021
@originalfoo
Copy link
Member

Not sure if in scope of this PR (can move to the scope creep issue if preferred) but...

Still testing this; I'm finding the choice of speed unit vs. sign theme to be uncomfortable. IMO it would be better with a single drop down list.

In mod options:

Speed display:
- American (MPH)
- British (MPH)
- Canadian (km/h)
- German (km/h)
- German (MPH)

And on the speed palette, clicking the speed unit button could show the drop down list?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Dec 28, 2021

@aubergine10 Yes the drop down list is listed in your suggestions in #1221
The real world themes, except the German, support only one unit. It would be unnatural and confusing to see British signs showing km/h, so that's the only one theme doing it. I can make it repeat twice once per unit, though.

@originalfoo
Copy link
Member

I can make it repeat twice once per unit, though.

Yup, that would be fine so long as units are clearly shown in the list items.

@krzychu124 krzychu124 self-requested a review December 29, 2021 19:57
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fc66449 @kvakvs have you tested that? It still does not work because in the main menu there is no instance of ModUI 😉

Also this when switching between Km/h and Mph

NullReferenceException: Object reference not set to an instance of an object
  at TrafficManager.U.Panel.BaseUWindowPanel.OnDestroy () [0x00000] in <filename unknown>:0 

image
underlined field is null

@krzychu124 krzychu124 self-requested a review December 30, 2021 19:16
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still does not work as expected. I also found memory leak in SpeedLimitsToolWindow

3x switched between speed units:

  • green - latest, working panel,
  • red - garbage, all contain the same set of components,

image

TLM/TLM/State/OptionsTabs/OptionsGeneralTab.cs Outdated Show resolved Hide resolved
@krzychu124
Copy link
Member

I'm 99.9% sure you should destroy Window gameObject at line 141, not just Window component here

private void DestroyWindow() {
// Create a generic self-sizing window with padding of 4px.
if (this.Window != null) {
this.Window.Hide();
// The constructor of new window will try to delete it by name, but we can help it
UnityEngine.Object.Destroy(this.Window);
}
this.Window = null;
}

@krzychu124 krzychu124 self-requested a review December 31, 2021 00:48
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally! XD

@kvakvs kvakvs merged commit c5f8c1e into CitiesSkylinesMods:master Dec 31, 2021
@krzychu124 krzychu124 mentioned this pull request Jan 8, 2022
@originalfoo originalfoo modified the milestones: 11.6.0, 11.6.2 Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing feature SPEED LIMITS Feature: Speed limits UI User interface updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants