Skip to content

Move GameState and UIGameplay into new subrecord in AppState #2276

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

Open
kostmo opened this issue Jan 6, 2025 · 1 comment
Open

Move GameState and UIGameplay into new subrecord in AppState #2276

kostmo opened this issue Jan 6, 2025 · 1 comment
Labels
Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.

Comments

@kostmo
Copy link
Member

kostmo commented Jan 6, 2025

Currently, AppState has a dedicated field RuntimeState for scenario-independent state, and a field GameState for scenario-dependent state. However, UIState contains an awkward mix of active-play-dependent widget states and play-independent states (see #2265).

data AppState = AppState
{ _gameState :: GameState
, _uiState :: UIState
, _keyEventHandling :: KeyEventHandlingState
, _runtimeState :: RuntimeState
}

When embarking on #2265 I noticed that GameState tends to be accessed often alongside UIGameplay. I propose a new toplevel member PlayState of the AppState that shall contain both GameState and UIGameplay. Eventually, PlayState should be made nullable.

This refactoring may be accomplished in phases:

  1. Move GameState underneath a new toplevel record PlayState.
  2. Move UIGameplay out of UIState into a second field of PlayState.
  3. Wrap the PlayState field in a Maybe
  4. Make scenarioRef mandatory in PlayState (scenarioRef should be a non-optional member of UIGameplay #2265)
@kostmo kostmo added the Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality. label Jan 6, 2025
@byorgey
Copy link
Member

byorgey commented Jan 9, 2025

Sounds good to me. When we first created the AppState record it seemed like a sensible organization to put all the gameplay stuff in one record and the UI stuff in another. However, I think you're right that the better top-level organizational principle should be "stuff that persists across scenarios" and "stuff that only exists while playing a scenario".

mergify bot pushed a commit that referenced this issue Mar 30, 2025
Performs phases 1 and 2 of #2276:

> 1. Move GameState underneath a new toplevel record PlayState.
> 2. Move UIGameplay out of UIState into a second field of PlayState.

This results in many instances of an extra `playState .` lens layer.  These will be incrementally removed again in subsequent PRs (e.g. #2362) as functions are refined to narrower state scope.
mergify bot pushed a commit that referenced this issue Mar 31, 2025
Continues toward #2276.

This PR incrementally pushes the state scope to the narrower `PlayState` rather than `AppState` for many utility functions, re-simplifying the code in many cases where an extra `playState` lens layer was added in #2282.  Counting the occurrences of the `playState` lens has been a good metric for the progress on this refactoring.

For functions where this is not yet feasible, common reasons are:
* need access to the `uiState . uiMenu`
    * In particular, `toggleModal`/`openModal` need access to the `Menu` for use by `generateModal`.
        * And more specifically, for the "win", "lose", and "quit" modals, either to determine if it is `NoMenu` or to determine which scenario comes next.
* need access to `uiState . uiDebugOptions`
* need access to the `ScenarioCollection` (by way of `normalizeScenarioPath`) for:
    * `saveScenarioInfoOnFinishNocheat`
    * `saveScenarioInfoOnQuit`
* Need access to `uiState . uiAchievements` for:
    * `runGameTick`
 
This exercise is informative as to which record members we may want to re-organize.
mergify bot pushed a commit that referenced this issue Apr 5, 2025
Toward #2276.


# In this PR
* Decompose `updateUI` into smaller functions
* add documentation for `UIGameplay` and `UIState` records
* As described in #2362, the `uiState . uiMenu` is used in a number of places, often just to check whether it is set to `NoMenu`.  We instead precompute this condition and pass it as a parameter.
* Instead of retrieving from the implicit `AppState`, we pass the `ScenarioCollection` ( `runtimeState . scenarios` ) explicitly to `getNormalizedCurrentScenarioPath`.

# Long-term goal

The "main layers" of the UI can either comprise the active playfield (`drawGameUI`) or the out-of-game menu (`drawMenuUI`):

https://github.com/swarm-game/swarm/blob/68e57fc040283b0d2659a6336ed6521b20c684f5/src/swarm-tui/Swarm/TUI/View.hs#L150-L154

Since these are mutually exclusive, we should extricate the state needed for either one, rather than passing the complete `AppState` to both.

In fact there are a couple of places in the code that describe "unreachable combinations" of state:

https://github.com/swarm-game/swarm/blob/68e57fc040283b0d2659a6336ed6521b20c684f5/src/swarm-tui/Swarm/TUI/View.hs#L158-L160

and

https://github.com/swarm-game/swarm/blob/68e57fc040283b0d2659a6336ed6521b20c684f5/src/swarm-tui/Swarm/TUI/Controller.hs#L155-L158

As part of the deconflation of state, we should also be able to solve this: #1064 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.
Projects
None yet
Development

No branches or pull requests

2 participants