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

Amend #851: Add missing strings and write config on toggle OSD #858

Merged
merged 7 commits into from
Apr 26, 2020

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Apr 22, 2020

  • Missing strings for mode descriptions added
  • OSD toggle now saves global config

@kvakvs kvakvs added the BUG Defect detected label Apr 22, 2020
@kvakvs kvakvs self-assigned this Apr 22, 2020
@originalfoo
Copy link
Member

Looks like there's some new translations in there too - who contributed those? (so we can mention on changelog)

@originalfoo originalfoo added Localisation Localised text and features UI User interface updates and removed BUG Defect detected labels Apr 22, 2020
@originalfoo originalfoo added this to the 11.5.0 milestone Apr 22, 2020
@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

Czech translation, entirely new, but the language code is not added to the game UI yet (out of scope of this amendment)
Contributed by Kuba Kukuč Pátek (user: jakubpatek, discord user: @ QuTek_)

@kvakvs kvakvs requested a review from kianzarrin April 22, 2020 18:07
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.

👍

@originalfoo
Copy link
Member

originalfoo commented Apr 22, 2020

So, I added cz to the language codes dictionary (will push the commit to this PR 858):

image

In-game, changed the language and noticed this (note top two items of list are same):

image

First item in list should be "Game Language" (in currently selected language)?

@originalfoo
Copy link
Member

originalfoo commented Apr 22, 2020

Slight scope creep, but hopefully quick to change... Parking icon is dimmer than nearby vehicle restriction and speed limits icons; if memory serves this is intentional in its unselected state.

image

However, when hovering (as the mouse was doing in image above) could it be given full opacity? Otherwise it feels like it's disabled (despite the blue background color on hover). Having the foreground icon become full opaque would provide more tactile feedback for the button.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

A problem 1: The sprites for Parking Restrictions normal and active look like they have correct brightness. The skin setup for PR button is exact same as for example SpeedLimits button, so should be correct too. Now why exactly that button looks differently?

A problem 2: Child of a window cannot be more opaque than its parent.

@originalfoo
Copy link
Member

Ooh, I think I found a bug.... Steps to reproduce (not sure if all steps are required, but just in case):

  1. From main menu I changed language to Czech to test the options screen looked ok
  2. Changed back to Game Language (English, in my case)
  3. Loaded my city
  4. Tested the key/mouse hints (in English)
  5. Decided to test them in Czech: Pause menu > TMPE mod options > Czech language

First, with lane arrow tool selected, clicking the ? button does not toggle hints panel:

image

Then I went to lane connector tool to see if that would work:

image

Note the ? at this point isn't remembering state; it keeps resetting to "off".

So now I try going back to Lane Arrows tool:

image

First click does nothing (note the darker blue background indicating the lane arrows tool should be active, but also the dark blue background indicating lane connectors tool is also still active):

image

Note: This specific bit of state fail seems to only occur if I click the ? button while in the lane connector tool (I've not tested all possible combinations).

A second click now removes (lane connector) node select circles but still has lane connector tool selected:

image

Note, however, that segments are hovering when I hover them - indicating that the Lane Arrows tool has input focus:

image

(note also button states in that image above)

A few more clicks of the lane arrows button and now both buttons are disabled and I'm getting green road selections (indicating that the 'mass edit' tool has input focus):

image

By this point it is very diffcult to get the lane arrows button selecting at all, although every so often it does select:

image

Logs:

@originalfoo
Copy link
Member

Regarding comment above, I suspect we will need to reset fsm's of buttons/etc when language changes?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

@aubergine10 i had similar problem before, and it was related to me trying to set visible of some control to false during input processing. If i don't try to hide anything, it works normally. If you remember that's when i was asking whether input processing is another thread or some other unsafe situation.

Possibly related to me detaching the hint controls when refreshing the hint panel. There was no easy way to delete them cleanly within the same frame so i started detaching them, i.e. resetting their parent, and they're sad when they lose parent UIView.

@originalfoo
Copy link
Member

I think Forest Brush also removes UI elements when not displayed and from vague memory (it was eitehr boformer or TPB) ran in to some similar issues - but they found some way to resolve them. Might be worth having chat with boformer on skylines.code.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

Pushed one line change, not detaching controls now. Can you verify if anything changed?
It should get rid of that UIView is null crash but it may affect layout of the hint panel "ghosting" empty spaces where previous generation controls were placed.

@originalfoo
Copy link
Member

originalfoo commented Apr 22, 2020

Just reporting this before I forget... These tooltips have typos...

Should be "Click incoming lane to select it":
image

Should be "Click outgoing lanes to toggle connections to the incoming lane":
image

@originalfoo
Copy link
Member

^ Edited comment above (in case git notifications already arrived with old text)

@originalfoo
Copy link
Member

originalfoo commented Apr 22, 2020

I can confirm the null reference / fsm bugs are fixed! (ref: #858 (comment))

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

The text is correct. It says exactly what i wanted to say.
Outgoing lane (traffic source)
connects to
Incoming lane (traffic destination).

Although now if i think the wording might be wrong. If we take the junction as a reference point we take a lane incoming to the junction/node, and then lane outgoing to redirect the flow.

@originalfoo
Copy link
Member

Can't remember if you wanted a reminder in this PR or next one, but just in case:

image

Display hint about being able to click road for the mass edit tools? Something like:

"Select a tool to use, or click a road for mass-edit options" ...?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

We can add that as one independent and small PR.
Is that always to be displayed when no tool is active?

@originalfoo
Copy link
Member

originalfoo commented Apr 23, 2020

If we take the junction as a reference point we take a lane incoming to the junction/node, and then lane outgoing to redirect the flow. (ref: #858 (comment))

Yup, it's always been the junction that we used as reference point, because the user has to select the junction first, so subseqent actions are in relation to the junction.

So they start by selecting incoming lane they want to edit, then they click outgoing lanes to toggle connection to the current incoming lane.

It's important that we mention 'toggle connection' (or 'add/remove connection'?) in the 'choose outgoing lanes' stage of the task, as there is always some users asking how to remove a connection after they've added it.

We can add that as one independent and small PR. (ref: #858 (comment))

Ok.

Is that always to be displayed when no tool is active? (ref: #858 (comment))

Yes, it would be, which could be confusing hence the mention of "Select a tool to use, or ..." at the start of of the hint text (there might be better alternate wording but I couldn't think of any yet).

The mass edit thing is weird to explain but feels intuitive once you know how to use it. Unlike all other tools, the "button" for the mass edit is the road itself.

At some later date we could potentially add other icons to the road world info panel for performing other tasks; eg. speed limit icon would switch to the speed limit tool with that road already selected. We could further extend by allowing node selection, so I could just select node to see info panel and then click junction restrictions icon to jump in to that tool with the node already selected. But that's stuff to ponder at later date.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 23, 2020

@aubergine10 I suggest introducing the user to the concept of a named road, or street, and integrate it more with all relevant tools. For example encourage users holding shift more, and seeing the streets more often. This will create awareness what the streets are, and then the hint will begin making sense.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 24, 2020

@aubergine10 Please suggest exact wording change for the modes, and let's get done with this PR :)

@originalfoo
Copy link
Member

I'll update later tonight (currently wading through mod checker stuff which will take another few hours).

@originalfoo
Copy link
Member

Default interaction hints...

Mass edit

When toolbar visible, but no tool selected (green road select "mass edit" can be used):

Choose a tool to use, or click a road

Lane arrows

When lane arrows tool active, but nothing selected:

Choose a segment to edit

Lane connectors

When lane connector tool active, but nothing selected:

Choose a node or junction to edit

When a node/junction is selected:

Choose which lane to edit

When an incoming (lane entering junction) is selected:

Click target lanes to toggle connections

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 26, 2020

Modified the strings and updated the English translations to what is suggested above.
Other translations which I can do will wait until final approval arrives.

The bug with "UIVIew is null" is reappearing sometimes. I am worried. The game was running idle in road build mode for a few minutes, and then error log appeared with that exception. Recompile and reloading the mod fixed it, but it means the problem is still somewhere there.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 26, 2020

Idle hint added now
Avoiding to hide the panel, instead empty panel is made 0% opaque and it should help.

bild
bild

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 merged commit 918c5be into CitiesSkylinesMods:master Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Localisation Localised text and features UI User interface updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants