91 River Commerce Bonus

[QuintillusCFC]

Resolves #91 .

Adds the river commerce bonus. You can view it via the right click action.

Notably, the AI will automatically pick this up when evaluating where to place cities - they should now be more likely to settle near rivers.

[pcen]

Is git doing something weird, or did the save size actually quadruple with this commit? Maybe we want to write some Json serialisation and deserialisation to combine certain boolean attributes into a bit mask in the Json. Not adding complexity is important, but we also probably don't want the save size blowing up every time we add a few fields to a data class

[QuintillusCFC]

Is git doing something weird, or did the save size actually quadruple with this commit? Maybe we want to write some Json serialisation and deserialisation to combine certain boolean attributes into a bit mask in the Json. Not adding complexity is important, but we also probably don't want the save size blowing up every time we add a few fields to a data class

The net number of lines added (25,600) is 4 times the net number removed (6400). However, even before the changes, I see 67,677 lines - most lines were not modified.

The cause for the inflation is tiles now including whether they touch a river on their vertices - north, south, east, or west - which is used by the commerce bonus. There are a lot of tiles, even on this Small map, so it adds up to a lot of lines.

It's a fair question whether we want to shrink this somehow. The compression will reduce it by about 90%, but you're right that especially for tiles, every addition will add (does math) 80 * 80 / 2 = 3200 lines. And it isn't very easy for the user to manually modify these river connections - possible, but not friendly.

I can think of two ways to possibly shrink it, both with at least a bit of complexity tradeoff:

• Instead of a variable for each direction, store a list of which directions have rivers. Will be shorter for most tiles, about the same in the worst case.
• Have a separate structure of Rivers, that defines which tiles rivers flow between, and then calculate the tile-centric river information after load. This has the advantage of removing the duplication of information between the tiles the river flows between.

[maxpetul]

I wish I had known it was possible to import that vertex river info from Civ 3 yesterday when I was working on implementing river crossing defensive bonuses. I instead implemented a little function that checks if a river is present at a tile vertex based on only the edge info. That code might still be useful since we could store only the edge info in the save and fill in the vertices at load time. But I wouldn't choose that approach. I think the best way is to remove the per-tile river info from the save and instead store a list of tile vertices that have rivers.

[QuintillusCFC]

The rabbit hole gets deeper too... it's possible in the Firaxis editor to create a river that is only at a vertex (does not follow an edge). I believe this does give the river bonus, although my Firaxis editor broke last week so I'm not 100% sure.

There are also two areas of the BIQ TILE with river info... "river connection info" (which is what we're using) and "river crossing data".

I couldn't tell you what the intended difference between the two is; in my editor I commented years ago that the latter appears to be zero. BUT there's at least one scenario, The Great War, where the rivers are obviously scrambled in both C7 and my editor, but correct in the Firaxis editor and in-game. When I print out the TILE data to a flat file from my editor, I can find tiles where "river crossing data" is not zero. I suspect that is related to why the river rendering is not correct.

It doesn't help that the Apolyton documentation of "river connection info" appears to be incorrect (it appears to follow the "river crossing data" format).

Most likely, to get true 100% compatibility, someone needs to do a deep dive on all of the possibilities between these similar-sounding data structures in the BIQ format. Maybe we're lucky and someone posted an answer at CFC at some point.

[QuintillusCFC]

I wish I had known it was possible to import that vertex river info from Civ 3 yesterday when I was working on implementing river crossing defensive bonuses. I instead implemented a little function that checks if a river is present at a tile vertex based on only the edge info. That code might still be useful since we could store only the edge info in the save and fill in the vertices at load time. But I wouldn't choose that approach. I think the best way is to remove the per-tile river info from the save and instead store a list of tile vertices that have rivers.

I don't disagree with the general thoughts. But it would generally be edges that have rivers, rather than vertices. With the caveat above that it technically can be vertices as well.

Rivers would be near the top of my wish list for more clear documentation of how the BIQ/Civ3 works.

[maxpetul]

Is it possible to have rivers at two neighboring vertices but no river along the edge connecting them?

[QuintillusCFC]

In a quick test, I haven't been able to create that situation with the Firaxis editor, but I was with Quintillus's editor, and the resulting situation is recognized as such in Firaxis's editor.

Random Map with City.zip

See the mountain at 23, 25 - it has rivers from both its west and south vertices, but not along its southwestern edge.

So it seems to be supported by the BIQ format. As for why it can be done with my editor, but is difficult with Firaxis's, I intentionally switched the model from "paint at vertices" to "paint at edges" because I'd found it frustrating trying to get rivers to go the direction I wanted them to go with Firaxis's editor. Firaxis's seems to automatically connect the edge when you click on two neighboring vertices, but with my editor you can specify exactly which edge you want to get the river.

[maxpetul]

Thanks for looking into it. I thought the Firaxis editor didn't let you have unconnected rivers at neighboring vertices but wasn't sure. I wonder if the map generator could ever produce that. In any case it's something we should support. A list of vertices won't work, so I guess the next simplest thing is to store river info for a vertex and two edges per tile, with an extra row & column at the map edges. Because of the extra space needed, it would probably be easiest if the info were not stored in the Tile object but externally indexed by Tile coords. That would also make it easy to omit info for tiles without rivers, for compactness. That's the approach I'd take, anyway.

[QuintillusCFC]

A vertex and two edges per tile? I'm somewhat struggling to visualize that; I had been thinking it would be simpler to define a river as going between two vertices (e.g. forming an edge), or just one vertex if it's a one-vertex-only river. Although we'd also have to define the format for defining a vertex. Are the vertex coordinates the same as the tile to the north, south, east, or west of that vertex?

In other words, I was thinking something along the lines of:

rivers: [
	{
		vertexOne: "34, 21",
		vertexTwo: "35, 22"
	},
	etc.
]

I've also started moving towards the "omit for things that don't have the property" method, such as for unit prototypes at #245 . I think it's both more compact and easier to examine and modify manually that way.

I think it's probably best to split that off from here, similarly to what we've done with the C7 rules format. I opened #248 just now to track those changes.

[WildWeazel]

is this ready to merge?

[maxpetul]

Should be, yeah. I tested it and it works. We were trying to think of a convenient & more compact way to store river data in the save since the current approach takes a lot of space, but it works so that matter can be left for later.

[WildWeazel]

Just a thought, but assuming we're keeping the convention of tiles being even/even or odd/odd coordinates, I wonder if it would help to treat even/odd coordinates as features between tiles. I think that would work out to being vertices, which may or may not be what we need.