Move terrainType to AtBScenario

[AaronGullickson]

We now have terrainType in Scenario (I think as part of the work by @kuronekochomusuke on weather), which references the preset map generation parameters in data/mapgen. However, the code in GameThread will not properly apply these mapgen settings because it uses:

File mapgenFile = new File("data/mapgen/" + scenario.getMap() + ".xml"); // TODO : remove inline file path

The problem is that scenario.getMap() does not access terrainType which is what needs to be accessed. I believe this is because somewhere in the innards of AtB code, terrain type gets translated into the map variable. However, this does not happen in vanilla Scenario.

This PR fixes the problem by just switching the code above to directly call scenario.getTerrainType() rather than scenario.getMap(). It should not affect AtB because AtB uses a different game thread. I also had to update a few other things to get proper null checks into the code and to make sure we didn't erase the default map settings before we confirm that we are actually going to get new ones.

[kuronekochomusuke]

I think terrainType needs to be convert into a mapType template. not sure if just using the terrainType works. each terrainType has multiple possible mapgen parameters.

Stratcon uses setMapFile() function to set map, so it may be better move this into scenario out of AtBScenario. then call it to set the mapType from the terrainType .

If doing this probably also need to move the mapTypes into its on xml out of StratconBiomeManifest.xml

public void setMapFile(String terrainType) {
    if (terrainType.equals("Space")) {
        setMap("Space");
    } else {
        Map<String, StratconBiomeManifest.MapTypeList> mapTypes = SB.getBiomeMapTypes();
        StratconBiomeManifest.MapTypeList value = mapTypes.get(terrainType);
        if (value != null) {
            List<String> mapTypeList = value.mapTypes;
            setMap(mapTypeList.get(Compute.randomInt(mapTypeList.size())));
        } else {
            setMap("Savannah");
        }
    }
}

<entry>
        <key>ColdForest</key>
        <value>
            <mapTypes>ColdForest</mapTypes>
	<mapTypes>Some-trees-snow</mapTypes>
            <mapTypes>Wooded-lake-snow</mapTypes>
            <mapTypes>Woods-deep-snow</mapTypes>
            <mapTypes>Wooded-valley-snow</mapTypes>
            <mapTypes>Woods-river-snow</mapTypes>
            <mapTypes>Wooded-tundra</mapTypes>
            <mapTypes>RuralHomesteadsCold</mapTypes>				
        </value>
    </entry>

[AaronGullickson]

Your suggested approach would tie vanilla scenarios into the specific logic of StratCon, which in my opinion is against design principles - the specific should not dictate to the general. StratCon assumes that the settings files in mapgen should be grouped thematically but this is not necessary for the code here to work. All of the XML files in mapgen are functionally independent from each other and there is no reason not to offer the player using a vanilla scenario approach access to them all equally.

[AaronGullickson]

I also think the way AtB/StratCon handles this is non-intuitive. Why convert the values from terrainType into map? That makes the map object confusing because it can be both the name of a terrain type mapgen file or the path to an actual map file. A better approach would have been to just overwrite the existing terrainType with a new value when sampling from the possible biome values. Fixing that in the future will help work toward removing annoying discrepancies between the two types.

[kuronekochomusuke]

if the function is moved it is no longer stractcon logic. its generic logic.

each terrain type has multiple map parameters to provide more variety to the maps generated. the terrain type defines the general setting for the region. where the map is a specific area within the region.

you may need to verify that all the terrain types have a mapgen parameter file setup, if you continue with your logic.

[AaronGullickson]

It still is StratCon logic though because for vanilla scenarios users can specify either a fixed map or a random one. The fixed map path is what is held in map. That will be overwritten by a setMap function. If terrain type and map really are supposed to represent distinct things, they should be proper classes and not ambiguous String variables. If we need to fix something, it ought to be the baroque code of AtB not the code of vanilla Scenario.

you may need to verify that all the terrain types have a mapgen parameter file setup, if you continue with your logic.

I don't understand what you mean by this. Where are all the terrain types defined? Because terrain type is not properly classed, but done in ad hoc way, there is no enum or anything defining all the terrain types. Terrain type is just literally the name of an xml file in the mapgen directory. By definition, then it exists.

If it helps to resolve this, I can just another string variable name like mapGen in vanilla Scenario instead of terrainType to access the map generation parameters. TerrainType should then be removed from Scenario and into AtBScenario because it has no purpose there and exists only for AtB logic.

[AaronGullickson]

If it helps to resolve this, I can just another string variable name like mapGen in vanilla Scenario instead of terrainType to access the map generation parameters. TerrainType should then be removed from Scenario and into AtBScenario because it has no purpose there and exists only for AtB logic.

I have gone ahead and done this in the most recent commit. This should help to avoid confusion about the purpose and design of the different variables. Ideally, I think the setMapType, should stop setting map and start setting mapGenerator, for consistency, but unsnarling the AtB code is beyond this PR.

[AaronGullickson]

Ok I think I have a better solution to this in mind so hold off on review or merge.

[AaronGullickson]

Ok, I am realizing that terrainType was a red herring. I can just use map directly and the usingFixedMap variable to distinguish which case we have (which is what the GameThread already does). When all that is corrected all this PR actually does is move terrainType to AtBScenario because that is where it is being used - and that way it won't be confusing (like it confused me).