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

Add Speed Limits for Miles per Hour #384

Merged
merged 31 commits into from
Jun 23, 2019

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Jun 16, 2019

  • Speed limits MPH/kmph option added
  • Default speed limits window also updated to show the current unit
  • Changed internal mod speed representation from index in speed limits table to game speed float. Speed limits table is removed and replaced with simple range 5..90+5 for miles and 10..140+10 for km/h
  • Extra textures added for GUI and for map display: German with step 5, British and US square
  • Theme selector added for MPH signs: German, British or square US.
  • MPH checkbox is also available in the speed limit tool window.

New look for MPH palette
image

new look for KM/H palette
image

Setting:
image

Default speed limits window:
image

Fixes #13

@kvakvs kvakvs changed the title MPH - km/h option added with new textures #13 MPH - km/h option added with new textures Jun 16, 2019
@originalfoo
Copy link
Member

The 12 mph one is weird, and 5 mph is unlikely to be used (humans walk at roughly 3-5 mph for comparison).

In #13, @krzychu124 stated:

In game speed is represented by range from 0.1 to 2.0, so there is no km/h nor mph.
It seems to me that max speed in mph would be 85.

And I deduced that the desirable list of speeds would be:

10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60, 65, 70, 75, 80, 85

Which is 3 more than we have room for on the current UI panel, unless we add another column and drop the 85 mph speed.

I've never checked how that UI panel works, but assume that each button (except the 'no limit' one) represents a number between 0.1 and 2.0.

It would be good if we could get the speeds 10 --> 80 mph as listed above.

And for the speed labels below the icons (eg. for mph icons, that would be the km/h labels) we could probably round to the nearest 5 (so 12 would be rounded to 10, 13 would be rounded to 15, etc) just to make it more readable?

Also, the panel heading could be changed from "Speed limits" to, depending on mod setting, "Speed limits (mph)" or "Speed limits (km/h)".

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

For simplicity I've kept it as a 1:1 mapping. This is a display change, not how the speed limits are chosen and handled internally. So adding more choices for mph will mean that if user switches back to km/h we're in trouble (and the complexity also was mentioned in that issue).

Doing it the way you suggest will require some code to add those buttons. And will have to figure a nice way to handle user switching back to km/h once some new speed limits have been applied which don't exist in km/h palette. Anyway i need to think more tomorrow.

Rounding 20km/h to 10mph I thought it would be a terrible approximation. But no problem, will do.

@FireController1847 FireController1847 changed the title #13 MPH - km/h option added with new textures Add Speed Limits for Miles per Hour Jun 17, 2019
@FireController1847
Copy link
Collaborator

FireController1847 commented Jun 17, 2019

I actually disagree with aubergine here. I think the MPH representation when in KPH mode should show the exact representation, not a rounded version. Same for the other way around. This allows people to get a better understanding of what the actual KPH to MPH translation is, helping people make a better decision on what one to use. Like you said, rounding 20 km/h to 10mph is a terrible approximation.

Overall I actually like the way you did it. I disagree with adding the "12 MPH" option, though. In either mode, they should increment by 5 (like aubergine said) starting at 10 and ending at 80. This means that yes, technically they would be different speeds upon switch, but that is the desirable effect.

Another desirable thing would be changing the speed limit icon to be more... American? For the MPH options? Because I don't ever seen any MPHs shown in a red circle, but I see them shown in the white rectangular one.

@originalfoo
Copy link
Member

Yup, that was the issue we ran in to - how to translate back and fourth between the speeds. That conundrum needs resolving IMO.

@originalfoo
Copy link
Member

We could perhaps have something that snaps the speed display (on the roads) to the nearest 'standard speed' (ie. thing we have speed sign for).

So we've got this range, 0.1 .. 2.0, and the road speeds could be any value along that scale, but when we display the speed icons on the roads we just snap to the nearest valid speed. Like, if it's 3 km/h difference, the user doesn't really care and can't really notice just by looking at the movement of vehicles.

@FireController1847
Copy link
Collaborator

What if instead of trying to translate between the speeds constantly, the internal speed would be stored as KM/H, and just display MPH. That means we use MPH for the icons, and just set their value in the code as their exact KM/H counterpart. Maybe I'm just not understanding the complexity of this, but this seems like the much simpler option. No having to translate between the speeds.

Then, since we know their exact representations, when we translate between the modes, we loop through all custom speeds determine which ones don't add up. What I mean by this is let's say the user was in KM/H mode at first, and set a road to 15. Now they switched to MPH. We find all the roads set to 15 KM/H and change them to be 24.1 KM/H (this is the 15 MPH equivalent). This works the other way around. No having to store any more data, just using the data already represented by the game.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

The speed has to map to the sign really fast, i.e. binary search is OK maybe, but nothing slower. I can do that i think. Different palettes and mapping to whatever signs we have for that unit system.

Currently indeed it is handled by TM:PE as kmph (i.e. i've no idea i did not touch that code) and display code is picking up a direct km -> mph translation. If it will differ, the code has to be fixed.

@FireController1847 FireController1847 added discussion Contains debate on certain topics enhancement Improve existing feature Usability Make mod easier to use LABS TM:PE LABS branch STABLE TM:PE STABLE branch labels Jun 17, 2019
@CosignCosine
Copy link

I really think this is being overthought; if it ain't broke, don't fix it.

MPH, in my opinion, if implemented at all, should be a tooltip. When you hover over 20 km/h, you should see a tooltip that comes up that says "12 mph".

e.g.:
icon.tooltip = Math.Round(value * 0.62) + " mph";

This:

  • reduces overhead (no extra icons and no crazy mapping formula code)
  • keeps the correlation between mph and km/h relatively exact
  • makes more sense to non-american users, who, as far as I have seen, are the majority of consumers.

@FireController1847
Copy link
Collaborator

@CosignCosine However, there is one issue I'd like resolved. By going by 10s in KM/H, it skips important speed limits in MPH like the speeds of 30 and 45 (there's really no good option for these two). We should add full implementation for MPH, as it's quite rude to straight up ignore a standard that at least 2 major countries use and 2 moderately sized countries use.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

Ok let's do a take two.
I will add missing German signs and rework the palette to include 10..130 km/h with 10 km/h step, and 5..80 mph (maybe 85?) with 5 mph step. And then see how amazing or how bad it looks.

I also had an idea to do sign style selections per country, with major styles (circle and US square https://images.megapixl.com/320/3207832.jpg ). This should be another PR later.

@originalfoo
Copy link
Member

originalfoo commented Jun 17, 2019

Back in 2015 I created some sign icons for T++

835e90a6-fa97-11e4-933d-6e907c1cab46

They were designed for use on a build bar:

02a3faaa-fe1b-11e4-96e9-45dc900b76c9

From memory the font used was Road Geek: https://github.com/sammdot/roadgeek-fonts

Now that there are mods like "Resize It" I think we could consider going back to build bar approach.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

@aubergine10
VC2 are Scandinavian orange signs, from the font looks like Swedish
what is VC1? the current default?
Btw I am personally very happy with current German style signs (no shading, flat white), just it seems like when switching to mph square signs would really please US players, while others won't make a big deal and will be fine with round signs.

I'd love to do this kind of styling job in general, for example as a selectable road theme pack in another PR. Just try to not grow the scope to the point when it becomes really hard to finish.

@originalfoo
Copy link
Member

VC2 are Scandinavian orange signs, from the font looks like Swedish
what is VC1? the current default?

I can't even remember what VC was shorthand for. I just did some trawling of wikipedia and other online resources to work out what the most common sign designs were.

Just try to not grow the scope to the point when it becomes really hard to finish.

Agreed, just keep with current design style only for now.

We've been discussing ideas around regional profiles for TM:PE, for example different traffic rulesets, icons, etc., allowing people to choose a region and have the game work more like that region. But that stuff needs a load of extra thought and planning so needs to ideally be treated separately.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

New look for MPH palette, new look for KM/H palette — updated top comment

140 is probably not necessary and can be trimmed.
This refactoring is bigger than i expected, have fun reviewing!

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 17, 2019

One thing remaining to investigate, is that we're now using float instead of short. There might be a data size limit affected by the changes to TrafficManager.Configuration.LaneSpeedLimit...
@VictorPhilipp
Will the savefiles be compatible with this field changed? Do we have to play trickery with detecting short value (old save)? And to reduce the save size?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 18, 2019

The 12 mph one is weird, and 5 mph is unlikely to be used (humans walk at roughly 3-5 mph for comparison).

5 mph is pedestrian priority zone. Road rules for km/h countries state that pedestrian zone speed limit is 7 km/h, 5 km/h in Spain on pedestrian lanes. I assume this might be wanted by some player :)

Co-Authored-By: Krzysztof <krzychu124@gmail.com>
kvakvs and others added 2 commits June 21, 2019 16:45
@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 21, 2019

Savegames now work correctly, the compatibility is restored.
Cars respecting speed limits work correctly.

@FireController1847
Copy link
Collaborator

I'll test again later today

Copy link
Collaborator

@FireController1847 FireController1847 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I only have one very minor change and then I'm ready to put this in the 3-day waiting period for others to review. Ready to merge the second krzychu124 approves as I'm eager to have this pushed.

Things I tested: Setting MPH, setting km/h, setting both MPH & km/h in the same savegame, saving the samegame with both of the settings set (they both loaded with the appropriate speeds), and viewing the actual speeds the cars were going (it was within reason). Everything worked fine for me.

@FireController1847 FireController1847 added the waiting An approved PR that's waiting for 3 days before available for merge. label Jun 22, 2019
@FireController1847 FireController1847 requested review from krzychu124 and removed request for originalfoo June 22, 2019 01:50
@FireController1847 FireController1847 removed the waiting An approved PR that's waiting for 3 days before available for merge. label Jun 22, 2019
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.

I've just tested and there is visual desync between display in mph option and mod options.

Few issues below:

  • [Debug] @ 3203835744 Saving speed limit of lane 95423: 750 km/h - after saving a few custom 10 mph speed limits (seems to be log display value calculation problem)
  • Spam of updates after changing only two segments speed limits:
[Debug] @ 3074921362 SpeedLimitManager: Setting speed limit of lane 95423 to 0.3119152
[Debug] @ 3074928151 SpeedLimitManager: Setting speed limit of lane 138206 to 0.3119152
[Debug] @ 3086889654 SpeedLimitManager: Setting speed limit of lane 151851 to 0.3119152
[Debug] @ 3086897800 SpeedLimitManager: Setting speed limit of lane 258349 to 0.3119152
[Debug] @ 3086917916 SpeedLimitManager: Setting speed limit of lane 151851 to 0.3119152
[Debug] @ 3086924455 SpeedLimitManager: Setting speed limit of lane 258349 to 0.3119152
[Debug] @ 3087416925 SpeedLimitManager: Setting speed limit of lane 151851 to 0.3119152
[Debug] @ 3087424025 SpeedLimitManager: Setting speed limit of lane 258349 to 0.3119152

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 22, 2019

@krzychu124 It logs for every lane.
Fixed logging bug.
Will see the desync now.

@krzychu124
Copy link
Member

@krzychu124 It logs for every lane.

Ok, but in this case I just changed one lane speed of 2-way segment:
(2 ped paths, 2 parking + 2 veh lanes)

[Debug] @ 4021214288 SpeedLimitManager: Setting speed limit of lane 100440 to 0.4678727
[Debug] @ 4021239074 SpeedLimitManager: Setting speed limit of lane 100440 to 0.4678727
[Debug] @ 4021721252 SpeedLimitManager: Setting speed limit of lane 100440 to 0.4678727
[Debug] @ 4021743425 SpeedLimitManager: Setting speed limit of lane 100440 to 0.4678727
[Debug] @ 4022228753 SpeedLimitManager: Setting speed limit of lane 100440 to 0.4678727
[Debug] @ 4022252211 SpeedLimitManager: Setting speed limit of lane 100440 to 0.4678727

Right, there I see 6 lanes but why it logs lanes which are not changed? Would be better to show only one

[Debug] @ 4022252211 SpeedLimitManager: Setting speed limit of lane 100440 to 0.4678727

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 22, 2019

This lane spam, does it also happen in the master? If yes then please make a ticket.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 22, 2019

Regarding checkbox not being updated in the Settings UI.
I do not see an easy way to fix this. Settings UI is created once on game startup, and once more when the save is loaded, and then it is never recreated again. I tried to address the checkbox directly and set its value, but it did not seem to work.

theme_Square_US US signs
theme_Round_UK British signs
theme_Round_German German signs
Copy link
Member

@krzychu124 krzychu124 Jun 23, 2019

Choose a reason for hiding this comment

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

PL Translations to update and we can merge this branch then ;)

Display_speed_limits_mph Wyświetlaj limity prędkości w mph zamiast km/h
Miles_per_hour Mile na godzinę
Kilometers_per_hour Kilometry na godzinę
Road_signs_theme_mph Motyw dla znaków MPH 
theme_Square_US Stany zjednoczone
theme_Round_UK Wielka Brytania
theme_Round_German Niemcy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

@FireController1847 FireController1847 merged commit fd5e7d4 into CitiesSkylinesMods:master Jun 23, 2019
@originalfoo
Copy link
Member

Who did the German translation update for this PR? (I need to add credit to milestone changelog)

@kvakvs
Copy link
Collaborator Author

kvakvs commented Jun 25, 2019

I did, because I know some German and Swedish (same language family as German) to tell if a phrase looks good or not.

@originalfoo originalfoo added SPEED LIMITS Feature: Speed limits Settings Road config, mod options, config xml labels Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Contains debate on certain topics enhancement Improve existing feature LABS TM:PE LABS branch Settings Road config, mod options, config xml SPEED LIMITS Feature: Speed limits STABLE TM:PE STABLE branch Usability Make mod easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPH vs. KPH speed display
5 participants