Remove all globals from random map libraries

[elexis]

It should always be the map that decides what values it needs.

• g_MapSettings read-only global set by C++ that holds the map JSON and gamesetup settings. Should likely become a var g_MapSettings = Engine.GetMatchSettings() in the map script.

• g_Camera holds the initial camera settings, should be moved to RandomMap prototype

• g_Environment holds the skyset, sunangle, watersettings and so forth, should be moved to RandomMap prototype

• g_NomadTreasureTemplates not sure about this. We also have some other default templates like chicken in the player.js library file. Since chicken and nomad treasures are currently the same on any map, it hurts to copy it to all 60 of them.

• TILE_CENTERED_HEIGHT_MAP this thing is totally bugged and needs to be replaced by an all-maps-encompassing redesign

• WATER_LEVEL_CHANGED this is semi-broken (default waterheight 19.9 but claiming to be 20?)

• g_WallStyles contains template data in the wall_builder. Refered to by maps as well. The maps should define this variable and pass it to the functions. Or the functions should just receive template names and load the Template directly from the engine.

• g_FortressTypes only used in two places, should be easy to remove equally.

• There are some engine constants in library.js. Should likely be moved to the maps and library functons. See according ticket somewhere.

• g_Map seems hard to remove. It holds the RandomMap prototype, which holds all ground textures, elevation grid and entity templates + positions to be exported to C++. There are exactly 0 use cases for having two instances of the prototype around. However having states (global variables) in libraries is a faux-pas in object-oriented programming per se. It makes separation of concerns harder to establish. All the helper functions ought to either become (1) deleted, (2) part of the RandomMap prototype if they are universal, or (3) receive a RandomMap prototype argument. However that means every single createArea call will receive this argument that never changes 1 to 4 times. So we might have to live with this global.

Maybe it could be removed by making createArea a function of the RandomMap prototype and letting the prototype set the RandomMap instance of the painters, placers and constraints used.

[elexis]

In 20932:

Move ExportMap code from global helper to RandomMap object, refs #4804.
Delete WATER_LEVEL_CHANGED global, refs #4964.

[elexis]

In 20995:

Delete InitMap and ExportMap proxies and let the random map define the RandomMap object, refs #4804.
Eventually there should be no direct reference to g_Map in the library, refs #4964.

[elexis]

In 20996:

Move getMapCenter and getMapBounds to the RandomMap object, refs #4964.

The reference to the global should not be hidden behind a proxy function and
object orientation is preferable over procedural code in general.

[elexis]

In 20997:

Move getMapSize to the RandomMap object, refs #4964.

[elexis]

In 21000:

Delete createTileClass proxy, refs #4964.
Even though it prolongs the code, OOP should be prefered.

[elexis]

In 21069:

rmgen random placement and Entity instantiation refactoring, fixes #4992.

ChainPlacer, ClumpPlacer, SimpleObject receive the vectors that are in place everywhere already, refs #4845.
Add public setCenterPosition to CenteredPlacer and Group rather than writing private properties of the prototypes.
ChainPlacer and ClumpPlacer simplifications, deduplication and renames, refs #4805.

Replace placeObject global with RandomMap placeEntity members, refs #4804.
Split to placeEntityPassable / placeEntityAnywhere, as well as validTilePassable / validTileAnywhere to more cleanly distinguish actor and casual entity placement.
No more does SimpleObject create Entity instances and register entityIDs if they are never placed.
Removes the map global reference from the Entity constructor, refs #4964.
By definition of what is passed to the engine, an Entity has an ID and position, so keep it impossible to create Entities without IDs.

Implement randomPositionOnTile so that there aren't different implementations thereof, including unintented ones as in ardennes_forest.js in rP21021.
On Caledonian Meadows, remove unused pathplacing code, to be superseded by #4368.
On Schwarzwald, delete unused startLocations following rP20864.
On Latium, replace complicated duplicated hardcoded fish location computation with a simple HeightConstraint, refs #4960.

[FeXoR]

Replying to elexis:

It should always be the map that decides what values it needs.

I agree though I don't see an urgent need. Also some things like the default building angle are design decisions and - while the map author should be able to change them (so don't make them constants) - they also should be the default IMO (so define them on one place every map can get it from - a lib). Did any problems arise inspiring this changes?

• g_NomadTreasureTemplates not sure about this. We also have some other default templates like chicken in the player.js library file. Since chicken and nomad treasures are currently the same on any map, it hurts to copy it to all 60 of them.

There may be other suitable entities added e.g biome specific ones. I'd say: If it's not an improvement (e.g. a lot less lines of code) don't change it. Neither a global with only one entity string neither 60 identical lines in maps with that same entity string (which might change) seem ideal to me.

• TILE_CENTERED_HEIGHT_MAP this thing is totally bugged and needs to be replaced by an all-maps-encompassing redesign

I don't see a way to do this without using another tile centered heightmap generated from the non-tile-centered Map.height to fulfill the needs of the used cases (e.g. heightmap.js getTileCenteredHeightmap() could do that) but that's only a snapshot and would need to be generated again if needed and Map.height changed (and generating it on the flow would be quite resource intense). What is "totally bugged" is the interaction between this setting/Map.height and placers/painters/constraints so one could point fingers on any one of them :/

• WATER_LEVEL_CHANGED this is semi-broken (default waterheight 19.9 but claiming to be 20?)

What is broken here? Was 20 when I added that comment.

• g_WallStyles contains template data in the wall_builder. Refered to by maps as well. The maps should define this variable and pass it to the functions. Or the functions should just receive template names and load the Template directly from the engine.

You really want to add the wall style definitions to every map that places default starting entities for Iberians? But you also want to drag any duplication out of the maps and into libs? I guess I don't get this correctly because that's contradictory to me. (In the end all functions are as global as this vars/consts). To keep things more tidy one could bundle stuff into objects e.g. playerPlacements.Circle() instead of playerPlacementCircle() and those then could also contain some values. But I guess that'd be a faux-pas, too?

• g_FortressTypes only used in two places, should be easy to remove equally.

And if we move this to the maps then we can also move placeFortress() to the maps. And when there are other maps added using this and we can move it back. I really don't see the point of this. The fortresses are perfectly logical placed in the wallbuilder IMO.

• There are some engine constants in library.js. Should likely be moved to the maps and library functons. See according ticket somewhere.

Those all chould be got from the engine so if they change no bugs arise. They (at least some of them) could also go to the Map class IMO.

• g_Map seems hard to remove. [...] There are exactly 0 use cases for having two instances of the prototype around.

One could generate a procedural larger-than-map area and split that across several maps and export all for campaigns, one could generate 4 maps from existing map scripts and bundle them onto one map so you have 4 simultaneous 1on1 games going and the winner is the fastest 1on1 winner - just to name 2.

However having states (global variables) in libraries is a faux-pas in object-oriented programming per se.

I can live with that. And we did years.

It makes separation of concerns harder to establish.

Interlinked functionality like placers/painters/constraints make the separation of concerns harder. Especially if the scope of the concerned parts are underestimated by design. You also get a limitation of usability from it - not what I expect to be helping for a creative endeavor like random map design.

All the helper functions ought to either become (1) deleted, (2) part of the RandomMap prototype if they are universal, or (3) receive a RandomMap prototype argument. However that means every single createArea call will receive this argument that never changes 1 to 4 times. So we might have to live with this global.

(1) Agreed, deleting as many as possible (without the need of adding more than one line per usage to the maps or twice the number of lines of code to libs).

(2) Uhm, only if it purely acts on it's attributes. Otherwise - please not.

(3) Oo Why?! Just because you read it is bad practice?

Maybe it could be removed by making createArea a function of the RandomMap prototype and letting the prototype set the RandomMap instance of the painters, placers and constraints used.

Wouldn't that make the about half the random map code non-separably intertwined? And that in the name of separation of concerns? I'm speachless.

I agree we should try to remove some functions - if applicable. I agree we should leave most decisions to the map designer. But I can definitely live with some globals.

[smiley]

Regarding TILE_CENTERED_HEIGHT_MAP, its only used for doing things based on height. Obviously the height of one vertex cant be used to paint a whole tile. Whats really needed is to have a function which can return the mean height of 4 verteces.

Regarding g_Map, I would suggest moving createArea and such to the RandomMap prototype. However, when the time comes for NewRandomMap it would be a pain again. I guess its here to stay.

[phosit]

Replying to elexis:

It should always be the map that decides what values it needs.

• g_MapSettings read-only global set by C++ that holds the map JSON and gamesetup settings. Should likely become a var g_MapSettings = Engine.GetMatchSettings() in the map script.

I disagree with that. IMO the script should get all data it _might_ need, Execute the code and expornt the map. Everything with as few script -> engine calls as possible. The best solution i see is to put "everything" in a function like this:

Engine.LoadLibrary("rmgen");
Engine.LoadLibrary("rmgen-common");

function GenerateMap(mapSettings)
{
	...
	return g_Map.MakeExportable();
}

This removes the need for g_MapSettings and Engine.ExportMap. If one also wants to remove the Engine.SetProgress one could make the function a generator and yield the progress.

[marder]

Replying to phosit:

Replying to elexis:

It should always be the map that decides what values it needs.

• g_MapSettings read-only global set by C++ that holds the map JSON and gamesetup settings. Should likely become a var g_MapSettings = Engine.GetMatchSettings() in the map script.

I disagree with that. IMO the script should get all data it _might_ need, Execute the code and expornt the map. Everything with as few script -> engine calls as possible. The best solution i see is to put "everything" in a function like this:

Engine.LoadLibrary("rmgen");
Engine.LoadLibrary("rmgen-common");

function GenerateMap(mapSettings)
{
	...
	return g_Map.MakeExportable();
}

This removes the need for g_MapSettings and Engine.ExportMap. If one also wants to remove the Engine.SetProgress one could make the function a generator and yield the progress.

Not sure if I can follow. I see the need to remove / reduce the globals, but could you further explain the downside of the Engine calls? I.e. why exactly are they bad?

[phosit]

The interface is simpler/smaller. Now it's undefined what happens if Engine.ExportMap is called multiple times or what happens if Engine.SetProgress is called after Engine.ExportMap is called.

From the C++ side it's simpler because the map (the argument to Engine.ExportMap) doesn't have to be temporally stored in the callback-data (the data accessible by Engine functions). Same applies to the progress.
Linear code is preferable to callbacks (what Engine functions effectively are).