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

Add ability to refresh from a Mediocre Mapper server #1

Closed
wants to merge 22 commits into from

Conversation

Thynix
Copy link

@Thynix Thynix commented Sep 7, 2019

I implemented this to allow a person in VR to quickly apply modifications made by others. Is this something you'd be interested in merging? I uploaded an alpha release here.

It doesn't currently provide feedback in-game when something goes wrong, but it does do network things on a separate thread to avoid hitching. (Name resolution, even for "localhost", takes 2 seconds on my machine.)

EDIT: I've tested this with MM Mk3Rev1, and would expect it to work with Mk4 as well, but Mk4Rev1 removes support for MMMM pending a rework to use the new format. This would require more work to support, but is probably doable.

The biggest bug in this was waiting for 4 fields when for reasons I don't yet understand
that's an off-by-one error and it needs 5. This hasn't been cleaned up and is missing
features but the core functionality is there.
I think I realized the reason for the off-by-one: the split by ";;;" at the end has a trailing
empty element in its result?
This uses SongCore's conversion and concurrency techniques.
It appears unused.
It's not super intuitive that Split() gives an empty element when the splitting token
as at the end of the string, so the 5 is surprising. Using an approach that works with 4
seems preferable for clarity.
This also prevents starting the coroutine more than once. Logging seems to work
fine still.
Previously the coroutine would hang if ReadWelcomeMessage() didn't return a value.
The IPA config is marked [0] as obsolete:

> Uses IniFile, which uses 16 bit system calls. Use BS Utils INI system for now.

This replaces the IPA config with the BS_Utils config support as suggested.

[0] https://beat-saber-modding-group.github.io/BeatSaber-IPA-Reloaded/api/IPA.Config.ModPrefs.html
This avoids repeatedly referring to BeatSaberPath.
@Thynix
Copy link
Author

Thynix commented Sep 7, 2019

We talked about this and decided it made more sense for this functionality to be a separate plugin. Thanks for reviewing!

@Thynix Thynix closed this Sep 7, 2019
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.

None yet

1 participant