Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3522 closed defect (fixed)

[PATCH] When a wonder is destroyed, the +50 pop bonus should disappear too

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

Description

After researching the +50 pop at the wonder, the building itself becomes quite useless. Sure it gives you territory, but that's by far not as important as the +50 pop tech.

People should have to guard it so that they can keep the +50 pop advantage.

Also gives an incentive to the attacker to focus on the wonder and cause a lot of economic damage, as the wonder needs to be rebuilt (like 4k wood and food or so).

Also how about some aura? After all, it's a wonder and people should be more willing to fight or something. Maybe the population bonus can also be implemented as an aura, thus it could easily disappear when the wonder is destroyed.

Attachments (10)

3522.diff (1.4 KB ) - added by Stan 8 years ago.
Invert the costs of the tech and the wonder. It doesn't really make sense though on resource POV.
3522.1.diff (1.4 KB ) - added by Stan 8 years ago.
This patch makes more sense in the resource Pov. I made it separate cause there will be differents proposal and this is the base.
3522.2.diff (5.8 KB ) - added by fatherbushido 8 years ago.
3522.3.diff (6.9 KB ) - added by fatherbushido 8 years ago.
missing space, purging old json file
3522.3.2.diff (5.5 KB ) - added by fatherbushido 8 years ago.
this patch doesn't remove the old json file
3522.3.3.diff (5.9 KB ) - added by fatherbushido 8 years ago.
merging things as suggested by elexis
3522.4.diff (14.6 KB ) - added by fatherbushido 8 years ago.
linked to #3792 patch
3522.5.diff (20.0 KB ) - added by fatherbushido 8 years ago.
3522.6.diff (20.0 KB ) - added by fatherbushido 8 years ago.
3522.6.2.diff (20.0 KB ) - added by fatherbushido 8 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 by Stan, 8 years ago

I think it should increase trade income as this basically what happens when you have a touristic site.

comment:2 by elexis, 8 years ago

Also it costs only 1000 foood 1000 wood 500 metal 500 stone or similar to build the wonder, but like 3k food wood to upgrade. It should be the other way around, so that losing the wonder without having it researched costs like 3/4 of the total cost, not 1/4.

by Stan, 8 years ago

Attachment: 3522.diff added

Invert the costs of the tech and the wonder. It doesn't really make sense though on resource POV.

by Stan, 8 years ago

Attachment: 3522.1.diff added

This patch makes more sense in the resource Pov. I made it separate cause there will be differents proposal and this is the base.

comment:3 by elexis, 8 years ago

Keywords: simple design patch added
Summary: When a wonder is destroyed, the +50 pop bonus should disappear too[PATCH] When a wonder is destroyed, the +50 pop bonus should disappear too

Using a global aura for the max-popcap might work, see this global aura of the briton hero Boudicca:

    <Aura1>
      <Type>global</Type>
      <Affects>Champion</Affects>
      <Modifications>
        <UnitMotion.WalkSpeed> <Multiply>1.25</Multiply> </UnitMotion.WalkSpeed>
        <Attack.Ranged.Pierce> <Add>5</Add> </Attack.Ranged.Pierce>
        <Attack.Melee.Hack> <Add>5</Add> </Attack.Melee.Hack>
        <Attack.Capture.Value> <Add>2</Add> </Attack.Capture.Value>
      </Modifications>
      <AuraName>Champion Army</AuraName>
      <AuraDescription>+5 Attack, +2 Capture and +25% Speed for Champion Units.</AuraDescription>
    </Aura1>

comment:4 by Stan, 8 years ago

You can't access player.maxPop though ...

comment:5 by elexis, 8 years ago

Keywords: simple removed

After doing some testing it seems an aura doesn't work as Aura.js and AuraManager.js only accept changes for units. Might be wrong.

In case we go with the aura, it should look like this:

  <Auras>
    <Aura1>
      <Type>global</Type>
      <Affects>Something</Affects>
      <AffectedPlayers>Player</AffectedPlayers>
      <Modifications>
        <Player.MaxPopulation> <Add>50</Add> </Player.MaxPopulation>
      </Modifications>
      <AuraName>Population Bonus</AuraName>
      <AuraDescription>Population Bonus</AuraDescription>
    </Aura1>
  </Auras>

comment:6 by Stan, 8 years ago

I think it should be done with the aura since it is the best way to add and remove bonuses on destroy.

comment:7 by wraitii, 8 years ago

Milestone: BacklogAlpha 20

in reply to:  6 comment:8 by elexis, 8 years ago

Replying to stanislas69:

should be done with the aura since it is the best way to add and remove bonuses on destroy.

in particular it rewards the capturing of a wonder.

As the +20% territory bonus of the gymnaseon is also implemented using an aura it would be somewhat consistent too.

comment:9 by elexis, 8 years ago

Milestone: Alpha 20Backlog

Backlogging due to lack of progress.

comment:10 by fatherbushido, 8 years ago

I have a patch who solve this defect as expected and fixes more general stuff. But i need :#3769 before.

by fatherbushido, 8 years ago

Attachment: 3522.2.diff added

comment:11 by fatherbushido, 8 years ago

ApplyValueModificationsToPlayer@ValueModifications.js is only called by GetMaxPopulation@Player.js. I modify it to take into account auras and to use computations from ApplyTemplateModifications instead of ApplyModifications. I also add an iddentity and a class to player entity, so auras can affect global players settings like technologies.

Related to the previous ideas of elexis and stanislas69, i set total cost of the wonder as the previous structure+technology.

With this patch :

  • when building the wonder, we get the +50 pop global auras.
  • If the wonder is destroyed, we lose it. If someone capture the wonder, he get the +50 pop.
  • If one have a wonder and capture the ennemy wonder, he get +100 pop. It doesn't change the total max units.

It would be very nice to enhance strategy choices (building, protect, capture...).

comment:12 by fatherbushido, 8 years ago

Milestone: BacklogAlpha 20

comment:13 by fatherbushido, 8 years ago

Keywords: review added

by fatherbushido, 8 years ago

Attachment: 3522.3.diff added

missing space, purging old json file

by fatherbushido, 8 years ago

Attachment: 3522.3.2.diff added

this patch doesn't remove the old json file

by fatherbushido, 8 years ago

Attachment: 3522.3.3.diff added

merging things as suggested by elexis

comment:14 by elexis, 8 years ago

You get +100pop if you have two wonders of two different civs. But if you have two wonders of the same civ, you only get +50pop.

Also it breaks the tests, which can be solved with an early return in ApplyValueModificationsToPlayer in case cmpTemplateManager wasn't found.

comment:15 by fatherbushido, 8 years ago

Thanks for the remarks.

  • We need to add this check.
  • It's an expected behavior of auras : if one have two macedonians theater (one and one captured), auras don't stack. But if one have one macedonian theater and one spartan theater (captured), auras stack. I think it's not really a problem since a wonder is related to the glory of a civ and we have one bonus per civ, not per player.
Last edited 8 years ago by fatherbushido (previous) (diff)

comment:16 by mimo, 8 years ago

The wonder cost has increased a lot because it is now the sum of the previous wonder + tech costs. IMO that is not what we should do, when there are some cases where you are not interested by the population bonus (when wonder victory for example). I think it would be better to keep the wonder and tech separation, but with the tech effect only enabling the aura. It should be doable relatively simply by adding a requiredTechnology property to auras, and then line 74 to 93 inside the Auras Init function would be executed only when it is empty, otherwise they would be executed when the required technologies are researched..

You've added the GarrisonHolder component to the template_structure_wonder.xml, but be aware that it was already defined in the specific civ wonders. To have them all garrisonable now is certainly better, but is there a reason to give them a Max garrison of 20 (while previously defined had 30) ? some justification about these choices would be good. And while modifying them, also adding some cleaning in these templates (for example the elements like EjectHealth or EjectClassesOnDestroy which are identical for all wonders do not need to be redefined in the civ specific ones).

Last edited 8 years ago by mimo (previous) (diff)

comment:17 by fatherbushido, 8 years ago

I cleaned the templates and set garrison at 30 for all civs.

Adding requiredTechnology doesn't work in the init function since we need entities owner to check if the technology is yet searched.

If we sum up, there is only one case where we are not interested by the population bonus : it's for wonder game. We juste make it more challenging with an expensive wonder.

Perhaps we can just decrease the costs a bit ?

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

by fatherbushido, 8 years ago

Attachment: 3522.4.diff added

linked to #3792 patch

comment:18 by mimo, 8 years ago

Cc: sanderd17 added

In fact, after looking at the Auras.js code, i think it would be even simpler to test on the techs availability in ApplyBonus and ApplyTemplateBonus (maybe also caching the result of this check to avoid doing it too often), and make a Clean when the required techs are researched (adding sanderd17 on cc).

comment:19 by fatherbushido, 8 years ago

As we discussed (mimo, sanderd17 and me) on irc, we implement modifications by technology of modifications of the aura.

by fatherbushido, 8 years ago

Attachment: 3522.5.diff added

by fatherbushido, 8 years ago

Attachment: 3522.6.diff added

by fatherbushido, 8 years ago

Attachment: 3522.6.2.diff added

comment:20 by mimo, 8 years ago

Keywords: review removed

Fixed by r17809, thanks for the patch

comment:21 by mimo, 8 years ago

Resolution: fixed
Status: newclosed

comment:22 by elexis, 8 years ago

In 17834:

Fix the test of the Player-component following r17809. Patch by fatherbushido, fixes #3803, refs #3522.

comment:23 by elexis, 8 years ago

Keywords: design removed
Milestone: Alpha 20Alpha 21

The feature was reverted in alpha 20 with r17907 due to (#3830 and #3792), It was fixed by r17982 in alpha 21.

Note: See TracTickets for help on using tickets.