Details

Reviewers

vladislavbelov
leper

Commits

rP19645: Add a -autostart-nonvisual option. Patch by sacha_vrand. Fixes #4577.

Summary

The purpose of this patch is to add a new option for the autostart. It loads an autostarted game but without any GUI and sounds. The engine will stop the game on its own when the game is finished and the end game statistics are directly printed on the console. The main purpose of this option is to analyze the AI.

Some improvement ideas :

The file simulation/components/interfaces/StatisticsTracker.js is empty because I don't need it anymore but it is loaded for some component tests. It can be removed but I'm not sure how to change the tests.
~~Add an option to the autostart to choose the victory conditions.~~
Make the terrain movement costs independant of any graphics code.

Test Plan

Run pyrogenesis with the options :
-autostart='skirmishes/Acropolis Bay (2)' -autostart-ai=1:petra -autostart-ai=2:petra -autostart-aidiff=1:5 -autostart-aidiff=2:5 -autostart-nonvisual -autostart-victory="conquest"

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

elexis added inline comments.Apr 28 2017, 4:59 PM

binaries/data/mods/public/gui/credits/texts/programming.json
179 ↗	(On Diff #1469)	nickname too
binaries/system/readme.txt
16 ↗	(On Diff #1469)	useful for AI testing I also see it very useful for testing the balancing of maps where gaia spawns, like survival of the fittest for example, which even has a dry run option
source/ps/Game.h
140 ↗	(On Diff #1501)	Why the negation in the function name? Why not return !m_GameView (or m_GameView respectively)?
source/ps/GameSetup/GameSetup.cpp
710 ↗	(On Diff #1501)	So that nonvisual replay check is obsolete now afaics. Sometime someone could also add an argument to enable replay saving in that mode.
975 ↗	(On Diff #1501)	(nonvisual replay check not here because nonvisual replay does its own thing unfortunately)
1513 ↗	(On Diff #1501)	Agree, kind of a dealbreaker, C++ should be JS agnostic as far as possible

Most of the C++ changes seem useless and just complicate things. The few that are needed should be merged (if possible) with the other visual/nonvisual options.

Also https://trac.wildfiregames.com/wiki/Coding_Conventions.

binaries/data/mods/public/simulation/components/StatisticsTracker.js
209 ↗	(On Diff #1501)	Unless I am misremembering (lack of context does this) having a StatisticsTracker without Player isn't supported.
source/main.cpp
79 ↗	(On Diff #1501)	Not very fond of needing these in here.
318 ↗	(On Diff #1501)	Why is this even needed? Should be possible to do that with a trigger script without that huge amount of C++ changes.
611 ↗	(On Diff #1501)	Since this hooks into both the Shutdown code quite a lot, why not do the actual hooking later (or at least merge this with the other autostart variants).
source/ps/Game.h
140 ↗	(On Diff #1501)	Explicit inlines are quite pointless if you are actually implementing the whole thing in the header.
source/ps/GameSetup/GameSetup.cpp
724 ↗	(On Diff #1501)	Meh, I liked the previous Shutdown, mostly because it kept things in one place only.
source/simulation2/TypeList.h
162 ↗	(On Diff #1501)	No, see comments for those files.
source/simulation2/components/ICmpStatisticsTracker.h
1 ↗	(On Diff #1501)	I see no reason for making this a C++ interface, especially given that you need a player to track statistics (and that already has a function to do what you want).

In D379#15917, @vladislavbelov wrote:

I think this diff should be committed after D144, because there're many modifications of the statistics tracker.

Do they actually conflict and if so would it be hard (a question for sacha maybe)?

They shouldn't conflict.

Also we should output not only basic statistics, but AI debug too.

The debug output is using casual warn() and other CLogger calls right? Those should be printed probably (probably are printed already?).
Turn numbers could appear then too maybe like replay does.
Perhaps only if a command line argument is given to print them too, so that they are muted by default and we only have the JSON output? Dunno, sounds like it could go in separaetly as well.

The warnings enabled by the debug level in config.js in petra are already printed if you want them. Don't know about the CLogger calls for the AI, but if there are some, they should be printed if they are enabled. The turn numbers could also be printed if you want them. And if there isn't enough debug message, I could add some, but what would be interesting to print and when do you want to enable them ?

Also there're few hardcoded things, like a turn length

Indeed, the duplication should be avoided. Can't you get the turnlength const in TurnManager.cpp? (nuking duplication even better if that works)

Yes, I should definitely use it. Didn't see there was one.

In D379#15933, @vladislavbelov wrote:

Also I don't like isGameFinished, because we already have win conditions, couldn't we use it?

With what leper suggested, I could do a trigger script for the non visual option on the OnPlayerWon event. Print the statistics and call Engine.Exit() here. And then add the script in the autostart and remove all the cpp components changes. Would this be a better solution ?

Couldn't NonVisualFrameLoop be merged with the main Frame?

Yes, it could. The simplest solution that I think of would be to update the simulation in Frame only when we are in non visual and skip most of the code.

sacha_vrand added inline comments.May 2 2017, 12:29 PM

source/ps/GameSetup/GameSetup.cpp
1513 ↗	(On Diff #1501)	ATM there are no victory conditions with the autostart option so the game doesn't end. An option to choose the victory conditions would be nice IMO. And then just load and add the scripts located in the victory condition file to the settings ?

Merged the NonVisualFrame into Frame.
Printed the turns in non visual.
Removed cpp changes used to use the StatisticsTracker component in cpp. Added a trigger instead.
Added an option to choose the victory conditions you want with the autostart, and also an option to choose the victory duration for some specific modes.
Attached IsGameFinished function to the Game class.
Move the initializations required for the non visual autostart mode into a function in GameSetup.
And modifiy some parts to respect the coding conventions. Hope I didn't forget some.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1124/ for more details.

Harbormaster completed remote builds in B1619: Diff 1822.May 10 2017, 10:39 PM

Already quite a lot nicer than that previous iteration, keep it up!

binaries/data/mods/public/gui/credits/texts/programming.json
179 ↗	(On Diff #1469)	I might be going out on a limb here, but you seem to be using sacha_vrand around here, so I'd go with that. (The purely name ones are mostly ancient contributions.)
binaries/data/mods/public/maps/scripts/NonVisualTrigger.js
7 ↗	(On Diff #1822)	`++playerId` Also this looks like something for which there could be some function in either components/PlayerManager.js or helpers/Player.js to make this a bit shorter. (I might be misremembering though).
binaries/data/mods/public/simulation/components/StatisticsTracker.js
217 ↗	(On Diff #1822)	How about `Used to print statistics for non-visual autostart games` Same information content, shorter comment.
source/main.cpp
75 ↗	(On Diff #1822)	Doesn't seem needed in here anymore.
81 ↗	(On Diff #1822)	Also doesn't seem needed anymore.
277 ↗	(On Diff #1822)	It might be pointless in the end, but having a branch in something that we will call quite a lot seems a bit bad. So how about making this take a boolean template parameter (more details where this is called)? Then the compiler would make that branching decision at compile time. Actually since the whole nonVisualFrame is about 10 lines long, having two actually different function doesn't seem bad. The calling part will still change (as hinted at above).
467 ↗	(On Diff #1822)	This can now be local, instead of global.
576 ↗	(On Diff #1822)	if (isNonVisual) while (!quit) NonVisualFrame(); // pending better name else while (!quit) Frame(); Probably use some braces, merging this with the if/else above might also be a good idea. (The now pointless idea of using a templated Frame would have done the same, just calling `Frame<true>()` or `Frame<false>()` statically, based on an if that happens only once and depends on `isNonVisual`)
source/ps/Game.cpp
465 ↗	(On Diff #1822)	Why can't this be a const function?
source/ps/GameSetup/GameSetup.cpp
1627 ↗	(On Diff #1822)	Could also just do `if (!nonVisual) InitPs(...)`, but meh.
source/ps/GameSetup/GameSetup.h
92 ↗	(On Diff #1822)	I'd move that closer to the other Init functions.

This revision now requires changes to proceed.May 12 2017, 2:44 AM

Use of a NonVisualFrame function instead of directly put in the Frame function the code for the non visual part. And other corrections requested by leper.

sacha_vrand marked 7 inline comments as done.May 12 2017, 3:22 PM

sacha_vrand added inline comments.

binaries/data/mods/public/maps/scripts/NonVisualTrigger.js
7 ↗	(On Diff #1822)	Didn't find something to help me iterate over the player IDs. The PlayerManager holds a list of player entity IDs but then I'll need a getter. And I'm not sure if it will be better.
source/main.cpp
81 ↗	(On Diff #1822)	I actually need it for the constant DEFAULT_TURN_LENGTH_SP.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1163/ for more details.

Harbormaster completed remote builds in B1669: Diff 1872.May 12 2017, 5:22 PM

Looks like I might have to test this sometime soon, unless someone beats me to it.

Also a follow up patch for the terrain thing would be great.

binaries/data/mods/public/maps/scripts/NonVisualTrigger.js
7 ↗	(On Diff #1822)	Ah, right I was thinking of that one function that returned that array (instead of a copy of it) which was removed. Still, prefix increment.
31 ↗	(On Diff #1872)	Fix this.
binaries/data/mods/public/simulation/components/StatisticsTracker.js
217 ↗	(On Diff #1822)	Without the `Note :` , also possibly move it above the `@return` @elexis opinion on this function comment?
source/main.cpp
272 ↗	(On Diff #1872)	Might be nice to keep this closer to `Frame`, so possibly move the `NonVisualFrame` in some direction.
277 ↗	(On Diff #1872)	Maybe we should use something different here, not that it really matters.
source/ps/GameSetup/GameSetup.cpp
734 ↗	(On Diff #1872)	All other occurences of this are `const`.
1531 ↗	(On Diff #1872)	Why not `push_back`?
1553 ↗	(On Diff #1872)	Now this block seems quite similiar to the other one that adds to TriggerScripts, can these be unified easily? (That is using some helper function)

This revision now requires changes to proceed.May 13 2017, 3:00 AM

Unified what is adding scripts to the TriggerScripts property in the autostart function. And other small corrections.

sacha_vrand marked 5 inline comments as done.May 15 2017, 2:50 PM

Moved the NonVisualFrame function beneath the Frame function.

sacha_vrand marked an inline comment as done.May 15 2017, 2:59 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1214/ for more details.

Harbormaster completed remote builds in B1740: Diff 1945.May 15 2017, 5:59 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1216/ for more details.

Harbormaster completed remote builds in B1742: Diff 1947.May 15 2017, 7:33 PM

elexis edited a custom field.May 16 2017, 3:52 PM

@mimo Care to test this, given that it sounds like something you might be interested in. I'm fine with the code (apart from a few comment nits), but you might have a better idea about the usability.

binaries/data/mods/public/maps/scripts/NonVisualTrigger.js
2 ↗	(On Diff #1947)	This will print the statistics at the end of a game. The rest is implied by the "end of a game" part IMO.
3 ↗	(On Diff #1947)	"changed"
30 ↗	(On Diff #1947)	I'm guessing that you should wrap these four lines in a block as done in D195.
source/main.cpp
271 ↗	(On Diff #1947)	I'd keep that one.
source/ps/GameSetup/GameSetup.h
1 ↗	(On Diff #1947)	date
source/simulation2/components/ICmpPlayer.cpp
1 ↗	(On Diff #1947)	date
source/simulation2/components/ICmpPlayer.h
1 ↗	(On Diff #1947)	date

Minor corrections requested by leper.

sacha_vrand marked 7 inline comments as done.May 22 2017, 10:47 AM

As an example of what we can do with that, here is a graph showing the win percentage of the first player on the map 'skirmishes/Acropolis Bay (2)' (doesn't seem well balanced). 250 games in conquest with Petra against Petra and a seed from 0 to 249. What do you guys think ?
Graph's link

elexis awarded a token.May 22 2017, 4:21 PM

In D379#21750, @sacha_vrand wrote:

As an example of what we can do with that, here is a graph showing the win percentage of the first player on the map 'skirmishes/Acropolis Bay (2)' (doesn't seem well balanced). 250 games in conquest with Petra against Petra and a seed from 0 to 249. What do you guys think ?
Graph's link

Interesting indeed, but to rule out any other potential problem (either from petra, simul or your own code) which would depend on the player's ordering, could you redo the same test after having modified the map xml by exchanging
<Player>1</Player> and <Player>2</Player> in the entities definition. From your plot, already with 50 games, we should have a confirmation that this is really a balance problem of the map.

Then, concerning leper's request, that's on my todo list to test this patch, hopefully in the next days.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1305/ for more details.

Harbormaster completed remote builds in B1889: Diff 2115.May 22 2017, 7:11 PM

I've tested the patch (but not looked at the code itself).
It works as announced, so looks ok to commit if the code has already been checked.

Potential improvments which would simplify its use (but for another patch):

write the summary in a log file in the current directory
add an option for multiple consecutive games (for example -num=100 would chain 100 games with the same settings but different seeds) and write all summaries in a file
add an option to impose the same civ on all players, although this civ is random (when you want to study the balance of a map, you do not want to add dispersion due to different civ)

Thanks.

Closed by commit rP19645: Add a -autostart-nonvisual option. Patch by sacha_vrand. Fixes #4577. (authored by leper). · Explain WhyMay 23 2017, 9:26 PM

This revision was automatically updated to reflect the committed changes.

elexis mentioned this in rP19645: Add a -autostart-nonvisual option. Patch by sacha_vrand. Fixes #4577..May 24 2017, 4:45 PM

This comment was removed by elexis.

vladislavbelov mentioned this in D2017: Allows replays in autostart-nonvisual mode.Jun 27 2019, 12:13 AM

Add a -autostart-nonvisual option
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 2159

ps/trunk/binaries/data/mods/public/gui/credits/texts/programming.json

ps/trunk/binaries/data/mods/public/maps/scripts/NonVisualTrigger.js

ps/trunk/binaries/data/mods/public/simulation/components/StatisticsTracker.js

ps/trunk/binaries/system/readme.txt

ps/trunk/source/main.cpp

ps/trunk/source/ps/Game.h

ps/trunk/source/ps/Game.cpp

ps/trunk/source/ps/GameSetup/GameSetup.h

ps/trunk/source/ps/GameSetup/GameSetup.cpp

ps/trunk/source/simulation2/components/ICmpPlayer.h

ps/trunk/source/simulation2/components/ICmpPlayer.cpp

Add a -autostart-nonvisual optionClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2159

ps/trunk/binaries/data/mods/public/gui/credits/texts/programming.json

ps/trunk/binaries/data/mods/public/maps/scripts/NonVisualTrigger.js

ps/trunk/binaries/data/mods/public/simulation/components/StatisticsTracker.js

ps/trunk/binaries/system/readme.txt

ps/trunk/source/main.cpp

ps/trunk/source/ps/Game.h

ps/trunk/source/ps/Game.cpp

ps/trunk/source/ps/GameSetup/GameSetup.h

ps/trunk/source/ps/GameSetup/GameSetup.cpp

ps/trunk/source/simulation2/components/ICmpPlayer.h

ps/trunk/source/simulation2/components/ICmpPlayer.cpp

Add a -autostart-nonvisual option
ClosedPublic
Actions

Revision Contents
Changeset List