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 breathing room to seek back between control points in editor #30328

Merged
merged 9 commits into from
Oct 22, 2024

Conversation

TaterToes
Copy link
Contributor

@TaterToes TaterToes commented Oct 17, 2024

closes #28885 (and #30070)

gives 450 ms to seek back after last control point in editor.

@TaterToes
Copy link
Contributor Author

Screenshot_20241018_104003_Chrome
What does this mean?

@bdach
Copy link
Collaborator

bdach commented Oct 18, 2024

Not anything you need to concern yourself with. That problem is already fixed in master.

@bdach
Copy link
Collaborator

bdach commented Oct 22, 2024

Stable logic for this is

            if (AudioEngine.ActiveInheritedTimingPointIndex < 0) return;

            if (AudioEngine.Time - AudioEngine.ControlPoints[AudioEngine.ActiveInheritedTimingPointIndex].Offset > 1000)
            {
                AudioEngine.SeekTo((int)AudioEngine.ControlPoints[AudioEngine.ActiveInheritedTimingPointIndex].Offset);
                return;
            }

            if (AudioEngine.ActiveInheritedTimingPointIndex == 0)
                return;

            AudioEngine.SeekTo((int)AudioEngine.ControlPoints[AudioEngine.ActiveInheritedTimingPointIndex - 1].Offset);

(https://github.com/peppy/osu-stable-reference/blob/38bbc10a53c78d83c9f04a04f7bf4818505cd313/osu!/GameModes/Edit/Editor.cs#L522-L536)

In particular note:

  • the different lenience interval (1000ms) - I'd probably apply this change
  • the lack of dispensation for when the track is running, which is present in this PR - I think checking track running state is fine, so I wouldn't want any changes with respect to this

(Side note but the stable implementation is bugged - if you have a non-inherited point and inherited point on the same timestamp, e.g. a "yellow line", you won't be able to seek back beyond it. Not relevant in this context anyhow.)

@peppy obligatory summon for UX opinion on desired behaviour

@peppy
Copy link
Member

peppy commented Oct 22, 2024

I really do question 8 commits for 10 LoC changes and it's still failing basic formatting requirements, but let's overlook that for now 🙈

Comment on lines 1106 to 1107
IAdjustableClock adjustableClock = clock;
seekMargin = 450 * adjustableClock.Rate;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think rate needs to be accounted for? Especially if we increase this to 1 second.

Copy link
Member

Choose a reason for hiding this comment

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

Probably fine to leave it, whatever works.

@peppy peppy requested a review from bdach October 22, 2024 09:10
@bdach
Copy link
Collaborator

bdach commented Oct 22, 2024

I really do question 8 commits for 10 LoC changes and it's still failing basic formatting requirements, but let's overlook that for now 🙈

I've given up on even remarking on that sort of thing at this point because it helps nothing and only looks snarky.

@peppy
Copy link
Member

peppy commented Oct 22, 2024

I guess in an attempt to make it non-snarky, @TaterToes consider not PRing until you're done with making changes, then squash your changes down to less commits if you want to seem more competent 😅

@bdach bdach merged commit 24dfc1b into ppy:master Oct 22, 2024
11 of 13 checks passed
@TaterToes TaterToes deleted the seekingControlPointFix branch October 23, 2024 15:23
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.

Getting stuck while navigating the timeline backwards using the up arrow key during active playback
4 participants