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: unable to quit application with application exit or in-game exit button [MTT-7003] #863

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

fernando-cortez
Copy link
Collaborator

@fernando-cortez fernando-cortez commented Aug 21, 2023

Description

This PR fixes the issue of not being able to quit the application after a game session.

This was due to not unsubscribing to the Application.wantsToQuit callback when attempting to quit. The callback would fire on a client when quitting, and in the attempt of quitting, would invoke m_LobbyServiceFacade.EndTracking(), that call would return an error, the application would invoke Application.Quit, and the loop would continue, stuck in a loop.

This PR simply unsubscribes to the Application.wantsToQuit callback when attempting to quit, invoking the cleanup coroutine only once.

Issue Number(s)

MTT-7003

How To Test

  1. Host with one built executable
  2. Join with another executable
  3. Proceed in-game after Character Selection
  4. Have host end session through in-game buttons
  5. As the client now in the Main Menu scene, quit the application via the OS-level quit button (or in-game button)
  6. See that the client is able to quit the application properly.

Contribution checklist

  • [ N/A ] Tests have been added for boss room and/or utilities pack
  • Release notes have been added to the project changelog file and/or package changelog file
  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • JIRA ticket ID is in the PR title or at least one commit message
  • Include the ticket ID number within the body message of the PR to create a hyperlink
  • [ N/A ] An Index entry has been added in readme.md if applicable

Open Question

This is more of a design question, but I'm noticing that on certain async Lobby requests, namely ones for deletion and leaving, the internal Lobby reference isn't cleared if the operation fails. This can happen if a client has been removed from a Lobby -> they try to leave that Lobby -> the Lobby doesn't exist -> async operation fails -> ResetLobby() is not invoked because of this.

So then the client is technically not part of a Lobby, but LobbyServiceFacade.CurrentUnityLobby is still not null. This can result in various attempts to leave a lobby when the application is quit, since LobbyServiceFacade.CurrentUnityLobby is not cleared.

I propose to refactor LobbyServiceFacade.LeaveLobbyAsync from:

 public async void LeaveLobbyAsync()
{
     string uasId = AuthenticationService.Instance.PlayerId;
      try
      {
          await m_LobbyApiInterface.RemovePlayerFromLobby(uasId, m_LocalLobby.LobbyID);
          ResetLobby();
      }
      catch (LobbyServiceException e)
      {
          // If Lobby is not found and if we are not the host, it has already been deleted. No need to publish the error here.
          if (e.Reason != LobbyExceptionReason.LobbyNotFound && !m_LocalUser.IsHost)
          {
              PublishError(e);
          }
      }
} 

to something like:

public async void LeaveLobbyAsync()
{
      string uasId = AuthenticationService.Instance.PlayerId;
      try
      {
          await m_LobbyApiInterface.RemovePlayerFromLobby(uasId, m_LocalLobby.LobbyID);
      }
      catch (LobbyServiceException e)
      {
          // If Lobby is not found and if we are not the host, it has already been deleted. No need to publish the error here.
          if (e.Reason != LobbyExceptionReason.LobbyNotFound && !m_LocalUser.IsHost)
          {
              PublishError(e);
          }
      }
      finally
      {
          ResetLobby();
      }
}

so that the Lobby is cleared regardless of async operation result.

Let me know your thoughts on this proposal. It would similarly be applied to the deletion async operation.

@fernando-cortez fernando-cortez added 2-Easy This PR is trivial and can be reviewed quickly 1-Needs Review PR needs attention from the assignee and reviewers labels Aug 21, 2023
@fernando-cortez fernando-cortez requested a review from a team as a code owner August 21, 2023 17:53
RikuTheFuffs
RikuTheFuffs previously approved these changes Aug 21, 2023
Copy link

@RikuTheFuffs RikuTheFuffs left a comment

Choose a reason for hiding this comment

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

Simple & Clean

@RikuTheFuffs
Copy link

On the open question: the refactoring looks good too. Based on the snippet it doesn't even seem like the current implementation uses the results, it just awaits for the call.

LPLafontaineB
LPLafontaineB previously approved these changes Aug 22, 2023
Copy link
Contributor

@LPLafontaineB LPLafontaineB left a comment

Choose a reason for hiding this comment

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

Approved! And nice catch for the lobby reset! I'll include that change in PR #860

LPLafontaineB
LPLafontaineB previously approved these changes Aug 31, 2023
@fernando-cortez fernando-cortez merged commit 910d6c2 into develop Aug 31, 2023
@fernando-cortez fernando-cortez deleted the fix/application-quit-rate-limit branch August 31, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-Needs Review PR needs attention from the assignee and reviewers 2-Easy This PR is trivial and can be reviewed quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants