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

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

Closed
MaxCWhitehead opened this issue Apr 6, 2024 · 2 comments · Fixed by #1037
Assignees
Labels
kind:bug Something isn't working

Comments

@MaxCWhitehead
Copy link
Collaborator

Description

In online play, it seems hat is selected after readying up, if the last player hits ready, the lobby transitions to map select before player has opportunity to select hat.

In local play, this is not an issue.

To Reproduce

No response

Expected Behavior

Should move hat select before ready up?

Additional Context

No response

Log Messages

No response

@MaxCWhitehead MaxCWhitehead added the kind:bug Something isn't working label Apr 6, 2024
@nelson137
Copy link
Contributor

I agree that hat selection should be a step before being ready. I'm going to start work on this if there's no concern about this approach.

@MaxCWhitehead
Copy link
Collaborator Author

@nelson137 go for it, thanks!

github-merge-queue bot pushed a commit that referenced this issue Oct 23, 2024
Fixes #963

## Changes

-
[6781542](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](a2393c4)
fix bug where label has incorrect text for 1 frame when pressing Menu
Back to remove player from slot
-
[f636d33](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](fishfolk/bones#479)
and [the advisory](https://rustsec.org/advisories/RUSTSEC-2024-0370))

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants