We-the-People-civ4col-mod / Mod

This is the repository where the mod resides.
90 stars 37 forks source link

Pedia text contains game data #261

Open Nightinggale opened 5 years ago

Nightinggale commented 5 years ago

The colony defense buildings (stockade, fort, fortress etc) all have an issue with the strategy text. It lists all the defense buildings, complete with how much defense each building provides.

Forum thread where this was reported.

There are a few problems:

Number inconsistency

It says citadel provides 150% while the game data only shows 30%. The thinking behind this is that it doesn't replace the fortress and citadel and fortress combined provides 150%.

Poor readability

Since all other defense buildings replaces the previous step, detecting that a citadel doesn't replace the fortress is done by detecting that it doesn't list what it replaces. It doesn't actually explicitly tell this, which makes it hard to detect for people who doesn't already know this (hence the intended kind of reader).

Hardcoded data

The pedia entry for each defense building has a unique strategy TXT_KEY. Each of those lists all defense buildings and how much defense each of them adds. It seems to display the correct values right now, but updating just one of the buildings means updating the number in the TXT_KEY for each defense building and in each language. It's essentially a "don't touch this or you will break it".

Solution (proposal)

Make a new concept, which tells about how fortifications can be upgraded to the different types. Currently concepts are TXT_KEYs as well, but we can easily fix that. Vanilla has a file called CvPediaHistory.py, which we haven't included (yet). In the function placeText(), whatever is in szText will be what is printed on the screen. If we want, we can add code like

if self.getInfo(self.iEntry).getType() == "CONCEPT_COLONY_DEFENSE":

This will allow us to make an entry just like we want with whatever data from xml, which we would like. It would be a good idea to test for None values (essentially null pointers) instead of just assuming, but that's a detail for when the code is actually written.

If we do this right, we can make the code a bit more generic, essentially getting the type and then run an if else if chain to support multiple concepts with game data entries. We should maintain that the default (not mentioned) entries should still use the pedia TXT_KEY, hence making it compatible with all the current concept entries.

We can make the entry generic in the sense that it can generate a list of defense structures based on xml data. This way if a new building is added, it can be added to the list automatically. It shouldn't be too hard to implement and will ensure an up to date pedia without too much hassle when updating xml.

Alignn commented 5 years ago

I don't think the defensive bonus numbers should be displayed in the strategy text, we already have the stats displayed in the left part of the screen. There's definitely no need for each building to list the bonus from each of the defensive buildings, as long as it's easy to navigate to the other entries.

My suggestion is to simply axe the list of buildings and defensive bonuses from the text, and add a bit about how the Citadel is a separate line of buildings that becomes available after the Fortress is built and allows cannons to fire out of the settlement.

Alignn commented 5 years ago

Example for how the entry for the Great Bastion would look (minus links, font coloring):

The Great Bastion is the sixth and last level of Defensive structures you can construct in your settlements.

Building structures for the defense of a city will provide units in that settlement with a combat bonus when defending against attacks, except for Mounted Units and Siege Units besides the Light Artillery. In addition, once you have built the Stockade, an infantry unit stationed in the settlement will automatically fire at one adjacent enemy unit at the beginning of each turn, damaging or destroying them at no risk to your unit. Each upgrade allows one additional infantry unit to fire.

The Citadel is a separate line of defensive buildings, available after the Fortress is built, and similarly allows Siege Units to fire on adjacent enemies, including ships, with each upgrade allowing one additional Siege Unit to fire.

Fortifications also lower the risk of suffering negative effects from Native raids. For more information about this, see Settlements.

Which reminds me, is this how the "fire on adjacent units" mechanic works? With each level allowing one more unit to fire? Or do you just need one unit to get the full number of "free shots" from the settlement?