Page MenuHomeWildfire Games

Print an error message in autostart if the player forgot the numplayer arguments when it was mandatory
AcceptedPublic

Authored by mimo on Feb 4 2018, 9:46 PM.

Details

Reviewers
elexis
Summary

As in autostart, the numplayers argument is only needed for random maps with more than 2 players, it is easily forgotten and this lead to some not easy to debug errors in the rmgen code. So it may be useful to have a clear error messages in such case.

Test Plan

Test the patch

Event Timeline

mimo created this revision.Feb 4 2018, 9:46 PM
Vulcan added a subscriber: Vulcan.Feb 4 2018, 9:51 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Feb 4 2018, 9:51 PM
Executing section Default...
Executing section Source...
Executing section JS...
elexis accepted this revision.Feb 19 2018, 5:35 PM
elexis added a subscriber: elexis.

Since 2 is the default, the condition is correct.
Keeping 2 as the condition as opposed to simplifying it and making it always mandatory is in line with the rest of the options which also come with defaults.

Are you sure you don't want to even throw PSERROR_Game_World_MapLoadFailed("hello world"); like is done

source/ps/GameSetup/GameSetup.cpp
1118

here

1227

and here

1281

and here

This revision is now accepted and ready to land.Feb 19 2018, 5:35 PM
mimo added a comment.Feb 19 2018, 7:20 PM

I'm not sure that patch is still needed as i can't anymore reproduce the original problem. Did you change the way PlayerData are used in random scripts?

Don't recall any such change. In fact I had removed the default fallback civ athen somewhere which can bug in Atlas in some way iirc.

But the gamesetup.cpp code looks like it handles the case of too many civs or AIs given already, so it doesn't appear like it was needed at any time this release?

18133  sanderd17 			// Instead of overwriting existing player data, modify the array
 9699        ben 			// Instead of overwriting existing player data, modify the array
13278    wraitii 			// Instead of overwriting existing player data, modify the array
15761        ben 				// Instead of overwriting existing player data, modify the array

I couldn't reproduce it either:

playa22 -autostart="random/alpine_lakes" -autostart-civ=1:athen -autostart-civ=2:brit -autostart-ai=2:petra -autostart-civ=3:athen
playa22 -autostart="random/alpine_lakes" -autostart-civ=1:athen -autostart-civ=2:brit -autostart-ai=2:petra -autostart-ai=3:petra

Sounds like abandoning the patch should be done.

I just found that passing autostart-civ= results in an out-of-memory when parsing the JSON

source/ps/GameSetup/GameSetup.cpp
1357

This part extends the object

1374

looks like 2 should have would have to be replaced with numPlayers, so we don't have to keep it in sync

1375

more than 2 AI players

1415

So does this.

AI, AI-Difficulty, Teams, Civ do extend the playerData array and those seem to be all player commandline options.
@mimo do you recall how you reproduced an issue originally or can imagine a way how it could be achieved?
The only thing I can think of is maybe passing player 1 and player 3 but not player 2.

mimo added a comment.Mar 18 2018, 1:48 PM
In D1280#57363, @elexis wrote:

AI, AI-Difficulty, Teams, Civ do extend the playerData array and those seem to be all player commandline options.
@mimo do you recall how you reproduced an issue originally or can imagine a way how it could be achieved?
The only thing I can think of is maybe passing player 1 and player 3 but not player 2.

Ah yes, i forgot about that one.
The problem is still there in fact if you forget the Civ attribute (not tested for the others).
Just launch an autostart with 4 players without the autostart-autoplayer: if you give the Civ for all, it will work, but if you forget it for any player, it will crash (without using the default arg) while when autostart-autoplayer is given, it will use the default civ.

Stan added a subscriber: Stan.Jan 9 2019, 6:13 PM

Might be good to have this @elexis.