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

default Timed traffic light #554

Merged
merged 20 commits into from
Dec 23, 2019

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Nov 17, 2019

Fixes #540
Fixes #5
Updates #324

  • added Mass edit modifier key (ctrl) for creating a default timed traffic light.
  • Updated tutorial message on https://crowdin.com/translate/tmpe/42/en-en#8862
  • Added tooltip keybind in the code but I don't know how to add a new key on crowdin.

I created this city to test my timed traffic light in all cases (that I could think of).

per lane type traffic light,
separate lanes
multi segment intersection,
different road directions
oneway roads

introduce a problem:
two sides are green rather than one/
Todo:Optimization and translations.
@originalfoo originalfoo added enhancement Improve existing feature TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc labels Nov 18, 2019
@originalfoo originalfoo added this to the 11.0 milestone Nov 18, 2019
Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

The keys to translation strings are too long, too detailed and contain extra spaces and typos.
Make the string keys shorter and move the text into Crowdin English and English UK variants.

TLM/TLM/UI/MainMenu/TimedTrafficLightsButton.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/SubTools/TimedTrafficLightsTool.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/SubTools/TimedTrafficLightsTool.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/SubTools/TimedTrafficLightsTool.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/SubTools/TimedTrafficLightsTool.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/AutoTimedTrafficLights.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/AutoTimedTrafficLights.cs Show resolved Hide resolved
@kvakvs
Copy link
Collaborator

kvakvs commented Nov 19, 2019

Does the code verify that when a Timed TL is created, the directions are actually possible? By checking the lane connectors, lane arrows?

@kianzarrin
Copy link
Collaborator Author

@kvakvs

Does the code verify that when a Timed TL is created, the directions are actually possible? By checking the lane connectors, lane arrows?

I call the LaneArrowManger.SeparateLanes() to setup turning lanes. This function does nothing on the segments that have the lane connector.

If you try it, you will see that it works just fine. I did actually thought this through. If one puts a green light toward a direction that cannot be used:
A) The user will not see it because the already existing GUI render overlay code hides such directions.
B) It causes no functional issues. All the happens is that cars will not go toward that direction.

Note that at each step I give a green light to one of the connected road segments to go to all directions. So no change of logic is needed if lane connector is used.

put better tooltip keys.
@kvakvs
Copy link
Collaborator

kvakvs commented Nov 23, 2019

Updated strings from Crowdin.
Tooltip.Keybinds: Auto TL not found. I have no idea how this works.

@kianzarrin
Copy link
Collaborator Author

I added wiki documentation here:
#540 (comment)

@kvakvs
Copy link
Collaborator

kvakvs commented Nov 26, 2019

Been slightly confused. Is 554 (this) and 540 are the same problem but one is the issue and another is the pull request? Please add "Fixes #540" in the description, so when this is merged, the other one will be closed too.

@kvakvs
Copy link
Collaborator

kvakvs commented Nov 26, 2019

Was wondering about a way to make this algorithm testable
(i.e. separate it from the game engine) and then create a simple unit test?
And then you can call the same auto TL engine from the test. Maybe read some text inputs with junction configurations and then check that output TL parameters are what we expect.

@kianzarrin
Copy link
Collaborator Author

@kvakvs

Was wondering about a way to make this algorithm testable

do you mean as part of this PR or another one?

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Nov 26, 2019

Been slightly confused. Is 554 (this) and 540 are the same problem but one is the issue and another is the pull request? Please add "Fixes #540" in the description, so when this is merged, the other one will be closed too.

This is just the first phase. There are more features discussed for example in #540 (comment)

Edit by aubergine: Fixed link

Do you think we should continue working on those in the same issue (#540) or should I create a new issue?

EDIT: tagging @aubergine10 @kvakvs

@kianzarrin
Copy link
Collaborator Author

I added in the translations. Is the code ready for approval? 😃

@kvakvs
Copy link
Collaborator

kvakvs commented Nov 26, 2019

As you're steaming with new other ideas, i'm afraid once this is merged you won't have time to rework it and make it testable. Might as well consider that possibility now. If too complex i can let it slip for now, you might still want to do it later.

The idea is that you make algorithm not call any game or TMPE code, take some data structures as inputs, and output some data structures which will then be applied by the TMPE code to the game. This will allow us simulate those inputs during the test and check the outputs as they are simple numbers and structures now instead of game data. For this to work you need to know beforehand which parts of the map the algorithm will read and pre-load them into some simple arrays or lists or dicts, but don't store entire nodes and segments, only store stuff that's relevant to your algorithm.

@krzychu124 krzychu124 self-requested a review November 26, 2019 22:37
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 have no idea how you created those translation files without building TM:PE Crowdin project, but they are wrong 😄 All translation file are generated, we don't touch those files manually. I will push new translation to your branch 😉

@krzychu124
Copy link
Member

To resolve conflicts pull master branch and choose all translation files to overwrite yours.

@kianzarrin
Copy link
Collaborator Author

OK i resolved the merge conflicts as you said.

@originalfoo
Copy link
Member

I'll do some testing on this tomorrow.

Note to self: Also, I need to create wiki page with docs

@originalfoo
Copy link
Member

❌ I'm not seeing the Ctrl+Click mentioned in the tooltip:

image

✔️ It is, however, mentioned in the adviser text.

❌ Upon ctrl+clicking an existing junction (that had traffic light and also junction restrictions), I got this error:

image

Will try and gather more infos on the error (can't see anything of use in the logs yet).

@originalfoo
Copy link
Member

For ref, here is the junction I was trying to customise:

image

@originalfoo
Copy link
Member

originalfoo commented Dec 1, 2019

Tested on several more junctions:

  • ⚠️ Any junction with 1-way roads throws up the empty info box
  • ✔️ Works fine on junctions that have only 2-way roads

Also, it doesn't seem to factor in lane connectors (maybe those could be covered by later PR?)

@originalfoo
Copy link
Member

originalfoo commented Dec 1, 2019

Following image shows where it works and where it doesn't:

image

For road on left, it doesn't matter which direction the one-way road goes in, the auto-lights can still be applied.

So maybe it's when there's adjacent 1-way roads (like on Right) that the problem occurs?

@kianzarrin
Copy link
Collaborator Author

@aubergine10 Also, it doesn't seem to factor in lane connectors (maybe those could be covered by later PR?)

Yes it does. Just like lane separation quick-edit feature it ignores noes/segments with lane connections (still puts the Timed traffic light but does not separate turning lanes). Maybe it can be more clever see #575 . But not in this PR.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 I'm not seeing the Ctrl+Click mentioned in the tooltip:

The English translation for for keybind was empty

"Tooltip.Keybinds:Auto TL","","","","","","","","","","Ctrl+Klik: Szybka instalacja","","","","","","","Автоматичне створення світлофора з таймером"	C:\Users\dell\source\repos\Cities-Skylines-Traffic-Manager-President-Edition\TLM\TLM\Resources\Translations\Menu.csv	21	1

Its was also empty in crowdin. I just added it: https://crowdin.com/translate/tmpe/24/en-en#8945

@aubergine10 @kvakvs @krzychu124 I do not know how to download translations the right way. Can anyone please help me?

@kianzarrin
Copy link
Collaborator Author

@aubergine10 ❌ Upon ctrl+clicking an existing junction (that had traffic light and also junction restrictions), I got this error:

Again a matter of translation key being empty. TTL quick-setup is not supported here. There has to be at least 3 exits from the junction. In here just add a simple traffic light. I don't see the need for timed traffic light. If you do then comment on #575

I will add all the missing English translations to crowdin. then I will wait for help from someone to import them.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 I do not support level crossing. I just added code to show error message regarding this unsupported scenario. see #575

@originalfoo
Copy link
Member

I do not know how to download translations the right way. Can anyone please help me?

You can download the .csv files and drop them in to the translations folder, then submit that to the PR - that's fine. Just be sure to leave a note in OP of PR listing which languages were changed and who changed them (so we can give credit in the change notes).

However:

  • All editing of the translations must occur via web interface of the Crowdin site; it is our single source of truth for current keys and translations
  • Adding new keys or languages to Crowdin needs to be done by an admin (myself, kvakvs, krzychu)

Note: I'm currently having some login issues with Crowdin (their support is investigating) so can't currently add languages or keys or approve translations.

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Dec 3, 2019

@aubergine10 You can download the .csv files and drop them in to the translations folder, then submit that to the PR - that's fine. Just be sure to leave a note in OP of PR listing which languages were changed and who changed them (so we can give credit in the change notes).

This is exactly what I did: #554 (comment) (If I understood correctly)

but then @krzychu124 told me: #554 (review)

they are wrong 😄 All translation file are generated

So @aubergine10 @kvakvs: can you please tell me what did I do wrong last time so that I can fix it this time?

PS: Also later on somehow the English translation strings in crowdin got wiped out leading to the empty error messages you saw! One of life's mysteries!

@kianzarrin
Copy link
Collaborator Author

OMG I totally forgot about left hand traffic! I will fix it soon.

@kianzarrin
Copy link
Collaborator Author

Fixed it.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 @krzychu124 @kvakvs The translations are in. This should be ready to merge
Screenshot (196)
Screenshot (195)

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.

From end-user perspective this LGTM 👍

@krzychu124 krzychu124 merged commit 30a7ce7 into CitiesSkylinesMods:master Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing feature TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add automatic timed traffic lights Auto-config traffic lights (single junction)
4 participants