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

Add highway animation #823

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

grishhung
Copy link
Contributor

@grishhung grishhung commented Jul 13, 2024

Notes

  • Removed existing PerformanceTextScaler class and replaced with DOTween implementations
  • Added highway raise animation; takes 0.5 seconds w/ keyframes on 1/3s and 1/2s
  • Added strikeline zoom aniamtion; takes 0.5 seconds w/ keyframes on 1/6s and 1/4s (and 1/2s of delay)
  • Shortened vocal performance text to 1 second to match all Rock Band games
    • Was previously 2 seconds; text lingers on screen for too long for some short phrases
  • Lengthened non-vocal performance text to 3 seconds to match all Rock Band and Guitar Hero games
    • Following some comments during in-person testing wherein newer players complained that 2 seconds was too short to read while playing
    • Can optionally change to 2.5 seconds by reducing the wait time in between the halves of the animation from 2.667f to 2.167f

Testing

Tested; works fine as far as I can tell.

Action items

  • Add a highway raise sound; cc: @kaduwaengertner @bbrsk
  • Make the highway raise disjointedly for different players (with left side starting first)
  • Make the vocals highway drop in from the top and the other UI elements come in from the side
    • May need to disable this in the case that the UI elements have custom positioning
    • Alternatively, make the animation dynamic based on the placement (could look very bad)
  • Add a matching highway lowering animation for failing out and completing the chart (lower on [end] events)
  • Discussed with @kaduwaengertner: after backgrounds are in, delay the highway raise to on the first tick of the chart so that it raises when you hear the count-in sound, just like in Rock Band (following the visual of the band on stage)
    • In order to account for poorly made charts where the first note is on tick 0, handle this animation so that the highway always raises at least 2 seconds before the first charted note (this is the whole reason we have the delay in the first place)
  • Figure out a less hacky way to make the camera position not break performance text; checking every frame for whether or not we've lifted the highway is gross
    • The reason that this breaks is because the camera gets positioned before the position of the TMP is set and position is based on the camera position

@RileyTheFox
Copy link
Collaborator

Branch needs to be based off of dev and targeting dev. Please also remove the YARG.Core pointer commit

@grishhung grishhung changed the base branch from master to dev July 13, 2024 16:25
Copy link
Member

@EliteAsian123 EliteAsian123 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I didn't look at the changes in-game yet but I pointed out some things I noticed in the code below.

Thanks again!

Assets/Script/Gameplay/HUD/TextNotificationQueue.cs Outdated Show resolved Hide resolved
}

private void Update()
{
// Hacky animation fix; breaks text notifications if put into Initialize()
Copy link
Member

Choose a reason for hiding this comment

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

This is likely because the position of the HUD is set relative to the world space of the camera here, after Initialize is called. In general, "hacky fixes" should be avoided when they can. A proper solution could be something such as making sure the position of the HUD is set before the camera is positioned by moving when the HUD positions are set.

Comment on lines 21 to 28
var transform = this.transform;
transform.localPosition = transform.localPosition.WithZ(_zOffset - 1.2f);

yield return new WaitForSeconds(0.5f);
yield return DOTween.Sequence()
.PrependInterval(0.25f) // Total duration of sequence
.Append(transform.DOMoveZ(_zOffset + 0.1f, 0.167f)).SetEase(Ease.OutCirc)
.Append(transform.DOMoveZ(_zOffset, 0.083f)).SetEase(Ease.InOutSine);
Copy link
Member

Choose a reason for hiding this comment

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

These animations seem to contain a lot of magic numbers. If you could reduce the amount of them to make them easier to configure when there are changes to the underlying object (for example the position of the strikeline in this case) that'd be great. Stuff like this is already done for pro keys so it'd make it a little easier to merge there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, defined some comments at the top of the class that should make things more readable. Also noticed a small bug here.

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.

None yet

3 participants