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

Expose async track Start/Stop/Play/Restart methods #5294

Merged
merged 6 commits into from
Jul 7, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented Jul 6, 2022

No description provided.

@peppy
Copy link
Member Author

peppy commented Jul 6, 2022

Sample usage at https://github.com/ppy/osu/compare/master...peppy:osu:async-track-operations?expand=1.

I'm not sure this is the correct direction to take with audio threading in general, but it's definitely the path of least resistance with how things stand at the moment.

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.

Rest looks fine

Comment on lines +230 to +232
public override Task StopAsync()
{
stopInternal();
isRunning = isPlayed = false;
});
return EnqueueAction(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about not calling the base method... Both here and in TrackVirtual's overrides... If I were to guess, you did this because you don't want the overhead.

I suppose it's fine for now... It's basically a smoking gun if Track itself ever gets changed to do something (like a disposal check) there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's a bit weird. Could change Stop to be abstract instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably all of them abstract yeah? I mean the existing TrackVirtual.Start() also elides the base call which does an IsDisposed check. Going by that, it should just be moved to TrackBass.

Comment on lines 50 to 52
await StopAsync().ConfigureAwait(false);
await SeekAsync(RestartPoint).ConfigureAwait(false);
await StartAsync().ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how correct this is, because it's 3 different async operations rather than a grouped one. Have you considered the following instead?:

EnqueueAction(() =>
{
    Stop();
    Seek();
    Start();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Might work better yeah.

@smoogipoo smoogipoo enabled auto-merge July 7, 2022 03:21
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.

2 participants