Opened 4 years ago

Closed 3 years ago

#3801 closed defect (fixed)

[PATCH] Structure tree - civic centre health wrong

Reported by: elexis Owned by: s0600204
Priority: Should Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by fatherbushido)

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!

Attachments (1)

civbonus_tooltips.diff (11.0 KB) - added by sanderd17 4 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 4 years ago by 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

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

comment:2 Changed 4 years ago by 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

comment:3 Changed 4 years ago by fatherbushido

  1. for carth walls, see my first comment : triple_walls is an autoresearched technology.
  1. 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.

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

comment:4 Changed 4 years ago by elexis

So is it a duplicate of #3747?

comment:5 Changed 4 years ago by fatherbushido

yes it is (except for library)

comment:6 Changed 4 years ago by s0600204

Owner: set to s0600204
Status: newassigned

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

comment:7 Changed 4 years ago by fatherbushido

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

comment:8 Changed 4 years ago by 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.

comment:9 Changed 4 years ago by Itms

See also #3212

comment:10 Changed 4 years ago by 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.

comment:11 Changed 4 years ago by 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).

comment:12 Changed 4 years ago by 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

comment:13 Changed 4 years ago by 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

comment:14 Changed 4 years ago by 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

Changed 4 years ago by sanderd17

Attachment: civbonus_tooltips.diff added

comment:15 in reply to:  2 Changed 4 years ago by fatherbushido

Replying to elexis:

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

(that one: struct tree says library doubles CC healthpoints, while it only adds 3000 instead of 3300)

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

comment:16 Changed 4 years ago by fatherbushido

civbonus_tooltips.diff​ -> add to review list

comment:17 Changed 4 years ago by fatherbushido

Keywords: review added
Milestone: BacklogAlpha 22
Summary: Structure tree - civic centre health wrong[PATCH] Structure tree - civic centre health wrong

comment:18 Changed 4 years ago by elexis

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:19 Changed 4 years ago by fatherbushido

In 19083:

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

comment:20 Changed 4 years ago by fatherbushido

Keywords: review removed

comment:21 Changed 4 years ago by elexis

Keywords: patch added
  • 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).

comment:22 Changed 4 years ago by 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...).

comment:23 Changed 3 years ago by fatherbushido

Description: modified (diff)
Milestone: Work In ProgressAlpha 22
Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.