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 first pieces of editor timing UI #18339

Merged
merged 21 commits into from
May 23, 2022
Merged

Add first pieces of editor timing UI #18339

merged 21 commits into from
May 23, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented May 20, 2022

This is a first step towards the timing experience I'm looking to implement. Currently this only adds a metronome display and some basic buttons. The main button will eventually become the "tap to time" button, but I want to keep the PRs bite-sized so will bring that in later.

Did consider implementing the metronome in isolation but as I'm not sure the following pieces will be completed this week, feels like a nice glimpse of what is coming which can potentially go into the next release!

osu.Game.Tests.2022-05-20.at.08.25.05.mp4

Note that currently the metronome tracks the beatmap not the current timing point, which is a bit weird. I need to figure out how to implement this in terms of the overall flow, so I'm turning a blind eye to this for now. You can see this happening here:

osu.2022-05-20.at.08.27.21.mp4

If it's seen as a blocker I can probably make something happen in this PR (ie. fading the metronome out and stopping updates when note in the current timing point's control area).

@bdach
Copy link
Collaborator

bdach commented May 21, 2022

On the UX note from the OP:

Note that currently the metronome tracks the beatmap not the current timing point, which is a bit weird. I need to figure out how to implement this in terms of the overall flow, so I'm turning a blind eye to this for now.

I actually think this makes a decent amount of sense, and would say that if anything, I would fix this by having the timing point selection on the list change to the latest applicable timing point as the track is playing, which would somewhat "mask" this by changing the highlight to always match the current track bpm.

This makes a decent amount of sense to me given that clicking on any timing point will already seek to that given time.

@peppy
Copy link
Member Author

peppy commented May 21, 2022

For sure. The weird thing is that it's going to kinda "reset" the display when changing points on the left. And potentially to an effect point which doesn't have timing data. I'm still figuring out the overall flow.

RelativeSizeAxes = Axes.X,
BackgroundColour = colourProvider.Background1,
Width = 0.68f,
Action = tap,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bit of a weird name for this action method. Almost thought it was going to implement the tap-to-set-BPM functionality before seeing what button it's attached to.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is what the button will become. I changed it temporarily for getting this PR shipped.

osu.Game/Screens/Edit/Timing/MetronomeDisplay.cs Outdated Show resolved Hide resolved
float currentAngle = swing.Rotation;
float targetAngle = currentAngle > 0 ? -angle : angle;

swing.RotateTo(targetAngle, beatLength, Easing.InOutQuad);
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 going to be a bit of an insane point but if we're implementing a metronome display in the game then I'll bring it up - for physical correctness I believe this should be

Suggested change
swing.RotateTo(targetAngle, beatLength, Easing.InOutQuad);
swing.RotateTo(targetAngle, beatLength, Easing.InOutSine);

if I remember my oscillator physics right. (Note that the weight placement is probably not all there either but I'm not nitpicking on that since (a) it looks close enough to a real metronome already with the non-linear placement of the weight and, (b) I'm not really sure what is correct - I think it'd be something or other to do with the inverse of the squared frequency value?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll check again, but Quad looked more correct to me. I originally did have it on sine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Quad still looks more correct to my eye. I'm willing to concede if someone does a comparison to a physical metronome. Sine just doesn't feel like what I've come to expect from a metronome swing (not sure Quad is perfect either, but feels closer).

Copy link
Collaborator

@ekrctb ekrctb May 23, 2022

Choose a reason for hiding this comment

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

Sine is small-angle approximation. There is no closed-formula for the exact parametric representation (requires numerical integration).

osu.Game/Graphics/Containers/BeatSyncedContainer.cs Outdated Show resolved Hide resolved
peppy and others added 2 commits May 23, 2022 02:00
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Turns out this isn't required in the end due to implementation at
`MasterGameplayClockContainer`.
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.

I like this, but I'm going to ask for backup on the BeatSyncedContainer changes just in case.

@bdach bdach requested a review from smoogipoo May 22, 2022 18:00
@ppy ppy deleted a comment from ArmaBox May 22, 2022
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Seems ok

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.

5 participants