Skip to content

adding a simplified version of my animation_anticipation change that … #182

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 19, 2021
Merged

adding a simplified version of my animation_anticipation change that … #182

merged 6 commits into from
Mar 19, 2021

Conversation

@dwoodruffsf
Copy link
Contributor

@dwoodruffsf dwoodruffsf commented Mar 17, 2021

  • Adds basic movement anticipation. As soon as you click, you go to an "AnticipateMove" node in the animation controller. Right now that node has the walk animation, because I think that looks better than the "walk start" animation--it is a little subjective, though, and probably has something to do with the animation node blending settings.
  • Fixes the bug in ClientInputSender where movement inputs only update at 2fps. I did leave them capped (at 20fps), because I thought leaving them uncapped didn't show best practices. It feels plenty responsive at 20fps update.

@SamuelBellomo SamuelBellomo added 1-Needs Review PR needs attention from the assignee and reviewers 2-Reviewed with Comments PR requires owner's attention labels Mar 17, 2021
eheimburg
eheimburg reviewed Mar 18, 2021
View reviewed changes
Assets/BossRoom/Scripts/Client/Game/Character/ClientCharacterVisualization.cs Outdated
public Animator OurAnimator { get { return m_ClientVisualsAnimator; } }

[Tooltip("These values must be set to match the values of the default node in each layer in the attached animation controller")]
public List<string> m_DefaultAnimationNodes = new List<string>
Copy link
Contributor

@eheimburg eheimburg Mar 18, 2021

Choose a reason for hiding this comment

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

Another way to do this is to use the Tag field of the animation nodes, and give all the starting nodes the same Tag. That's a little less error-prone because people won't need to maintain this ordered list of default nodes. (can just use IsTag(m_DefaultNodeTag) in place of the IsName() calls)

Copy link
Contributor Author

@dwoodruffsf dwoodruffsf Mar 18, 2021

Choose a reason for hiding this comment

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

that's so much better! I have made that change.

fernando-cortez
fernando-cortez reviewed Mar 18, 2021
View reviewed changes
@SamuelBellomo SamuelBellomo added 2-One More Review One review in, one to go and removed 2-Reviewed with Comments PR requires owner's attention labels Mar 18, 2021
LukeStampfli
LukeStampfli approved these changes Mar 18, 2021
View reviewed changes
Copy link
Contributor

@LukeStampfli LukeStampfli left a comment

Choose a reason for hiding this comment

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

Tested this with host + client and it works and looks good. Also agree with the name changes in this PR.
I wonder if we could make the rampup animation already turn the character towards the target direction you plan to move. I think that might look even better.

@LukeStampfli LukeStampfli added 2-Reviewed with Comments PR requires owner's attention and removed 1-Needs Review PR needs attention from the assignee and reviewers 2-One More Review One review in, one to go labels Mar 18, 2021
@SamuelBellomo SamuelBellomo merged commit 16ddab4 into release/v0.1.0-experimental Mar 19, 2021
@SamuelBellomo SamuelBellomo deleted the feature/movement_anticipation branch March 19, 2021 20:29
SamuelBellomo added a commit that referenced this pull request Mar 23, 2021
@SamuelBellomo
Merge branch 'release/v0.1.0-experimental' into develop
512d07b 
* release/v0.1.0-experimental:
  event added for ClientCharacterVisualization on destroy to remove UI elements (#190)
  Feature/template cleanup (#177)
  adding a simplified version of my animation_anticipation change that … (#182)
  cleanup for downed player on late join (order of operations) & health check (#174)
  update mlapi version (#183)
  Removed unused component reference from CharLight (5) game object, (#189)
  redundant meta files removed
  ReadMe update with Discord Channel awareness (#186)
  Update LFS content in README (#151)
  ready button disabled sprite field updated (#178)
  fix: Remove development test scenes (#179)
  Update CONTRIBUTING.md
  Sam's comments
  #
  version update (#170)

# Conflicts:
#	Assets/BossRoom/Scenes/BossRoom.unity
#	Assets/BossRoom/Scenes/CharSelect.unity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SamuelBellomo SamuelBellomo SamuelBellomo approved these changes

+3 more reviewers

@eheimburg eheimburg eheimburg left review comments

@LukeStampfli LukeStampfli LukeStampfli approved these changes

@fernando-cortez fernando-cortez fernando-cortez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@LukeStampfli LukeStampfli

@fernando-cortez fernando-cortez

Labels

2-Reviewed with Comments PR requires owner's attention

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dwoodruffsf @LukeStampfli @SamuelBellomo @eheimburg @fernando-cortez