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

Show spectating users during gameplay #31527

Merged
merged 12 commits into from
Jan 21, 2025
Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jan 15, 2025

argon triangles classic
osu! osu_2025-01-15_14-00-02 osu_2025-01-15_14-00-21 osu_2025-01-15_14-00-29
taiko osu_2025-01-15_14-01-17 osu_2025-01-15_14-01-23 osu_2025-01-15_14-01-28
catch osu_2025-01-15_14-01-44 osu_2025-01-15_14-01-50 osu_2025-01-15_14-01-55
mania osu_2025-01-15_14-02-21 osu_2025-01-15_14-02-28 osu_2025-01-15_14-02-35

Please forgive the playfield on the above looking a bit broken, it's what happens when the skin doesn't fully correctly reload during pause. That was the quickest way to capture these screenshots. (By the way this sort of thing is what needs to happen now that every ruleset can have elements in arbitrary places...)

@bdach bdach force-pushed the spectator-list-ready branch from defc3fa to 99c7e16 Compare January 15, 2025 13:30
@bdach bdach self-assigned this Jan 15, 2025
@bdach bdach added the notable feature Attach to pull requests which should be highlighted on the changelog. Things users want to read. label Jan 15, 2025
@peppy peppy self-requested a review January 17, 2025 04:02
smoogipoo
smoogipoo previously approved these changes Jan 17, 2025
Comment on lines 248 to 249
((IBindableList<SpectatorUser>)Spectators).BindTo(client.WatchingUsers);
((IBindable<LocalUserPlayingState>)UserPlayingState).BindTo(gameplayState.PlayingState);
Copy link
Member

Choose a reason for hiding this comment

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

I find it super odd that these bindings are done here. I get that this is to allow the base class to be tested, but it doesn't sit well. Two proposals:

  • Rename this class to OnlineSpectatorList (dunno)
  • Change how the testing is done to not require that and move the ISerialisableDrawable to the base implementation

Leaning towards the second.

@bdach bdach force-pushed the spectator-list-ready branch from abf4b7b to 3c4bfc0 Compare January 17, 2025 10:23
Comment on lines 77 to 78
((IBindableList<SpectatorUser>)Spectators).BindTo(client.WatchingUsers);
((IBindable<LocalUserPlayingState>)UserPlayingState).BindTo(gameplayState.PlayingState);
Copy link
Contributor

@smoogipoo smoogipoo Jan 21, 2025

Choose a reason for hiding this comment

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

Because this is a threaded component, these should not be bound on the BDL thread. Especially and in particular the BindableList<>.

There is a potential race condition where:

  • This thread is in the middle of copying elements from the remote list on the BDL thread.
  • The remote list changes on the Update thread.

I believe these days we use LoadComplete() to bind bindables in general.

Here's an example:

[Test]
public void RunTest()
{
    var list1 = new BindableList<int>([0]);

    new Thread(() =>
    {
        int nextAdd = 1;
        int nextRemove = 0;

        while (true)
        {
            if (RNG.Next() % 3 == 0)
                list1.Remove(nextRemove++);
            else
                list1.Add(nextAdd++);
        }
    })
    {
        IsBackground = true
    }.Start();

    new Thread(() =>
    {
        while (true)
        {
            var list2 = new BindableList<int>();
            list2.BindTo(list1);
        }
    })
    {
        IsBackground = true
    }.Start();

    while (true)
    {
        Thread.Sleep(2000);
    }
}

@smoogipoo smoogipoo merged commit 8f82462 into ppy:master Jan 21, 2025
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:skinning area:spectator notable feature Attach to pull requests which should be highlighted on the changelog. Things users want to read. size/L type:online
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants