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

Feature/player persistence visuals #356

Merged
merged 8 commits into from
Sep 9, 2021

Conversation

fernando-cortez
Copy link
Collaborator

Part 3/3 of Player Persistence. Jira here.

This PR fixes any visuals that were omitted from Part 2. This re-introduces names/health/portrait to PartyHud, and re-introduces the world-space displayed name for a player avatar.

Notable changes:

  • Graphics variants have been created for every avatar, 8 in total. These get spawned (& cached for reuse) inside CharSelect scene.
  • GameStateRelay has been removed, as character select data can be passed to PersistentPlayer. A late-joiner gets assigned a random avatar class.
  • A NetworkGameState NetworkObject is spawned alongside the host. This keeps track of the win state. Server sets this component's win state, and clients read from this state value when presenting win/loss screen.
  • PartyHud refactor to initialize state as avatars are spawned inside BossRoom scene. This removed a fair amount of hard referencing inside of ClientCharacterVisualization. Systems are decoupled.

@fernando-cortez fernando-cortez added the 1-Needs Review PR needs attention from the assignee and reviewers label Jul 19, 2021
@fernando-cortez fernando-cortez added 4-On Hold PR can't proceed because it's blocked or is otherwise waiting on something. and removed 1-Needs Review PR needs attention from the assignee and reviewers labels Jul 30, 2021
@fernando-cortez
Copy link
Collaborator Author

Placed on hold to verify potential refactorings from MLAPI features.

@fernando-cortez fernando-cortez added 1-Needs Review PR needs attention from the assignee and reviewers and removed 4-On Hold PR can't proceed because it's blocked or is otherwise waiting on something. labels Aug 5, 2021
Cosmin-B
Cosmin-B previously approved these changes Aug 24, 2021
{
// TODO: Re-implement CharSelect preview (GOMPS-550)
//m_InSceneCharacter.gameObject.SetActive(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering when this will be done

@pdeschain pdeschain added WIP Work In Progress and removed 1-Needs Review PR needs attention from the assignee and reviewers labels Aug 24, 2021
@pdeschain
Copy link
Contributor

Had a call with Fernando - we discussed changing PlayerAvatar structure now instead of later, incorporating NetworkAnimator. This way we won't need to deal with another PR that changes structure of how player gameobjects are structured.

Copy link
Contributor

@pdeschain pdeschain left a comment

Choose a reason for hiding this comment

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

see prior comment

@fernando-cortez fernando-cortez added 1-Needs Review PR needs attention from the assignee and reviewers and removed WIP Work In Progress labels Aug 30, 2021
@fernando-cortez fernando-cortez added the 2-One More Review One review in, one to go label Sep 8, 2021
targetPosition = targetNetworkObj.transform.position;
}

if ((m_Parent.transform.position - targetPosition).sqrMagnitude < (padRange * padRange))
Copy link
Contributor

Choose a reason for hiding this comment

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

On a stylistic note - I would much rather prefer this boolean value to be captured into a variable with an explicit name VS having a comment. Has to do with comments being fragile code smells almost in all cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a request to change anything - more so a general observation about our codebase. We have these comments that describe what's happening in the logic, where a properly placed and named variable would do a better job in the readability and maintainability department.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree on this. Best to just be explicit.

@fernando-cortez fernando-cortez merged commit 8431933 into develop Sep 9, 2021
@fernando-cortez fernando-cortez deleted the feature/player-persistence-visuals branch September 9, 2021 13:53
Copy link

@hsdgd hsdgd left a comment

Choose a reason for hiding this comment

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

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-One More Review One review in, one to go
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants