Districts R27

[instafluff0]

Hey @maxpetul,

I hope things are going well. I've been hard at work on the new additions to districts. As seems to be my MO these days, I set out with a very specific set of goals and a relatively narrow scope. That lasted about ... 2 weeks. As I reworked things and added new features, those in turn inspired me to add more ... and more. I also have gotten a lot of feedback from modders and so on on CivFanatics and did my best to add straightforward features that weren't much of a lift, or seemed worth it.

Anyway, the PR has become quite big (again), for which I can only apologize for asking you to do yet another huge code review. Thank you very much, as always.

After this gets merged and any outstanding bugs get worked out, I plan to go quiet for a while and take a break. At this point I have no intended major additions I want to do to districts - this PR is kind of the kitchen sink. After this I think districts will actually be very mature, in terms of features and robustness. So I can't make any promises, but at this point the only remaining dev I intend to do is adding a seasonal cycle, which will mostly be art anyway.

Changelog

Here's a reasonably exhaustive list of changelog items:

• 11 new districts: Port, Canal, Bridge, Municipal, Central Rail Hub, Data Center, Energy Grid, Great Wall, Ski Resort, Offshore Extraction Zone, Water Park
• 7 new natural wonders: Wadi Rum, Eyjafjallajokull, Ha Long Bay, Lofoten Skerries, Geirangerfjord, Delicate Arch, Savanna (mostly a desert/tundra theme this time)
• 3 new wonders: Great Lighthouse, Colossus, Hoover Dam
• Add support for maritime districts
• Move Research Lab from Campus to Data Center dependency
• Add Great Wall auto-build around borders event after Great Wall wonder is completed
• Add support for Named Tiles, which shows custom verbiage on a given tile (can be set in-game or via scenario.districts.txt)
• Fix bug with districts not rendering on right-click tile detail modal
• Fix bug with workers "building" a natural wonder if user clicks "Move all units of type" button
• Fix bug with custom scenario district art not loading from scenario folder
• Add support for alternative directions of Wonders, based on city vs. wonder tile location
• Add negative district bonuses
• Add custom logic for river- & port-related rendering
• Add granular configuration control over where districts can be built by tile and adjacent tile types
• Add support for districts being dependent on wonders & natural wonders
• Add support for districts enabled by alliances with other civs that can build them
• Add support for restrictions of who can build districts by civ name, trait, culture, government
• Allow multiple advance prerequisites
• Touch up art for Industrial Zones, Newton's University, Holy Site, Bach's Cathedral
• Add support for buildings being buildable if one of multiple possible districts is built
• Add support for districts generating resources
• Add support for districts requiring resources, both in cities or on a tile
• Add optional flag for districts to serve as alternative irrigation source
• Add patches to enable buildings dependent on adjacent water sources to be enabled if water in work area (eg, Ports can be built if city has water in work radius), except for Aqueducts & Coastal Fortresses, which still require adjacency
• Add flag for districts with dependent buildings to be custom tile improvements, which encourages the AI to evaluate whether to build them on each tile, like irrigation and mines
• Add much smarter AI for distribution hubs, with ai_distribution_hub_build_strategy = auto (AI assess civ-wide and city level needs to determine how best to use tiles and distro hubs to build)
• Add more balance distribution hub yields, with amount of shields/food each city receives decreasing as the number of recipient cities grows
• Add fairly sophisticated AI build strategy and analysis of where to place canals and bridges
• Add cap on max distribution hubs relative to number of cities
• Add flag for show_ai_city_location_desirability_if_settler, which automatically shows city desirability over tiles if a settler is selected. This is extremely useful if minimum_city_separation is higher than normal, as it's hard to tell where cities can be settled otherwise
• Add flag for auto_zoom_city_screen_for_large_work_areas, which prevents the city screen from auto-zooming out if the city_work_radius is >= 4 (my personal preference, but off by default)
• Add support for extraterritorial colonies, where colonies can be built in another civ's territory or be built beforehand but not disappear as borders grow, at a relation penalty
• Add happiness_bonus to district yields
• Add display_name, in case name shown to user is different
• Add optional custom height/width & x/y offsets for district art
• Add error messaging to main screen if buildings / techs aren't parsed correctly, like other config options (in R26 this silently fails)
• Distro hub yields on city screen are drawn even if outside city work radius (useful as in R26 they aren't drawn if outside the work radius, even if city is receiving distro hub yields)
• Make tile highlights green, not red - don't kill me, and let me know if you don't like this. I just found green to be more inviting and intuitive in a "deeper green is better" sense
• Replace all redundant, hard to read district tile loops with proper macros
• Add custom logic for air unit position and bombing if using aerodromes. This was necessary as I found that the AI never moved air units otherwise, and turned out the vanilla code explicitly looped over cities & airfields (duh, I suppose), so if cities weren't valid places to move to, the AI never did
• Add support for alternative rendering logic (in districts config, render_strategy = "by-building") for handling districts whose buildings are not dependent on each other. The main use case for this was the Municipal District, but I wanted to generalize it so anyone could do this and we wouldn't need to add custom districts in the future
• Add custom logic for naval units to heal in ports and pillage enemy ports. This was necessary as otherwise, if naval_units_use_port_districts_not_cities = true, the AI never healed ships and never attacked others' ports
• Add custom logic for unit movement on bridges, canals, great wall
• Allow flexible additive bonuses for districts in config based on nearby cities' available buildings and district tile & overlay types
• Allow naval units to pillage (maritime districts)

Current issues need advice on

I'm making the PR now as (1) I don't anticipate any other major changes to the codebase unless you request it, and (2) I've basically hit a wall on a few items I don't know how to resolve and need your help. Most of these are related to unit movement, which I think you understand much better than I do.

Those are:

1. Naval units can't actually enter canals. I've reviewed the vanilla code extensively and tried many different patches, but I can't get it to work. The pathfinder works fine, and correctly calculates the number of turns it would take to move and suggests the unit can pass. But when I actually have a ship try to enter, it just doesn't. Kind of pulling my hair out on this one. Relevant functions are patch_Unit_can_move_to_adjacent_tile and patch_Unit_move_to_adjacent_tile.
2. Land units can successfully move on bridges and workers can enter coastal tiles (if workers_can_enter_coast = true), but even if set on a multi-turn path, they wake up every turn. This is annoying. I suspect this is due to water rules with land units in transports, where they wake up if not sleeping, but again haven't been able to pin down exactly where in the codebase this actually happens.
3. Related to (2), as I said, land units can successfully move on bridges, including handling railroads (yay), but when they leave a bridge and move to land - even if both tiles have roads/railroads - it still takes a full turn. I assume this is due to vanilla logic of land units moving from a transport to land, but haven't figured out where this happens or where to patch.
4. Figuring out how to enable the Pillage command/button for naval units was a huge pain. I initially tried to do so via checks in patch_Unit_can_perform_command and patch_Leader_can_do_worker_job, but those seemed to have no effect and I think for naval units they perhaps aren't checked at all? Not completely sure. As a workaround, I patched patch_Main_GUI_set_up_unit_command_buttons and put the check there, BUT the code I got working to actually enable the button is ugly and took a lot of poking around the source to understand the offsets and get the right images. I hate it but couldn't figure out a better way. This works but I would love suggestions for a better way to do it.
5. I was able to figure out how to negatively affect a Leader's attitude toward one another by setting a penalty for each extraterritorial colony, in patch_Leader_get_attitude_toward. The relation penalty in the config is called per_extraterritorial_colony_relation_penalty. I couldn't figure out how to actually loop over colonies directly though, so as a workaround loop over every tile on the map. In the source code I was able to find and add p_colonies to civ_prog_objects.csv, but don't quite understand how to loop over it. This is probably really dumb but I could not quite figure out why this works for p_cities and not p_colonies.
6. The district property buildable_by_civs checks civ names by strings. I really wanted to do this by just figuring out the civ_id and indexing that on start/load, but as configs are loaded before civs are decided, I couldn't do my typical approach. If you're ok with this I am too, but just wanted to point that out.

I think the code is relatively clean, and I worked hard to incorporate your feedback from the first districts PR and keep everything tight. Overall I think it is solid. As you review the code though, you will probably come across align_variant_and_pixel_offsets_with_coastline and a few other functions which handle rendering maritime district stuff. I'll just say I hated putting specific if/else statements there to try to handle pixel offsets for specific terrain tile sheets and sprites, but it really looked bad without it and I couldn't find any cleaner way. In terms of how far beach/coast/land is on the coastal terrain art, it really is wacky and all over the place. Without handling that, ports and so on often appear way out in the water and look stupid. Anyway long-story-short: I know the code there is ugly and am very open to feedback. But the game looks much better because of it.

Remaining stuff to do

Like I said the code is basically done, besides refinement based on feedback and thie issues I need help on above. I'm planning to refactor and add a buildable_on_overlays property for districts that is distinct from buildable_on, and should be done with that in a day or two. (this is done)

There is still some art to finish as well. ZergMazter is doing the art for Central Rail Hub and Municipal District, and probably needs a couple more weeks. I'm working on the light annotations for the new districts as well. So let's wait to actually merge this in until after those are done, but I saw no need to wait for that in order to start code reviews.

Phew! Sorry for the super long explanation. Thank you for your help. I'm very heartened by how many people have reached out and said how happy they are with the new stuff we're adding. A number of people have told me details about custom districts and scenarios they are working on, which makes it all worth it.

[maxpetul]

I have not looked into those other items yet because I wanted to understand the code first. I'll get to them soon.

[instafluff0]

Ok, as always I'm floored by your sleuthing skills and attention to detail. I think I've addressed everything.

1. Regarding patch_Main_Screen_Form_handle_right_click_on_tile: This method interferes with shift + right click on cities to choose production...

instafluff0@34a6560 - I set the right-click to also check and make sure the tile doesn't have a city on it. Note that I intentionally didn't put that check in tile_can_be_named, as tiles with cities can be named, but need to do so via predefined #NamedTile entries in a scenario (this is basically Civinator's use case).

It's hilarious, after all these years I had no idea one could Shift+right click to set production. Thanks for catching that.

2. Re tri_next: The iterator over tile rings does not work properly. For each ring, it iterates tiles up to and including that ring, not just the ones in the ring.

instafluff0@010d032 - nice find, some day I'll learn how to properly write a while loop.

3. Re leader_has_pact_ally_district_access: Shouldn't that check peace treaty and MPP?

instafluff0@6d2a764 - D'oh, yes.

4. When you're checking whether or not to show a message to the player about their civ, you should check if the player in question is the UI controller, not just any human player, so the code works in multiplayer.

instafluff0@85dee1e - I recall I missed this last time with neighborhood messages as well, sorry. Should be good now.

5. Re is_district_command: district_id may be used without having been initialized

instafluff0@f239d2a - Good call. Incidentally this was the fix for "Clicking move units of the same type causes them to 'build' a natural wonder" bug, though I'm guessing you figured that out already.

6. Re patch_City_get_building_defense_bonus: disable_great_wall_city_defense_bonus won't work if optimize_improvement_loops is turned off.

instafluff0@ce7dfb0 - Good catch!

[maxpetul]

Looks good except one thing. In patch_City_get_building_defense_bonus, the logic to cancel the great wall's defense bonus is inside the branch is->current_config.optimize_improvement_loops so if that config option is off it will never run and instead base game logic will. You should modify that condition to check for the optimize setting or cancel_great_wall_boost, running the custom logic if there's anything that might need it. combat_defense_improvs gets initialized in any case so there's no harm in running the custom logic even when the optimize flag is off. Only downside is that turning on the great wall setting will change performance slightly by effectively turning on the improvement loop optimization for that case, but I can't imagine anyone would notice. It's not worth worrying about.

I've finally finished reading through injected_code.c! What an epic it's become, 36000 lines. I have more notes but I've already shared the important things. I'll go over them once more to decide what of the rest is worth bothering you with.

[instafluff0]

Looks good except one thing. In patch_City_get_building_defense_bonus

Got it, I'll take a closer look and fix that. I think I missed the larger implications here.

I've finally finished reading through injected_code.c! What an epic it's become, 36000 lines. I have more notes but I've already shared the important things. I'll go over them once more to decide what of the rest is worth bothering you with.

Whoa! Nice! And thank you for reviewing. I agree it is becoming quite epic. And that sounds good, I'm happy to clean/fix up whatever is needed.

[instafluff0]

Looks good except one thing. In patch_City_get_building_defense_bonus

7bb1943 - Ok, patch_City_get_building_defense_bonus now checks for either optimize_improvement_loops or cancel_great_wall_boost, as discussed.

[maxpetul]

Perfect. I noticed a couple more things since my earlier post:

• In ai_move_district_worker, the if branch under the comment "Allow replace of Great Wall if the civ has the tech to make it obsolete" is empty
• In patch_Unit_ai_move_air_defense_unit and patch_Unit_ai_move_air_transport, most of the inserted logic does not run due to early returns. Not sure if that's intentional. If it is, there should be a comment saying that.

Also I looked into those issues you raised originally:

1. Naval units can't enter canals because there's a check inside Unit::move_to_adjacent_tile that goes like: if the target tile is land and does not have a city and the unit is a sea unit, then disembark passengers and return. See the call to m35_Check_Is_Water at 0x5B91B4.
2. About units getting constantly interrupted when moving over a bridge, in Unit::move_to_adjacent_tile, you're returning the unit itself from select_transport so the method doesn't quit early. Having a non-NULL transport causes the method toward the end to set the unit to the fortified state. I think you're aware of that because your patch to that method restores the unit's state with coast_override_active. The problem is that removing the unit from the go-to state also clears its path_len, path_dest_x, and path_dest_y fields so simply restoring the state doesn't allow it to continue along its path. You have to restore those variables as well.
3. Units stepping off a bridge lose the rest of their turn because of a check again in Unit::move_to_adjacent_tile that goes: if the prev tile was sea and the target is land and the unit is a land unit, then set spent moves to maximum. See the call to Unit::get_max_move_points at 0x5B94E4.
4. To get the pillage button to appear, it should be sufficient to edit patch_Unit_can_perform_command. Looking through set_up_unit_command_buttons, I don't see any reason that wouldn't work. You say it's as if that function is not checked at all for naval units, did you forget that units must have the pillage special action enabled in the editor to be able to pillage? Naval units don't have that action under the standard game rules.
5. [Already answered. I tried skipping straight to no. 6 but GitHub seems to "fix" the list by renumbering all the items.]
6. I'm fine with buildable_by_civs being checked based on string values. The only reason not to do that is performance, and I highly doubt that would be an issue in this case. Though if you wanted to use IDs instead of strings, you could either look up race IDs at the time the config is loaded or look up civ IDs after game loading is done. That would be in patch_do_load_game, notice how era aliases are applied there, that's also something that has to wait until the player objects have been loaded.

[instafluff0]

Awesome, thank you very much for the reviews above. Some of these things I can't believe I missed or forgot about.

In ai_move_district_worker, the if branch under the comment "Allow replace of Great Wall if the civ has the tech to make it obsolete" is empty

b960724 - Ah, I had meant to rework this and add a general check for district obsolescence, after initially thinking to explicitly check Great Wall, then lost track. Implemented now.

---

In patch_Unit_ai_move_air_defense_unit and patch_Unit_ai_move_air_transport, most of the inserted logic does not run due to early returns.

same commit as above - I had been testing and comparing AI movements compared to vanilla code and forgotten to take out the early returns, removed now.

---

Naval units can't enter canals because there's a check inside Unit::move_to_adjacent_tile

7b6cf93 - THANK YOU! This is so strange, I could swear that I tried patching that exact call multiple times before, but for whatever reason I couldn't get it to work. Taking a fresh look now and doing so did the trick. Wow, that is incredibly satisfying.

---

About units getting constantly interrupted when moving over a bridge, in Unit::move_to_adjacent_tile, you're returning the unit itself from select_transport so the method doesn't quit early.

1704b9f - This was the only issue that I wasn't able to fully resolve, and workers on coastal tile go-to states still seem to awaken. You're right, I was aware of non-NULL transport causing the unit to go into fortified state, but hadn't thought to restore those variables as well.

Since Go_To also depends on the path fields, I changed the patch to preserve and restore path_len, path_dest_x, and path_dest_y as well when the unit started the move in UnitState_Go_To:

(line ~27,372)

if (is->coast_walk_restore_goto_path) {
	this->Body.path_len = is->coast_walk_prev_path_len;
	this->Body.path_dest_x = is->coast_walk_prev_path_dest_x;
	this->Body.path_dest_y = is->coast_walk_prev_path_dest_y;
}

Unfortunately I'm not seeing any change. Workers still wake up on water tiles while following a multi-turn route. I'm still investigating, but if you see something obviously off let me know.

---

Units stepping off a bridge lose the rest of their turn because of a check again in Unit::move_to_adjacent_tile that goes: if the prev tile was sea and the target is land and the unit is a land unit, then set spent moves to maximum.

Same as previous commit + 3afa3e2 - Perfect, thank you. I implemented a repl call with patch_Unit_get_max_move_points_for_bridge_exit. Before calling vanilla move_to_adjacent_tile, I detect the bridge -> land case, compute what the spent moves should actually be using the normal patched movement-cost logic, and keep that in is->move_spend_override_value. Then at the disembark callsite I return that precomputed value instead of full max movement, but only for that one case. So normal disembark behavior is unchanged.

I made the injected state variable names a bit more general in case we ever need to reuse them. If I'm overcomplicating this or you want me to rename those, just let me know.

---

To get the pillage button to appear, it should be sufficient to edit patch_Unit_can_perform_command.

7699f0f - Crap, yes you are right. It didn't even occur to me that naval units simply wouldn't have that ability. I will add a note in Districts documentation about this to give people a heads up, but right, it doesn't make sense to force this in C3X code itself given that it can be done via editors.

I'm fine with buildable_by_civs being checked based on string values.

Alright, in that case I'll leave as-is.

---

In other news, ZergMazter completed the Central Rail Hub district art and I'm adding light annotations. I should be able to finish those this week and will merge in as well. Light annotations are also done. Once we're good with the changes discussed here, I will make a mergeable branch in a new PR, as discussed last month.