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

Change editor speed adjustment back to adjusting tempo #28521

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 18, 2024

The important distinction here is that to prevent misuse when timing, the control will revert to 1.0x speed and disable when moving to timing screen, with a tooltip explaining why.

2024-06-18.09-43-53.mp4

- Partially reverts ppy#12080
- Addresses ppy#27830,
  ppy#23789,
  ppy#15368, et al.

The important distinction here is that to prevent misuse when timing,
the control will revert to 1.0x speed and disable when moving to timing
screen, with a tooltip explaining why.
@OliBomby
Copy link
Contributor

Is timing at lower speeds accurate if it did only change audio frequency? Maybe we can enable the playback speed in the timing screen if it adjusts frequency instead of tempo there.

@bdach
Copy link
Collaborator Author

bdach commented Jun 18, 2024

Is timing at lower speeds accurate if it did only change audio frequency?

There are have been concerns brought up before like #27826 or #26421 which tend to suggest that it might not be. I'm somewhat open to having it stay as a frequency adjustment if those are considered minor.

@Hiviexd
Copy link
Member

Hiviexd commented Jun 18, 2024

if it's concluded that timing stays (mostly) accurate with the frequency change then we can definitely have the best of both worlds by keeping the tempo change for compose page (to better identify snaps/rhythms in the song), and having the frequency change in the timing page.

@bdach bdach self-assigned this Jun 19, 2024
@OliBomby
Copy link
Contributor

OliBomby commented Jul 3, 2024

I did a test where I time a 100 BPM metronome as music with different playback speeds in the timing panel. (with frequency change like in master)

I determine the timing offset that sounds on time for each playback speed and then record it at 100% speed with the in-game metronome playing over the music metronome. Lastly I check the recording in audacity to determine the difference in timing between the two metronomes.

Results:

  • 25% = my timing is 31ms late
  • 50% = my timing is 21ms late
  • 75% = my timing is 10ms late
  • 100% = my timing is 2ms late

This evidence shows that there is a significant difference in the timing offsets depending on the playback speeds. I guess its fine to disable playback speeds in the timing panel until these offsets are fixed.

Unrelated but interesting: when I use the correct timing, the timeline waveform shows my timing being being quite early.
osu!_vEjBF8z38i

@peppy
Copy link
Sponsor Member

peppy commented Jul 4, 2024

Unrelated but interesting: when I use the correct timing, the timeline waveform shows my timing being being quite early.

When you say "correct" timing, what is this based on?

@peppy peppy self-requested a review July 4, 2024 07:19
@peppy
Copy link
Sponsor Member

peppy commented Jul 4, 2024

Let's give this a try.

I'm waiting for people to complain about the disable in the timing screen, but I can confirm that in its current state it definitely leads to incorrect timings (both master and this PR, but especially this PR).

@peppy peppy merged commit a4c575f into ppy:master Jul 4, 2024
13 of 17 checks passed
@OliBomby
Copy link
Contributor

OliBomby commented Jul 4, 2024

Unrelated but interesting: when I use the correct timing, the timeline waveform shows my timing being being quite early.

When you say "correct" timing, what is this based on?

With "correct" timing I mean the offset that results in the metronome of the song and the in-game metronome being exactly in sync in the audio recording.

@peppy
Copy link
Sponsor Member

peppy commented Jul 4, 2024

that sounds like your UO may be in need of adjustment 😅

@bdach bdach deleted the tempo-adjust-editor branch July 4, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants