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

Implement beat-synchronized animation in skip overlay #29866

Closed
wants to merge 4 commits into from

Conversation

nekupaw
Copy link
Contributor

@nekupaw nekupaw commented Sep 13, 2024

I think the new beat-synced animation in the break overlay is really cool, so I implemented pretty much the same approach in the skip overlay.
I also rounded the corners by replacing the box with a circle.

Let me know what you think.

skipOverlay_beat-synced-animation.mp4

@nekupaw
Copy link
Contributor Author

nekupaw commented Sep 13, 2024

ahh, I didn't mean to merge it directly in the main. It's my first pull request, so sorry.

@bdach
Copy link
Collaborator

bdach commented Sep 13, 2024

More importantly, why is the diff listing 405 files changed? It very much should not.

Please force push that away or reopen your change with the proper diff.

@nekupaw
Copy link
Contributor Author

nekupaw commented Sep 13, 2024

Yeah, I was surprised too. It happened after I ran a code formatter over it, or rather, entered the command dotnet into the command line.
I'll fix it right away, just a moment please.

@nekupaw nekupaw changed the title Implement beat-synchronized animation in break overlay Implement beat-synchronized animation in skip overlay Sep 13, 2024
@bdach
Copy link
Collaborator

bdach commented Sep 13, 2024

Yeah we don't use dotnet format anymore here.

@peppy
Copy link
Member

peppy commented Sep 17, 2024

Should not be merged without squash.

Also @nekupaw please stop merging master.

@nekupaw
Copy link
Contributor Author

nekupaw commented Sep 17, 2024

Sorry, I forgot to create a branch at the start. I just intended to include the new merges.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

while addressing this review please fix history as requested by force-pushing (this PR should consist of a single commit)

Anchor = Anchor.BottomCentre,
Origin = Anchor.BottomCentre,
Colour = colours.Yellow,
RelativeSizeAxes = Axes.X,
Masking = true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Redundant, Circle already enables masking by default (it wouldn't be a circle otherwise)

Comment on lines +219 to +222
if (fadeOutBeginTime <= gameplayClock.CurrentTime)
return;

float progress = (float)(gameplayClock.CurrentTime - displayTime) / (float)(fadeOutBeginTime - displayTime);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is fadeOutBeginTime checked against current time (therefore clamping from the left), but progress is not forcefully clamped to be no larger than 1?

@nekupaw
Copy link
Contributor Author

nekupaw commented Sep 18, 2024

I'll create a pull request using a new branch now, I think this is the easiest way.

@nekupaw nekupaw closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants