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.

Sign up for GitHub

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

Jump to bottom

Add CLI option to start the game paused #2080

Merged
merged 6 commits into from
Jul 29, 2024
Merged

Add CLI option to start the game paused #2080

merged 6 commits into from
Jul 29, 2024

Conversation

xsebek
Copy link
Member

@xsebek xsebek commented Jul 28, 2024

You can test it with:

cabal run swarm -O0 -- --scenario Tutorials/move --paused

After one step (Ctrl+O) the Goal dialog will show up along with the rest of the UI.

@xsebek xsebek requested review from kostmo and byorgey July 28, 2024 18:55
@xsebek xsebek mentioned this pull request Jul 28, 2024
Closed
xsebek
xsebek commented Jul 28, 2024
View reviewed changes
app/game/Main.hs Outdated
Comment on lines 58 to 70
appOpts :: Parser AppOpts
appOpts = do
userSeed <- seed
userScenario <- scenario
scriptToRun <- run
pausedAtStart <- paused
autoPlay <- autoplay
speed <- speedFactor
cheatMode <- cheat
colorMode <- color
userWebPort <- webPort
let repoGitInfo = gitInfo
return AppOpts {..}
Copy link
Member Author

Choose a reason for hiding this comment

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

Strange, it seems GHC 9.2 and 9.4 did not understand the ApplicativeDo I wanted to use here.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe they don't deal properly with ApplicativeDo + record wildcards?

Copy link
Member

Choose a reason for hiding this comment

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

That is unfortunate. Perhaps just put it back the way it was, but add a comment (+ an issue?) to update the code once we drop support for GHC 9.4?

Copy link
Member

@byorgey byorgey Jul 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh! Try using pure instead of return, perhaps? The error message says No instance for (Monad Parser) arising from a use of ‘return’

This page seems to indicate this style has been possible for a while: https://chshersh.com/recordwildcards

Copy link
Member Author

Choose a reason for hiding this comment

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

The error message is wrong - the problem is in the line above containing let. As GHC docs note:

In particular, slight variations such as return . Just $ x or let x = e in return x would not be recognised.

Later versions of GHC seem to have gotten better at this.

byorgey
byorgey reviewed Jul 28, 2024
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

It seems a bit strange to me that if you use the -p option without --scenario, it starts the menu but then auto-pauses whatever is the first scenario you choose from the menu. I guess it makes sense though.

src/swarm-tournament/Swarm/Web/Tournament/Validate.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/View.hs Outdated Show resolved Hide resolved
app/game/Main.hs Outdated
Comment on lines 58 to 70
appOpts :: Parser AppOpts
appOpts = do
userSeed <- seed
userScenario <- scenario
scriptToRun <- run
pausedAtStart <- paused
autoPlay <- autoplay
speed <- speedFactor
cheatMode <- cheat
colorMode <- color
userWebPort <- webPort
let repoGitInfo = gitInfo
return AppOpts {..}
Copy link
Member

Choose a reason for hiding this comment

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

That is unfortunate. Perhaps just put it back the way it was, but add a comment (+ an issue?) to update the code once we drop support for GHC 9.4?

@xsebek
Copy link
Member Author

xsebek commented Jul 29, 2024

It seems a bit strange to me that if you use the -p option without --scenario, it starts the menu but then auto-pauses whatever is the first scenario you choose from the menu. I guess it makes sense though.

@byorgey, it's not just the first scenario you open; it changes the default for all scenarios to start paused. 🙂

I hope it is an understandable and useful behavior.

Could you please also have a look at the added Bool parameters? I am not entirely happy with them, but I followed compilation errors and connected the CLI to GameState. I wonder if pause even belongs in GameState, but the wrapper functions make it hard to tell where it is used.

@noahyor
Copy link
Member

noahyor commented Jul 29, 2024

I wonder if pause even belongs in GameState, but the wrapper functions make it hard to tell
where it is used.

I think it should be in the UI state in preparation for asynchronous UI (pr/issue soon).

@byorgey
Copy link
Member

byorgey commented Jul 29, 2024 •
edited
Loading

it's not just the first scenario you open; it changes the default for all scenarios to start paused

Ah, that makes sense.

I wonder if pause even belongs in GameState

I think it should be in the UI state in preparation for asynchronous UI

On the contrary, I think it does belong in GameState, and thinking about asynchronous UI shows more clearly why. To answer the question of what belongs in the GameState and what belongs in the UIState, ask yourself the question: is this something specific to this particular UI, or is it something inherent to the game which would be needed even if we put a different UI on top (web-based, GUI-based, etc.)? Especially if we want to have the game running in one thread and the UI running in another thread, then the game itself needs to keep track of whether it is currently paused, so that it can know whether to step independently of the UI telling it so. For example, the game may run for several ticks during a single frame, but if an objective is completed during one of those ticks, the game needs to immediately auto-pause without waiting for the UI to tell it that it should do so, which could come several ticks late. (Actually I am unsure whether the game currently does this correctly or not.)

byorgey
byorgey approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

I think the extra Bool parameters make sense.

@xsebek xsebek added the merge me Trigger the merge process of the Pull request. label Jul 29, 2024
@xsebek
Copy link
Member Author

xsebek commented Jul 29, 2024

Thanks, @byorgey for thinking that through - that explanation could well be a note in Temporal Game State. 👍

@byorgey byorgey mentioned this pull request Jul 29, 2024
Open
@mergify mergify bot merged commit e98660b into main Jul 29, 2024
12 checks passed
@mergify mergify bot deleted the paused2 branch July 29, 2024 20:27
@xsebek xsebek mentioned this pull request Jul 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

@kostmo kostmo Awaiting requested review from kostmo

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option to start the game paused
3 participants
@xsebek @noahyor @byorgey