Feature: Add Max Players Game Setting and Fix Join Logic #478

roberthsheng · 2025-04-16T05:20:37Z

Thanks for making shengji on web - I've spent countless hours on this site :-)

Closes #424

Summary:

This pull request implements a new game setting that allows the host to set a maximum player limit for a game room. It also addresses several bugs related to the player joining process, particularly when a game lobby is full.

Changes:

Max Players Feature:
- Introduced a max_players option within the game's configuration (core/src/settings.rs).
- Added backend support (actions, messages) for setting and propagating this limit.
- Integrated a UI element in the game settings screen (frontend/src/Initialize.tsx) for hosts to adjust the limit.
- Updated the public games list (frontend/src/PublicRoomsPane.tsx) to display both the current number of players and the maximum limit, if set (e.g., "5/8"). The backend API (backend/src/state_dump.rs) was updated accordingly.
Limit Enforcement:
- Modified the backend logic (add_player, make_player in core/src/settings.rs) to prevent players or observers from joining if the max_players limit has been reached.
Bug Fixes:
- Incorrect Error on Full Game Join: Resolved an issue where trying to join a game at maximum capacity incorrectly resulted in a generic connection error message after a delay. The fix ensures the specific "Maximum number of players reached" error is displayed promptly to the user. This involved adjustments to backend error handling and connection management (backend/src/shengji_handler.rs, backend/src/utils.rs).
- Unexpected Spectator Join: Corrected a bug where a user who failed to join a full game might later be automatically added as a spectator if the game's capacity was increased. The solution involves ensuring game state updates are targeted correctly after actions and refining frontend state management (backend/src/utils.rs, frontend/src/websocketHandler.ts).
- Join Button State Management: Addressed an issue where the "Join" button could remain disabled after a failed join attempt, even after the user refreshed the public games list. The button state is now correctly updated when the user indicates intent to retry (frontend/src/PublicRoomsPane.tsx, frontend/src/JoinRoom.tsx).
Code Maintenance:
- Addressed compiler warnings related to Redis query return types in storage/src/redis_storage.rs by specifying explicit types, improving future compatibility.

Testing:

Manual testing was performed throughout development to verify:

The max players setting can be configured and is displayed correctly.
Joining a full game displays the correct error and prevents entry.
Joining a game after the limit is increased works as expected.
The join button state updates correctly after failed attempts and refreshes.
The automatic spectator join issue is resolved.

…ry_async

rbtying · 2025-04-16T06:07:48Z

@roberthsheng wow, this is a cool set of changes. Thank you for working on it!

Out of curiosity, are public rooms actually being used / hitting max capacity?

I'll take a moment to properly review later, but if you get the linters to pass I can throw up the build on shengji-beta so you can playtest for a bit?

rbtying · 2025-04-16T06:08:53Z

backend/src/shengji_handler.rs

-        let logger_ = logger_.new(o!("player_id" => player_id.0));
-        debug!(logger_, "Received player ID");
+    if let Ok(player_id_option) = subscribe_player_id_rx.await {
+        let logger_ = logger_.new(o!("player_id" => format!("{:?}", player_id_option.map(|p|p.0))));


I think you can just use dbg! here?

rbtying · 2025-04-16T06:09:31Z

backend/src/shengji_handler.rs

-                        .map(|state| GameMessage::State { state })
-                } else {
-                    Some(v)
+                // Only filter state if we have a valid player ID


should the player stay connected if they didn't get a player ID? that means that the room is at capacity, right? can we just close the connection and ask the player to reconnect in the frontend?

The way I understand it we close the connection fairly quickly now when registration fails. The flow goes
1. register_user returns an Err when the room is full
2. handle_user_connected catches this Err
3. Instead of returning Ok(()), we have handle_user_connected propagate the Err
4. The top-level entrypoint function receives this Err, logs it, and the task for that specific connection terminates

I think the player_subscribe_task might stay alive for a very short time after the error is sent by execute_operation until the oneshot channel signals failure (with None), but the main handler that would process further user messages (run_game_for_player) is never started, and the overall connection context is torn down due to the error in entrypoint.

and so in doing so the "game full" error message is reliably sent before the connection closes completely, which helped me avoid an issue where the connection closure raced the error message delivery. This way, I think explicit frontend reconnect prompts should no longer be necessary - lmk your thoughts on this

Ah, got it. I understand what you're trying to do now

rbtying · 2025-04-16T06:10:32Z

backend/src/shengji_handler.rs

        }
        UserMessage::Action(action) => {
-            execute_operation(
+            let action_clone_for_log = action.clone();


nit: I usually just call these action_ and logger_

rbtying · 2025-04-16T06:11:41Z

backend/src/shengji_handler.rs

 ) {
-    execute_operation(
+    let room_name = room.as_bytes().to_vec();
+    let room_name_str = String::from_utf8_lossy(&room_name);


hm, what's going on here? room is a String so it should be UTF-8 already right?

It looks like on 458 we just need an &str, so perhaps revert this bit

Yes my bad! Reverting

rbtying · 2025-04-16T06:13:31Z

backend/src/utils.rs

+        Ok(new_version) => {
+            match backend_storage.clone().get(room_name_.clone()).await {
+                Ok(updated_versioned_game) => {
+                    if updated_versioned_game.monotonic_id == new_version {


my brain isn't awake enough to think about whether this part is correct but curious why this needed to change?

Calling execute_operation_with_messages runs the game logic and updates the state atomically in storage-> returns new version number if successful

After return Ok new version, we do a separate backend_storage.get(...) to get the newly updated state

The if updated_versioned_game.monotonic_id == new_version check verifies that the state we fetched from the get is actually the state corresponding to new_version that was just successfully written.

I just added this as a defense against possible (though unlikely I think with current storage implementations) race conditions or eventual consistency issues where the get might momentarily return a slightly older state immediately after the atomic update completes, just to make sure we reflect the immediate result of initiating client action

Erring to the side of safety if this is hand wave-y I can change it back around

rbtying · 2025-04-16T06:14:28Z

storage/src/redis_storage.rs

                .arg("states_created")
                .ignore()
-                .query_async(connection_manager)
+                .query_async::<_, ()>(connection_manager)


thanks! though tbh prod is still using a hashmap, it works fine hahaha

roberthsheng · 2025-04-18T12:04:40Z

re: are public rooms actually being used / hitting max capacity?

definitely were being used a lot more a few years back. the main way my friends and I use the feature now is actually so we don't have to send the link to each other and just join the game by clicking from the home page. im hoping shengji spikes back up in popularity and a max player cap would actually be put to good use one day :')

rbtying

One larger comment on the eager read after write - I don't think it's necessary as a result of your other changes. But let me see if I can get this to a point where it's on -beta!

rbtying · 2025-04-19T05:37:02Z

backend/src/shengji_handler.rs

-                        .map(|state| GameMessage::State { state })
-                } else {
-                    Some(v)
+                // Only filter state if we have a valid player ID


Ah, got it. I understand what you're trying to do now

rbtying · 2025-04-19T05:37:32Z

backend/src/shengji_handler.rs

-            .await;
+            .await
+            {
+                error!(logger, "Beep operation failed"; "error" => format!("{:?}", e));


cargo clippy will probably ask you to dbg this one too

rbtying · 2025-04-19T05:43:52Z

backend/src/utils.rs

+                        .await;
+                    Ok(())
+                } else {
+                    let err_msg = format!("Operation succeeded but version mismatch after fetching state (expected {}, got {})", new_version, updated_versioned_game.monotonic_id);


So, I think it's a correct/valid state as part of the protocol for a new version to come back with a higher monotonic ID than the one we tried to write - that means that we raced with another change, but the current state is still overall correct. I'm not sure that we should error in such a case.

It's also I think ~fine for the state in this process to fall out of date because any writes require the latest monotonic ID - do we need the optimistic read?

rbtying · 2025-04-19T06:39:20Z

@roberthsheng it looks like you made the "mistake" of upgrading my ancient frontend deps - I'm trying to split that out here #479

roberthsheng added 6 commits April 12, 2025 21:13

limiting max players

a51eb5e

reversion of tractor requirements message

47c51e7

error message when game is full correctly showing

cadb2ba

show max players in lobby list

c44f0dd

join logic when room is full -> not full fixed

a690cd5

accidentally did not push all + fixing compiler warnings in redis que…

be71083

…ry_async

rbtying reviewed Apr 16, 2025

View reviewed changes

max player fixes

67ea685

prettier

6d87af3

rbtying reviewed Apr 19, 2025

View reviewed changes

Feature: Add Max Players Game Setting and Fix Join Logic #478

Are you sure you want to change the base?

Feature: Add Max Players Game Setting and Fix Join Logic #478

Uh oh!

Conversation

roberthsheng commented Apr 16, 2025

Uh oh!

rbtying commented Apr 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberthsheng commented Apr 18, 2025

Uh oh!

rbtying left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbtying commented Apr 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants