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

1.31 Support #45

Merged
merged 40 commits into from
Jan 16, 2024
Merged

1.31 Support #45

merged 40 commits into from
Jan 16, 2024

Conversation

roydejong
Copy link
Collaborator

@roydejong roydejong commented Jul 1, 2023

This PR adds support for Beat Saber 1.31:

  • adds DisableSsl patch to bypass SSL / encryption / certificate checks on Ignorance/ENet connections
  • minor code change for 1.31 compatibility (environmentInfos)
  • support use_ssl/useSsl in multiplayer status data, add MpStatusRepository to allow BeatTogether to access this
  • bumps MpCore version to 1.5

@roydejong roydejong marked this pull request as draft July 1, 2023 20:19
@roydejong roydejong changed the title 1.31 WIP 1.31 Support Jul 5, 2023
@roydejong roydejong marked this pull request as ready for review July 5, 2023 01:08
rcelyte added a commit to rcelyte/BeatUpRcelyte that referenced this pull request Jul 5, 2023
This is a somewhat hacky mix of multiplexing and heuristics, but is necessary for players with MultiplayerCore installed (see Goobwabber/MultiplayerCore#45)
@roydejong
Copy link
Collaborator Author

roydejong commented Jul 5, 2023

I've made some changes to this and the BeatTogether client PR to address feedback re: encryption and settings.

Regarding the SSL setting:
MultiplayerCore / BeatTogether:

  • You can now pass a use_ssl bool in multiplayer status data (default: false). MultiplayerCore stores this but cannot enforce it, that is currently the responsibility of mods that select/override servers like BeatTogether.
  • MultiplayerCore now has a MpStatusRepository utility that allows other mods like BeatTogether to practically access this information and receive update events.
  • BeatTogether will now automatically update its configuration based on the multiplayer status data. If your master server reports "useSsl":true then BeatTogether will remember and apply that setting.

ServerBrowser, unrelated to these changes:

  • Will always detect & report encryption mode per lobby, and apply it when connecting.

@rcelyte
Copy link
Contributor

rcelyte commented Jul 5, 2023

(default: false)

I take issue with having defaults incompatible with Official servers. Protocol extensions like this should build on the source seamlessly, not requiring hard-coded edge cases like if(isOfficial) where things defer.

@roydejong
Copy link
Collaborator Author

I take issue with having defaults incompatible with Official servers. Protocol extensions like this should build on the source seamlessly, not requiring hard-coded edge cases like if(isOfficial) where things defer.

I don't disagree from a purist perspective. But pragmatically speaking, this default makes the most sense to me for third-party servers as I expect they will generally not do DTLS.

Extended server status data is not used for official servers at all, because all mods do indeed have special handling for official. I think "if official, disable modded extensions" type logic will always exist and is not unreasonable.

Flipping the default would have no effect except that it might be "more correct" and would require explicit opt-out from modded servers.

@rcelyte
Copy link
Contributor

rcelyte commented Jul 5, 2023

Modded servers already have an explicit opt-out. The only status responses not containing a use_ssl field would be servers that either expect vanilla behaviour or never supported ENet in the first place (i.e. outdated instances).

@aaronjamt
Copy link
Contributor

I've further updated this PR to 1.34, when will this be merged? I don't want to submit my own PR and take @roydejong's credit for their code until this PR has been merged.

@michael-r-elp
Copy link
Collaborator

@roydejong would it be fine if we merge this into our dev branch instead of main, then we can open a new PR for 1.34.0

@roydejong roydejong changed the base branch from main to dev January 16, 2024 15:19
@roydejong
Copy link
Collaborator Author

Sounds good, merging into dev

@roydejong roydejong merged commit 2a5d6e6 into Goobwabber:dev Jan 16, 2024
@aaronjamt aaronjamt mentioned this pull request Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants