Opened 8 years ago
Last modified 3 years 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: | rfc, patch, simple |
Cc: | fatherbushido | Patch: |
Description (last modified by )
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.
Attachments (6)
Change History (32)
by , 8 years ago
Attachment: | 3670_req_tooltip.patch added |
---|
comment:1 by , 8 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 , 8 years ago
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.
comment:4 by , 8 years ago
by , 7 years ago
Attachment: | 3670_reqTooltips.patch added |
---|
comment:5 by , 7 years ago
Keywords: | rfc patch added |
---|---|
Milestone: | Backlog → Work 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 , 7 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 froml10n.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 aVisibleClasses
entry, that name is not extracted according tomessages.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 inmenu.js
, it is possible to add\n
before the arguments.
comment:7 by , 7 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 , 7 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)
by , 7 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 , 7 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 justpush(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 allclass
entries in these JSON files as visible classes and mark them for translation.
translate(" and ")
andtranslate("; 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.
comment:10 by , 7 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 , 7 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 , 7 years ago
An rule in the code can enforce things while a good recommendation in a wiki page can be overseen or forgotton.
by , 7 years ago
Attachment: | 3670_remove_requirementsTooltip_attribute.patch added |
---|
Additonal patch that removes the requirementsTooltip
property from technology json files where one exists. Not merged in with main patch due to length.
by , 7 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 , 7 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 , 7 years ago
- Didn't test, but looks like
GetTemplateData(templateName).name.generic
needs atranslate()
- 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 , 7 years ago
Attachment: | 3670_reqTooltips_v1-3.patch added |
---|
Relocate GetTemplateNamesByClass
function to GuiInterface
follow-up: 17 comment:16 by , 7 years ago
Replying to elexis:
Didn't test, but looks like
GetTemplateData(templateName).name.generic
needs atranslate()
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.
comment:17 by , 7 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 atranslate()
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 , 7 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 , 7 years ago
Cc: | 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 , 7 years ago
Description: | modified (diff) |
---|
comment:22 by , 7 years ago
comment:23 by , 7 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).
comment:24 by , 5 years ago
Component: | UI & Simulation → In-game UI |
---|
Move tickets to In-game UI
as UI & Simulation
got some sub components.
comment:25 by , 3 years ago
Keywords: | simple removed |
---|---|
severity: | → simple |
comment:26 by , 3 years ago
Keywords: | simple added |
---|
Ticket #3670