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.
alienvalkyrie 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).
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"?
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...)
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
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.
alienvalkyrie 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.
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).
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.
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.
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.
- 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.
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.
Fixed. Also, added a couple of things forgotten before (inability to buld signal causes and req contradiction).
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.
Oops, testing with #45982 has shown one more general logical flaw. Fixing.
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
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).
Ok, so now this depends on #46029 (at least that's how they apply without problems)
Rebased again
"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).