Ticket #45907

Add continent control for "CityTile" req

Date d'ouverture: 2022-10-18 01:05 Dernière mise à jour: 2022-11-27 09:48

Rapporteur:
Propriétaire:
État:
Atteints
Composant:
Jalon:
Priorité:
5 - moyen
Sévérité:
5 - moyen
Résolution:
Fixed
Fichier:
5

Détails

"SameContinent" if the tile is on the same continent as city center, and possibly "NearSameOcean" if tile adjacency shares a negative continent number with city center adjacency. May be used in tile output effects, unit bribe cost effect, and possibly in the future for increasing airport trade bonus for intercontinental trade (civ2).

Ticket History (3/38 Histories)

2022-10-18 01:05 Updated by: ihnatus
  • New Ticket "Add continent control for "CityTile" req" created
2022-10-18 01:06 Updated by: ihnatus
  • Details Updated
2022-10-18 05:54 Updated by: ihnatus
Commentaire

A patch is made. Likely, later it should be accompanied by some rssanity checking (the reqs are useful in few effects since tile is mostly either city center or city is not specified) and to autosettlers' behaviour.

2022-10-19 01:56 Updated by: alienvalkyrie
Commentaire

For the NearSameOcean: I don't like how, by virtue of only applying to negative continent numbers (i.e. oceans), this is an additional explicit difference between land and oceanic terrain – that would cause more trouble down the line when unhardcoding terrain classes (and likely more so than the existing hardcoded differences).

2022-10-19 18:43 Updated by: ihnatus
Commentaire

Reply To alienvalkyrie

that would cause more trouble down the line when unhardcoding terrain classes (and likely more so than the existing hardcoded differences).

Well, what would *not* cause a trouble unhardcoding terrain classes... I can just exclude this option from this patch for now, any way I don't have specific plans for it. But we may also do it another way: same "continent" in adjacency of the city center (while "SameContinent", "Adjacent" uses adjacency to the target tile). Just how would it be naturally called, "Connected by TClass"?

2022-10-20 06:54 Updated by: cazfi
Commentaire

Did you consider the possibility of the city itself being on ocean tile? Code looks suspicious in that respect, but I didn't think very hard what it would actually do.

--

+ case CITYT_LAST:
+ /* Invalid */
+ return FALSE;

Likely want 'fc_assert(req->source.value.citytile != CITYT_LAST);' here.

--

+ case CITYT_NEAR_SAME_OCEAN:
+ fc_strlcat(buf, _("Tile connected by a body of water"), bufsz);
+ break;

Indendation is off (in the patch, no idea what osdn formatting will do...)

2022-10-21 14:50 Updated by: ihnatus
Commentaire

My current idea is "Connected Other TClass" option (i.e. different from one of the city's center, and tested for target tile itself). These two options plus "TerrainClass", "Tile" can provide ocean connection in a game with oceanic cities (direction is where caravan goes) and probably it still works if polar ice becomes third TClass:

;Land->Land by ocean
  "CityTile", "Connected Other TClass", "Adjacent", TRUE
  "TerrainClass", "Land", "Tile", TRUE
  "CityTile", "Connected Other TClass", "Tile", FALSE

;Oceanic->Land
  "CityTile", "SameContinent", "Adjacent", TRUE
  "CityTile", "SameContinent", "Tile", FALSE
  "TerrainClass", "Land", "Tile", TRUE

;Oceanic->Oceanic
  "CityTile", "SameContinent", "Tile"
  "TerrainClass", "Oceanic", "Tile"

;Land->Oceanic
  "CityTile", "Connected Other TClass", "Tile", TRUE
  "TerrainClass", "Oceanic", "Tile", TRUE
  "CityTile", "Connected Other TClass", "Tile", FALSE

2022-10-21 16:58 Updated by: cazfi
Commentaire

Reply To ihnatus

if polar ice becomes third TClass

Back when I turned land/ocean boolean into enum I were hoping to some day see a fantasy setting where "lava lakes" would be the third third class.

2022-10-21 19:22 Updated by: alienvalkyrie
Commentaire

Reply To cazfi

Reply To ihnatus

if polar ice becomes third TClass

Back when I turned land/ocean boolean into enum I were hoping to some day see a fantasy setting where "lava lakes" would be the third third class.

I think that might already be possible to a degree by declaring lava to be freshwater. Which, of course, would be the most immediate change for supplied rulesets when unhardcoding terrain classes; giving tclasses fields for "only use this class for areas with at most / at least this many tiles" to make lakes a separate class.

2022-10-21 19:31 Updated by: cazfi
Commentaire

I think the HARD part of terrain class unhardcoding is the AI ferry system. It expects that units are to be ferried over one hardcoded class (ocean), between areas of the second class (land).

2022-10-21 23:56 Updated by: ihnatus
Commentaire

Reply To cazfi

I think the HARD part of terrain class unhardcoding is the AI ferry system. It expects that units are to be ferried over one hardcoded class (ocean), between areas of the second class (land).

That's why I talked about polar caps. Whatever of the two terrain classes you declare for basically impassable regions bordering both continents and oceans, it will confuse the unit movement planning.

2022-10-22 00:00 Updated by: cazfi
Commentaire

Reply To ihnatus

Reply To cazfi

I think the HARD part of terrain class unhardcoding is the AI ferry system. It expects that units are to be ferried over one hardcoded class (ocean), between areas of the second class (land).

That's why I talked about polar caps. Whatever of the two terrain classes you declare for basically impassable regions bordering both continents and oceans, it will confuse the unit movement planning.

This is getting off-topic (for the ticket), but I wonder if it would be a step forward, or just side-step with no use in the future, if we made AI to at least treat current two tclasses symmetrically, i.e., if land units could carry canoes from lake to lake.

2022-10-22 04:33 Updated by: ihnatus
Commentaire

Named the req "Bordering TClass Region", I hope the English is understandible in the patch, if one is going to fix it, you're welcome.

2022-10-22 06:33 Updated by: cazfi
Commentaire

- universal_name_translation() should be short (that's mentioned in the function header & ideas to make it more clear welcome - maybe say it IN ALL CAPS). Can we figure out a way to shorten the ones we're about to add here?
- Also indentation of '+ fc_strlcat(buf, _("Tile of a nearby another terrain class region"),' is off
- Use type 'Continent_id' instead of int where appropriate

This may play badly with the issue that some parts of code still assume that any citytile requirement is a requirement for city center. I thought that I've filed a bug about it some time ago (at least I noticed some buggy code), but can't find the ticket now. Not that fixing all of it would belong to this ticket.

2022-10-23 05:18 Updated by: ihnatus
Commentaire

Also, some comments not edited from the previous patch. Reply To cazfi

- universal_name_translation() should be short (that's mentioned in the function header & ideas to make it more clear welcome - maybe say it IN ALL CAPS). Can we figure out a way to shorten the ones we're about to add here?

"Same continent tile" and... "Nearby ocean tile" would be comprehensive but sometimes a lie ("continent" is at least somewhat generic technical term). "Port access tile"?

(at least I noticed some buggy code)

I'll try to glance at VUT_CITYTILE uses, maybe fix something.

2022-10-27 04:06 Updated by: ihnatus
Commentaire

Fixed. Also, added a couple of things forgotten before (inability to buld signal causes and req contradiction).

2022-10-27 04:07 Updated by: ihnatus
  • File citytile-continent-reqs.patch (File ID: 10676) is attached
2022-10-27 04:24 Updated by: ihnatus
  • File citytile-continent-reqs.patch (File ID: 10676) is deleted
2022-10-27 05:18 Updated by: ihnatus
Commentaire

Oops, sent an untested patch *'_'*. Fixed. The "can't build" information does not work because of a bug to be handled in a separate ticket but the rest should.

2022-10-27 05:19 Updated by: ihnatus
  • File citytile-continent-reqs.patch (File ID: 10678) is attached
2022-10-27 06:39 Updated by: ihnatus
  • File citytile-continent-reqs.patch (File ID: 10678) is deleted
2022-10-27 06:40 Updated by: ihnatus
Commentaire

Oops, testing with #45982 has shown one more general logical flaw. Fixing.

2022-11-01 15:40 Updated by: cazfi
Commentaire

Reply To cazfi

This may play badly with the issue that some parts of code still assume that any citytile requirement is a requirement for city center. I thought that I've filed a bug about it some time ago (at least I noticed some buggy code), but can't find the ticket now. Not that fixing all of it would belong to this ticket.

My memory was failing. The issue that I thought was actually about CityStatus: #45562

2022-11-01 15:47 Updated by: cazfi
Commentaire

Reply To ihnatus

The "can't build" information does not work because of a bug to be handled in a separate ticket

-> #45982

Ideally that would get fixed first, but I'm not keen on keeping this one waiting and catching bitrot. Should we consider it a dependency, or not?

2022-11-01 16:08 Updated by: ihnatus
Commentaire

Reply To cazfi

Reply To ihnatus

The "can't build" information does not work because of a bug to be handled in a separate ticket

-> #45982 Ideally that would get fixed first, but I'm not keen on keeping this one waiting and catching bitrot. Should we consider it a dependency, or not?

I don't think it is a dependency. But I'll try to send a fix for 3.0 of 45982 in near future (the patch for master is already there).

2022-11-02 10:19 Updated by: cazfi
  • Propriétaire Update from (Aucun) to cazfi
  • Résolution Update from Aucun to Accepted
2022-11-02 11:29 Updated by: cazfi
Commentaire

Needs rebase (after #45602) ? (Not sure about that yet, but I got compile errors from the autogame runner where this (and bunch of other patches) are applied on top of current HEAD - my main work-branch where this applied fine is based on something before #45602)

2022-11-02 17:56 Updated by: ihnatus
Commentaire

Reply To cazfi

Needs rebase (after #45602) ?

Probably, I'll try to have a look. (Actually, I was developing a pair of patches redesidning is_req_active() for some latest days, so I need to recode something any way.)

2022-11-06 02:45 Updated by: ihnatus
Commentaire

Rebased on top of #46209 with some fixes.

(Edited, 2022-11-06 02:46 Updated by: ihnatus)
2022-11-06 16:37 Updated by: cazfi
Commentaire

Ok, so now this depends on #46029 (at least that's how they apply without problems)

2022-11-10 21:33 Updated by: ihnatus
Commentaire

Rebased again

2022-11-23 07:10 Updated by: cazfi
Commentaire

Reply To ihnatus

Rebased again

Taking this to my test queue now.

2022-11-27 09:48 Updated by: cazfi
  • État Update from Ouvert to Atteints
  • Résolution Update from Accepted to Fixed

Attachment File List

Modifier

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Connexion