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

Broadcast game state through web sockets #18129

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

LeNitrous
Copy link
Contributor

@LeNitrous LeNitrous commented May 7, 2022

Addresses #13011 and supersedes #12274.

  • This adds a new setting under "Integration Settings" to enable or disable game state broadcasting.
  • When enabled, the game state broadcaster can be accessed through ws://localhost:7270/state/.
  • The broadcaster currently emits ruleset, beatmap, user activity changes as well as live statistics during gameplay.

Example:

2022-05-06.22-58-39.mp4

Adds WebSocketServer, GameStateBroadcastServer and GameStateBroadcaster components.

  • WebSocketServer is an abstract CompositeDrawable capable of listening and accepting web socket requests.
  • GameStateBroadcastServer hosts child GameStateBroadcaster components which allows dependency injection and allows removal and adding of children as needed.
  • GameStateBroadcaster is an abstract Component capable of broadcasting JSON text of the current game's state. The JSON payload broadcasted uses this schema:
{
    "Type": "<string>",
    "Message": "<any>"
}

Differences with the original PR:

  • This reliably handles disconnections requested by the server or client.
  • The broadcasters have been modularized for extensibility and ease of integration instead of it being a large JSON payload.

Example client in Javascript:

var socket = new WebSocket("ws://localhost:7270/state/");
socket.onmessage = function(e) {
    let { Type, Message } = JSON.parse(e.data);
    console.log(Type);      // string
    console.log(Message);   // any
}

@LeNitrous
Copy link
Contributor Author

Previously discussed on Discord, HttpListener is considered as deprecated API and only exists for compatibility reasons. Microsoft encourages use of ASP.NET. However upon testing in an isolated project, attempting to run a Kestrel server with minimal APIs using Microsoft.AspNetCore.Server.Kestrel 2.2.0 will fail to run if Microsoft.AspNetCore.SignalR.Client is included as a package reference. Should we still be using HttpListener in the meantime?

@frenzibyte
Copy link
Member

Sounds like you might want to report this against their side.

@LeNitrous
Copy link
Contributor Author

Reported the issue (dotnet/aspnetcore#41575) with the ASP.NET team and according to the respondents, it is a valid issue but its also a "wont fix" case as the Kestrel package on NuGet is deprecated. This is rather unfortunate as the only real way left is implementing it from scratch using a TcpListener which at this point goes beyond the scope of this PR.

@davidfowl
Copy link

I don't know what the constraints are but why not target .NET 6?

@peppy
Copy link
Member

peppy commented May 9, 2022

I don't know what the constraints are but why not target .NET 6?

This is our eventual goal, but we are currently limited by mobile platforms (where we still rely on xamarin). That said, you make a valid point in this case as the code being changed here likely only needs to work on desktop platforms, where we can conditionally target .NET 6.

I haven't gone through this PR yet, but @LeNitrous you may be able to move the implementation to the osu.Desktop project and isolate to .NET 6, if that helps.

@smoogipoo
Copy link
Contributor

It's not that we can't use the full ASP.NET framework, but the contributor above was somewhat concerned with keeping the footprint of our application as low as possible by not pulling in too many unnecessary dependencies.

I've tested a simple dummy application and a publish inclusive of ASP.NET adds 20MB to the output. I'm not sure if this is unreasonable just yet, and it's also an isolated test so potentially many dependencies would duplicate existing ones, but it's something to keep in mind.

Personally, I'm not super against pulling in ASP.NET if there's no lightweight alternative to websockets fit for our needs, and waiting on assembly trimming/linking for the future.

@davidfowl
Copy link

davidfowl commented May 9, 2022

No problem. If you care about the performance, then you should use ASP.NET Core (and maybe SignalR) but I don't know what the constraints are so I'll leave you to make the best decisions for your game 😄

@LeNitrous
Copy link
Contributor Author

I have taken up @peppy's suggestion in moving the the websocket server to osu.Desktop and use the Microsoft.AspNetCore.App framework to use Kestrel. Additionally, I removed the ability for the game state broadcaster to broadcast player state changes until I find an elegant solution (or may be included in a separate PR instead).

@davidfowl
Copy link

Do you want any feedback on the websocket implementation? How much do you care about performance?

@bastianpedersen

This comment was marked as off-topic.

@peppy peppy removed the proposal label Sep 9, 2022
@bastianpedersen

This comment was marked as off-topic.

@frenzibyte
Copy link
Member

There are no discussions left in this PR, this is only pending review.

@LeNitrous
Copy link
Contributor Author

Preliminary analysis on possible solutions based on my findings and suggestions made in Discord on how to deal with ASP.NET.

Current Situation

Currently, to be able to use a Websocket Server, the Kestrel Server is needed to be installed as a package dependency. However, installing the dependency alone is not enough as the entirety of the ASP.NET Core framework is required. This introduces unnecessary bloat to osu! itself as we are only interested in a Websocket broadcast server.

Solution 1: Enable Trimming

This may affect custom rulesets as enabling trimming disallows dynamic assembly loading.

Solution 2: Write a websocket server from scratch

This may be an overkill solution and I highly doubt we may want to maintain this.

Solution 3: Make use of a third-party implementation

Watson Websocket is an MIT-Licensed third-party implementation of the Websocket standard which is around 200 KB in package size (as stated in the NuGet page) and does not need any more additional dependencies. This may be one of the feasible solutions I can suggest.

Although I do want to hear thoughts from others about the solutions proposed before I proceed in making changes.

@peppy
Copy link
Member

peppy commented Nov 21, 2022

This may affect custom rulesets as enabling trimming disallows dynamic assembly loading.

One would hope with the template and built-in rulesets we would have 100% coverage of ruleset API code. If not, we would probably need to forcefully exclude it from trimming (I'd hope this is possible). This does seem like the best path forward though.

@SirWaddles
Copy link

I thought I'd give this a try since I stream osu sometimes and was previously using gosumemory for this. I did make a merge to master, but I'm not very experienced with .NET 6 so I probably made a few mistakes.

It works really well! It's nice that this could be a feature native to osu. The player state I think is missing at the moment though.

I'm not sure if this can be accommodated, but in the past I had my websocket bound to my local address rather than localhost. This is because I'm using a separate streaming PC and I had it running the overlay. I could set up a proxy if need be, but I thought I'd mention it.

Thanks for building this @LeNitrous

@peppy peppy marked this pull request as draft June 16, 2023 06:09
@ilsubyeega
Copy link
Contributor

ilsubyeega commented Aug 12, 2023

I would like to present a different perspective.
Currently, I understand that this feature is implemented by building a WebSocket server to broadcast the data. This feature is limited to the osu.Desktop platform.
If it is tricky to create a WebSocket server, an alternative approach worth considering could be implementing this feature as a client instead of a server. Of course, this would require a separate third-party WebSocket server, but I believe this would allow this feature to be fully utilized not only on the osu.Desktop platform, but also on mobile devices such as smartphones and tablets.

Nevermind.

@longnguyen2004
Copy link

Are you still working on this? I think using a third-party library is the way to go, but we should ask for dev team's opinion

@smoogipoo
Copy link
Contributor

This is currently still low priority for the time being.

@Akarinnnnn
Copy link

Previously discussed on Discord, HttpListener is considered as deprecated API and only exists for compatibility reasons. Microsoft encourages use of ASP.NET. However upon testing in an isolated project, attempting to run a Kestrel server with minimal APIs using Microsoft.AspNetCore.Server.Kestrel 2.2.0 will fail to run if Microsoft.AspNetCore.SignalR.Client is included as a package reference. Should we still be using HttpListener in the meantime?

We can use WebSocket.CreateFromStream after .NET 6. Such implementation is provided by assembly System.Net.WebSockets and doesn't requires Kestrel. In case we can't add ASP.NET Core as a dependency, it can be used as alternative.

@Chiffario
Copy link

Previously discussed on Discord, HttpListener is considered as deprecated API and only exists for compatibility reasons. Microsoft encourages use of ASP.NET. However upon testing in an isolated project, attempting to run a Kestrel server with minimal APIs using Microsoft.AspNetCore.Server.Kestrel 2.2.0 will fail to run if Microsoft.AspNetCore.SignalR.Client is included as a package reference. Should we still be using HttpListener in the meantime?

This got a bit lost in out-of-Github discussions, but, as was discovered, HttpListener isn't deprecated but just frozen. After mentioning this to @bdach back in October, he said that using HttpListener shouldn't be an issue, as newer APIs and features aren't necessary for this use-case

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.