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

support for guide manager #653

Merged
merged 21 commits into from
Feb 10, 2020

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Feb 5, 2020

fixes #593
I created base support for guide manager. A big chunk of this code is a rip-off from tutorials.

Usage instructions

  • call MainTool.Guide.Activate() when user makes a mistake
  • call MainTool.Guide.Deactivate() when user does the right thing.

EDIT: guide message will show after a couple of failed tries. just like the original games. when deactivated it will not activate for a while. the activation delay can be tweaked in GuideWrapper.cs

Usage Occurrences

I replaced several usages of ShowError() with ActivateGuide(). except for MainTool.ShowError(T("Dialog.Text:Node has timed TL script")); and a couple of other TL script related ones. I decided to use Error panel rather than Guide panel in this case because I though its important. No strong feelings though, if you think I should use guide let me know.

Technical matters

Guide manager cannot be activated from anywhere but ToolBase.SimulationStep(). This is why I dispatch the guide action to the be handled by override of ToolBase.SimulationStep(). Also I added A LogError message in case stack did not contain SimulationStep on activation

Crowding:
Can someone please help me make these modifications?
DONE

NEW FILE : Guide.csv
// tool name included to help with icon just in case. tool names match tutorial names.
"TMPE_GUIDE_HEAD_LaneArrowTool_Disabled due to highway rules" => "Changing lane arrows"
"TMPE_GUIDE_BODY_LaneArrowTool_Disabled due to highway rules" =>  COPY VALUE FROM "Dialog.Text:Disabled due to highway rules"
DELETE "Dialog.Text:Disabled due to highway rules"


"TMPE_GUIDE_HEAD_LaneArrowTool_Disabled due to lane connections" => "Changing lane arrows"
"TMPE_GUIDE_BODY_LaneArrowTool_Disabled due to lane connections" => COPY VALUE FROM "Dialog.Text:Disabled due to lane connections"
DELETE "Dialog.Text:Disabled due to lane connections"

"TMPE_GUIDE_HEAD_TimedTrafficLightsTool_Auto TL no need" => "TL Script"
"TMPE_GUIDE_BODY_TimedTrafficLightsTool_Auto TL no need" => COPY VALUE FROM "Dialog.Text:Auto TL no need"
DELETE "Dialog.Text:Auto TL no need"

If Crowding key does not exist, the string key is shown as guide message.

Testing:

  • I tried to change lane arrows while there where lane connections or highway rules => Guide message appeared.
  • Then I changed lane arrows where it was permitted. => Guide message disappeared.
  • I did the same thing with TL script where TL script was not needed.
  • The error message still shows when accidentally trying to replace an existing TL script.

Screenshots:

Screenshot (392)

proof that adding the Crowding keys will display the desired message:
Screenshot (391)

@originalfoo
Copy link
Member

originalfoo commented Feb 5, 2020

Awesome enhancement!

Is it possible to replace the globe icon on that panel with the TMPE logo?

Note: That is original icon image, way too big just for icon use. Would probably need shrinking down to 48x48 px as at some point we'll also want to use it for the TM:PE menu button (that currently has that weird crown thing).

@originalfoo originalfoo added this to the 11.1 milestone Feb 5, 2020
@originalfoo originalfoo added enhancement Improve existing feature LANE ROUTING Feature: Lane arrows / connectors TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc UI User interface updates Usability Make mod easier to use labels Feb 5, 2020
@kianzarrin
Copy link
Collaborator Author

@aubergine10

Awesome enhancement!
Is it possible to replace the globe icon on that panel with the TMPE logo?

Note: That is original icon image, way too big just for icon use. Would probably need shrinking down to 48x48 px as at some point we'll also want to use it for the TM:PE menu button (that currently has that weird crown thing).

It gives user impression that TMPE is legit!

Icon can be easily tweeted in GuideWrapper.cs

I thought about using TMPE logo but I just Ended up using the same icon that was used by tutorial. A big chunk of this code is a RIP-off from tutorial actually.

I'll have a look into it. Should I change tutorial icon too?

@kvakvs
Copy link
Collaborator

kvakvs commented Feb 5, 2020

If you use that traffic light as a small icon, user won't distinguish between a random traffic light and TM:PE. Maybe find a way to include text "TM:PE" into that reduced logo.

@originalfoo
Copy link
Member

Should I change tutorial icon too?

Yup, might as well :) Will make it much more obvious the advisor relates to TM:PE (espeically with the "TM:PE " prefix mentioned below).

If you use that traffic light as a small icon, user won't distinguish between a random traffic light and TM:PE. Maybe find a way to include text "TM:PE" into that reduced logo.

The panel title text (both guide and advisor panels) could be automatically prefixed with string "TM:PE " (no need to include in localisation text as it's same for every language).

@kianzarrin
Copy link
Collaborator Author

@aubergine10 @kvakvs
how about the crown icon? (simple solution)

each tool also can have icon of its own (a bit more coding required)

@originalfoo
Copy link
Member

Differnet icons for different tools would be cool. Maybe we should put that on hold until kvakvs finishes #656? It would be good to have the icons for each tool centrally defined, or defined on the tool class, so if we change an icon we don't need to update multiple classes?

@kianzarrin
Copy link
Collaborator Author

added TM:PE string prefix:
Screenshot (408)

@kianzarrin
Copy link
Collaborator Author

new translation keys are working:
Screenshot (409)

@originalfoo
Copy link
Member

Not sure if this is right place for this, but users are getting confused by the "Quick-setup is not supported" warning - it doesn't tell them why it's not supported. (From vague memory it's something to do with consecutive one-way roads at a junction?)

Could it be changed to "The road layout at this junction is not currently supported by quick setup, sorry."

Also, "TL Script" title is a bit vague, maybe just "Timed Traffic Lights"

@kvakvs
Copy link
Collaborator

kvakvs commented Feb 7, 2020

TL abbreviation wasn't meant for the player. Its for the developer to quickly recognize that TL=some traffic lights are involved.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 @kvakvs should I modify it to 'timed script' or 'timed traffic light'?

In any case this should not block merge. this has to go into crowdin.

@kvakvs
Copy link
Collaborator

kvakvs commented Feb 7, 2020

Yes please write in full, and check whether Crowdin already has a string for Timed TL.
Timed Traffic Light might be good.

@kianzarrin
Copy link
Collaborator Author

changed to timed traffic light https://crowdin.com/translate/tmpe/47/en-en#9001

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 👍

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 suggest rethinking guide handling because it looks a bit hacky.
PRO-TIP:
use Singleton<SimulationManager>.instance.AddAction to run async task on simulation thread
in this case it could be:

Singleton<SimulationManager>.instance.AddAction(delegate()
{
	Singleton<GuideManager>.instance.Activate(guide, activationInfo);
});
  • guide is GenericGuide,
  • activationInfo is new GenericGuide.GenericActivationInfo(/*GuideInfo instance*/)

TLM/TLM/UI/Localization/Translation.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/TrafficManagerTool.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/Helpers/GuideWrapper.cs Show resolved Hide resolved
TLM/TLM/UI/Helpers/GuideWrapper.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/TrafficManagerTool.cs Outdated Show resolved Hide resolved
@originalfoo
Copy link
Member

originalfoo commented Feb 9, 2020

There's a couple of other things that could merit using the guide instead of error dialog, both on Toggle Traffic Lights tool:

  • Clicking a level crossing (warning about traffic lights being mandatory)
  • Clicking a junction with timed traffic lights (warning about needing to remove those first)

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Feb 9, 2020

@aubergine10

There's a couple of other things that could merit using the guide instead of error dialog, both on Toggle Traffic Lights tool:
* Clicking a level crossing (warning about traffic lights being mandatory)
* Clicking a junction with timed traffic lights (warning about needing to remove those first)

Who doesn't love a IN YOUR FACE obstructing red colored panel to punish him for mistakes? lol

EDIT: oh no that means I need to add more translation keys. @aubergine10 can you please do the honors?

@kianzarrin
Copy link
Collaborator Author

please don't ask me to document self explanatory code
/// Activates guide for the locale key - this information is very useful ... NOT!
Guide.Activate(localeKey)

@kianzarrin
Copy link
Collaborator Author

@krzychu124

PRO-TIP:
use Singleton<SimulationManager>.instance.AddAction to run async task on simulation thread

Wow that is awesome! Maybe using this I can also fix my pedestrian bridges! (another mod i am working on)
Screenshot (324)

@kvakvs
Copy link
Collaborator

kvakvs commented Feb 9, 2020

please don't ask me to document self explanatory code
/// Activates guide for the locale key - this information is very useful ... NOT!
Guide.Activate(localeKey)

Of course you're right with this one.
But when a function is multiple lines and it takes effort to parse and understand the code, having a quick doc really saves brain fuel and time.

@krzychu124
Copy link
Member

@krzychu124

PRO-TIP:
use Singleton<SimulationManager>.instance.AddAction to run async task on simulation thread

Wow that is awesome! Maybe using this I can also fix my pedestrian bridges! (another mod i am working on)
Screenshot (324)

If you look closely what GuideManager does when it want to activate/deactivate panel you will know why it needs to be called from simulation thread. Answer is: Activate method expects that it's called from Simulation/other thread because it explicitly dispatching action on UI thread

ThreadHelper.dispatcher.Dispatch(delegate(){
	this.ShowTutorialPanel(trigger, activationInfo);
});

Probably you will get error or exception if you try dispatching action when caller is already on UI thread, since CO knows that every tool always make changes to environment on simulation thread, so they hardcoded ui calls via dispatcher.

@kianzarrin
Copy link
Collaborator Author

Clicking a level crossing (warning about traffic lights being mandatory)

@aubergine10
Considering that we already added the translation keys don't you think its a bit too late to add guide for TL on level crossings?
If you want the changes then please add the following keys and merge them into this branch:

Guide.csv
"TMPE_GUIDE_HEAD_ToggleTrafficLightsTool_Node is level crossing" => "Traffic lights"
"TMPE_GUIDE_BODY_ToggleTrafficLightsTool_Node is level crossing" => COPY VALUE FROM "Dialog.Text:Node is level crossing"

@kianzarrin
Copy link
Collaborator Author

I suggest rethinking guide handling because it looks a bit hacky.
PRO-TIP:
use Singleton<SimulationManager>.instance.AddAction to run async task on simulation thread

@krzychu124 this issue has been resolved.

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.

Other than that little name consistency issue, looks great now.
I thought you'll choose field and Lookup property from master branch instead yours 🙂

TLM/TLM/UI/Localization/Translation.cs Outdated Show resolved Hide resolved
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.

Looks very good now 👍

@kianzarrin kianzarrin merged commit 37ab5d6 into CitiesSkylinesMods:master Feb 10, 2020
@kianzarrin kianzarrin deleted the 593-guide-manager branch February 10, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing feature LANE ROUTING Feature: Lane arrows / connectors TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc UI User interface updates Usability Make mod easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improved user messages (guide controller)
4 participants