-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fixes #9
Fixes #9
Conversation
This fixes Civcraft#5, Civcraft#6, Civcraft#8
Can one of the admins verify this patch? Type 'ok to test' to test. |
public void playerRespawnEvent(PlayerRespawnEvent e) { | ||
Player p = e.getPlayer(); | ||
if (!TOSManager.isTermPlayer(p, "CivMenu Agreement")) { | ||
locations.remove(p.getUniqueId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means if someone kills himself, he can walk around freely after he respawns (until he gets kicked). Instead a new location should be inserted here, to which the players is bound (the respawn location).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 68-72 correct that issue, and makes sure it doesn't happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, it would be a bit clearer if it also readded it here, but overall it should work perfectly fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Max it should be added here as well just to make the code more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Readding the location here does not seem to work properly(i found that by copy-pasting your pull manually) by my tests, for some reason. When the location was readded, it kept putting me inside blocks and suffocating me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test it with random spawn installed on the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's not the spawning itself that puts me inside blocks, its the teleport from the move event that does it. I had this right after line 85:
locations.put(p.getUniqueId(), e.getRespawnLocation());
With that added, the teleport when going too far puts you inside the ground. I am not sure why...
ok to test |
Can I even do that? |
I already have #4 finished, should i just commit and pin it onto this pull or just wait until after this pull is merged so its separate? |
make a new branch, pull from civcraft's master into that branch, then do your changes and make a new pull request from that branch. |
Sounds good. |
This should be good to merge. |
ok doing so. |
This fixes #5, #6, #8
I have tested all changes.