bugFreeciv - Bugs: bug #22079, AI settler doesn't consider base...

 
 
Show feedback again

bug #22079: AI settler doesn't consider base defense bonus when picking city spot

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sat 24 May 2014 10:09:42 PM UTC  
 
Category: aiSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: pepeto <pepeto>Open/Closed: Closed
Release: Operating System: Any
Planned Release: 2.5.0, 2.6.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)

Tue 22 Jul 2014 09:44:26 AM UTC, SVN revision 25686:

Make AI settler to consider base defense bonus when picking city spot.

Reported by Jacob Nevins
Patch by Emmet Hikory and me

See gna bug #22079

(Browse SVN revision 25686)

pepeto <pepeto>
Project MemberIn charge of this item.
Tue 22 Jul 2014 09:44:20 AM UTC, SVN revision 25685:

Make AI settler to consider base defense bonus when picking city spot.

Reported by Jacob Nevins
Patch by Emmet Hikory and me

See gna bug #22079

(Browse SVN revision 25685)

pepeto <pepeto>
Project MemberIn charge of this item.
Sun 20 Jul 2014 02:02:32 PM UTC, comment #6:

Patches attached:

  • really link tile_city(vtile) to vcity ;
  • use upgrade_city_XXX() to simulate building the free extras on the virtual tile ;
  • do it only once per tile.

Note this will also fix that rivers (like other not buildable extras) will be also be taken in account.

(file #21477, file #21478)

pepeto <pepeto>
Project MemberIn charge of this item.
Tue 08 Jul 2014 11:39:33 AM UTC, comment #5:

In that case, my patches don't work, and I'm unsure how to make them work without deep requirements inspection, which I'd really not prefer. Also, looking at upgrade_city_extras(), it probably makes sense to try to find a way to check for conflicting extras (keeping in mind that if two extras conflict, and both would be created, the first in the iteration sequence would have blocked the creation of the second), which the candidate patches didn't handle correctly (tiles that could have multiple conflicting defensive extras would gain more bonus than they ought to have done).

Emmet Hikory <persia>
Project Member
Tue 08 Jul 2014 06:51:52 AM UTC, comment #4:

> do you mean that when passing vtile to
> player_can_build_extra(), is_req_active() is considering
> tile_city(vtile), which does not return vcity?


Yes. tile_city(vtile) will return what tile_city(ptile) would return. And no chance to return vcity.

pepeto <pepeto>
Project MemberIn charge of this item.
Tue 08 Jul 2014 05:24:07 AM UTC, comment #3:

I don't really understand the comment: do you mean that when passing vtile to player_can_build_extra(), is_req_active() is considering tile_city(vtile), which does not return vcity? If so, yes, this would break the logic. Is there a best way to ensure that is_citytile_in_range()is returning the correct result in this situation?

would_extra_exist_in_city() is intended to check whether a given extra would exist if a city was present on the provided tile (either be constructed automatically with the founding of the city, or if already present, not be destroyed when the city was built).

Thinking about this more, this probably should be fixed, as even when only considering roads, the set of roads on a tile after a city is founded may be very different than the set of roads considered by the AI before founding the city (and there's no reason not to include the bases portion for S2_5).

Emmet Hikory <persia>
Project Member
Tue 01 Jul 2014 03:36:37 PM UTC, comment #2:

I have read your patch (but not tested).

Something looks suspicious. create_city_virtual() does not create free extras. And tile_city(vtile) == tile_city(ptile) != vcity (because tile_set_worked() hasn't been called). I don't understand really what these functions are supposed to do, so I can't help you deeper.

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 30 Jun 2014 08:40:00 PM UTC, comment #1:

I believe the attached patches solve the problem: adding bases and considering the effects of RF_ALWAYS_ON_CITY_CENTER and RF_AUTO_ON_CITY_CENTER for 2.5, and the same consolidated logic for extras for 2.6. Autogames with these patches applied complete, but I'm not really sure how to validate if this does the right thing (and don't believe the autogames really exercise much, as I'm not running against a ruleset that would have a lot of this, and the AI doesn't build this kind of base for these rulesets anyway).

Also, I'm not confident of my use of virtual tiles and virtual cities here: from reading the code, I think this does the right thing, but I'd appreciate if someone else could review my patches and confirm this (and don't plan to apply this until that happens, such is my uncertainty).

Note that I don't know if this should be solved, only suggest that the attached patches would solve it: if someone thinks we oughtn't do this, I'm happy to see them discarded.

(file #21217, file #21218)

Emmet Hikory <persia>
Project Member
Sat 24 May 2014 10:09:42 PM UTC, original submission:

Patch #3680 commit message says: "Ai settler evaluating city spot ignores bases as they would be removed when city is founded." And this still seems true of aisettler.c:defense_bonus() on S2_5/trunk.

But the commit comment is no longer true as of patch #3826; it's possible for pre-existing bases to survive inside cities, and for such a base to confer a defence bonus, which the AI settler ought in theory to consider.

Is it worth fixing this? (Given that IIRC the AI is unlikely to build bases with defence bonuses, except by accident.)

(On trunk there's a comment /* TODO: Check those extras that would survive on city tile, not roads */; but the issue applies to S2_5 too.)

Jacob Nevins <jtn>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

 

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 persia (Updated the item)
  • -unavailable- added by jtn (Submitted the item)
  •  

    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
    Tue 22 Jul 2014 09:44:41 AM UTCpepetoStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun 20 Jul 2014 02:02:32 PM UTCpepetoAttached File-=>Added trunk_consider_extras_defense_bonus_when_founding_city.patch, #21477
      Attached File-=>Added S2_5_consider_bases_defense_bonus_when_founding_city.patch, #21478
      StatusNone=>Ready For Test
      Assigned toNone=>pepeto
    Tue 08 Jul 2014 11:39:33 AM UTCpersiaStatusWorks For Me=>None
      Assigned topersia=>None
    Mon 30 Jun 2014 08:40:00 PM UTCpersiaAttached File-=>Added consider-base-defense-bonus-when-founding-city.patch, #21217
      Attached File-=>Added consider-extras-defense-bonus-when-founding-city.patch, #21218
      StatusNone=>Works For Me
      Assigned toNone=>persia
      Planned Release=>2.5.0, 2.6.0
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup