bugFreeciv - Bugs: bug #21470, Fix non-nested requirement ranges

 
 
Show feedback again

bug #21470: Fix non-nested requirement ranges

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sat 11 Jan 2014 07:11:05 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Release: 2.4.2Operating System: Any
Planned Release: 2.4.3,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)

Mon 21 Apr 2014 02:06:03 PM UTC, SVN revision 24787:

Requirements with Adjacent/CAdjacent ranges always check the centre tile too.
Fixed the TerrainClass, Base, and CityTile requirement types.

See gna bug #21470.

(Browse SVN revision 24787)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 21 Apr 2014 01:56:19 PM UTC, SVN revision 24786:

Requirements with Adjacent/CAdjacent ranges always check the centre tile too.
Fixed the TerrainClass, Base, Road, TerrainFlag, and CityTile requirements.
Also, TerrainClass/TerrainFlag City-ranged requirements now cope with
unknown terrain within the city radius.

See gna bug #21470.

(Browse SVN revision 24786)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 21 Apr 2014 01:47:08 PM UTC, SVN revision 24785:

Requirements with Adjacent/CAdjacent ranges always check the centre tile too.
Fixed the TerrainClass, Extra, TerrainFlag, BaseFlag, RoadFlag, CityTile,
and MaxUnitsOnTile requirements.
Also, TerrainClass/TerrainFlag City-ranged requirements now cope with
unknown terrain within the city radius.

See gna bug #21470.

(Browse SVN revision 24785)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sun 20 Apr 2014 12:59:52 AM UTC, comment #4:

OK, will fix on S2_4 too without further ceremony.

I've fixed all requirement types in a single patch, but I could split it out if need be.

In passing I've also fixed possible trouble on S2_5/trunk with City-ranged TerrainClass/TerrainFlag requirements if there was unknown terrain within the city radius (i.e. probably on the client).

A survey of rulesets that might be affected shows no negative effects and only minor existing bugs fixed, with none in shipped rulesets on the stable 2.4 branch. (Disclaimer: none of this is tested, it's just from me reading rulesets.)

  • Alien: "Filter" building not allowed in an ocean city in a 1-tile lake
  • Trouble with 1-tile rivers.
    • This has various mitigations:
      • Map generator unlikely to generate these
      • On S2_4, rivers are VUT_SPECIAL which got this right (so any bugs here are regressions in 2.5)
      • Irrig_Possible effects often exist for ocean too, and the map generator will place a river next to an ocean, masking this
    • This could affect the following:
      • Ability to build Hydro Plant / Hoover Dam in a city on such a tile
      • Ability to irrigate such a tile
      • (civ2civ3) can't build "Aqueduct, River"
      • (civ1, trunk) city on such a tile is immune to flood
      • (alien: all possible bugs with Water Flow masked by effects from prerequisite techs)

(file #20530, file #20531, file #20532)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sun 12 Jan 2014 08:45:49 AM UTC, comment #3:

> My inclination is to leave the stable branches as they are
> (format freeze)


I can see this either way: "It's not working according to S2_4 expectations" -> bug that should be fixed, or "It's a feature" -> subject to freeze.
FWIW I believe I've done such behavior changes to stable branches in the past on the principle that some REQ_RANGE_ADJACENT requirement ignoring central tile is a hard bug.

Marko Lindqvist <cazfi>
Project Administrator
Sat 11 Jan 2014 07:41:18 PM UTC, comment #2:

> My inclination is to leave the stable branches as they are
> (format freeze)

...the slight disadvantage being that if I do patch #4400 and friends on S2_4 then the divergent descriptions mean a bit of rework for translators when they move to S2_5.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 11 Jan 2014 07:16:39 PM UTC, comment #1:

These split into effects released on stable branches and ones that have not yet seen the light of day. Only TERRAINCLASS, BASE, and CITYTILE fall into the former category.

My inclination is to leave the stable branches as they are (format freeze), fix it all in 2.5.0 to be strictly nested, and leave instructions on how to get the old behaviour back in <http://www.freeciv.org/wiki/How_to_update_a_ruleset_from_2.4_to_2.5>.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Sat 11 Jan 2014 07:11:05 PM UTC, original submission:

Current code is inconsistent whether REQ_RANGE_ADJACENT/CADJACENT looks at the centre tile (the one checked by REQ_RANGE_LOCAL) or just the surrounding ones.

The following do include the centre tile;

  • SPECIAL (up to 2.5)
  • TERRAIN (at least 2.3+)
  • RESOURCE (2.5+)

The following do not:

  • TERRAINCLASS (at least 2.3+)
  • BASE (at least 2.3 - 2.5)
  • CITYTILE (2.4+)
  • ROAD (2.5)
  • TERRFLAG (2.5+)
  • BASEFLAG (2.6+)
  • ROADFLAG (2.6+)
  • EXTRA (2.6+)
  • MAXTILEUNITS (2.6+)

(Where supported, REQ_RANGE_CITY always does include the centre tile, I think.)

I think there are a few reasons that we should make it a general principle that requirement ranges should be strictly nested (e.g. adjacent implies centre), wherever it makes sense:

  • Various code treats the ranges as a total order, with higher-numbered ranges enclosing lower-numbered ones.
  • When multiple ranges of effect are available, the AI will pick the highest range to consider.
  • For ADJACENT, centre+surroundings is more likely to be what the ruleset author wanted, and we can't usually express 'or' in requirements lists; however, with centre+surrounding semantics, the ruleset author can obtain a just-the-surroundings effect with range=ADJACENT + range=LOCAL|present=FALSE.

(The nesting is probably not completely precise; I wouldn't be surprised if there are corner cases as you move from tile-based ranges to player-based ones, or for requirement types like DIPLREL. I don't think that matters hugely.

Another example of this principle (to which I think we do already adhere) is that a requirement in the ALLIED range should be satisfied if we ourselves have the thing (as for PLAYER), not just if one of our allies does.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #20530:  trunk-fix-adjacent-req-ranges.patch added by jtn (19kB - text/x-diff - trunk/S2_5/S2_4 r24782)
file #20531:  S2_5-fix-adjacent-req-ranges.patch added by jtn (16kB - text/x-diff - trunk/S2_5/S2_4 r24782)
file #20532:  S2_4-fix-adjacent-req-ranges.patch added by jtn (10kB - text/x-diff - trunk/S2_5/S2_4 r24782)

 

Depends on the following items: None found

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -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 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 21 Apr 2014 02:08:44 PM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun 20 Apr 2014 12:59:52 AM UTCjtnAttached File-=>Added trunk-fix-adjacent-req-ranges.patch, #20530
      Attached File-=>Added S2_5-fix-adjacent-req-ranges.patch, #20531
      Attached File-=>Added S2_4-fix-adjacent-req-ranges.patch, #20532
      StatusNone=>Ready For Test
      Assigned toNone=>jtn
      Release=>2.4.2
      Operating SystemNone=>Any
      Planned Release=>2.4.3,2.5.0,2.6.0
      Summary[metaticket] Fix non-nested requirement ranges=>Fix non-nested requirement ranges
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup