-
-
Notifications
You must be signed in to change notification settings - Fork 979
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
Random teleport improvements #4271
base: 2.x
Are you sure you want to change the base?
Conversation
@pop4959 can you resolve conflicts for this whenever you have time |
Updated, would like to see a few more people test this before merging though. Grab a build from actions if you want to try it, and just be sure to backup tpr.yml so you can easily switch back to regular Essentials builds. |
You can enter a value after it for tpr, but you can always get by in the farmworld. |
Not quite sure what you mean here, can you elaborate? |
#4679 |
Can you provide a list of the worlds you have on your server and your tpr.yml file |
Ok i got it. After a few teleoports with / tpr, I suddenly landed in the Void in the Nether. Can that still be fixed? |
…randomteleport-improvements
Hey, just wondering if there are any updates on this? Edit: Sadly can't help test because the build artifact is expired :/ |
@Chris6ix Updated the build for you :) This work is for the most part done, probably just a few minor changes here and there, but otherwise just needs to be tested thoroughly before the 2.20 milestone. |
Alright I downloaded it, and I'm going to test it when I got some spare time (Have Exams rn :/ ) Is there anything specific that needs to be tested or something? Btw, is there an approximate release date for 2.20? |
No problem. Nothing specific, just making sure multi-world teleports, /tpr, /settpr commands, configuration, etc all appear and behave as expected and are intuitive to use, not buggy, etc. The basics have been tested so ideally try it out on a server and see if you find any issues using it. There isn't an ETA For 2.20 currently but you can track progress in the milestone here on Github. |
Okay thank you! |
Hey @pop4959 i found some free time and tested the multi world rtp feature :) I teleported myself to each of the world's spawns using I then teleported myself to each world and made about 20 random teleports in each world, each time checking with Xaeros World Map that the teleports were really random and that it was not picking a location more than once or something. So I think the nether part needs to be adjusted a bit with a max height just under the nether roof for example and a fix for the teleporting into walls. The rest looks fine to me :) Thanks again for working on this great feature! Cheers |
@Chris6ix Hey, thanks for doing such a thorough test! Glad to hear everything's working for you. 😄 Not sure about the teleporting into walls/roof, I don't believe any changes have been made with regards to that. It should be using Essentials safe teleport feature (as all other teleport commands do), so unless you've turned that off random teleports should behave correctly. I sometimes forget to keep this branch up-to-date, but feel free to continue testing it. Just ping me if you need any changes merged in from the current mainstream dev builds. |
Hey @pop4959 thanks for the quick reply! |
Yes it is probably because you were in creative. The teleport is "safe" in creative mode since you do not take damage. Teleportation safety primarily concerns whether or not the player takes damage teleporting to the location. You can test in survival or try setting that option to true if that is your preference. |
Mhm okay I have Edit: This could be a config option if the nether roof should be treated as safe or not |
Okay, that's something we can look into then. The intent is definitely not to teleport on top of the roof. Probably either the location being selected too high, or teleportation safety choosing a spot above the roof (although I believe it's already supposed to do this). |
Want to update that I've been using this PR on my server for more than a few months and it has been working great. |
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.
few things otherwise good!
@@ -153,6 +153,10 @@ public static void uuidFileConvert(final IEssentials ess, final Boolean ignoreUF | |||
ess.getLogger().info("To rerun the conversion type /essentials uuidconvert"); | |||
} | |||
|
|||
public void updateRandomTeleport() { | |||
ess.getRandomTeleport().updateConfig(); |
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.
A couple problems with this upgrade;
- This should only be ran once (by checking/setting the
doneFile
with a unique upgrade key) - The logic for this upgrade should be contained in the
EssentialsUpgrade
class and ideally not call any method inRandomTeleport
as to not create an accidental regression down the road.
randomTeleport.getCachedLocations().clear(); | ||
if ("center".equalsIgnoreCase(args[0])) { | ||
randomTeleport.setCenter(user.getLocation()); | ||
randomTeleport.reloadConfig(); |
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 invokes an I/O operation. Is there a reason why it's ran let alone before any arguments are even validated?
@Deprecated | ||
public UserRandomTeleportEvent(final IUser user, final Location center, final double minRange, final double maxRange) { |
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.
Event constructors aren't API, you can remove this if you're not using it
TODO
|
Supersedes #3538.
Closes #1893, closes #3469, closes #3479.
Various improvements for Essentials' random teleportation feature.
Should be complete as-is, other than potential code clean-up and testing before merging.
Miscellaneous points of interest:
pre-cache
configuration removed (it was hardly in use anyways, and doesn't make any difference after first tp + with multiple locations the number of locations to compute could get very expensive).random-respawn
andrandom-respawn-location
configuration options.randomteleport
to theenabledSigns
list in the config, writing[RandomTeleport]
in the first line on the sign, and optionally the name of the location on the second line.default-location
for default location (where to teleport when typing /tpr with no arguments) that defaults to the current world.per-location-permission
for per location permissions (essentials.tpr.location.<name>
), functioning similarly to how per warp permissions work./settpr
now requires the location name as an argument to set center/ranges for.essentials.tpr.others
and the command/tpr <name> <player>
./tpr
with the name and player both as required arguments.