Moddable terrain improvements #786
Conversation
Add zIndex field to the TerrainImprovement class
Data on improvement yield bonuses moved from the TerrainType to TerrainImprovement
| public readonly string key; | ||
|
|
||
| public readonly Layer layer; | ||
| public readonly StrengthBonus? defenseBonus; | ||
|
|
||
| // On the game map, overlapping terrain improvement with a higher zIndex cover those with a smaller one | ||
| public readonly int zIndex = 0; |
There was a problem hiding this comment.
How does this differ from the layer?
There was a problem hiding this comment.
zIndex only affects the drawing order on the game map. The layer affects the gameplay (each tile can have only one type of improvement per layer). Two improvements can belong to the same layer, but have different zIndexes (e.g. irrigation is drawn before roads, and mine is drawn after roads). I added a comment with an explanation to the layer field.
|
|
||
| private Dictionary<(TerrainType, Tile.YieldType), int> bonusYields = []; | ||
| private readonly SaveTerrainImprovement dataSource; | ||
|
|
||
| public TerrainImprovement( |
There was a problem hiding this comment.
Do we need both constructors?
There was a problem hiding this comment.
Edit: is one for the hardcoded civ3 rule list?
There was a problem hiding this comment.
The first constructor is used in EdgeWalkerTest to create a test road instance
C7/Lua/rules/civ3/terraforms.lua
Outdated
|
|
||
| local terraforms = { | ||
| build_mine = { | ||
| validator = function(context) |
There was a problem hiding this comment.
I found it a little confusing at first that we have some tables here with a validator, and some with an effect, but none with both.
I think that's just a naming artifact, right? Would naming these like irrigate_validator/mine_validator/etc work, or would that break things?
If it would break things, it might be worth the duplication to duplicate the clear_foliage validator so the wetlands/forests have a validator and an effect, and then have a comment in mine/irrigate that the default effect is to add the improvement.
There was a problem hiding this comment.
I agree, this table should be simpler. There's no need for each function to have a sub-table. Fixed it.
| local terrain_improvements = { | ||
| railroad = { | ||
| tile_modifier = function(yield) | ||
| local production_improvement = yield.tile.overlays:ImprovementAtLayer(Layer.ResourceDevelopment) |
There was a problem hiding this comment.
maybe "yield_improvement"? I don't normally think of irrigation as a production improvement
There was a problem hiding this comment.
Good catch, done.
| end, | ||
| return { | ||
| buildings = require "civ3.buildings", | ||
| terraforms = require "civ3.terraforms", |
There was a problem hiding this comment.
It might be worth a comment in the Lua file explaining what the difference between a terraform and a terrain improvement is.
There was a problem hiding this comment.
I added a comment for now, but I think terraform is just a bad name for this type of entity. We should name it something clearer like "worker job".
| namespace C7GameData.Save; | ||
|
|
||
| public class SaveTerrainImprovement { | ||
| public static IEnumerable<SaveTerrainImprovement> Civ3Improvements() { |
There was a problem hiding this comment.
Could you leave a comment explaining why we need this list of improvements?
|
@TomWerner Thanks for the review! I think I've addressed all your comments. I'll leave this PR open for a few days, in case anyone else wants to contribute a review. |
esbudylin
added
Lua
This PR continues the work on Terrain Improvements from #710. Previously, all terrain improvements were hard-coded in the game code. This PR moves terrain improvement definitions into the JSON file, which makes them moddable.
This PR also moves the tile yield bonuses into the TerrainImprovement class: previously, they were part of the TerrainType class which allowed defining bonuses only for mine, irrigation, and road improvements. With the new system, a bonus yield of any type can be defined for any terrain improvement.
To fully match the Civ3 bonuses, a special tileModifier field was introduced to the TerrainImprovement class. Similar to the building class (#697), it holds a delegate loaded from a Lua function. Its goal is to allow bonus yields depending on tile context: for the Civ3 ruleset, this is useful for implementing the railroad bonus (only grant additional yield if a mine or irrigation is present on a tile).
Finally (the size of this PR got out of hand!), I made a few changes to the Terraform class. Now terraforms (worker's jobs) can be associated directly with a terrain improvement. Also, the existing logic for terraform effects and prerequisites was moved to Lua.
Right now we have almost all the elements for moddable terrain improvements: it is possible to define new terrain improvements, worker jobs for them, and associate them with custom graphics. The only missing system is the UI for worker jobs: right now the game displays buttons only for a hard-coded list of worker actions. Making it moddable would be a logical step forward.