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

fix: require hat select before ready up #1037

Merged

Conversation

nelson137
Copy link
Contributor

@nelson137 nelson137 commented Oct 21, 2024

Fixes #963

Changes

  • 6781542 PlayerSlot is now (sort of) a state machine, starting out Empty then progressing through SelectingPlayer and SelectingHat until becoming Ready
    • Slot for a network player only uses the SelectingHat and Ready states for simplicity
    • Angled brackets < and > show around the thing being selected (player/hat), brackets now don't show when ready
  • a2393c4 fix bug where label has incorrect text for 1 frame when pressing Menu Back to remove player from slot
  • f636d33 Upgrade bones to latest
  • Set gitattributes for *.ftl files so that they are treated as text for diffs and not binary files
  • Ignore advisory RUSTSEC-2024-0370 proc-macro-error is unmaintained (see related bones issue and the advisory)

This also fixes a bug I was seeing on main where another player in a LAN lobby pressing A/D would behave as if everyone pressed A/D, e.g. pressing D to select the next skin would change to the next skin in both slots.

Rationale for the rough state machine:

Previously there were 3 states for a player slot: Empty, SelectingPlayer, and Ready + SelectingHat. This state was maintained by two booleans, active and confirmed, which tell you whether the slot is not empty and whether the player has readied-up respectively. Together they represent the 3 states as: (active/confirmed) false/false, true/false and true/true; with false/true being an invalid state.

Now that there are four states to the ready-up process, one way of implementing this is to add a third boolean, but this introduces 3 more invalid states. Let's say the boolean is called selectedPlayer, these are the possible boolean combinations and the states they represent (active/selectedPlayer/confirmed):

false/false/false Empty
false/false/true <invalid>
false/true/false <invalid>
false/true/true <invalid>
true/false/false SelectingPlayer
true/false/true <invalid>
true/true/false SelectingHat
true/true/true Ready

The implementation I opted for is a simple state machine. While it added a lot more code to some areas of this module I found it made reasoning about some sections much easier. It also made explicit certain edge cases in the handle_match_setup_messages system where e.g the client could theoretically receive a confirmation message from a peer when it had not yet received any player selection messages. A warning log is now generated in the unlikely case this occurs.

@nelson137 nelson137 force-pushed the fix/963-select-hat-before-ready-up branch from 31cc231 to 8a0f697 Compare October 21, 2024 08:38
Gather available input sources after reacting to user inputs so that,
when rendering, we have the most up-to-date list of inputs.

For example, when there is one active slot and the rest are empty, and
the user presses Menu Back, there is a 1 frame glitch where the first
slot says "Press Comma/South" and the rest say "Press
Space/Comma/South". This is because the `player_select_panel` gathered
the available input sources before reacting to user inputs. And when the
user presses Menu Back to remove their player there is now one more
available input, causing the label to be incorrect for that one frame.
@nelson137 nelson137 force-pushed the fix/963-select-hat-before-ready-up branch 2 times, most recently from 9a8d873 to 1fa10a9 Compare October 22, 2024 04:46
Copy link
Collaborator

@MaxCWhitehead MaxCWhitehead left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! I agree that using enum seems more clear than stacking more bools 👍

@MaxCWhitehead
Copy link
Collaborator

This also fixes a bug I was seeing on main where another player in a LAN lobby pressing A/D would behave as if everyone pressed A/D, e.g. pressing D to select the next skin would change to the next skin in both slots.

Nice catch on that!

@MaxCWhitehead MaxCWhitehead added this pull request to the merge queue Oct 23, 2024
Merged via the queue into fishfolk:main with commit 429dc2a Oct 23, 2024
27 checks passed
@nelson137 nelson137 deleted the fix/963-select-hat-before-ready-up branch October 24, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Player Select: Last player readying up cannot choose hat in online play (match starts immediately)
2 participants