Opened 5 years ago

Last modified 20 months ago

#3670 new enhancement

[PATCH] Add "Unlocks X.", "Requires Y." to tech tooltips automatically.

Reported by: leper Owned by:
Priority: Nice to Have Milestone: Work In Progress
Component: UI – In-game Keywords: simple rfc patch
Cc: fatherbushido Patch:

Description (last modified by elexis)

Currently we have many techs that have explicit "Requires foo." or "Unlocks bar." in their tooltips. Since this both requires translators to match foo to the tech foo to keep things consistent, but also requires repeat translation of the "Requires ..." part we should generate those automatically.

This way changing the name of one technology does not require additions to multiple files and simplifies the tooltips of multiple technologies.

http://i.imgur.com/iigxJm2.jpg

Attachments (6)

3670_req_tooltip.patch (2.7 KB ) - added by Matias Lavik 4 years ago.
Ticket #3670
3670_reqTooltips.patch (3.8 KB ) - added by s0600204 4 years ago.
Patch that takes into account #3993/#4263 tech reqs work. Does not strip out old requirementsTooltip property from tech templates (for now) so as to make it easier to compare old vs new in game.
3670_reqTooltips_v1-1.patch (6.2 KB ) - added by s0600204 4 years ago.
Attempt at improving styling of resultant requirements string over last patch, and take into account pair techs
3670_remove_requirementsTooltip_attribute.patch (108.7 KB ) - added by s0600204 4 years ago.
Additonal patch that removes the requirementsTooltip property from technology json files where one exists. Not merged in with main patch due to length.
3670_reqTooltips_v1-2.patch (11.1 KB ) - added by s0600204 4 years ago.
Show list of templates that match the required class instead of the class itself (http://i.imgur.com/iigxJm2.jpg) Works with both Classes or VisibleClasses.
3670_reqTooltips_v1-3.patch (10.6 KB ) - added by s0600204 4 years ago.
Relocate GetTemplateNamesByClass function to GuiInterface

Download all attachments as: .zip

Change History (30)

by Matias Lavik, 4 years ago

Attachment: 3670_req_tooltip.patch added

Ticket #3670

comment:1 by Matias Lavik, 4 years ago

I have added a patch which does the following:

  • If no requirement-tooltip is set, then it generates a tooltip where requirements are separated by commas.

For class-requirements (example: "you need 3x foo to do bar"), I have simply added "(3x)" behind the class-name. This should look OK in all languages, I suppose?

In cases where "any" or "all" is specified in the JSON-file, I added "Any:" or "All:" before the list of requirements. My first thought was to connect the requirements with commas and finally "and" or "or", but this does not work well in all languages.

This is my first patch, so please let me know if I overlooked something important. I'm not very experienced with the codebase yet.

comment:2 by fatherbushido, 4 years ago

Thanks for your interest in that ticket.

Last edited 4 years ago by fatherbushido (previous) (diff)

comment:3 by fatherbushido, 4 years ago

If you want you can work at #4180.

in reply to:  3 comment:4 by Matias Lavik, 4 years ago

Replying to fatherbushido:

If you want you can work at #4180.

Thanks, I will take a look at that.

by s0600204, 4 years ago

Attachment: 3670_reqTooltips.patch added

Patch that takes into account #3993/#4263 tech reqs work. Does not strip out old requirementsTooltip property from tech templates (for now) so as to make it easier to compare old vs new in game.

comment:5 by s0600204, 4 years ago

Keywords: rfc patch added
Milestone: BacklogWork In Progress
Summary: Add "Unlocks X.", "Requires Y." to tech tooltips automatically.[PATCH] Add "Unlocks X.", "Requires Y." to tech tooltips automatically.

comment:6 by elexis, 4 years ago

Code looks good on first sight. i18n must be improved though:

  • translatePlural("%(techNames)s technology", "%(techNames)s technologies", requiredTechs.length) why is that argument a name and not a count when you pass a count?
  • "\"" + getEntityNames(GetTechnologyData(tech)) + "\"" -> Some languages might want to use something other than quotes or extend that by some string or characters, so use a sprintf(translate("\"%(techName)s\"")) with a // Translation: comment, or remove the quotes altogether since we don't use them elsewhere, do we?
  • translate(" & ") would require a translation comment explaining what that is about, likely the same comment as with the quotes, or probably more readable, do something like this from l10n.js:
    		amounts = sprintf(translate("%(previousAmounts)s and %(lastAmount)s"), {
    			// Translation: This comma is used for separating first to penultimate elements in an enumeration.
    			"previousAmounts": amounts.join(translate(", ")),
    			"lastAmount": lastAmount
    		});
    
  • Don't see why we shouldn't remove some of the redundant tooltips as the fruit of your work, for example the entirely duplicate ally-dropsite requirements tooltip.
  • "entities" is uncommon and probably used nowhere else in the GUI, so we should avoided, not sure how though since we don't have plural forms of the visible classes (yet?)
  • Are all class names translated? If a tech refers to a Classes entry and not a VisibleClasses entry, that name is not extracted according to messages.json, hence broken. So perhaps extract all classes to cover future cases as well, or carefully check all classes and add some hint somewhere (code? wiki?) hoping that future devs don't run into this pitfall.
  • That one translatePlural is huge. As seen in menu.js, it is possible to add \n before the arguments.

comment:7 by s0600204, 4 years ago

why is that argument a name and not a count when you pass a count?

I'm either not following, or you've got confused. The count is passed to translatePlural to determine which string to use. It is not passed as a replacement for %(techNames)s - that is provided to sprintf on the next line of the script.

As to the quote marks, which of these three is preferred/most clear?:

Requires Town Phase and Wedge and Mallet technologies

Requires "Town Phase" and "Wedge and Mallet" technologies

Requires Town Phase and Wedge and Mallet technologies

Are all class names translated?

Nope. There is also no way of currently telling which classes (visible or otherwise) belong to units and which belong to structures. Hence, "entities". Admittedly neither

5 entities of class "Town" (3 remaining)

or

5 units or structures of class "Town" (3 remaining)

is particularly nice (whether or not "s are used).

comment:8 by fatherbushido, 4 years ago

(s0600204: elexis meant that classes wich appears in json files should be VisibleClasses in the respective templates (see also doc in Identity.js), but I guess it changes nothing to your patch)

Last edited 4 years ago by fatherbushido (previous) (diff)

by s0600204, 4 years ago

Attachment: 3670_reqTooltips_v1-1.patch added

Attempt at improving styling of resultant requirements string over last patch, and take into account pair techs

comment:9 by elexis, 4 years ago

  • Ah yes, that translatePlural confused me. I'm not sure about the need of adding technology / technologies here and whether using a plural makes sense this way (since one needs only one technology of each), but seems passable (otherwise transifex people will bug).
  • We don't need that tip variable anymore and can just push(templatething, deriveTechRequirementsTooltipthing).
  • If non-visible classes are added as a requirement, they won't be translated, thus players with chinese localization will still have english strings in there. I advise against ignoring this discovered issue. A third alternative would be checking whether there exists a translation of that classname and if not, show a warning. But that will bug modders who don't have any translations at all. In the worst case, document this caveat somewhere as mentioned before (unfortunately we don't have schematics for the JSON files, so no central point to append documentation). Perhaps we should ask leper whether it is indeed as bad to have a non-visible class visible as it seems to me and how to address.

A fourth approach that seems to work would be to inform messages.json to treat all class entries in these JSON files as visible classes and mark them for translation.

  • translate(" and ") and translate("; or ") still need a translation comment informing transifex users of where this is used
  • So if the hardcoded strings that are equivalent to the new dynamic string aren't removed in the same patch, will there be a follow up patch removing them?
  • L231 L232 too much indentation
  • Agree that the "units or structures" is worse. I guess if we wouldn't use classnames but template names, that issue were solved. Well, rewrite everything isn't on the menu.
Last edited 4 years ago by elexis (previous) (diff)

comment:10 by fatherbushido, 4 years ago

So if the hardcoded strings that are equivalent to the new dynamic string aren't removed in the same patch, will there be a follow up patch removing them?

Does not strip out old requirementsTooltip property from tech templates (for now) so as to make it easier to compare old vs new in game.

I guess we'll remove them just before or when commiting.

comment:11 by fatherbushido, 4 years ago

I don't get the issue with VisibleClasses. We can just add in the technology wiki doc (and perhaps in Identity doc) that as the requirements will be displayed in the gui, the said Classes must be VisibileClasses? Or I miss something.

comment:12 by elexis, 4 years ago

A rule in the code can enforce things while a good recommendation in a wiki page can be overseen or forgotton.

Last edited 4 years ago by elexis (previous) (diff)

comment:13 by fatherbushido, 4 years ago

Indeed.

by s0600204, 4 years ago

Additonal patch that removes the requirementsTooltip property from technology json files where one exists. Not merged in with main patch due to length.

by s0600204, 4 years ago

Attachment: 3670_reqTooltips_v1-2.patch added

Show list of templates that match the required class instead of the class itself (http://i.imgur.com/iigxJm2.jpg) Works with both Classes or VisibleClasses.

comment:14 by elexis, 4 years ago

Even if it seems weird to say that 5 Houses or Storehouses or ... are needed, this solutions yields much better strings in my opinion.

Take care, the translation comments need to appear before the line before of the translate call, not afterwards.

(Also unless adding a new field PluralName, we won't be able to use plural strings, as the untranslated plural string needs to be specified in english somewhere).

comment:15 by elexis, 4 years ago

  • Didn't test, but looks like GetTemplateData(templateName).name.generic needs a translate()
  • The loading of templates might be a bit controversial as it's data is solely used in the GUI and never in the simulation. Maybe it's okay to load the list of entities from the simulation though, since that has access to the current map and player settings.
  • The "just in time" loading looks prone to cause OOS if one client would call GetSimulationState earlier than another. Better init on init/deserialize.
  • This data in the player component is serialized, while it shouldn't (savegames should be minimal for disk space and rejoin speed). The caching could be done in the GUIInterface, as that already has some custom serialization code excluding some things.

by s0600204, 4 years ago

Attachment: 3670_reqTooltips_v1-3.patch added

Relocate GetTemplateNamesByClass function to GuiInterface

comment:16 by s0600204, 4 years ago

Replying to elexis:

Didn't test, but looks like GetTemplateData(templateName).name.generic needs a translate()

It doesn't - that is handled by GetTemplateData().

The loading of templates might be a bit controversial as it's data is solely used in the GUI and never in the simulation.

The output of the new function could theoretically be used by Petra to help remove some hard-coded template names. But that's a topic for another ticket.

The "just in time" loading looks prone to cause OOS if one client would call GetSimulationState earlier than another. Better init on init/deserialize.

The init of what? GuiInterface is a system-level component and is thus initialised before player-level components such as cmpPlayer and cmpTechnologyManager, both of which are required to compile the list; and as there's no guarantee of the initialisation order of same-level components, cmpTechnologyManager will not necessarily be initalised and ready for cmpPlayer's init to access it.

in reply to:  16 comment:17 by elexis, 4 years ago

  • I feel too many people will complain about missing plural forms :/
  • indexOf(ent) < 0 I'd always check for -1 if existance should be tested, since other negative numbers never occur
  • L745 duplicate \n

Replying to s0600204:

looks like GetTemplateData(templateName).name.generic needs a translate()

It doesn't

Ah yes, because that is the session proxy of GetTemplateData, not the GUIInterface one.

the new function

Maybe, still need to run this through the fatherbushido program.

OOS

Yes, init order of components might be relevant. Anyway, since you don't serialize playerTemplatesByClass in the GUIInterface, it shouldn't trigger an actual OOS (in the worst case, GetSimulationState would be called at different times, but with the same content returned).

(Also was wondering whether adding non-unique information (wiki/Single_source_of_truth) like this to GetSimulationState would unnecessarily bloat the size of savegames, but doesn't as only few things are extracted from it)

comment:18 by s0600204, 4 years ago

I feel too many people will complain about missing plural forms :/

It will mean affecting a lot of templates. Do you want that done in a separate ticket, or are you happy with combining it with the patches here?

comment:19 by elexis, 4 years ago

Cc: fatherbushido added

First we need a second developer opinion before you waste any time if the approach is rejected (and splitting things up is always welcome).

comment:20 by elexis, 4 years ago

Description: modified (diff)

comment:21 by fatherbushido, 4 years ago

I prefer v1 approach.

in reply to:  21 comment:22 by s0600204, 4 years ago

Replying to fatherbushido:

I prefer v1 approach.

Approach to which part? (Just so we're clear)

comment:23 by fatherbushido, 4 years ago

Displaying VisibleClasses seems more robust than displaying Name. So imo the complexity introduced by GetPlayerTemplatesByName() isn't worth it (and I guess we should avoid stuff like L778-L780).

(by the way -not really related- we should add somewhere in wiki that pair tech and phase tech filenames must be pair_* phase_* to not break -at least- the structure tree).

Last edited 4 years ago by fatherbushido (previous) (diff)

comment:24 by Imarok, 20 months ago

Component: UI & SimulationIn-game UI

Move tickets to In-game UI as UI & Simulation got some sub components.

Note: See TracTickets for help on using tickets.