Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #880455

city_landlocked_sell_coastal_improvements is broken

Added by Anonymous over 1 year ago. Updated 10 months ago.

Status:
Closed
Priority:
Normal
Category:
Server
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

city_landlocked_sell_coastal_improvements() in server/citytools.c tries to check buildings that have missing reqs after the terrain of an adjacent tile changes (Harbor, Coastal Defense etc.).

It's broken starting at 2.6 in that it only checks reqs of type "Terrain" or "TerrainClass", not e.g. "TerrainFlag" which is used as a req for Harbors to exclude Lakes (in civ2civ3). [1] [2] Also, it sells all buildings with any Terrain reqs, including e.g. the different sorts of Aqueducts, regardless of if the reqs are filled or not. This appears to be because it doesn't pass the terrain struct to is_req_active().

It seems that the only reason it works at all for Harbors is that there's this hardcoded check for ocean tiles: !is_terrain_class_near_tile(tile1, TC_OCEAN)

Suggest changing it to also pass the tile struct and simplify it by calling are_reqs_active() as in can_city_build_improvement_direct() [3], which I suppose is what is checked when actually building the buildings.

(That still wouldn't make it work with e.g. City ranged Terrain reqs, since the function is only called when neighboring terrain changes.)

[1] https://github.com/freeciv/freeciv/blob/S2_6/server/citytools.c#L2985
[2] https://github.com/freeciv/freeciv/blob/S2_6/data/civ2civ3/buildings.ruleset#L502
[3] https://github.com/freeciv/freeciv/blob/S2_6/common/city.c#L805


Related issues

Related to Freeciv - Task #880778: remove_obsolete_buildings_city works too rareNew

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

History

#1 Updated by Alexandro Ignatiev over 1 year ago

I think we should fix only passing the tile in 2.6 to not change things beyound obvious bugfix of disappearing Aqueducts (btw it has been broken well before v.2.5). Maybe also change ajc_iterate to square_iterate r=1 for the case if marine cities are allowed. In later versions, we can make the function more generic for checking not only landlocking but any terrain reqs incompatibility (cities in max city radius must be checked). I don't know if we should run it for all kinds of reqs, TerrainFlag seems nothing worse than Terrain[Class] but what if we want to leave the building where it stays (Harbour in civ2civ3)? Maybe make a building flag "CheckReqsOnTerrainChange" and check only buildings with it?

#2 Updated by Anonymous over 1 year ago

Right, checking all reqs would also check techs. I don't know if losing techs can lead to losing buildings otherwise, but if not, it would be odd for a terrain change to trigger that.

I can't see a reason for keeping useless Harbors any more than keeping a useless Coastal Defense though. The patch here seems to work for me to fix Aqueducts and to remove Harbors when their reqs are gone (tested against the included civ2civ3 ruleset).

#3 Updated by Alexandro Ignatiev over 1 year ago

Another thought: maybe, since some version just remove this clumsy code part and move the useless building removal reqs into obsolete_by vector? of the ruleset? It will postpone the building removal to the end of the city owner's phase but it changes little in most cases.

#4 Updated by Anonymous over 1 year ago

Using obsolete_by seems to work, also for e.g. City-ranged Terrain reqs. It doesn't appear to actually sell the buildings though (looks like that only happens when a new tech, any tech, is researched), but they got marked as obsolete and turned ineffective as far as I tested. The code could be changed to sell them immediately, but it seems to work even with just ruleset changes.

So, `obsolete_by` pretty much seems to already do what it says in the comments about generalizing `city_landlocked_sell_coastal_improvements()` to check all buildings every turn.

#5 Updated by Alexandro Ignatiev over 1 year ago

  • Related to Task #880778: remove_obsolete_buildings_city works too rare added

#6 Updated by Marko Lindqvist 11 months ago

  • Category set to Server
  • Status changed from New to In Progress
  • Assignee set to Marko Lindqvist
  • Sprint/Milestone set to 2.6.3

#7 Updated by Marko Lindqvist 11 months ago

Patch here seems like a good improvement for the time until we get a better solution. Will you make a version that applies to later branches (it didn't for master, didn't test for S3_0)?

#8 Updated by Marko Lindqvist 11 months ago

  • Assignee deleted (Marko Lindqvist)

#10 Updated by Marko Lindqvist 10 months ago

  • Status changed from In Progress to Resolved
  • Assignee set to Marko Lindqvist
  • Sprint/Milestone changed from 2.6.3 to 2.6.4

I'm a bit worried that is_terrain_class_near_tile() condition removal can have some surprising consequences, so not rushing this in to 2.6.3 at this point but wanting to expose it to more testing (in 2.6.4 cycle) before releasing it.

#11 Updated by Anonymous 10 months ago

Marko Lindqvist wrote:

I'm a bit worried that is_terrain_class_near_tile() condition removal can have some surprising consequences, so not rushing this in to 2.6.3 at this point but wanting to expose it to more testing (in 2.6.4 cycle) before releasing it.

I think it could also just be left there.

With the !is_terrain_class_near_tile(tile1, TC_OCEAN) condition, any oceanic tile protects all terrain-dependent improvements, which seems technically wrong. The difference I can see is with a city that is adjacent to a Lake and an Ocean, and has both Aqueduct, Lake, and a Harbor. :) With that condition, they only get sold when both are lost; without it, they get sold according to which one of Lake or Ocean gets lost. Not a very general case, I suppose.

#12 Updated by Anonymous 10 months ago

Ilkka Virta wrote:

I think it could also just be left there.

That patch being a version that doesn't touch that hard check.


About the issue not directly related to the bug, generalizing the terrain checks, some thoughts:

One possibility would be to add a new reqs list, e.g. exist_reqs and treat the current reqs as "build reqs". The latter would be checked only when building, and the former would be checked on every turn, for each building. That somewhat duplicates what obsolete_by does, but wouldn't need to be tied to new techs being researched and carries a different message. And could be used to send literally different messages from the server code. Alternatively add a hard flag to the reqs for those reqs that should be checked every turn. Then of course, there's the case of buildings that should stop working based on some condition, but not get auto-sold... That can probably be done with effects well enough, though, and isn't that important so I'll stop digging deeper into the rabbit hole now.

#13 Updated by Marko Lindqvist 10 months ago

Ilkka Virta wrote:

Marko Lindqvist wrote:

I'm a bit worried that is_terrain_class_near_tile() condition removal can have some surprising consequences, so not rushing this in to 2.6.3 at this point but wanting to expose it to more testing (in 2.6.4 cycle) before releasing it.

I think it could also just be left there.

I think we take to 2.6.4 the patch version where it is, i.e., I still plan to push the previous version once we get 2.6.3 out of the way.

#14 Updated by Marko Lindqvist 10 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF