Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quickstart AI Battle #1565

Merged
merged 19 commits into from Mar 30, 2023
Merged

Conversation

wichern
Copy link
Contributor

@wichern wichern commented Mar 18, 2023

This change allows to quick-start into an AI-only game.

Example:
./bin/s25client -m share/s25rttr/RTTR/MAPS/NEW/Bagel5.SWD --ai aijh --ai aijh --ai dummy

Will result in
Screenshot from 2023-03-18 22-15-57

Implementation:
This change adds a new program option (--ai) and extends QuickStartGame().
In order to have the requested AIs ready in dskGameLobby, the AI configuration is stored in GameClient.
In GameClient::GameLoaded() we call ToggleHumanAIPlayer() to turn the human player into an AI as well.

Notes:
There are no further limitations. The user can still add buildings and roads.

@wichern wichern marked this pull request as ready for review March 18, 2023 22:24
Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, well done overall, just a few bits for polishing 👍

libs/s25client/s25client.cpp Outdated Show resolved Hide resolved
libs/s25client/s25client.cpp Outdated Show resolved Hide resolved
libs/s25main/QuickStartGame.cpp Outdated Show resolved Hide resolved
libs/s25main/QuickStartGame.cpp Outdated Show resolved Hide resolved
libs/s25main/QuickStartGame.cpp Outdated Show resolved Hide resolved
libs/s25main/network/GameClient.cpp Outdated Show resolved Hide resolved
libs/s25main/network/GameClient.cpp Outdated Show resolved Hide resolved
libs/s25main/network/GameClient.h Outdated Show resolved Hide resolved
libs/s25main/network/GameClient.h Outdated Show resolved Hide resolved
libs/s25main/network/GameClient.h Outdated Show resolved Hide resolved
@wichern
Copy link
Contributor Author

wichern commented Mar 22, 2023

@Flamefire
Thank you very much for your review!
I tried to fix all your findings.

Tomorrow, I'll check if it is possible to save and load an ai battle.

@wichern
Copy link
Contributor Author

wichern commented Mar 23, 2023

Starting an AI-Battle from a savegame seems to work.

The provided AI players (--ai) have no effect in that case, except when none is given at all. In the latter case, player 0 will be played by the human player.

Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far, just a few minor improvements, especially making stuff const correct/const-where-possible,
Just a remark: Short-ish single-line statements after if/for/... don't need braces. Indentation (enforced via clang-format) already take care of potential bugs. No need to remove them as they don't hurt, just might save you some work.

There is still a bug though now that I'm thinking about it: aiBattlePlayers_ is never reset, this means after the first such quickstart every game will be considered an AI battle. I guess this reset (clear) can be best done in GameClient::Stop

libs/s25main/QuickStartGame.cpp Outdated Show resolved Hide resolved
libs/s25main/QuickStartGame.cpp Outdated Show resolved Hide resolved
libs/s25main/QuickStartGame.cpp Outdated Show resolved Hide resolved
libs/s25main/desktops/dskGameLobby.cpp Outdated Show resolved Hide resolved
libs/s25main/network/GameClient.h Outdated Show resolved Hide resolved
libs/s25main/desktops/dskGameLobby.cpp Outdated Show resolved Hide resolved
libs/s25main/desktops/dskGameLobby.cpp Outdated Show resolved Hide resolved
libs/s25main/desktops/dskGameLobby.cpp Outdated Show resolved Hide resolved
@wichern
Copy link
Contributor Author

wichern commented Mar 24, 2023

I did not see an existing UT for QuickStartGame(). How should I handle missing code coverage? Would you like me to create a UT for it?

@Flamefire
Copy link
Member

I did not see an existing UT for QuickStartGame(). How should I handle missing code coverage? Would you like me to create a UT for it?

As that method is not critical I'm OK with missing coverage for that, as we have the review at least. If you can come up with a test then feel free. However I imagine that this would be very complicated due to the client, server, windowmanager and desktop being involved (so it's rather an integration test). Hence the idea to factor out the AI name parsing to have at least that tested, and skip the rest which "should work" after inspection ;-)

Flamefire
Flamefire previously approved these changes Mar 25, 2023
libs/s25main/desktops/dskGameLobby.cpp Outdated Show resolved Hide resolved
libs/s25main/network/GameClient.h Outdated Show resolved Hide resolved
@Flamefire Flamefire merged commit 2982d66 into Return-To-The-Roots:master Mar 30, 2023
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants