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

Rewrite ssc #1982

Draft
wants to merge 5 commits into
base: general-devel
Choose a base branch
from
Draft

Rewrite ssc #1982

wants to merge 5 commits into from

Conversation

QuiCM
Copy link
Member

@QuiCM QuiCM commented Jun 3, 2020

This is wildly untested and smashed out in a night.
Some hardcore review (and testing if possible!) would be appreciated.

Pulled as much of the SSC subsystem out of the various handlers and methods it was in as I could and turned it into a first class system. It now hooks things on its own and is hopefully a lot cleaner to look at

@maka-albarn
Copy link

maka-albarn bot commented Jun 3, 2020

Thanks for the pull request! TShock's maintainers would like you to go ahead and give yourself credit by updating the CHANGELOG.md as soon as you can. Your pull request will likely not be accepted without this. This both helps us document changes to TShock, as well as give you credit for your work. You deserve it, so go take credit! ✨

@QuiCM
Copy link
Member Author

QuiCM commented Jun 3, 2020

No idea if overridessc and uploadssc commands still work.
No idea how stable anything is.
No idea if there's other login/logout/account/etc use cases I haven't considered yet
Haven't tested that it works with an aged SSC database, but not much has changed in terms of data in/out

/// <summary>
/// Provides and executes CRUD operations on server side player data
/// </summary>
public class ServerSideCoordinator
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about this name or this class.
Feels like its trying to do too much at once (hence an ambiguous name like 'Coordinator'), so suggestions on design here would be appreciated

@QuiCM QuiCM marked this pull request as ready for review June 3, 2020 15:05
@rickojp
Copy link
Contributor

rickojp commented Jun 10, 2020

No idea if overridessc and uploadssc commands still work.
No idea how stable anything is.
No idea if there's other login/logout/account/etc use cases I haven't considered yet
Haven't tested that it works with an aged SSC database, but not much has changed in terms of data in/out

Some test results:

-Overridessc and uploadssc commands do not work (Log: Command: ERROR: System.NullReferenceException)
-Logout account does not ignore the player the same way as when the player enters the server the first time
-I used a clean database created by that branch and in another test an existing database and apparently there were no problems
-Superadmin accounts or groups with permission 'tshock.ignore.ssc', usually ignore SSC inventory, but some errors occur.
As I don't usually use this function, I don't know if in the old way it is intentional to save the current items in the database. So one must treat or add the check if the player has permission in SaveAllPlayersData, OnPlayerLogout, OnPlayerHpChange, OnPlayerManaChange

Thank for this work!

@hakusaro hakusaro marked this pull request as draft October 18, 2022 01:09
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