Opened 2 years ago

Closed 21 months ago

Last modified 20 months 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 stanislas69 2 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 stanislas69 2 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 21 months ago.
3522.3.diff (6.9 KB) - added by fatherbushido 21 months ago.
missing space, purging old json file
3522.3.2.diff (5.5 KB) - added by fatherbushido 21 months ago.
this patch doesn't remove the old json file
3522.3.3.diff (5.9 KB) - added by fatherbushido 21 months ago.
merging things as suggested by elexis
3522.4.diff (14.6 KB) - added by fatherbushido 21 months ago.
linked to #3792 patch
3522.5.diff (20.0 KB) - added by fatherbushido 21 months ago.
3522.6.diff (20.0 KB) - added by fatherbushido 21 months ago.
3522.6.2.diff (20.0 KB) - added by fatherbushido 21 months ago.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 2 years ago by stanislas69

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

comment:2 Changed 2 years ago by elexis

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.

Changed 2 years ago by stanislas69

Attachment: 3522.diff added

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

Changed 2 years ago by stanislas69

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 Changed 2 years ago by elexis

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 Changed 2 years ago by stanislas69

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

comment:5 Changed 2 years ago by elexis

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 Changed 2 years ago by stanislas69

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

comment:7 Changed 2 years ago by wraitii

Milestone: BacklogAlpha 20

comment:8 in reply to:  6 Changed 2 years ago by elexis

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 Changed 22 months ago by elexis

Milestone: Alpha 20Backlog

Backlogging due to lack of progress.

comment:10 Changed 21 months ago by fatherbushido

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

Changed 21 months ago by fatherbushido

Attachment: 3522.2.diff added

comment:11 Changed 21 months ago by fatherbushido

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 Changed 21 months ago by fatherbushido

Milestone: BacklogAlpha 20

comment:13 Changed 21 months ago by fatherbushido

Keywords: review added

Changed 21 months ago by fatherbushido

Attachment: 3522.3.diff added

missing space, purging old json file

Changed 21 months ago by fatherbushido

Attachment: 3522.3.2.diff added

this patch doesn't remove the old json file

Changed 21 months ago by fatherbushido

Attachment: 3522.3.3.diff added

merging things as suggested by elexis

comment:14 Changed 21 months ago by elexis

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 Changed 21 months ago by fatherbushido

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 21 months ago by fatherbushido (previous) (diff)

comment:16 Changed 21 months ago by mimo

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 21 months ago by mimo (previous) (diff)

comment:17 Changed 21 months ago by fatherbushido

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 21 months ago by fatherbushido (previous) (diff)

Changed 21 months ago by fatherbushido

Attachment: 3522.4.diff added

linked to #3792 patch

comment:18 Changed 21 months ago by mimo

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 Changed 21 months ago by fatherbushido

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

Changed 21 months ago by fatherbushido

Attachment: 3522.5.diff added

Changed 21 months ago by fatherbushido

Attachment: 3522.6.diff added

Changed 21 months ago by fatherbushido

Attachment: 3522.6.2.diff added

comment:20 Changed 21 months ago by mimo

Keywords: review removed

Fixed by r17809, thanks for the patch

comment:21 Changed 21 months ago by mimo

Resolution: fixed
Status: newclosed

comment:22 Changed 21 months ago by elexis

In 17834:

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

comment:23 Changed 20 months ago by elexis

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.