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

Compatibility for Beat Saber v1.6.0 and updated to use BSIPA #30

Merged
merged 8 commits into from
Jan 1, 2020

Conversation

Sarayalth
Copy link
Contributor

No description provided.

Copy link
Owner

@opl- opl- left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

I can't test if all the messages work right now so I'll try to get that covered later, however I can tell that you made some changes that affected the code style. Please, make your changes adhere to the established style. I marked the first appearance of each mistake to make it easier for you. This is primarily to keep the commit diffs easy to read and the code consistent (and therefore also easier to read).

Other than that it seems like it should work assuming nothing significant changed in the game internals that would make things quietly fail. I'll try to get it tested as soon as I can.

BeatSaberHTTPStatus/BeatSaberHTTPStatusPlugin.csproj Outdated Show resolved Hide resolved
BeatSaberHTTPStatus/Plugin.cs Outdated Show resolved Hide resolved
BeatSaberHTTPStatus/Plugin.cs Outdated Show resolved Hide resolved
BeatSaberHTTPStatus/Plugin.cs Outdated Show resolved Hide resolved
BeatSaberHTTPStatus/Plugin.cs Outdated Show resolved Hide resolved
BeatSaberHTTPStatus/manifest.json Outdated Show resolved Hide resolved
protocol.md Outdated Show resolved Hide resolved
BeatSaberHTTPStatus/BeatSaberHTTPStatusPlugin.csproj Outdated Show resolved Hide resolved
BeatSaberHTTPStatus/BeatSaberHTTPStatusPlugin.csproj Outdated Show resolved Hide resolved
@Sarayalth
Copy link
Contributor Author

Sorry about the styling, everything should be ok now

@opl-
Copy link
Owner

opl- commented Dec 22, 2019

Almost! There are still two else ifs and the BOM change. As far as I remember I had some issue with MSBuild and the BOM in the past, so I'd prefer that didn't change.

As far as testing goes, it seems I won't be able to handle that myself at least until tomorrow, so I'll try to look into how well BeatMods would handle release candidate versions. Might just let the community test it that way. 👀

@opl-
Copy link
Owner

opl- commented Dec 22, 2019

Looks good! If I don't catch anything wrong during testing (when I can) it will get released.

Thank you!

@Sarayalth
Copy link
Contributor Author

o7

@Sarayalth
Copy link
Contributor Author

i just tested the changes to see if it works (i've been busy this days) and something might have changed with this game update, because as far as i could test something in OnNoteWasCut seems to fail with this error:
ArgumentException: An item with the same key has already been added. Key: NoteCutInfo

I think I fixed that (code in dev branch on my fork), but combos and something else seems to be broken now :/

@Sarayalth
Copy link
Contributor Author

Hey fixed that!
I've been testing everything for a while at seems to work fine now

Copy link
Owner

@opl- opl- left a comment

Choose a reason for hiding this comment

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

Sorry for the slow replies. Holidays and other things kept me busy. I left some review comments for the new changes and some issues with the old stuff I missed last time.

Thank you for your patience and once again thanks for the PR. The changes so far should be tested some time this year.


List<CutScoreBuffer> list = (List<CutScoreBuffer>) afterCutScoreBuffersField.GetValue(scoreController);
List<CutScoreBuffer> list = (List<CutScoreBuffer>)afterCutScoreBuffersField.GetValue(scoreController);
Copy link
Owner

Choose a reason for hiding this comment

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

Formatting.

@@ -353,11 +355,17 @@ public class Plugin : IPlugin {
statusManager.EmitStatusUpdate(ChangedProperties.PerformanceAndNoteCut, "noteMissed");
}
}
}

public void OnNoteWasCut2(NoteController noteController, NoteCutInfo noteCutInfo) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is a very undescriptive name. How does it differ from OnNoteWasCut? Also, why is this one necessary and what order are they called in?

@opl- opl- merged commit d4a8efd into opl-:master Jan 1, 2020
opl- added a commit that referenced this pull request Jan 1, 2020
Breaks:
- `playerSettings.disableSFX` is now `playerSettings.sfxVolume`

Changes:
- Beat Saber 1.6 support (#30, by @Sarayalth)

Adds:
- BSIPA support (#30, by @Sarayalth)
@opl-
Copy link
Owner

opl- commented Jan 1, 2020

Published as v1.9.0 and published on BeatMods and GitHub releases, as usual. Unfortunately couldn't do any comprehensive testing, but I trust that you tested it and that it works, especially considering the additional commits you made.

Thank you yet again for this PR and sorry for being so slow.

Oh, I should also mention that I did look into the whole release candidate thing: BSIPA supports semver version strings correctly and BeatMods seems to have no code that actually checks the format of the version string, so it should be possible. Still, I decided not to do it and decided to just release a patch if anything turns out to still be broken.

@Sarayalth
Copy link
Contributor Author

Sarayalth commented Jan 2, 2020

Oh, sorry for not responding, i didn't see the notifications.

About the OnNoteWasCut2 thing, after some testing that did "work" and i thought it fixed everything, but apparently wasn't processing last cut info. (Score updated to the last to last cut, and rank wasn't working fine)

I fixed this after realising that with the change to BSIPA you don't need to subscribe to OnActiveSceneChange, it just works with a method with that name, but couldn't upload the commits as i've been without internet until just now.

I'll clean everything and upload in a bit, sorry about all this 🙇.

@Sarayalth
Copy link
Contributor Author

Merged your changes with the fixes, tested looking at the score and rank, and it's working. Do I open another pr for this changes?

@opl-
Copy link
Owner

opl- commented Jan 2, 2020

Yes, if you have any more fixes you should open a new PR explaining what issues they fix and how they do it.

@opl-
Copy link
Owner

opl- commented Jan 2, 2020

Sorry, I read your explanation for OnNoteWasCut2 again and I think I don't understand what you're trying to say. Does it work as is or is there currently some issue with it in v1.9.0?

@Sarayalth
Copy link
Contributor Author

Sarayalth commented Jan 2, 2020

There's an issue with it in v1.9.0, i just sent a pr with a fix for it.

The thing is BSIPA don't need
SceneManager.activeSceneChanged += this.OnActiveSceneChanged;

And because i didn't know that at the time i started working on this i tested something and found that OnNoteWasCut2 "worked", but some data was not processed correctly with that.

After realising what was the problem i fixed it and now it works as expected, processing correctly the score and rank of the last cut made. (The mod was displaying score for the cut made before the last cut, and rank/accuracy was completely broken)

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.

2 participants