NAMeC-team / CRAbE

GNU General Public License v3.0
4 stars 3 forks source link

feat: add game data and change role of GameState #79

Closed Wanchai290 closed 1 year ago

Wanchai290 commented 1 year ago

Cherry-picked from commit hash 8bf21400e6f7e4f8f355761c0670e9bbe2590e9f

This changes the role of GameState to instead represent the current state as decided by the external game controller (such as when the game is halted, see the SSL rulebook, appendix B Game States).

GameData will instead hold information about ally and enemy team, color of the team on the positive half, and the current GameState.

From #75

EtienneSchmitz commented 1 year ago

For me, this PR cannot be disassociated from the PR for state machine. What are your thoughts on this matter?

Wanchai290 commented 1 year ago

Two reasons led to doing this :

in the end, the result will stay the same. As long as we do it one way or another and that the people doing it understand, if we have the result then we won't even think about it later on

Wanchai290 commented 1 year ago

additional note : the Side Filter feature (see 74e8779fcea122b5d5194383c03f8ee92d21b842 for more information) heavily depends on this. would be nice to add it before switching to GitLab

EtienneSchmitz commented 1 year ago

The commit https://github.com/NAMeC-SSL/CRAbE/commit/74e8779fcea122b5d5194383c03f8ee92d21b842 relies on the state machine, or at least on the gc pre_filter. In the main branch, the positive_half variable is not updated as gc pre_filter has not been implemented. Therefore, I propose the following steps to address this:

  1. Create a new PR titled "Rename GameState to GameData". Include the same commit but remove the state variable and the new Game State structure. We will reintroduce these with a PR for the State machine later.
  2. Create a PR that solely addresses the side filter, excluding the side changing functionality mentioned in the commit below.
  3. Split the state machine implementation into multiple PRs: at least one for the pre_filter time and another one for the post_filter and the manager.
  4. Following the merger of the pre_filter PR, create a new PR to introduce side changing functionality.

I welcome your thoughts on this proposal.

The primary reason for my reluctance towards the original PR is its introduction of the game_state structure that belongs to State Machine not on side changing or side filter.

Additionally, in consultation with @benjaminchew4, we've noticed that the commit https://github.com/NAMeC-SSL/CRAbE/commit/74e8779fcea122b5d5194383c03f8ee92d21b842 encompasses several minor elements that are not aligned with the project's requirements. It would be beneficial to split these elements into multiple PRs to address the issues and review each functionality individually. The functionalities in question include:

It would also be beneficial to incorporate tests for the side changing functionality. While it's a valuable feature, a malfunction could result in an own goal.

EtienneSchmitz commented 1 year ago

I make this PR https://github.com/NAMeC-SSL/CRAbE/pull/85 to rename game_state to game_data. I will work on a new PR for the filters part of the state machine.

Closed this PR in favor new PR