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

Added soundpack command (used by the new soundpack) #276

Closed
wants to merge 7 commits into from
Closed

Added soundpack command (used by the new soundpack) #276

wants to merge 7 commits into from

Conversation

JessicaTegner
Copy link
Contributor

This pr adds a soundpack command.

Right now this only contains version (so soundpack version abc) but can be expanded later if we want.

This goes with the new mush soundpack that calls soundpack version on login to check for updates.
It's the server that tells the player if their soundpack is out of date.

@Timtam
Copy link
Collaborator

Timtam commented Apr 3, 2023

Just as a side note, I don't like a soundpack command here. That feels like we're mixing up two independent projects. You'd always need to push and restart (!) the server for a soundpack update announcement, that doesn't feel right in the slightest. You can achieve the same by checking values on the GitHub repository of the soundpack if that is what you want.
BTW, what is it all about with the new soundpack, why not continue to maintain the one that already exists, its public over all: https://github.com/Timtam/yugioh-soundpack/

@JessicaTegner
Copy link
Contributor Author

yep good point as @tspivey also said. Also figured that the soundpack wasn't being updated anymore and haven't heard of anyone seeing you around

@Timtam
Copy link
Collaborator

Timtam commented Apr 3, 2023

Me being busy and not playing ygo right now doesn't mean that I don't maintain my projects over here on GitHub, which is probably a thing for many open source devs out there. Just propose a PR and i'll gladly accept it if its useful. I also showed up recently and fixed some of the public decks, will hopefully continue to do so sooner or later again.

@JessicaTegner
Copy link
Contributor Author

I would have nothing against sending prs over. The same argument I had for TSP I'll tell you. I'm trying to streamline and automate as many things about the mud as possible , since the current devs (you and tsp) aren't really around (and in tsps case, doesn't really like the game).

I'm simply trying to make everyones life easier

@Timtam
Copy link
Collaborator

Timtam commented Apr 3, 2023

Thats fine, I don't see anyone arguing against that. Thats exactly why I said that contributing to the current soundpack is so much more efficient, it saves your time, instead of doing everything from scratch you can just build on top of what is already there, and GitHub as a network allows us to stay connected.

@tspivey
Copy link
Owner

tspivey commented Apr 3, 2023

Once we get a soundpack updater in place, #272 should be as easy as a merge into both soundpack and game, then ask players to update.

@Timtam
Copy link
Collaborator

Timtam commented Apr 3, 2023

I'm still against moving part of the update process of a project into another project, namely the soundpack into the game itself. Why not build a soundpack-only solution? I've done something like similar already and it works totally fine. You can gather some inspiration from it here: https://github.com/Timtam/Avalon/blob/master/worlds/plugins/avalon/updater.xml
That'd be much cleaner and future proof, as we don't need to update two independent projects in the case that something, whatever it may be, changes in the future.

@JessicaTegner
Copy link
Contributor Author

the issue with that would be that you can't override files while they are in use. That's why it's splitted up into a separate process

@Timtam
Copy link
Collaborator

Timtam commented Apr 3, 2023

Yeah, thats not the issue that troubles me here. You already got a bat in place which downloads and replaces the soundpack don't you? That can just stay as it is, I just don't like the soundpack version check and download on the server side.

@JessicaTegner JessicaTegner closed this by deleting the head repository Mar 16, 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.

3 participants