The Everything Update (UI Overhaul + Bug Fixes)#30
The Everything Update (UI Overhaul + Bug Fixes)#30speecil wants to merge 94 commits intoScoreSaber:mainfrom
Conversation
Qwasyx
left a comment
Qwasyx
left a comment
There was a problem hiding this comment.
Likely the last time I can do a review this year, but here you go.
| public void HandleMultiplayerGameStartPostfix(MultiplayerLevelSelectionFlowCoordinator __instance, ILevelGameplaySetupData levelGameplaySetupData) { | ||
|
|
||
| // i dont like this, but i have to do it, just in case the users selected level doesnt match what the game started with | ||
| BeatmapLevel beatmapLevel = SongCore.Loader.GetLevelByHash(levelGameplaySetupData.beatmapKey.levelId); |
There was a problem hiding this comment.
This does not work for multiple reasons:
GetLevelByHashrequires a hash, not levelid- It can only deal with custom maps.
What you really want to use is GetLevelById which fixes both problems
| beatmapKey.beatmapCharacteristic.SerializedName(), | ||
| hash, | ||
| (int)beatmapLevel.songDuration, | ||
| ((int)beatmapKey.difficulty * 2) + 1, |
There was a problem hiding this comment.
Are we still going with the (by the game long removed) numeric difficulty instead of string difficulty? (Sorry if this was already sufficiently resolved, but this big review is starting to get a bit out of hand and confusing.)
There was a problem hiding this comment.
Numeric would be easier overall, but im fine with changing it if the backend changes it.
There was a problem hiding this comment.
In what sense would numeric be easier? Don't you have to keep converting it everywhere?
ScoreSaber/Core/AppInstaller.cs
Outdated
| Container.Bind<ReplayService>().AsSingle().NonLazy(); | ||
| Container.BindInterfacesAndSelfTo<ScoreSaberRichPresenceService>().AsSingle(); | ||
| Container.BindInterfacesAndSelfTo<RichPresenceService>().AsSingle(); | ||
| Container.Bind<ScoreSaberHttpClient>().FromInstance(new ScoreSaberHttpClient(new("ScoreSaber-PC", Plugin.Instance.LibVersion, 5, 120))).AsSingle(); |
There was a problem hiding this comment.
please use named parameters here, otherwise just random numbers are not great for readability.
| string difficulty, | ||
| ScoreSaberLeaderboardViewController.ScoreSaberScoresScope scope, | ||
| int page = 1, | ||
| bool hideNA = false) : base(ApiConfig.UrlBases.APIv1) { |
There was a problem hiding this comment.
This indentation looks weird… put leaderboardId still into line 6 and then align all other parameters with that.
There was a problem hiding this comment.
Speaking of that @Umbranoxio, we should probably add a sensible auto formatter/linter to this project too, especially with more people working on it now.
There was a problem hiding this comment.
fixed the indentation. Once a formatter is chosen let me know
| string authType, | ||
| string nonce, | ||
| string friends, | ||
| string name) : base(ApiConfig.UrlBases.APIv1) { |
| if (loginStatus != LoginStatus.Success) { | ||
| ChangeLoginStatus(LoginStatus.Error, "Failed to authenticate with ScoreSaber! Please restart your game"); | ||
| ChangeLoginStatus(LoginStatus.Error, $"Failed, attempting again ({attempt} of 3 tries...)"); | ||
| await Task.Delay(4000); |
There was a problem hiding this comment.
Isn't this also being shown after the 3rd attempt too with a wait of 4s until it shows you that it failed without every trying again?
| private const string _protocolAndSubdomain = "wss://realtime."; | ||
| private const string _socketAddress = "/socket"; // change to scoresaber subdomain once ready. wss://realtime.scoresaber.com/socket | ||
|
|
||
| public string TimeRightNow => DateTime.UtcNow.ToString("yyyy-MM-ddTHH:mm:ss"); |
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public static class IsExternalInit { | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Awful file name (unless this is actually required like this, but I doubt it)
There was a problem hiding this comment.
this was from umbra, im not sure what for
| LibVersion = Assembly.GetExecutingAssembly().GetName().Version; | ||
| HttpInstance = new Http(new HttpOptions() { baseURL = "https://scoresaber.com/api", applicationName = "ScoreSaber-PC", version = LibVersion }); | ||
| harmony = new Harmony("com.umbranox.BeatSaber.ScoreSaber"); | ||
| harmony.PatchAll(Assembly.GetExecutingAssembly()); |
There was a problem hiding this comment.
Considering we are no longer honoring Settings.disableScoreSaber, we should at the very least add comment in the Settings.cs file that this setting no longer does anything.
| <OutputType>Library</OutputType> | ||
| <LangVersion>8.0</LangVersion> | ||
| <LangVersion>9</LangVersion> | ||
| <Nullable>disable</Nullable> |
There was a problem hiding this comment.
Why explicitly disable this if you are gonne reenable it in tons of files? We should be consistent here and either enable it everywhere or disable it everywhere.
4 participants
I have probably forgotten something...