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

Group related fields of GameState into child records #872

Closed
kostmo opened this issue Nov 20, 2022 · 5 comments
Closed

Group related fields of GameState into child records #872

kostmo opened this issue Nov 20, 2022 · 5 comments
Assignees
@kostmo
Labels
C-Low Hanging Fruit Ideal issue for new contributors. G-Game State An issue having to do with the game state record. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Developer Experience This issue seeks to make life easier for developers writing Scenarios or other Swarm code. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.

Comments

@kostmo
Copy link
Member

kostmo commented Nov 20, 2022

GameState is a very "wide" record. Currently it has 34 fields. Several of those fields are closely related to each other, but unrelated to the remaining fields.

E.g.:

  , _recipesOut :: IntMap [Recipe Entity]
  , _recipesIn :: IntMap [Recipe Entity]
  , _recipesReq :: IntMap [Recipe Entity]

and

  , _seed :: Seed
  , _randGen :: StdGen

and

  , _viewCenterRule :: ViewCenterRule
  , _viewCenter :: V2 Int64

and

  , _messageQueue :: Seq LogEntry
  , _lastSeenMessageTime :: Integer

I propose that we create new records for each of the groupings above (and perhaps more). Each of new records will become consolidated fields of GameState.
@kostmo kostmo added Z-Feature A new feature to be added to the game. Z-Developer Experience This issue seeks to make life easier for developers writing Scenarios or other Swarm code. labels Nov 20, 2022
@xsebek
Copy link
Member

xsebek commented Nov 20, 2022

I don’t think we need seed in game state.

AFAIK its used to initialise the generator and to show to the player in UI. We could realistically save it in one of our other records, namely it is already present in the scenario.

@xsebek
Copy link
Member

xsebek commented Nov 20, 2022

Message queue needs a small rewrite to create a more efficient Message panel. I would suggest leaving that as is for now and maybe adding a TODO note.

I completely agree with refactoring recipes and in general making the game state smaller. 👍

@xsebek xsebek added Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality. C-Low Hanging Fruit Ideal issue for new contributors. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. G-Game State An issue having to do with the game state record. and removed Z-Feature A new feature to be added to the game. labels Nov 20, 2022
@kostmo kostmo self-assigned this Dec 18, 2022
@xsebek
Copy link
Member

xsebek commented Feb 11, 2023

Related Issue:

@kostmo kostmo mentioned this issue Sep 10, 2023
Merged
mergify bot pushed a commit that referenced this issue Sep 11, 2023
@kostmo
Decompose GameState into sub-records (#1510)
bb31126 
Towards #872

Previously, the `GameState` record had `41` toplevel members.  It now has `22`.   Logical grouping of the fields makes it easier to peruse the code and paves the way to split `State.hs` into smaller modules.  Some functions now may even be able to operate exclusively on subsets of the game state, rather than having to pass in `GameState` as a whole.

There is potential to go even farther, by extracting view-related and robot-related members to their own records, but I figured I'd pursue this refactoring incrementally.
@byorgey
Copy link
Member

byorgey commented Dec 6, 2023

@kostmo Is there still more decomposition of GameState you want/intend to do? Or can we close this now?

@kostmo
Copy link
Member Author

kostmo commented Dec 6, 2023

Is there still more decomposition of GameState you want/intend to do?

I suppose the option to for refactor even further will remain open, but for now we can call this issue "mission accomplished".

@kostmo kostmo closed this as completed Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kostmo kostmo

Labels
C-Low Hanging Fruit Ideal issue for new contributors. G-Game State An issue having to do with the game state record. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Developer Experience This issue seeks to make life easier for developers writing Scenarios or other Swarm code. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@kostmo @byorgey @xsebek