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

MBL-19 Remove some hard coded logics in ADeploymentsPlayerController. #94

Merged
merged 17 commits into from
Feb 19, 2020

Conversation

raymonwang
Copy link
Contributor

@raymonwang raymonwang commented Feb 3, 2020

Remove hard coded locator url in ADeploymentsPlayerController.
Remove hard coded dev auth token in ADeploymentsPlayerController.
Remove some codes which use worker sdk interface directly.
Reuse some functions from USpatialWorkerConnection class.
Dev auth token could be put into command line parameters like -devAuthToken xxxx .

@improbable-prow-robot
Copy link

Corresponding JIRA ticket: https://improbableio.atlassian.net/browse/MBL-19

@improbable-prow-robot improbable-prow-robot added the size/M Denotes a PR that changes 40-149 lines, ignoring generated files. label Feb 3, 2020
Copy link
Contributor

@jessicafalk jessicafalk left a comment

Choose a reason for hiding this comment

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

added comments, also adding two more people from the GDK team as reviewers.
Please make sure to always add people from the GDK team to your PRs :)

Copy link
Contributor

@jessicafalk jessicafalk left a comment

Choose a reason for hiding this comment

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

One thing that I just noticed is that we have removed the usage of the timer completely. This means afaics that we will never check whether any deployments got updated. This was important for 2 reasons:

  1. A deployment is maybe not ready yet or has already stopped by the time the player queried the deployments. By requesting to get all available deployments every few seconds, we would ensure that the client would get an up-to-date view
  2. The PlayerIdentityToken expires after a while. We receive this as the first thing when we query the deployments and then need to wait until the player makes a choice. there is a small chance that the PIT expires in the meantime and the user is not able to connect to the chosen deployment.

@raymonwang
Copy link
Contributor Author

One thing that I just noticed is that we have removed the usage of the timer completely. This means afaics that we will never check whether any deployments got updated. This was important for 2 reasons:

  1. A deployment is maybe not ready yet or has already stopped by the time the player queried the deployments. By requesting to get all available deployments every few seconds, we would ensure that the client would get an up-to-date view
  2. The PlayerIdentityToken expires after a while. We receive this as the first thing when we query the deployments and then need to wait until the player makes a choice. there is a small chance that the PIT expires in the meantime and the user is not able to connect to the chosen deployment.

That should be considered, however I feel it not in the scope of this ticket, it is better to create another ticket to track this.

@jessicafalk
Copy link
Contributor

I would agree, if it was a new feature. However, you removed this functionality with this PR, so I do think that it should be in scope of this ticket.

@raymonwang
Copy link
Contributor Author

I would agree, if it was a new feature. However, you removed this functionality with this PR, so I do think that it should be in scope of this ticket.

The previous logic just refresh LoginToken regularly, however you mentioned that PlayerIdentityToken expires after a while, does that mean we have to run the whole request dev auth flow regularly?

@jessicafalk
Copy link
Contributor

jessicafalk commented Feb 6, 2020

I would need to double-check if it's only the login token or also the PIT (my bad, I did mean the login token in the previous comment) that expires after a while.

The login token definitely does and that's why we would need to refresh and re-populate the deployments list. It also allows us to show the current status of the deployment instead of an out-dated view if the user doesn't choose immediately. What I would like to ask you to do is to add the functionality of refreshing the deployments after a certain amount of second back in as that is pretty useful.

Copy link
Contributor

@jessicafalk jessicafalk left a comment

Choose a reason for hiding this comment

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

looks much cleaner !

…ntroller.cpp

Co-Authored-By: jessicafalk <31853332+jessicafalk@users.noreply.github.com>
@raymonwang raymonwang merged commit a42585a into master Feb 19, 2020
@raymonwang raymonwang deleted the MBL-19 branch February 19, 2020 03:01
MatthewSandfordImprobable pushed a commit that referenced this pull request Feb 28, 2020
…#94)

* MBL-19 Remove some hard coded thing in ADeploymentsPlayerController and keep existing features.
MatthewSandfordImprobable added a commit that referenced this pull request Mar 3, 2020
* MBL-19 Remove some hard coded logics in ADeploymentsPlayerController. (#94)

* MBL-19 Remove some hard coded thing in ADeploymentsPlayerController and keep existing features.

* Update to player controller.

* Removing tryloadfromcommandline as it is unecessary for release.

Co-authored-by: wangxin <raymonwang@improbable.io>
oblm added a commit that referenced this pull request Mar 17, 2020
* [UNR-1715][MS] Fix for error messages being deisplayed when character is killed. (#89)

Essentially there is a race on death between the health component being updated and the player controller unpossessing it's character actor.
I have just added a delay as a quick hacky fix so that both updates are recieved.

* Decrementing player count when player disconnects. (#98)

* [UNR-2425][MS] Fixing bug with sim players not moving.

* [UNR-2608][MS] Decrementing player count when player disconnects.

* Revert "[UNR-2425][MS] Fixing bug with sim players not moving."

This reverts commit 3c662ca.

* Fixing none component on Player character. (#100)

The Nameplate component had "Owner no see" enabled which means that the component will not exist for the owner of the actor.
The fix is simply to check the Nameplate component before using it.

# Conflicts:
#	Game/Content/Characters/BP_FPS_Character.uasset

* Merge of MBL-19 (#108)

* MBL-19 Remove some hard coded logics in ADeploymentsPlayerController. (#94)

* MBL-19 Remove some hard coded thing in ADeploymentsPlayerController and keep existing features.

* Update to player controller.

* Removing tryloadfromcommandline as it is unecessary for release.

Co-authored-by: wangxin <raymonwang@improbable.io>

* [CHINF-886][MS] Fixing a crash caused by the merge into release related to the jira issue. (#111)

* [UNR-3041][MS] Addin 14.3 into release (#112)

* Crash fix when hitting cancel button. (#114)

* [UNR-3071][MS] Updating Worker SDK version to 14.5 (#116)

* Readme update: new docs site + maturity guidelines

* More docs links updates

* Update DefaultSpatialGDKSettings.ini

Co-authored-by: MatthewSandfordImprobable <matthewsandford@improbable.io>
Co-authored-by: wangxin <raymonwang@improbable.io>
Co-authored-by: Ernest Oppetit <ernest@improbable.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/MBL size/M Denotes a PR that changes 40-149 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants