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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

New countries for speed limit designs #1260

Merged
merged 20 commits into from
Jan 2, 2022

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Dec 31, 2021

Fixes #1251

  • New speed limit themes for multiple countries
  • Priority signs and parking signs also included in the theme

I have checked Steam popularity per country, skipped countries which are very similar to German and existing British road signs, and figured out we can add 5 new country themes:

  • Sweden 馃嚫馃嚜 (because i live here alright, and because Paradox Interactive and community managers for C:S operate from here it might please them too 馃榾 )
    bild

  • Brazil 馃嚙馃嚪
    bild

  • France 馃嚝馃嚪
    bild
    bild

  • Indonesia 馃嚠馃嚛
    bild

  • South Korea 馃嚢馃嚪 (similar to German but more square font)
    bild

  • Japan
    bild

New priority and parking signs for existing themes:

  • Canada
    bild

  • Germany
    bild

  • UK
    bild

  • USA
    bild

@kvakvs kvakvs marked this pull request as draft December 31, 2021 08:36
@originalfoo
Copy link
Member

These look great!

I think the only other notable one would be Japan which has blue text on the speed limit signs.

@originalfoo originalfoo added Overlays Overlays, data vis, etc. Settings Road config, mod options, config xml SPEED LIMITS Feature: Speed limits UI User interface updates labels Dec 31, 2021
@originalfoo originalfoo added this to the 11.6.0 milestone Dec 31, 2021
@originalfoo
Copy link
Member

The icons are embedded in the .dll file? If so, they're all still getting loaded in to RAM along with the .dll?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Dec 31, 2021

The icons are embedded in the .dll file? If so, they're all still getting loaded in to RAM along with the .dll?

Yes I did not change the way textures are delivered into the resulting DLL, they are embedded but as compressed PNG, that's about 20kb per file. They are loaded on demand though.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Dec 31, 2021

Plus something tells me Windows loads DLL files using mmap. Should doublecheck if actual real memory is used.
https://stackoverflow.com/questions/43548270/are-embedded-resources-in-a-net-assembly-loaded-from-disk-or-from-memory-at-run

@krzychu124
Copy link
Member

I would be more concern if we instantiate textures -> they will eat VRAM, RAM is not really a problem

The icons are embedded in the .dll file? If so, they're all still getting loaded in to RAM along with the .dll?

Textures created from those images are worse, since they eat VRAM which you can't really extend without huge performance drop. I think we should compare master with this PR in terms of RAM difference (e.g. in noWorkshop mode), because now we can only guess. TMPE already takes probably somewhere ~200MB when loaded in game (dll of master has +- 6.5MB right now), so 10-15MB more won't make much of a difference.

@originalfoo
Copy link
Member

The new speed limits UI only has one set of textures for speed signs and overwrites those textures should user choose different speed sign icon theme (hence lag when switching between themes). At least that's what the code says, I've not tested to see if Unity is doing some weirdness under the hood.

@originalfoo
Copy link
Member

originalfoo commented Jan 1, 2022

EDIT: Apparently SACU no longer exists https://commons.wikimedia.org/wiki/User:Fry1989/Gallery/Road_Signs/SACU

Found another one that might be worthy to add due to it's extremely unusual colour - SACU (South African Customs Union) - SVGs of most of their signs available at link below:

https://commons.wikimedia.org/wiki/Category:SVG_road_signs_in_the_Southern_African_Customs_Union

Could potentially also provide custom Priority sign icons too:

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jan 1, 2022

South African signs look very much like a mix of German signs and orange Swedish-Finnish style backgrounds. Also the country is not represented in top 15 Steam users, so I assumed also not in top 15 C:S users
https://www.statista.com/statistics/826870/steam-distribution-country/

@kvakvs kvakvs marked this pull request as ready for review January 1, 2022 11:20
Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

Indonesia NoParking.png sign seems to be the old Parking.png (green parking allowed) sign?

image

I also noticed that, after changing theme via mod options, the parking overlays won't appear upon tool selection - until I move the camera (probably nothing to do with this PR and only very minor issue tbh).

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jan 1, 2022

All issues fixed now:

  • Indonesian no-parking added a special sign
  • Found a bug when parking sign was missing in the current theme, a parking-allowed (green P) was always selected
  • Camera cache reset forced when parking tool activates
  • Also converted all fonts to curves, to make it easier to do minor edits without installing a dozen of fonts

@originalfoo
Copy link
Member

Not for this PR, but would be cool if changing icon theme updated the button icons on tmpe toolbar. (Priority tool shows Yield.png icon, Speed tool shows 30.png, parking tool shows Parking.png)

@originalfoo
Copy link
Member

For visual simplicity, would it be worth removing the text box from under the French Yield.png?

image

It's unreadable in the overlays unless zoomed all the way in, and if we ever wanted to use the icon for the priority signs button on toolbar it would get in the way.

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

LGTM from in-game testing 馃憤

@kvakvs kvakvs force-pushed the feature/new-roadsign-themes branch from 0abfa8e to 34ccd71 Compare January 1, 2022 21:51
@krzychu124
Copy link
Member

Works great, but...

  1. Missing translation keys:
    image

  2. What about loading previously selected theme - backward compatibility? Not a big deal, but I didn't tested what will happen when loaded savegame with theme different than default (German)

  3. Why we are loading textures after loading translations? They are used in game so I'm not sure if is that necessary. I also added log call to Load and Unload methods and I see that we are calling load after translations (that was mentioned) but then again after loading savegame -> there is no Unload call before which suggests small memory leak, but we are changing game Scene between Main Menu to InGame so there is a chance that textured are destroyed similarly to gameobjects without DontDestroyOnLoad call

  4. Ok... there is a memory leak -> I don't see the code for unloading textures on mod disable

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jan 2, 2022

For memory leak my suggestion is to create one or more TextureManager's which will get lifecycle start level loaded event and load, and then will get lifecycle end level unloaded event and unload. In this PR or in separate.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jan 2, 2022

What about loading previously selected theme - backward compatibility? Not a big deal, but I didn't tested what will happen when loaded savegame with theme different than default (German)

Theme name is stored in the savegame. When config option contains theme name that is not known, it will reset to default Kmph theme (Kmph_Germany) or to default MPH theme (MPH_UK).

@krzychu124
Copy link
Member

For memory leak my suggestion is to create one or more TextureManager's which will get lifecycle start level loaded event and load, and then will get lifecycle end level unloaded event and unload. In this PR or in separate.

It can be done in separate PR, no problem. I'll approve this one, since it works pretty well.

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.

馃憤 memory leak issues can be fixed in the next PR

@kvakvs kvakvs merged commit 83a7ca3 into CitiesSkylinesMods:master Jan 2, 2022
@kvakvs kvakvs deleted the feature/new-roadsign-themes branch January 4, 2022 17:57
@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
Overlays Overlays, data vis, etc. Settings Road config, mod options, config xml SPEED LIMITS Feature: Speed limits UI User interface updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localised "give way" (yield) and "priority" signs for gb
3 participants