Opened 8 years ago
Closed 7 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 )
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)
Change History (24)
follow-up: 15 comment:2 by , 8 years ago
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 by , 8 years ago
- for carth walls, see my first comment : triple_walls is an autoresearched technology.
- 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.
comment:6 by , 8 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
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 by , 8 years ago
s0600204: so in structure you will display the value with autotech modifications ? nice.
comment:8 by , 8 years ago
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:10 by , 8 years ago
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 by , 8 years ago
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 by , 8 years ago
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 by , 8 years ago
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 by , 8 years ago
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
by , 8 years ago
Attachment: | civbonus_tooltips.diff added |
---|
comment:15 by , 8 years ago
comment:17 by , 7 years ago
Keywords: | review added |
---|---|
Milestone: | Backlog → Alpha 22 |
Summary: | Structure tree - civic centre health wrong → [PATCH] Structure tree - civic centre health wrong |
comment:20 by , 7 years ago
Keywords: | review removed |
---|
comment:21 by , 7 years ago
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
toarmor
(Also sometimes capitalized in the JSON strings, sometimes not). 4m extra range
-> meters?Carthaginian walls
uses3x
andtwice
, could bethrice
or2x
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 by , 7 years ago
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 by , 7 years ago
Description: | modified (diff) |
---|---|
Milestone: | Work In Progress → Alpha 22 |
Resolution: | → fixed |
Status: | assigned → closed |
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 :