Skip to content

Conversation

ieva-improbable
Copy link
Contributor

We are currently not doing any authority checks for updating the two session components, this PR makes sure that only the authoritative server makes the updates.

Note: we need to merge release into master (see this PR) before merging this change in, as it will otherwise come with a bunch of unrelated commits.

{
// There's an offset of 1 between the corresponding states of session progress and session state.
int SessionState = (int)SessionProgress + 1;
SendStateUpdate(SessionState);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realise you haven't changed this, but could we switch SendStateUpdate to internally use SessionProgress (and use a static_cast<> to cast it to and int), and offset it by 1 in the component update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better now?

Copy link
Collaborator

@m-samiec m-samiec left a comment

Choose a reason for hiding this comment

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

Few nits - looks great!

joshuahuburn pushed a commit that referenced this pull request May 31, 2019
}

void AGDKSessionGameState::SendStateUpdate(int NewState)
void AGDKSessionGameState::SendStateUpdate(EGDKSessionProgress SessionProgress)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is hiding a member variable name and will cause a warning, perhaps rename to InSessionProgress.

ieva-improbable and others added 2 commits June 5, 2019 16:15
Co-Authored-By: Michael Samiec <michaelsamiec@improbable.io>
@ieva-improbable ieva-improbable merged commit 79b8c48 into master Jun 5, 2019
@ieva-improbable ieva-improbable deleted the bugfix/fix-multiserver-session-state-updates branch June 5, 2019 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants