bugFreeciv - Bugs: bug #16080, The Great Wall affects all units...

 
 
Show feedback again

bug #16080: The Great Wall affects all units regardless of whether they're inside cities

Submitted by:  None
Submitted on:  Wed 26 May 2010 07:30:40 AM UTC  
 
Category: NoneSeverity: 4 - Important
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Originator Email: -unavailable-
Open/Closed: ClosedRelease: 2.2.0
Operating System: NonePlanned Release: 2.2.2,2.3.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sat 14 Aug 2010 09:13:48 PM UTC, comment #11:

This ticket remained open pending a decision about whether to backport to S2_1. Since no-one's said anything, I think I'm not going to; users may be sticking with 2.1.x rather than moving to 2.2.x because they're happy with the gameplay, whether or not it's what was originally intended, or to finish existing games; this seems like a big gameplay change to drop into a series that's late in its maintenance cycle.

So I've closed this without fixing on S2_1. I've rescued kriss' original documentation improvements in patch #1854.

kriss: if you want to go ahead with the Wall->Def rename, you probably want to raise a new patch ticket for it. I don't know whether we have a policy on such renames.
I think you'll need to work on the help; for F_IGDEF, "Ignores the effects of defense bonus" could be misinterpreted as ignoring all defence bonuses (e.g., from terrain); you'll need to find a way to marry it up with the EFT_DEFEND_BONUS and F_BADDEFATTACKER in user-visible terms in the autogenerated help, or alternatively stop mentioning it at all and rely on the ruleset-specific help to describe these effects in terms that make sense for that ruleset. (Neither of the latter two are documented in autogenerated help currently.)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Thu 12 Aug 2010 08:51:02 AM UTC, comment #10:

Please, see also bug #16308.

pepeto <pepeto>
Project Member
Fri 09 Jul 2010 11:06:47 PM UTC, comment #9:

Because IGWALL and BADWALLATTACKER could be thought has having something to do with walls, when in fact they actually do not, and never did so because of their influence on other buildings (eg. Coastal Fortress), we could give them names closer to their semantic. This change could occur both in the code and in the rulesets to limit confusion for both developers and modders.

Thus, I have attached a patch that renames each references of IGWALL or BADWALLATTACKER to IGDEF or BADDEFATTACKER, respectively, as well as the variables dealing with them. Comments have also been added/updated in the code and in the rulesets to reflect the changes.

The patch is applicable only to the trunk, but please note that, whereas bundled rulesets have been updated to use the new values for these flags ("IgDef" & "BadDefAttacker") to preserve compatibility with the new code, its application could make current/alternative rulesets incompatible without minor edits.

(file #9469)

Christophe Vidal <kriss>
Tue 06 Jul 2010 10:38:47 PM UTC, comment #8:

On trunk, the experimental ruleset also needed fixing.

What about S2_1? We don't have the CityTile effect there (it originally came from RT#40207, and a different fix was used on S2_1). Will there ever be another 2.1.x release, and if so, is this an appropriate thing to fix in it, given how long it's been the way it is? If so I guess CityTile needs backporting.

>> EFT_DEFEND_BONUS is already pretty tightly tied to implementing
>> city walls or similar city defences
>
> Doesn't existence of this bug prove otherwise :-)


:) I mean that it's all tied up with things called IGWALL and BADWALLATTACKER (apropos of which, there's some documentation changes in kriss' original patch that I want to rescue). I suppose there's nothing stopping you implementing something non-wall-like with those flags despite their names.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 06 Jul 2010 10:28:13 PM UTC, SVN revision 17541:

The Great Wall's defensive effect was not limited to units in cities.

Reported anonymously; patch by me and Christophe Vidal (kriss@gna).

See gna bug #16080

(Browse SVN revision 17541)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Tue 06 Jul 2010 10:27:38 PM UTC, SVN revision 17540:

The Great Wall's defensive effect was not limited to units in cities.

Reported anonymously; patch by me and Christophe Vidal (kriss@gna).

See gna bug #16080

(Browse SVN revision 17540)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 30 Jun 2010 04:44:25 PM UTC, comment #5:

I have to admit that jtn's fix is better. Indeed, I forgot about the existence of the CityTile requirement type ;)

Following the same principle, I have included a fix for civ1 & civ2 rulesets, which, besides the default ruleset, are both affected by the problem. The fix is applicable to both the trunk and the S2_2 branch.

(file #9395)

Christophe Vidal <kriss>
Wed 30 Jun 2010 02:53:59 PM UTC, comment #4:

I definitely prefer jtn's fix. We should not be adding hardcoded properties to the effects, but making system more flexible.

> EFT_DEFEND_BONUS is already pretty tightly tied to implementing
> city walls or similar city defences


Doesn't existence of this bug prove otherwise :-)

Marko Lindqvist <cazfi>
Project Administrator
Wed 30 Jun 2010 12:11:32 AM UTC, comment #3:

Here's the essence of an alternative fix, which adds requirements to the effects rather than changing the semantics of EFT_DEFEND_BONUS in the code.

I'm not sure I don't prefer your fix, though. EFT_DEFEND_BONUS is already pretty tightly tied to implementing city walls or similar city defences, so it probably doesn't hurt. Plus your fix could be back-ported to S2_1 if desired, whereas mine couldn't (the CityTile universal doesn't exist on S2_1 IIRC).

(file #9391)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 28 Jun 2010 10:36:40 PM UTC, comment #2:

The problem is due to the fact that the effect EFT_DEFEND_BONUS is potentially applicable to any unit - including outdoor units - in the game and that for the Great Wall, as the game is checking for this wonder player-wide, the bonus would be applied player-wide too.

Resolving this problem yielded another related issue: for the same reasons, the firepower reset of units with F_BADWALLATTACKER attacking fortified cities did actually happen when attacking any unit - including outdoor units - of the nation controlling the Great Wall, making this nation even more hard to fight with custom rulesets using F_BADWALLATTACKER.

Hard-coding the EFT_DEFEND_BONUS range to garrisoned units only, attached patches fix the problems for both the trunk and S2_2.

(file #9382, file #9383)

Christophe Vidal <kriss>
Wed 23 Jun 2010 09:38:54 PM UTC, comment #1:

Confirmed on S2_2. Blimey, that seems like quite a serious error -- Great Wall confers a massive advantage (+200% defence for all land units). I wonder if that's behind any of the reports on the forums of invulnerable AI?

From inspection I think the same error applies to S2_1, but I haven't tested it. I think it probably came in around r11179, PR#14314 (2005!)

On the head of S2_2 I think it affects defense_multiplication() and also get_modified_firepower() (in the F_BADWALLATTACKER case, which isn't used in any of the default rulesets).

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 26 May 2010 07:30:40 AM UTC, original submission:

All outdoor land units gets the defence bounus
if its nation builds the wonder of Great Wall.

Anonymous

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #9469:  trunk-defend-bonus-semantic.diff added by kriss (15kB - application/octet-stream)
file #9395:  greatwall-effect.diff added by kriss (1kB - application/octet-stream)
file #9391:  S2_2-greatwall-effect.diff added by jtn (608B - text/x-patch - Alternative fix via existing effects)
file #9382:  trunk-great-wall.diff added by kriss (2kB - application/octet-stream)
file #9383:  S2_2-great-wall.diff added by kriss (2kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by kriss (Updated the item)
  • -unavailable- added by jtn (Posted a comment)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 14 Aug 2010 09:13:48 PM UTCjtnOpen/ClosedOpen=>Closed
      Operating SystemMicrosoft Windows=>None
    Fri 09 Jul 2010 11:06:46 PM UTCkrissAttached File-=>Added trunk-defend-bonus-semantic.diff, #9469
    Tue 06 Jul 2010 10:28:47 PM UTCjtnPlanned Release2.2.2=>2.2.2,2.3.0
    Tue 06 Jul 2010 10:28:38 PM UTCjtnSeverity3 - Normal=>4 - Important
      StatusNone=>Fixed
      Planned Release=>2.2.2
    Sat 03 Jul 2010 01:04:17 AM UTCjtnAssigned toNone=>jtn
    Wed 30 Jun 2010 04:44:25 PM UTCkrissAttached File-=>Added greatwall-effect.diff, #9395
    Wed 30 Jun 2010 12:12:55 AM UTCjtnSummaryThe Great Wall affects all units which is staying out of City=>The Great Wall affects all units regardless of whether they're inside cities
    Wed 30 Jun 2010 12:11:32 AM UTCjtnAttached File-=>Added S2_2-greatwall-effect.diff, #9391
    Mon 28 Jun 2010 10:36:40 PM UTCkrissAttached File-=>Added trunk-great-wall.diff, #9382
      Attached File-=>Added S2_2-great-wall.diff, #9383
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup