Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#4973 closed defect (fixed)

Delete BaseHeight and BaseTerrain, cleanup rmgen Init

Reported by: elexis Owned by: elexis
Priority: Should Have Milestone: Alpha 23
Component: Maps Keywords:
Cc: Patch:

Description

r8494 has introduced BaseTerrain and BaseHeight property parsing of random map scripts in the gamesetup.

r9096 has introduced random map script JSON files containing these values.

But it totally doesn't belong there, but should be grouped with all the other elevation and terrain constants. The gamesetup shouldn't be able read nor write to it (but can). It's even exposed to the simulation. Furthermore it makes comprehending the rmgen code much harder. It has also led to a side effects of maps being initialized with the base values just to be overwritten right afterwards with different values.

Change History (6)

comment:1 by elexis, 6 years ago

In r20894:
Implement MapBoundsPlacer that returns all points on the tilemap.
Use it to replace the hacky resetTerrain rmgen2 function from r17903 and initHeight from r20419.

Highlights that createArea is the primary tool of rmgen, rather than nested for-loops over the mapsize.

In r20900:
Merge InitMap, initTerrain and resetTerrain successors from r20894.

Lets the random map script decide which baseTerrain and baseHeight it wants to use before initializing the map,
rather than painting what is in the JSON file, then changing ones mind and painting something else on top of it.

comment:2 by elexis, 6 years ago

In 20904:

Delete BaseHeight property from random map JSON files and rmgen library, refs #6, #4973, r8494, r9096.
The height is only a concern to the random map script, not the rmgen library, nor the gamesetup, nor the simulation.

Reveals the actual initial elevation to the random map script author without looking up the JSON file.
Group and rename all height constants of random maps below the template names.
Rename waterHeight to heightSeaGround to prevent confusion with the actual waterlevel.
Remove useless paintTerrainBasedOnHeight calls on African Plains and Polar Sea.

comment:3 by elexis, 6 years ago

On CircularMap:

Since it's the random map script determining which of the two map shapes (circular, square) it computes, one might consider removing the CircularMap flag from the JSON file as well. The value could be passed to the RandomMap constructor instead.

The simulation and session GUI are both able to derive about the map shape without the JSON setting:

	let cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager);
	if (cmpRangeManager)
		ret.circularMap = cmpRangeManager.GetLosCircular();

So the CircularMap property is only useful in the JSON file if this information can be used in the gamesetup.

So: is this art or can we throw it away? What would be the use case? Free-hand drawing of starting locations of players? Showing a transparent sprite on top of the map preview depending on the map shape (thinking of Delenda Est)? I guess it has to remain in JSON because hypothetical gamesetup use cases can be constructed.

But the startheight and initial terrain is totally usecaseless.

comment:4 by elexis, 6 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

In 20912:

Move BaseTerrain from the random map JSON files to all other terrain names used by the script.

The gamesetup will never be able to do something useful with it, especially change it as an option, so it shouldn't be in the JSON file.
Fixes #4973.

comment:5 by elexis, 6 years ago

Milestone: BacklogAlpha 23

CircularMap seems pointless too actually. transparent map thumbnail sprites can work without knowing the shape of the map. Players selecting the player placement in the gamesetup requires the JSON to specify more detailed settings.

It seems it's in the JSON for simulation/helpers/Setup.js, but I wouldn't be surprised if that call could be moved to C++ instead and use the data passed from the RandomMap object. XML files of skirmish/scenario maps have that flag as well.

comment:6 by elexis, 6 years ago

For skirmish and scenario maps, the CircularMap flag could be in the PMP file, or be an attribute of the map XML, rather than in the JSON of the map XML.

The gamesetup even has code that needs to ensure not to influence this setting. It ought to be read-only access:

Index: binaries/data/mods/public/gui/gamesetup/gamesetup.js
===================================================================
--- binaries/data/mods/public/gui/gamesetup/gamesetup.js	(revision 20909)
+++ binaries/data/mods/public/gui/gamesetup/gamesetup.js	(working copy)
@@ -1808,11 +1808,11 @@ function ensureUniquePlayerColors(player
 }
 
 function selectMap(name)
 {
 	// Reset some map specific properties which are not necessarily redefined on each map
-	for (let prop of ["TriggerScripts", "CircularMap", "Garrison", "DisabledTemplates", "Biome", "SupportedBiomes", "SupportedTriggerDifficulties", "TriggerDifficulty"])
+	for (let prop of ["TriggerScripts", "Garrison", "DisabledTemplates", "Biome", "SupportedBiomes", "SupportedTriggerDifficulties", "TriggerDifficulty"])
 		g_GameAttributes.settings[prop] = undefined;
 
 	let mapData = loadMapData(name);
 	let mapSettings = mapData && mapData.settings ? clone(mapData.settings) : {};
 
}}
Note: See TracTickets for help on using tickets.