patchFreeciv - Patches: patch #3826, Allow bases on city tiles

Show feedback again

patch #3826: Allow bases on city tiles

Submitted by:  Emmet Hikory <persia>
Submitted on:  Tue Apr 2 16:29:57 2013  
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 2.5.0, 2.6.0Contains string changes: None

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 Apr 30 14:37:26 2013, SVN revision 22794:

Instead of always preventing bases in city tiles and allowing, or even
granting, roads in cities, make that ruleset configurable in a
consistent manner.

Patch by Emmet Hikory

See patch #3826

(Browse SVN revision 22794)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue Apr 23 20:46:56 2013, comment #15:

My apologies. I suppose I'm used to significantly less mature projects, where a feature freeze tends to happen only immediately prior to release (but releases correspondingly tend to be buggier without the overlap).

Attached is a patch with the trivalue logic. I noticed while producing this that we unconditionally remove obsolete buildings on city takeover: while I think it outside the scope of this patch (improvements don't currently use negated requirements to indicate obsolescence, nor do roads/bases have a cache of potential obsolecense conditions), it probably makes sense to review this code section for 2.6, and give the same treatment for improvements and extras: the new player may not want some obsolete infrastructure which no longer provides a useful bonus (or may even cause a penalty, depending on applicable effects).

(file #17815)

Emmet Hikory <persia>
Project Member
Tue Apr 23 20:04:52 2013, comment #14:

Shipping rulesets have no bases surviving in the cities, but if we are adding it as a feature for ruleset, it should work. Note that almost any ruleset where some base can exist in city is subject to "superior nation conquered city from weaker enemy" -case.
We've been trying to get major releases out more frequently, but fact is that there usually is years between them -> 2.6 release (hopefully with extras -framework) is far away (we've not released 2.4! yet)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon Apr 22 22:35:41 2013, comment #13:

Do you think it's worth adding trivalue logic for 2.5, when it will all be swept away for 2.6 with extras? At least for the shipping rulesets, there are no cases of this.

Emmet Hikory <persia>
Project Member
Mon Apr 22 22:23:30 2013, comment #12:

As you are handling road and base upgrades separately, I assume some messages to player be suboptimal if there's both road and base upgradet at the same time. Namely: Will it say "The people ... stunned by your technological insight!" twice when city is conquered, or "New hope sweeps like..." if new tech allows both road and base type?

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon Apr 22 22:03:36 2013, comment #11:

Redundant call of tile_remove_base() now patch #3876

Emmet Hikory <persia>
Project Member
Mon Apr 22 21:41:29 2013, comment #10:

Oh, sure. These all assume that destroy_base() will call tile_remove_base(). I was mostly concerned about callbacks, because they could potentially not call tile_remove_base(), which would require the extra call in tile_change_terrain().

I'll open a different patch to drop the redundant call, and leave a comment indicating that tile_change_terrain() relies on any callback set for fc_funcs->destroy_base() to perform tile_remove_base().

Emmet Hikory <persia>
Project Member
Mon Apr 22 21:22:55 2013, comment #9:

There's no other callers via fc_funcs->destroy_base(), but there's direct callers in server

grep -r "destroy_base" server/

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri Apr 19 19:57:16 2013, comment #8:

Hrm? maphand.c:destroy_base() unconditionally calls tile_remove_base() as the last operation. On the server, fc_funcs->destroy_base is set to maphand.c:destroy_base(), and on the client, fc_funcs->destroy_base is set to NULL. I don't see any other callers. Even in the event that fc_funcs->destroy_base fails to remove it, shouldn't tile_change_terrain() either double-check by calling tile_remove_base within the conditional or dropping the else clause and calling it unconditionally, rather than somewhere between these options?

I had thought that there might be some difference in the application of fc_funcs related to the nature of the build (like fc_assert), which might cause the entire conditional to be meaningless, but if it is just caller tracking, I suspect this is a bug (although one with very little impact, as BV_CLR() is safe to call repeatedly without unexpected behaviour).

Emmet Hikory <persia>
Project Member
Fri Apr 19 19:44:24 2013, comment #7:

I don't think there's any actual need for double removal, but otherwise destroy_base() would need to either be passed boolean parameter telling if caller is removing base itself, or it would need to be able to deduct the need itself. Some callers other than tile_change_terrain() need it.
Or maybe tile_change_terrain() could remove base only if callback is not set, and to rely on any callback ever assigned there to take care of the removal.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri Apr 19 19:30:36 2013, comment #6:

Updated patch adjusts base destruction to not be player-specific (so that players may build cities on city-compatible bases they could not build themselves, and keep the base), as well as removing any city-incompatible roads when founding a city.

During the production of this patch, I noticed what looks like a duplicate call to tile_remove_base() in tile.c:tile_change_terrain(): is this a bug, or is there some reason to double-remove the base here because of the nature of fc_funcs that I don't understand? (note that destroy_base() calls tile_remove_base() itself if used.)

(file #17773)

Emmet Hikory <persia>
Project Member
Fri Apr 19 08:22:56 2013, comment #5:

Then I noticed how roads and bases are handled differently when new city is founded. Existing roads remain, but bases are removed unless city owner would be able to build new such base.
I think you should be removing only those bases (and roads, for consistency) that cannot exist in city, regardless if player is able to build them. This might even already happen (for both or roads only) outside this code.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri Apr 19 04:17:57 2013, comment #4:

The attached patch should address all comments, and applies against trunk. Thanks for pointing out how roads/bases are removed when terrain is changed. For some reason, in the initial testing, I wasn't successful in restricting bases in cities from the ruleset without passing the city, but it seems to work now, and I cannot remember the specific failing test.

(file #17762)

Emmet Hikory <persia>
Project Member
Thu Apr 11 20:36:17 2013, comment #3:

- git commit message claims that pcity is needed for requirements preventing building base/road in cities, but "CityTile", "Center", "Local" requirement is used for that.
It's probably a good idea to pass that city information to requirement parsing, but it's out of scope for this patch.
- FIXME comment about removal of roads/bases from city tiles should be removed, or changed to point out that in that sense city tiles act like any other tile - extras are removed in tile_change_terrain()

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue Apr 2 17:10:34 2013, comment #2:

Aha! revision 21864. Sorry about that. allow-bases-in-cities+fixed-rulesets.patch should restore the prior behaviour properly, unless I missed something between 21864 and 22649 that further adjusted the values.

(file #17658)

Emmet Hikory <persia>
Project Member
Tue Apr 2 16:51:21 2013, comment #1:

> whether to set "AlwaysOnCityCenter" or "AutoOnCityCenter" flag
> for road_type "road" in civ1, civ2, and civ2civ3

civ1/civ2 always had the road on city center. As up to freeciv-2.4, freeciv always gave, and expected, road to all city center tiles, including oceanic ones!

> AlwaysOnCityCenter is correct unless someone had intentionally
> changed this behaviour.

That was intentionally changed when it became possible to have city center without road (one of the first commits to TRUNK after S2_4 was branched). Idea was to test how it works in gameplay and somehow change it if need arises. So far nobody has complained.
Note that code has evolveld so that when "city center needs road" was first changed to "roads requirements are always respected", it was impossible to have ruleset with road always on city center. "AlwaysOnCityCenter" flag was only later introduced.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue Apr 2 16:29:57 2013, original submission:

This patch allows one to build bases in cities, allows cities to have inherent bases (for example, if one wanted some basic infrastructure that was best implemented with a base and upgraded appropriately with the tech tree), and allows one to only have some bases or roads automatically built for cities. All of this only supports ruleset authors: there should be no change in the behaviour for any shipping rulesets.

Note that I'm a little uncertain about whether to set "AlwaysOnCityCenter" or "AutoOnCityCenter" flag for road_type "road" in civ1, civ2, and civ2civ3 rulesets, as I don't have enough historical experience playing them, nor have I ever played the games they apparently intend to model. I've done my best based on guessing from various comments in the code or reference websites, but if someone knows the intended behaviour better, these values could be adjusted. For alien, it doesn't matter because there is no requirement for Bridge Building. For classic/experimental/multiplayer, historical behaviour has been to provide a road even on river tiles prior to the discovery of Bridge Building, so AlwaysOnCityCenter is correct unless someone had intentionally changed this behaviour.

Also note that while I've set Buoys to be automatically destroyed if a
city is built upon them, in most of the rulesets, this is irrelevant, as they don't permit Oceanic cities: I added this mostly so that anyone performing minimal modifications to add oceanic cities wouldn't suddenly end up with buoys not being deleted.

Lastly, I'm not sure I understand precisely why the previous code wanted to block base effects before creating the virtual city, but also couldn't find a good way to eliminate unsuitable bases much earlier in create_city(): there may be some interaction between the effects of unsuitable bases and some of the earlier actions taken while setting up the city. I don't believe the call can be any earlier than tile_set_owner() and map_claim_ownership(), because at that point pcity doesn't get defined in player_can_build_base().

Emmet Hikory <persia>
Project Member


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

Attach File(s):

Attached Files
file #17815:  allow-bases-in-cities+less-stunning.patch added by persia (38kB - application/octet-stream)
file #17773:  allow-bases-in-cities+road-removal.patch added by persia (37kB - application/octet-stream)
file #17762:  allow-bases-in-cities+comment-fixes.patch added by persia (36kB - application/octet-stream)
file #17658:  allow-bases-in-cities+fixed-rulesets.patch added by persia (37kB - application/octet-stream)
file #17657:  allow-bases-in-cities.patch added by persia (37kB - application/octet-stream)


Depends on the following items: None found

Items that depend on this one: None found


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


    Error: not logged in



    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue Apr 30 14:37:43 2013cazfiStatusReady For Test=>Done
    Sun Apr 28 20:40:07 2013cazfiStatusNone=>Ready For Test
      Assigned toNone=>cazfi
      Planned Release=>2.5.0, 2.6.0
    Tue Apr 23 20:46:56 2013persiaAttached File-=>Added allow-bases-in-cities+less-stunning.patch, #17815
    Fri Apr 19 19:30:36 2013persiaAttached File-=>Added allow-bases-in-cities+road-removal.patch, #17773
    Fri Apr 19 04:17:57 2013persiaAttached File-=>Added allow-bases-in-cities+comment-fixes.patch, #17762
    Tue Apr 2 17:10:34 2013persiaAttached File-=>Added allow-bases-in-cities+fixed-rulesets.patch, #17658
    Tue Apr 2 16:29:57 2013persiaAttached File-=>Added allow-bases-in-cities.patch, #17657
    Show feedback again

    Back to the top

    Powered by Savane 3.1-cleanup