davechurchill / StarcraftAITournamentManager

Tournament Manager Software for StarCraft AI Competitions
MIT License
77 stars 43 forks source link

Bugfixes to Tournament Module permissions for BWAPI versions 4.2.0 & 4.1.2 #32

Closed chriscoxe closed 5 years ago

chriscoxe commented 5 years ago

From #31 points 7 then 2. I.E.:

31 point 7 is that there is a bug in the Tournament Module that causes it to allows all permissions for BWAPI version 4 (e.g. 4.2.0, 4.1.2, 4.0.1 Beta)!

Due to a change to a BWAPI function's argument type as part of the first release of version 4 that was not correspondingly migrated into the Tournament Module (i.e. the TournamentModule::onAction function's first argument type was changed from an into to a BWAPI::Tournament::ActionID), permissions like enableFlag(CompleteMapInformation), setLocalSpeed(42) are not enforced by TournamentModule.dll, because the default implementation of the function that is defined in BWAPI is being used, which always returns true. This is why the Sling bot was able to successfully do setLocalSpeed(42) and setFrameSkip(0) until Sling's source code was patched, and the Tyr/TyrProtoss bot was able to successfully do setLocalSpeed(10) in the AIIDE and CIG competitions, which slowed down the runs. If any bot that uses BWAPI 4 had cheated by doing things like enableFlag(CompleteMapInformation), the tournament module wouldn't have stopped them. Note that this is true for non-DLL bots and for DLL bots (originally I thought it only affected non-DLL bots, but after investigating and finding the cause of the bug I realized it affects DLL bots too).

31 point 2 should be addressed after point 7 is fixed. I.E. ensure the Tournament Module allows some permissions for some versions of BWAPI.

After the bug in point 7 is fixed, rebuild the TournamentModule.dll's (for BWAPI 4.2.0 and 4.1.2 at least - I wouldn't bother fixing 4.0.1 Beta, and I wouldn't bother changing 3.7.4 which doesn't allow any permissions) to allow bots to do setLatCom(false) and setCommandOptimizationLevel(a value other than 1, i.e. especially 0 or 2, but perhaps some other authors might want 3 or 4). They don't affect the opponent bot at all - it only affects what info BWAPI returns to you, and some bots rely on them. The default should probably stay as 1, but I think bots should be allowed to set it to 0 and other values if they want to, even though a value of 0 may increase the bot's APM and replay file sizes, because some bots rely on them and currently there is no way for a bot to check what the command optimization level is. And anyway, currently, BWAPI version 4.2.0/4.1.2/4.0.1 Beta bots have always done this until now, due to the bug, so I don't think it's a problem. I suggest to also allow leaveGame(), so that bots can leave the game to indicate they are resigning/surrendering in lost positions, or in bugged states to attempt to avoid crashing later (and this may help address bwapi/bwapi#719 ). This might also help increase the overall game rate, so long as Starcraft or bots don't/rarely crash when leaving, because bot-vs-bot endgames can often take a lot of time if the winning bot is weak/slow. Personally, I don't care whether sendText/printf/setTextSize are allowed, so I would probably disallow them on anti-collusion grounds and in case they help to increase performance, but I think the others should be disallowed.

chriscoxe commented 5 years ago

I've fixed it for both points, tested the new TournamentModule.dll files on my Win7 SP1 VMs (I modified the latest version & an old version of my bot to try using all the various permissions for the bugged TournamentModule.dll files for BWAPI 4.2.0 and 4.1.2 and confirmed that all permissions are allowed, then tested against the fixed TournamentModule.dll files for BWAPI 4.2.0 and 4.1.2 and verified the resulting behavior is as expected), and committed and pushed all the changes to the ladder branch of my fork, but only for BWAPI versions 4.2.0 and 4.1.2. I have not fixed it for BWAPI version 4.0.1 Beta (nor changed it for BWAPI version 3.7.4) because I don't think it is worth bothering and because I don't have the old compiler versions nor builds of old BWAPI versions set up (which would be a lot of hassle).

chriscoxe commented 5 years ago

Note: BASIL's Tournament Module (https://github.com/Bytekeeper/sc-tm) allows and disallows the same permissions as my new version except that BASIL also allows sendText/printf/setTextSize and has a config setting for the UserInput flag that defaults to allowing it. StarcraftAITournamentManager disallows them for BWAPI 3, so to avoid rocking the boat, I decided not to allow them in my changes.

I know SSCAIT allows leaveGame, sendText, and printf at least, and I gather it also allows setLatCom(false). I don't know whether it allows setCommandOptimizationLevel(0 or other values). Apart from that, I have no idea what SSCAIT allows and disallows because I don't think the source code is available (or if it is, at least not the up-to-date source code). I also have no idea what the defaults are for things like isLatComEnabled(), or what the command optimization level is initially.

richard-kelly commented 5 years ago

Fix from @chriscoxe merged into ladder branch.

chriscoxe commented 5 years ago

Re. 4.0.1 Beta, the AIIDE Rules page and the CIG Rules page don't mention 4.0.1 Beta, so no worries.