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

Phaser refactor #1474

Closed
wants to merge 5 commits into from
Closed

Phaser refactor #1474

wants to merge 5 commits into from

Conversation

rr-
Copy link
Collaborator

@rr- rr- commented Sep 1, 2024

Checklist

  • I have read the coding conventions
  • I have added a changelog entry about what my pull request accomplishes, or it is an internal change

Description

  • Removes .param = 0 from GAMEFLOW_COMMAND assignments, as it's not used by the consumers and is okay to be left uninitialized.

  • Renames GF_PHASE_CONTINUE to GF_CONTINUE_SEQUENCE, as the way it was used was never about the phases but rather referred to letting the game carry on executing current game flow sequence.

  • Removes GF_PHASE_BREAK enum member in favor of a new struct, PHASE_CONTROL as explained in the next point.

  • Tidies up the nomenclature:

    • Phaser control routine result - status whether to continue the current phase, along with an optional game flow command to run afterwards. This is the PHASE_CONTROL struct.
    • Game flow action - the global action to perform, such as exit the game, play a level etc. (This prompted a rename of GAMEFLOW_COMMAND.command member to .action.)
    • Game flow command - a game flow action with an optional parameter.
    • Game flow sequence - a sequence of steps associated with playing a single level. This can be adding items to inventory, playing a cutscene, showing the level stats screen etc.

rr- added 4 commits September 1, 2024 19:37
In case of the affected game flow commands, the parameter is never used,
so it's okay to leave it uninitialized for readability.
The way it was used was never about the phases but rather referred to
letting the game carry on executing current game flow sequence.
@rr- rr- added the Internal The invisible stuff label Sep 1, 2024
@rr- rr- added this to the 4.4 milestone Sep 1, 2024
@rr- rr- self-assigned this Sep 1, 2024
@rr- rr- requested review from a team as code owners September 1, 2024 17:45
@rr- rr- requested review from lahm86, walkawayy and aredfan and removed request for a team September 1, 2024 17:45
@rr-
Copy link
Collaborator Author

rr- commented Sep 1, 2024

Note to reviewers – should be an easy no-op change.

  • GF_PHASE_CONTINUE got changed to (PHASE_CONTROL){.end = false} within phasers
  • GF_PHASE_BREAK got changed to (PHASE_CONTROL){.end = true, .command = {.action = GF_CONTINUE_SEQUENCE}} within phasers
  • And most importantly, we got rid of the
        if (command.action == GF_PHASE_BREAK) {
            command.action = GF_CONTINUE_SEQUENCE;
        }
    semi-hack in Phase_Run which mixed the phaser flow with the game flow.

TBH I'm not sure about the last commit, since the same conclusion that led to making it, can now be drawn about the GF_CONTINUE_SEQUENCE which does the same kind of mixing in GameFlow_InterpretSequence (game flow commands and game sequences) as GF_PHASE_BREAK used to in Phase_Control. I do have a feeling that the proposed design is cleaner, though.

Removes `GF_PHASE_BREAK` enum member in favor of a new struct,
`PHASE_CONTROL` that controls phasers alone.
Copy link
Collaborator

@walkawayy walkawayy left a comment

Choose a reason for hiding this comment

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

LGTM. Didn't notice any issues while testing.

@rr-
Copy link
Collaborator Author

rr- commented Sep 2, 2024

Merged manually: f4dd3fb

@rr- rr- closed this Sep 2, 2024
@rr- rr- deleted the phaser-refactor branch September 2, 2024 07:10
@rr- rr- added the TR1 label Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal The invisible stuff TR1
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants