[PATCH] Add skirmish map support to -autostart option

[Teiresias]

This is to allow the -autostart command line option to run the "skirmish" maps as well as the scenario maps.

At the time of writing, the !GameSetup code assumes all "fixed" maps are located in the maps/scenarios folder (in GameSetup.cpp rev. 14953, there is a "// TODO: support akirmish maps"). With this patch, the game will look in the maps/scenarios folder for the map as usual, but if the map is not found there, it is taken from the maps/skirmishes folder if possible.

Unfortunately, no CxxTests provided with this patch - the GameSetup code is closely interwoven with the actual game startup code, so testing would likely require

1. stubbing of CGame, CSimulation2 and CScriptInterface (plus installation of a class factory for CGame) or
2. setting up a virtual VFS (see patch at #1396) plus lots of stub objects, likely to give a very fragile test.

For an external-supplied patch, these costs appear prohibitive, given that -autostart is considered a "development feature" (see ​IRC log at 19:43).

Patch tested with MSVC2008/Win7-64.

[sanderd17]

It shouldn't do auto-detection. It's perfectly possible to have an Acropolis map as skirmish and scenario.

Either it should require a prefex (so -autostart=scenario/Acropolis or -autostart=skirmish/Acropolis). Or it should have a different option (like we already have -autostart-random). Also don't forget to update the various documentation (there's a README somewhere and it's also on our wiki, but that's for after the commit).

[Teiresias]

Ok, i've changed the patch as discussed on IRC. Two options were discussed:

1. Define a new command line switch to tell skirmish and scenaro apart. Introducing yet another command line switch seems a useless complication - for consistency, scenarios had to get their selection switch as well (or a general -mapType=... switch had to be used).
2. Have a (partial) prefix for the map to select the type, e.g. -autostart=maps/scenarios/NameOfMap. Given that the introdution of any further map types likely requires a change to GameSetup.js, a hard-wired mapping of directory names to map types seems acceptable. Omitting the common maps/ prefix is probably worth it since maps really shouldn't be placed anywhere outside the regular directory structure. Chosen.

No CxxUnit tests provided for reasons already stated. No docs updates included as patch needs approval first.

[sanderd17]

The arguments you picked are good for me. So you can document it in the readme (in binaries/system) . I'll review the code tomorrow, but did you notice the comment at line 1066 is wrongly formatted now?

[Teiresias]

I've added the changes to readme.txt to the patch.

Regarding the formatting issue in 1066 - unfortunately, i didn't see it (i added this as a last-change when creating the patch, with my internet machine having no advanced editor whatsoever - lesson learned: Never take shortcuts). Fixed in the new version of the patch.

[sanderd17]

Code style is good. But when testing, I noticed two problems.

First, if no civ is given for skirmish maps, it shouldn't crash, it should just default to something. I believe random maps always default to "athen", that's good enough. Can you look into this (it might be needed in the JS side rather than the C++ side)

And secondly, when starting a skirmish game that does load, I get the following warning:

WARNING: JavaScript warning: simulation/helpers/InitGame.js line 31
reference to undefined property settings.mapType

It would also be nice if that warning is gone (and if you could look why the mapType is missing, you do seem to set it).

Thanks

[Teiresias]

I've replaced the patch with a version that fixes the crash. While i've not investigated each detail on how the crash 'works', it is caused by CGame::RegisterInit() loading the game "settings" from the attribs object created by Autostart(). So, when no players are defined at the command line, the PlayerData array is empty and the game crashes.

The new patch now peeks into the map xml file to fetch the pre-defined ScriptSettings and do away with the crash. At the same time, this also does away with the loading screen showing Loading "undefined" when a scenario map is loaded. This behavior occured with the original GameSetup.cpp as well.

The warning about the missing mapType is gone as well - i don't know why, but the map type has to be put into the settings as well as the attrs.

The following problems still occur:

1. If an ai-player or a civ for an out-of-range player id is defined, the game still crashes (e.g. -autostart-ai=3:aegis for a two-player map). Adding error-checking code for that is possible but i think it is not worth it, given that the autostart feature is intended for developers.
2. When used in multiplayer mode, 0ad instanced launched via autostart run in "observer mode" (map uncovered, no player assigned). I would like to not tackle that issue in this patch, but rather start a new ticket for that.
3. When used in multiplayer mode, only the host instance of 0ad can autostart ai players, passing the respective argument to the autostart-clients has no result. I feel this should go into a separate ticket as well, as it appears independant of the map type.

For clarity, i've extracted the map 'peeker' into a separate routine. As i'm not too familiar with proper use of Xeromyces, any comments welcome. Please delete Doxygen header in front of LoadSettingsOfScenarioMap() if inappropriate - i'm trained to always build them :-/

[sanderd17]

1. No problem. I just like my shell commands to be short. So having to define a civ for every player was a drawback
2. When developing, we can always "switch perspective", so that's fine.
3. That's fine too.

Btw, we also use doxygen: ​http://svn.wildfiregames.com/docs/

Your code looks fine so far, I'll test it later on

[Teiresias]

Patch to support skirmish maps in autostart games, with crash and loading-screen bug fixed. Improved on some misleading error messages/comments.

[sanderd17]

In 15257:

Allow the CLI to load skirmish maps. From now on, the directory must be included in the map path to load any maps (see the readme). Patch by Teiresias. Fixes #2572