Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#4406 closed enhancement (fixed)

[PATCH] Display some info on AI levels in gamesetup window

Reported by: mimo Owned by: mimo
Priority: Nice to Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

Following the discussions on the forums where some newcomers had some trouble with the AI difficulty, it happens that some of them were not aware that this difficulty can be changed (gear icon not really intuitive for complete beginners), and some were used to have (in other games) a default level which was really easy. I then propose to add a warning when starting a SP game which can be removed by experienced using a checkbox.

Attachments (3)

ai-info.patch (3.7 KB ) - added by mimo 7 years ago.
ai-info-v2.patch (8.5 KB ) - added by mimo 7 years ago.
ai-info-v3.patch (9.2 KB ) - added by mimo 7 years ago.

Download all attachments as: .zip

Change History (12)

by mimo, 7 years ago

Attachment: ai-info.patch added

comment:1 by elexis, 7 years ago

Keywords: rfc added; review removed
Milestone: BacklogWork In Progress
Type: defectenhancement
  • Typo:
    • reasonnably
    • adviced -> advised
    • to a lower level -> against a lower level?
  • Checkboxes should enable, not disable things (otherwise leading to double negation fun like "Disable Treasures: disabled" in the objectives dialog)
  • Tip not visible in MP, makes sense though
  • Right 40% of the checkbox in the gamesetup don't react to clicks (8px too small or something)
  • Is it only an aiInfo.txt or might it even be singleplayerTips.txt (to show other important hints for first time players)?
  • Init changes should be moved to InitGUIObjects (which is executed after the first OnTick, see // First tick happens before first render, so don't load yet), wouldn't complain if it became a new function initAIInfo, initSPInfo or initSingleplayerSettings.
  • If aiInfo is hidden by default in the XML, then we don't need that hidden=true line (will be changed anyway with the gamesetup rewrites tbd). Isn't same true for the checked line?
  • "" + foo seems like a hack to me in comparison to the String constructor String(foo)

in reply to:  1 comment:2 by mimo, 7 years ago

Thanks for the review

Replying to elexis:

  • Typo:
    • reasonnably
    • adviced -> advised
    • to a lower level -> against a lower level?

ok

  • Checkboxes should enable, not disable things (otherwise leading to double negation fun like "Disable Treasures: disabled" in the objectives dialog)

i agree, but have not done it before to stay consistent with what was done for splashscreen. Both are changed now and i took the opportunity to move them in the gui set.

  • Tip not visible in MP, makes sense though

yes, was done on purpose as MP players should already know the basics

  • Right 40% of the checkbox in the gamesetup don't react to clicks (8px too small or something)

i can't reproduce it ???

  • Is it only an aiInfo.txt or might it even be singleplayerTips.txt (to show other important hints for first time players)?

ok make it more general with a json file so that we can have additional tips in the future

  • Init changes should be moved to InitGUIObjects (which is executed after the first OnTick, see // First tick happens before first render, so don't load yet), wouldn't complain if it became a new function initAIInfo, initSPInfo or initSingleplayerSettings.

done

  • If aiInfo is hidden by default in the XML, then we don't need that hidden=true line (will be changed anyway with the gamesetup rewrites tbd). Isn't same true for the checked line?

done for the hidden. The checked is different as we can in the future have a way to show this panel which does not rely on the checked status (as is done for the splashscreen with the version), so better keep it

done i'm fine with both, but we should keep consistency. Maybe add it in the coding convention?

by mimo, 7 years ago

Attachment: ai-info-v2.patch added

comment:3 by elexis, 7 years ago

  • Merge conflicts with the splashscreen cleanup in #4399
  • Translation broken in both patches since messages.json doesn't know about the new file
  • JSON does not allow linebreaks in strings, so TranslateLines will be equal to Translate I think.
  • Just using a txt file and showing all or none of the lines is sufficient in the scope of this ticket. The distinction between individual tips seems more like ballast until we have manifested use cases for distinguishing the tips individually (for example a checkbox for every tip). If we have those use cases, the implementation could be completed too. We could also identify the tooltips by the hash of their content, see also #4399.
  • default.cfg structure might be cleaner with gui.splashscreen.enable, gui.splashscreen.version, gui.gamesetup.singleplayertips.
  • Checkbox pixels: I had it with other checkboxes as well, for example the splash screen dialog one fixed by 0686617. Maybe its more like 21.4% of the area that doesn't react to clicks.
  • I'd be happy to add these things to the wiki:Coding_Conventions, but it should likely be decided by programmers in a forum thread including other points of contention as well
  • Gamesetup code looks more modfriendly to me now. The two gamesetup options should be neighbors.
  • Singleplayer is more readable than SP, not only in the code but even more importantly in the options.

by mimo, 7 years ago

Attachment: ai-info-v3.patch added

in reply to:  3 comment:4 by mimo, 7 years ago

Replying to elexis:

  • Merge conflicts with the splashscreen cleanup in #4399

That's unavoidable if we want to keep consistency and the needed fixes of #4399 are small. But if you prefer, you can add those changes about splashscreen (inverted convention and new naming) in #4399 and i'll remove them from that patch

  • Translation broken in both patches since messages.json doesn't know about the new file
  • JSON does not allow linebreaks in strings, so TranslateLines will be equal to Translate I think.
  • Just using a txt file and showing all or none of the lines is sufficient in the scope of this ticket. The distinction between individual tips seems more like ballast until we have manifested use cases for distinguishing the tips individually (for example a checkbox for every tip). If we have those use cases, the implementation could be completed too. We could also identify the tooltips by the hash of their content, see also #4399.
  • default.cfg structure might be cleaner with gui.splashscreen.enable, gui.splashscreen.version, gui.gamesetup.singleplayertips.
  • Checkbox pixels: I had it with other checkboxes as well, for example the splash screen dialog one fixed by 0686617. Maybe its more like 21.4% of the area that doesn't react to clicks.

I can't reproduce it. I've no problem to click in these checkbox.

  • I'd be happy to add these things to the wiki:Coding_Conventions, but it should likely be decided by programmers in a forum thread including other points of contention as well
  • Gamesetup code looks more modfriendly to me now. The two gamesetup options should be neighbors.
  • Singleplayer is more readable than SP, not only in the code but even more importantly in the options.

comment:5 by elexis, 7 years ago

Keywords: review added; rfc removed

With regards to the checkbox width: Try clicking on the border of the icon. It's not much at all, but I ran into it twice when trying to click on them genuinely.

For comparison:

  • Splash screen, AI Info use 14px
  • Gamesetup options, Lobby full game filter 20px
  • Options 30px

From my testing 20px seems to be the best of these three choices. The ModernTickBox style definition in modern/styles.xml and ModernTickOn/Off definition in modern/sprites.xml indicate 22px. The patch should use one of these values in my opinion.

Patch accepted from my side otherwise. Didn't test the latest revision though.

The 500px reserved for the Show this message in the future string should also be sufficient for translations. The order of properties is in the XML is inconsistent, but that's mostly irrelevant.

comment:6 by mimo, 7 years ago

Owner: set to mimo
Resolution: fixed
Status: newclosed

In 19057:

add some optional info on ai level in the gamesetup panel, fixes #4406
redefine the splashscreen options
reviewed by elexis

comment:7 by mimo, 7 years ago

Thanks for the reviews (I've increased the dropbox to 20px)

comment:8 by elexis, 7 years ago

Keywords: review removed

Thanks for the patch and pixels xP

comment:9 by elexis, 7 years ago

Milestone: Work In ProgressAlpha 22
Note: See TracTickets for help on using tickets.