Skip to content

Conversation

@hufang360
Copy link
Contributor

Fixed SSC not save ateArtisanBread, usedAegisCrystal, usedAegisFruit, usedArcaneCrystal, usedGalaxyPearl, usedGummyWorm, usedAmbrosia, unlockedSuperCart, enabledSuperCart flags, and Server will correct read them.

…ruit`, `usedArcaneCrystal`, `usedGalaxyPearl`, `usedGummyWorm`, `usedAmbrosia`, `unlockedSuperCart`, `enabledSuperCart` flags, and Server will correct read them.
@punchready
Copy link
Contributor

LGTM, as far as i can see this is functionally correct

@hufang360
Copy link
Contributor Author

Fixed Server not tracks deaths, Now players can be viewed with the /death, /pvpdeath, /alldeath and /allpvpdeath commands.

@hakusaro
Copy link
Member

@hufang360 in the future, please try to submit a PR for only one thing at a time. We can't merge this branch directly now because there's an approval for what @punchready reviewed but not a PR for the subsequent changes, which are unrelated.

@hufang360
Copy link
Contributor Author

@hufang360 in the future, please try to submit a PR for only one thing at a time. We can't merge this branch directly now because there's an approval for what @punchready reviewed but not a PR for the subsequent changes, which are unrelated.

OK. What can i do?
I see the docs/changelog.md occur conflicts, May I resolved it?
Or other something to do?
Thank you.

Copy link
Contributor

@drunderscore drunderscore left a comment

Choose a reason for hiding this comment

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

SSC changes are fine, I've review and tested them and couldn't find anything wrong.

As for the death commands, just testing out the PVE death counter something seems wrong -- if we're going to store deaths into our SSC (the database), then we should be sure to never honor the client's values, and properly respect the ones we have.

image

Same complaint as particles earlier -- if this were two PRs, it would be much easier to merge the SSC changes and wait for the death changes, but now both changes are in limbo :(

/// PlayerSpawn - When a player spawns
/// </summary>
public static HandlerList<SpawnEventArgs> PlayerSpawn = new HandlerList<SpawnEventArgs>();
private static bool OnPlayerSpawn(TSPlayer player, MemoryStream data, byte pid, int spawnX, int spawnY, int respawnTimer, PlayerSpawnContext spawnContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameter names should be lowerCamelCase


if (string.IsNullOrEmpty(tag))
{
args.Player.SendInfoMessage("There are currently no players online.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs i18n


if (string.IsNullOrEmpty(tag))
{
args.Player.SendInfoMessage("There are currently no players online.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@hakusaro
Copy link
Member

@hufang360 this is ready-for-merge; if you don't want to you don't need to resolve conflicts as we can still merge (it's just the changelog).

The other changes that were in this PR can be merged separately, if that's okay with you!

@hufang360
Copy link
Contributor Author

SSC changes are fine, I've review and tested them and couldn't find anything wrong.

As for the death commands, just testing out the PVE death counter something seems wrong -- if we're going to store deaths into our SSC (the database), then we should be sure to never honor the client's values, and properly respect the ones we have.

image

Same complaint as particles earlier -- if this were two PRs, it would be much easier to merge the SSC changes and wait for the death changes, but now both changes are in limbo :(

Thanks. The death counter seems wrong, I rollback the commit, Now it only contain one PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants