[PATCH] Structure tree - civic centre health wrong

[elexis]

The structure tree says that the macedonian civic centre has 3000 HP, while it actually has 3300. The briton CC has less than 3000 HP but also shows that value.

Thanks Hannibal_Barca for reporting!

[fatherbushido]

Perhpas it's due to the autoresearched technologies of this civs ? (HP, buildtime).

Yes that's it. For brits, gauls, athenians, spartans, macedonians (for structure) and mauryans, carthagenians (for walls) the autoresearched technologies are not take into account in tech tree. You would see it easily i guess if maxpop was written (it would have been 300 for persians, spartans, mauryans).

There is 3 way to fix it :

• take this into account in tech tree
• write somewhere in tech tree that there is this bonus
• move those bonus to templates

2) seems the better imo

[elexis]

More reports from Hannibal_Barca, likely the same issue:

• struct tree says carth walls have 1000-3000 HP, while it is actually 6000 (wall) and 12000 (wall turret (!))
• struct tree says library doubles CC healthpoints, while it only adds 3000 instead of 3300

[fatherbushido]

1. for carth walls, see my first comment : triple_walls is an autoresearched technology.

2. For the library (special_hellenistic_metropolis tech), it's not related to structure tree.

2.a. It's expected as described in technology templates doc : multiply - adds (multiply-1)*originalValue to the property. Note this is not cumulative..

2.b. CC HP original value is 3000 (as said in structure tree). Autoresearch techs multiply by 1.1 so add 300 and library tech multiply by 2 HP so add 3000. Finally we get 3000+300+30000 = 6300 HP.

To sum up, structure tree does good in showing original value. We just need to add a frame for the autoresearched technologies.

[elexis]

So is it a duplicate of #3747?

[fatherbushido]

yes it is (except for library)

[s0600204]

elexis:

So is it a duplicate of #3747

Not quite. Similar problem, but it will require a different solution. (I was the person who wrote the structure tree so I should know :P)

Unfortunately, I can't think of a way for the structree to auto-detect the presence of civbonuses/penalties, so they'll have to be hard-coded somewhere. *sigh* Fortunately, I know just the place...

That said, I'm going to have to recode parts of the civinfo page to do it. If I suggested we got rid of the civinfo page (Once A20's out the door), would anyone be willing to support me on that? (If anyone actually likes the civinfo page, I do have a ​branch over on GitHub that incorporates similar functionality into the structree. But prettier(-ish))

[fatherbushido]

s0600204: so in structure you will display the value with autotech modifications ? nice.

[sanderd17]

s0600204: IMO, civinfo and structure tree are two different things. Civinfo doesn't give a lot of information about the game, but gives information about the real history. While structure tree gives info about the game.

That's also the reason why the structure tree is available from inside the game, and civinfo can only be accessed from the main menu. So I think they are complementary. But if you have something better, that could be good too.

However, civinfo has a lot of text, so we have to be really considerate if we want to remove that text. Since adding that text again will cause a lot of work for the translators.

PS: many units have a "history" tag too, which explains the history of that unit. It could be nice if this would be visible somewhere.

[Itms]

See also #3212

[s0600204]

fatherbushido: Theoretically, yes.

sanderd17, I'm not suggesting we remove the text, just relocate it.

I created the afore-mentioned branch on GitHub because I figured there was some worth in having that information (in particular the History) laid out in such a manner, but that it should be at the very least be kept up-to-date, automatically if possible. IMHO the main problem of the civinfo page is that it gathers its information from the {civ}.json files, and so unless someone remembers to update those files when things change in the entity templates, the page becomes out of date.

As the structree iterates through the entity templates in order to build its tree, it made sense to have it make note of a civ's Heroes, Special Structures and Technologies as it was doing so, then display them in a ​suitable manner. (The layout's not final). And yes, this makes use of the history tag in entity templates.

Oh, and the reason I didn't add unit/structure histories to the tooltips in the structree is because some are "." or "TBD" or "To be written later". And they're not currently translated. And I was trying to avoid changing entity templates just to get the structree to work/look good.

[fatherbushido]

As we discussed on irc, the issues of this tickets can be fixed by moving some civ bonus from autoresearched tech to template (which works nice since relative templates are implemented in r17386).

[sanderd17]

Looking further into this, all hellenic structures seem to be affected. Which is a lot of templates to modify.

So I would propose to list the civ bonuses as such in the tech tree:

• Give the civ bonus techs an icon and tooltip if they're missing one
• Extract the requirement checking from the technologies to a global function (with no-researched techs, no buildings or units made, ... as defaults)
• Display the autoresearched techs with no requirements and autoresearch somewhere on the top of the structree, under their icon with their tooltip

[fatherbushido]

Here are those techs, most (all) havn't tooltip nor icon.

• carthaginians/civbonus_triple_walls.json
• phase_village.json
• celts/civbonus_celts_wooden_struct.json
• persians/civbonus_pers_popcap.json
• mauryans/civbonus_maur_popcap.json
• mauryans/wooden_walls.json
• hellenes/teambonus_athen_delian_league.json
• hellenes/civpenalty_spart_popcap.json
• hellenes/civbonus_hellenic_architecture.json

And (see #3747)

• elite_unit_bonus.json
• advanced_unit_bonus.json

[sanderd17]

I added tooltips and icons to most of them (not to the village phase, as that doesn't have any effects). See the patch attached

For the icons, some placeholders need to be copied from other locations, executing the script in ps/trunk/binaries/data/mods/public/art/textures/ui/session/portraits should do.

svn cp structures/wall.png technologies/
svn cp structures/palisade_fort.png technologies/wooden_buildings.png
svn cp structures/wooden_gate.png technologies/wooden_walls.png
svn cp units/hele_ship_trireme.png technologies/ship_buildtime.png

[fatherbushido]

Replying to elexis:

This "issue" is now fixed in r18207 as it is now +50% of actual value instead of base value.

[fatherbushido]

civbonus_tooltips.diff​ -> add to review list

[elexis]

Moving to the new WIP milestone.

[fatherbushido]

In 19083:

Add tooltip and icon to some autoresearched techs to allow displaying them in the structure tree. Patch by sanderd17. Refs #3801.

[elexis]

• advanced class might be written with a capital letter, since it is a very specific thing (only the units that are Advanced (TM), but not all that could be considered advanced commonly (for example champions could commonly considered advanced, but don't have that class).

• Strings are in american english and there were some commits changing armour to armor (Also sometimes capitalized in the JSON strings, sometimes not).
• 4m extra range -> meters?
• Carthaginian walls uses 3x and twice, could be thrice or 2x for consistency

• Not too happy about introducing copies of images, but I see that the hardcoded image path is in the way of avoiding the copy.

• Not too fond with the hardcoded numbers in the strings, but there is a different ticket for that (and giving players the accurate numbers helps them).

Thanks for the patch, review and commit (was thinking about committing that some time ago as it was pre-reviewed in accordance with the commit guidelines, but couldn't find the ticket anymore).

[fatherbushido]

Thanks for your "commit review". May you commit those strings improvements? (would be nice). Yes path is hardcoded. Anyway doing so, we can replace those (placeholder) images are currently placeholder. (Numbers, for those entries, it's not the worst case, but that's another story...).

[fatherbushido]

r19410