Repository navigation
Conversation
benwoo1110
left a comment
benwoo1110
left a comment
There was a problem hiding this comment.
Nice! I think it's a good start, but I will have to request some improvements before this can be merged.
-
I would prefer the logic for spawn radius to be a separate class, most likely a separate class within the
teleportationpackage and maybe have some integration with BlockSafety and LocationManipulation (since we need to ensure the random spawn lookup location is safe for teleport). I'm open to other ideas, but definitely having all the logic just in MVPlayerListener is a no-go for me. -
This may be a behavioural change that some servers do not want. So applying spawn radius on respawn needs to be a config option that defaults to false for all existing installs (via config migration). Im ok with it being true for new installs.
…adius config option
|
Thanks for the review! I've pushed an update:
I also made it skip worlds without sky light (e.g. the nether and the end), same as vanilla, and added tests for the config migration and the respawn behavior. |
| .comment("When this option is enabled and players respawn at the world's spawn location,") | ||
| .comment("they will be spread to a random safe location around it based on the `respawn_radius` game rule") | ||
| .comment("(`spawnRadius` on older versions), mimicking the vanilla behavior.") | ||
| .comment("Same as vanilla, this does not apply if the respawn radius is 0 or the world's game mode is adventure.") |
There was a problem hiding this comment.
There is also something about spawn radius not applicable for worlds without sky light right?
There was a problem hiding this comment.
Yes, good catch. Updated the comment to mention that it also doesn't apply in worlds without sky light (e.g. the nether and the end).
| .comment("----") | ||
| .comment("Set this to false if you want players to always respawn exactly at the world's spawn location.") | ||
| .defaultValue(true) | ||
| .name("use-respawn-radius") |
There was a problem hiding this comment.
Wondering if apply-respawn-radius fits better?
There was a problem hiding this comment.
Agreed, renamed it to spawn.apply-respawn-radius (and the getter/setter to get/setApplyRespawnRadius).
| * @return A safe location within the respawn radius of the spawn, or the spawn location itself if none is found. | ||
| */ | ||
| @ApiStatus.AvailableSince("5.9") | ||
| public @NotNull Location getLocationWithinSpawnRadius(@NotNull Location spawnLocation, @NotNull GameMode gameMode) { |
There was a problem hiding this comment.
I think it makes more sense to pass in a LoadedMultiverseWorld instead of the 2 parameters separately?
Maybe as an extension we can have a method that only takes in location and does the spawn location calculation without any prechecks of gamemode and sky light?
There was a problem hiding this comment.
Changed it to take a LoadedMultiverseWorld, which checks the game mode and sky light before spreading. I also added an overload that takes just a Location and does the spawn radius calculation without those checks (it still returns the location as is when the radius is 0), plus a test for it.
✅
|
Fixes #2821
When Multiverse overrides the respawn location with the world spawn, players always land exactly on the spawn block, so the
respawn_radius(spawnRadiuson older versions) game rule has no effect. This spreads the respawn location around the world spawn in that case, similar to vanilla:The game rule is looked up with
GameRule.getByName, tryingrespawn_radiusfirst and thenspawnRadius, so older servers keep working.Tested on Paper 26.2: with
respawn_radius10, eight respawns all landed at different spots within 10 blocks of the spawn; with 0, every respawn was at the spawn location. Also added tests inMVPlayerListenerTest.