[PATCH] Structure tree doesn't consider promotion

[elexis]

The structure tree shows that the spartan swordsmen have 5 armor and 5.5 hack attack. But if you train them, they are spawned with full promotion, yielding 7 armor and 7.7 hack attack, i.e. the structure tree shows a wrong value.

Looking at the template spart_champion_infantry_sword.xml, the line

<Promotion disable=""/>

must be responsible for this behavior. Not sure if it's a bug or intended. Either way, the structure tree show's an incorrect value.

[leper]

No, that line isn't the cause. The cause is the Rank of the unit and the Promotion techs. But no tech has any effect on the values in the structree since we do not know what is researched.

A better solution would be to support techs for the in-game structree (so use the currently researched techs if we have that data available) and apply it. (Don't we have a ticket for that somewhere?)

[fatherbushido]

The remaining problem is that this techtree is displayed without any simulation on (from starting screen for example).

[s0600204]

If no-one objects, I'll claim this ticket. Got a couple of days free so patch incoming soon.

[s0600204]

Proposed patch.

[sanderd17]

Looking at the code, you're trying to reproduce technology behaviour. So that's just code duplication, and whenever something changes to the technology code, it will have to be updated on the structure tree.

IMO, it should be better to check if you are in the simulation, and then get the values from the GuiInterface (checking if you are in the simulation could be done by checking if you get a valid answer from the GuiInterface).

As long as you're not in the simulation, you can't apply techs (even not auto-researched ones at the start of the game), since you don't know what the civ of the player will be (it's possible someone captures the CC).

It might also be good, when you're in simulation, to show the difference between the default value, and the tech modified value.

[fatherbushido]

I just ref some related tickets #3801 #3212

[elexis]

An alternative approach would be to change the templates of those spartan swordsmen, so they have the same stats but don't start promoted. Similarly for #3801, the templates could be changed so that autoresearched techs don't need to be researched. Not really satisfying though, as we don't want to forbid autoresearched techs or producing promoted units.

[sanderd17]

So the issue with this is that spart_champion_infantry_sword.xml doesn't inherit from champion infantry, but from regular citizen-soldier stats. But then it gets an "Elite" flag applied, which gives these differences in stats.

This is a rather strange construction IMO, and I see no reason why we should keep this template as such. It would be better to make it a real champion and let it inherit from the champions (probably with some adapted stats).

[sanderd17]

My proposed changes to the template (it's somewhere between basic infantry and champion now, which fits the design)

[fatherbushido]

From discussions on irc, it seems that one of the best solution is to do as ​http://trac.wildfiregames.com/ticket/3801#comment:12 for promotion tech.

[elexis]

IMO we should combine sanderd17 and s0600204 approach.

Showing autoresearched techs like the elite rank and civ boni in a separate icon is nice and sound.

But if the tech tree shows skiritai commandos spawning as elite, it shoul show the stats for elite template rather than the standard template which will never appear in a game.

Equally, the other autoresearched techs should be applied to show the accurate values after the gamestart. The hellenic buildings should already apply the +10% HP bonus, the carthaginian walls should show that value also.

Review:

• attachment:spart_changes.diff​​ won't happen since it would remove the ability to gather resources
• attachment:3747_promotionModifier.patch​​ does what is wanted. However
  • (1) needs to be rebased
  • (2) hardcodes the ranking access. If its inevitable to hardcode things, it should be in a global const array, so it can easily be extended by only changing that variable.
  • (3) duplicates logic as mentioned by sanderd17. That might be inevitable if we want to show the correct values without having a game started. But perhaps you can improve the code by calling simulation functions (like those from ValueModifications.js) through the GUIInterface.js.
  • (4) should attempt to apply the other techs mentioned by fatherbushido and sanderd17 in #3801 (unless you can convince me that promotion is completely different from those in that ticket)

[s0600204]

Rebased patch, as requested. Applies modifiers from promotion and civbonuses.

[elexis]

Putting in review queue so it gets rereviewed.

[s0600204]

Alternate approach. Requires #3993 applied.

[s0600204]

With style changes as suggested by elexis. Still requires #3993.

[s0600204]

Rebased, and requires v4 patch of #3993

[fatherbushido]

I will first put it in rfc queue as it needs #3993 before.

[Itms]

Pushed to A22 as suggested by fatherbushido.

[elexis]

Moving to the new WIP milestone.

[fatherbushido]

s0600204: We can reinvest that one now. Does it need some rebasing/cleaning before? Have you other ideas or should I start to review?

[s0600204]

Rebase after Tech Reqs work (#4263) was merged into main trunk

[fatherbushido]

r19410.