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

Refactor OptionsGeneralTab.cs #1267

Closed
wants to merge 30 commits into from
Closed

Conversation

originalfoo
Copy link
Member

@originalfoo originalfoo commented Jan 2, 2022

Updates #1262, #62

The General settings tab was getting a bit cluttered (visually), especially with new options for speed limits UI. This PR does some refactoring to:

Todo:

  • Locale keys for new UI groups

Translators:

  • Chamëleon, MenschLennart, John Deehe, Skazov, AlexofCA, CrankyAnt, Иван Соколов, AduitSSH, John Lok Ho, DNSE, shg166, GiorgioHerbie, footbool, krzychu124, aubergine18, Dmytro Lytovchenko, Fizzy_LaFizz, Vladimír Krajč, alvaroer, Mahmoud Al Gammal, ipd, 田七不甜, vitalii201, 許景翔, Mehmet Ömer Tüzer, vicugna-pacos, kianzarrin, Mbyron26, Headspike, lennier3, kevinlin18, Ikki_di_Phoenix

Not in this PR:

TMPE.zip

Updates #1262, #62

- Collate related options in to UI groups
- Move bulky option code in to methods
- Clarify intent in `MakeSettings_General`
@originalfoo originalfoo added Usability Make mod easier to use UI User interface updates Settings Road config, mod options, config xml labels Jan 2, 2022
@originalfoo originalfoo self-assigned this Jan 2, 2022
@originalfoo originalfoo marked this pull request as draft January 2, 2022 20:52
@originalfoo
Copy link
Member Author

originalfoo commented Jan 2, 2022

click for WIP preview

image
image

@kvakvs
Copy link
Collaborator

kvakvs commented Jan 3, 2022

We changed window from transparency to opacity
Why not we also change overlay transparency to opacity? There was a helper class made for that U.UOpacityValue

@kvakvs
Copy link
Collaborator

kvakvs commented Jan 3, 2022

Instead of colorizing maybe try increasing font size/scale for heading labels?

@originalfoo
Copy link
Member Author

Instead of colorizing maybe try increasing font size/scale for heading labels?

The group heading text is already a little bolder than normal option text. IIRC, with vanilla font manager, different text sizes result in additional textures being created (I think that's one of the things FBS Booster mod tackles with its improved fonts system?). Also, larger headings = more vert scrolling.

An alernate would be colourise the option text, and that approach would also allow for additional info text to be displayed near options (ie. put the hint text in to its own text box under the option).

Case study: Tree Anarchy mod by Quistar

image

image

Case Study: Prop Anarchy mod by Quistar

Group box and custom sliders:

image

@kvakvs
Copy link
Collaborator

kvakvs commented Jan 3, 2022

Let's leave info text for a second pass of this work, not now.
Larger font size should still look good, an additional font texture is probably a small price for that. Maybe 30-50% larger?
I really want to add extra help to each option, a hint like in the screenshots, or multiline tooltip, or click ? to toggle hints, possibly even a popup page with some illustrations or something like that, or a link to the Wiki page which will open Steam or system browser.

@originalfoo
Copy link
Member Author

Let's leave info text for a second pass of this work, not now.

Yup, once I get the translation keys sorted that should suffice for this PR. How is that handled btw?

possibly even a popup page with some illustrations or something like that, or a link to the Wiki page which will open Steam or system browser.

Yup, see: #733

We changed window from transparency to opacity
Why not we also change overlay transparency to opacity? There was a helper class made for that U.UOpacityValue

Yup, it will be much better having them both use same approach. What needs to change in the code? Is it just the display/setting of the value, or will it require updates to tool UI (thus separate PR)?

@kvakvs
Copy link
Collaborator

kvakvs commented Jan 3, 2022

Translations are handled via Crowdin https://crowdin.com/project/tmpe
You click Content -> Strings, select CSV file where it will be added, and add the translation keys
Then you click the project main page, where the languages are, click English and in the ... menu on the right click Translate
Translated keys then need to be approved, there's a separate editor mode for approvals, but you can also self-approve from the translation UI by clicking ✔ on the translation you've just added.
When done translating into all languages you know, in the main project page there's big green build&download button. The contents of that zip go into TLM/TLM/Resources/Translations, as is unmodified.

@kvakvs
Copy link
Collaborator

kvakvs commented Jan 3, 2022

Yup, it will be much better having them both use same approach. What needs to change in the code? Is it just the display/setting of the value, or will it require updates to tool UI (thus separate PR)?

The opacity is 1.0f-transparency so it should be straightforward change to the tools using this value.
Separate ticket is fine

@krzychu124
Copy link
Member

krzychu124 commented Jan 3, 2022

The group heading text is already a little bolder than normal option text. IIRC, with vanilla font manager, different text sizes result in additional textures being created (I think that's one of the things FBS Booster mod tackles with its improved fonts system?). Also, larger headings = more vert scrolling.

@aubergine10 Nope. Every font used to render text in game has own, separate texture for characters (res. max 4096x4096).

When you use non standard font size (not the one already used) game will request all new characters using the texture of selected font.

Unity starts filling texture font from 256x256, then doubles width or height to resize - all characters will get requested again in that case which usually results in "alien characters" rendered until next deep update of text component because of UI is reading old glyph positions using a new recreated texture as a lookup table (glyph position is not guaranteed between font texture resizes)

Based on code from AVO mod, but with some tweaks
- Button opens localisation guide in wiki
- Removed URL from Crowdin button translation key
- Both buttons show full URL in tooltip when hovered
Latest translations from Crowdin
@originalfoo originalfoo marked this pull request as ready for review January 4, 2022 02:38
@originalfoo originalfoo added the Localisation Localised text and features label Jan 4, 2022
@originalfoo
Copy link
Member Author

Ready for review.

@originalfoo
Copy link
Member Author

Is it possible to put buttons side by side?

image

@kvakvs
Copy link
Collaborator

kvakvs commented Jan 4, 2022

You can check how its done in Keybinds component in KeybindSettingsBase.AddAlternateKeybindUI which creates a panel with horizontal autolayout then plops 2 editbox components with 2 ❎ buttons into it.

@kianzarrin
Copy link
Collaborator

I think if you want to change the code it would be nice to do so properly and use:
https://github.com/CitiesSkylinesMods/TMPE/blob/master/TLM/TLM/UI/Helpers/CheckboxOption.cs

If there is no duplicate code fixing this would be easier #562

@originalfoo
Copy link
Member Author

@krzychu124 I tried adding panel but couldn't seem to get it to work - specifically creating the actual panel. (Once I have panel I can config it and add stuff to it)

#1267 (comment)

I'm not sure what you meant by: panel is UIPanel

@originalfoo originalfoo modified the milestones: 11.6.4, 11.7.0 Jan 22, 2022
@originalfoo
Copy link
Member Author

Pushing back to 1.7.0 - settings look weird (particularly Policies tab) with only half the checkboxes updated - will do additional PRs to port each tab to CheckboxOption, etc.

@originalfoo originalfoo marked this pull request as draft January 22, 2022 22:18
@originalfoo originalfoo modified the milestones: 11.7.0, Not Applicable Feb 8, 2022
@originalfoo originalfoo added ⏸Paused Paused for now DO NOT MERGE YET Don't merge this PR, even if approved, until further notice labels Feb 8, 2022
@originalfoo
Copy link
Member Author

This is now superceded by a much better implementation via #1369, #1411, #1432

See also: #1356

Buttons will be reintroduced later as part of #813, #733, #659, etc.

Translations were merged in: #1415 , etc.

@originalfoo originalfoo deleted the refactor-general-settings-1262 branch February 23, 2022 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE YET Don't merge this PR, even if approved, until further notice Localisation Localised text and features ⏸Paused Paused for now Settings Road config, mod options, config xml UI User interface updates Usability Make mod easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants