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

Join queue despaghettification #2006

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

SpaceMonkeyy86
Copy link
Contributor

@SpaceMonkeyy86 SpaceMonkeyy86 commented Mar 18, 2023

  • The join queue logic is now centralized in a single async method and no longer relies on fallible callbacks
  • It is now impossible for the queue to get stuck even if a player crashes their game while connecting
  • Players enter the queue once the base game has finished loading to reduce unnecessary delays

@Sunrunner37 Sunrunner37 marked this pull request as draft March 18, 2023 19:11
@SpaceMonkeyy86 SpaceMonkeyy86 marked this pull request as ready for review March 22, 2023 20:57
@dartasen dartasen requested review from killzoms, Sunrunner37 and tornac1234 and removed request for killzoms and Sunrunner37 April 3, 2023 09:00
Copy link
Collaborator

@tornac1234 tornac1234 left a comment

Choose a reason for hiding this comment

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

Probably some things I've not seen yet and didn't test that many figure cases.
One issue I've found is PlayerDisconnected() doesn't trigger a queue continuation. If you join with 2 players, and when both are in the waiting queue and you suddenly Atl+F4 one of the clients, the server will be put in pause state without resuming for the other one (it waited for 5 minutes tho but that's a "worst case" situation while we would have just been able to resume the queue once we know that the loading player was disconnected).

Also there's no way to tell that you're waiting once you're in the loading. You should get informations with like the number of player in front of you in the queue and maybe a maximum wait time calculated by number of players in queue * InitialSyncTimeout, we should definitely add some more visual elements for the player instead of locking him in there without any information. I think we discussed this once, I proposed that we would have some sort of UI on top of the screen showing the informations we want (those proposed above and the timing passed waiting also) and a way to leave the queue and head back to the main menu.

I would not want this merged without at least some information given to the player, and then we'd set the UI I proposed for a later PR.

NitroxClient/MonoBehaviours/Multiplayer.cs Outdated Show resolved Hide resolved
NitroxServer/GameLogic/PlayerManager.cs Outdated Show resolved Hide resolved
@SpaceMonkeyy86
Copy link
Contributor Author

I agree, this PR needs a dedicated UI screen to be production-ready. I think we should go ahead and add it now because it's bad practice to merge a PR that says "Finish me later with another PR."

@dartasen dartasen marked this pull request as draft May 6, 2023 10:08
@tornac1234
Copy link
Collaborator

Would like to hear about where this is going. Should we try to restrict the scope of this PR (e.g. postpone early loading which needs more work as said above) to at least have the fixes merged ?

@SpaceMonkeyy86
Copy link
Contributor Author

I think so, adding new menus sounds more like a beta feature. We do need to add at least some kind of additional UI to the loading screen, however.

@tornac1234
Copy link
Collaborator

How's this going ?

@tornac1234 tornac1234 added the Area: netcode Related to packet serialization and networking algorithms label Dec 23, 2023
@SpaceMonkeyy86
Copy link
Contributor Author

I took a very long break from this project because I was busy and my interests had changed, so I haven't been keeping up with things very closely. Actually, I was thinking of getting back into Nitrox, and this is a good time to do it. This PR is functional (last time I checked) but to be "polished" it still needs some sort of UI on the loading screen to show progress, or just to modify the vanilla progress bar. That would be better suited for a separate PR. Other than that, it should work (I just need to fix the conflicts).

@SpaceMonkeyy86 SpaceMonkeyy86 marked this pull request as ready for review January 2, 2024 01:16
@SpaceMonkeyy86
Copy link
Contributor Author

SpaceMonkeyy86 commented Jan 2, 2024

It still works!

@Jannify
Copy link
Member

Jannify commented Jan 3, 2024

Would this make sense to be merge before or after #2023 ?

@SpaceMonkeyy86
Copy link
Contributor Author

I don't think it matters. They shouldn't break each other, right?

@SpaceMonkeyy86
Copy link
Contributor Author

The PR is ready for review now. Just one thing - currently, if the client is in the queue, the loading screen disappears and they can play as normal. However, when they start syncing it reappears and anything they did is undone. This is unintentional, but I left it in for now since it's nice to be able to jump/swim around while waiting instead of just looking at a static image on a loading screen. Should we keep this in? Maybe add something to the queue enter message to let the player know what is happening?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: netcode Related to packet serialization and networking algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants