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

[leper]

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.

[Matias Lavik]

Ticket #3670

[Matias Lavik]

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.

[fatherbushido]

Thanks for your interest in that ticket.

• At first, I'll suggest you to wait for #3993 wich will fix some requirements issues (see #4108 too).
• Then you should be really carefull with the combinaison of requirements. I guess you have yet checked ​http://trac.wildfiregames.com/wiki/Technology_Templates.
• At end, there are some identation problems, you can read for example: ​http://trac.wildfiregames.com/wiki/CodeQuality and ​http://trac.wildfiregames.com/wiki/Coding_Conventions.

[fatherbushido]

If you want you can work at #4180.

[Matias Lavik]

Replying to fatherbushido:

If you want you can work at #4180.

Thanks, I will take a look at that.

[s0600204]

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.

[elexis]

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.

[s0600204]

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).

[fatherbushido]

(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)

[s0600204]

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

[elexis]

• 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?

[fatherbushido]

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.

[fatherbushido]

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.

[elexis]

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

[fatherbushido]

Indeed.

[s0600204]

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

[s0600204]

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.

[elexis]

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).

[elexis]

• 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.

[s0600204]

Relocate GetTemplateNamesByClass function to GuiInterface

[s0600204]

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.

[elexis]

• 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)

[s0600204]

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?

[elexis]

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

[fatherbushido]

I prefer v1 approach.

[s0600204]

Replying to fatherbushido:

I prefer v1 approach.

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

[fatherbushido]

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).

[Imarok]

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