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.

Sign up for GitHub

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

Fix inconsistencies between offline/online spawn position getter #11960

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+56 −35
Open

Fix inconsistencies between offline/online spawn position getter #11960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions paper-api/src/main/java/org/bukkit/OfflinePlayer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ public default BanEntry banPlayer(@Nullable String reason, @Nullable java.util.D
@Nullable
@Deprecated(since = "1.20.4")
public Location getBedSpawnLocation();

// Paper start
/**
* Gets the last date and time that this player logged into the server.
Expand All @@ -290,13 +291,28 @@ public default BanEntry banPlayer(@Nullable String reason, @Nullable java.util.D
// Paper end

/**
* Gets the Location where the player will spawn at, null if they
* Gets the Location where the player will spawn at, {@code null} if they
* don't have a valid respawn point.
* <br>
* Unlike online players, the location if found will not be loaded by default.
*
* @return respawn location if exists, otherwise {@code null}.
*/
@Nullable
default Location getRespawnLocation() {
return this.getRespawnLocation(false); // keep old behavior for offline players
}
Comment on lines +302 to +304
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on deprecating this method (without overload) altogether?
Player extends OfflinePlayer, and both have different default chunk loading behavior, which is quite confusing (also would be nice to specify the default behavior in respective javadocs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the javadoc, not sure for the deprecation, i kinda expect offline player data to be less accurate than online by default. What i think is confusing is the fact that an online player extends an offline player in the api.

Copy link
Contributor

Choose a reason for hiding this comment

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

What i think is confusing is the fact that an online player extends an offline player in the api.

Yes, that was my concern. Technically, you can have OfflinePlayer object representing an online Player instance (#getOfflinePlayer too returns online player if present), so you may call the method on an offline player expecting not to load chunks, but they will load due to the object being an online player.

New devs might stumble upon this quirk and will have to account for it. Deprecation would require to explicitly specify the wanted behavior, but I don't have a strong feeling against leaving this as is.


/**
* Gets the Location where the player will spawn at, {@code null} if they
* don't have a valid respawn point.
*
* @return respawn location if exists, otherwise null.
* @param load load the current location to retrieve the exact position of the spawn block
* and check if this position is still valid or not
* @return respawn location if exists, otherwise {@code null}.
*/
@Nullable
public Location getRespawnLocation();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering, what really is the rule for interfaces and explicit public modifiers? I have seen files/PRs with and I have seen files/PRs without.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, it was an unnecessary rule enforced by spigot.

Location getRespawnLocation(boolean load);

/**
* Increments the given statistic for this player.
Expand Down
6 changes: 4 additions & 2 deletions paper-api/src/main/java/org/bukkit/entity/HumanEntity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,9 +469,11 @@ default Location getPotentialBedLocation() {
* to validate if the current respawn location is still valid.
*
* @return respawn location if exists, otherwise null.
* @deprecated this method doesn't take in account the respawn angle, use
* {@link Player#getRespawnLocation(boolean)} with load = false instead
*/
@Nullable
Location getPotentialRespawnLocation();
@Deprecated(since = "1.21.4")
@Nullable Location getPotentialRespawnLocation();
// Paper end
// Paper start
/**
Expand Down
10 changes: 3 additions & 7 deletions paper-api/src/main/java/org/bukkit/entity/Player.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,15 +560,11 @@ public interface Player extends HumanEntity, Conversable, OfflinePlayer, PluginM
@Deprecated(since = "1.20.4")
public Location getBedSpawnLocation();

/**
* Gets the Location where the player will spawn at, null if they
* don't have a valid respawn point.
*
* @return respawn location if exists, otherwise null.
*/
Comment on lines -563 to -568
Copy link
Contributor

Choose a reason for hiding this comment

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

The Javadocs have been removed on this method. Is this intentional, or are you going to be adding different Javadocs soon?

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc it should now inherit from offline player?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think JDs are inherited by default?

Copy link
Member

Choose a reason for hiding this comment

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

oh, no, nvm, they are, ineritDoc is for additive

@Nullable
@Override
public Location getRespawnLocation();
default Location getRespawnLocation() {
return this.getRespawnLocation(true);
}

/**
* Sets the Location where the player will spawn at their bed.
Expand Down
31 changes: 21 additions & 10 deletions paper-server/src/main/java/org/bukkit/craftbukkit/CraftOfflinePlayer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import net.minecraft.nbt.CompoundTag;
import net.minecraft.nbt.ListTag;
import net.minecraft.nbt.NbtOps;
import net.minecraft.nbt.Tag;
import net.minecraft.server.level.ServerPlayer;
import net.minecraft.server.players.UserWhiteListEntry;
import net.minecraft.stats.ServerStatsCounter;
import net.minecraft.world.level.storage.PlayerDataStorage;
Expand All @@ -28,12 +30,11 @@
import org.bukkit.configuration.serialization.ConfigurationSerializable;
import org.bukkit.configuration.serialization.SerializableAs;
import org.bukkit.craftbukkit.entity.memory.CraftMemoryMapper;
import org.bukkit.craftbukkit.profile.CraftPlayerProfile;
import org.bukkit.craftbukkit.util.CraftLocation;
import org.bukkit.entity.EntityType;
import org.bukkit.entity.Player;
import org.bukkit.metadata.MetadataValue;
import org.bukkit.plugin.Plugin;
import org.bukkit.profile.PlayerProfile;

@SerializableAs("Player")
public class CraftOfflinePlayer implements OfflinePlayer, ConfigurationSerializable {
Expand Down Expand Up @@ -392,25 +393,35 @@ public Location getBedSpawnLocation() {
}

@Override
public Location getRespawnLocation() {
public Location getRespawnLocation(boolean load) {
CompoundTag data = this.getData();
if (data == null) return null;

if (data.contains("SpawnX") && data.contains("SpawnY") && data.contains("SpawnZ")) {
// Paper start - fix wrong world
final float respawnAngle = data.getFloat("SpawnAngle");
org.bukkit.World spawnWorld = this.server.getWorld(data.getString("SpawnWorld")); // legacy
if (data.contains("SpawnX", Tag.TAG_ANY_NUMERIC) && data.contains("SpawnY", Tag.TAG_ANY_NUMERIC) && data.contains("SpawnZ", Tag.TAG_ANY_NUMERIC)) {
org.bukkit.World spawnWorld = null;
if (data.contains("SpawnDimension")) {
com.mojang.serialization.DataResult<net.minecraft.resources.ResourceKey<net.minecraft.world.level.Level>> result = net.minecraft.world.level.Level.RESOURCE_KEY_CODEC.parse(net.minecraft.nbt.NbtOps.INSTANCE, data.get("SpawnDimension"));
net.minecraft.resources.ResourceKey<net.minecraft.world.level.Level> levelKey = result.resultOrPartial(LOGGER::error).orElse(net.minecraft.world.level.Level.OVERWORLD);
net.minecraft.server.level.ServerLevel level = this.server.console.getLevel(levelKey);
spawnWorld = level != null ? level.getWorld() : spawnWorld;
spawnWorld = level != null ? level.getWorld() : null;
}
if (spawnWorld == null && data.contains("SpawnWorld")) { // legacy
spawnWorld = this.server.getWorld(data.getString("SpawnWorld"));
}
if (spawnWorld == null) {
return null;
}
return new Location(spawnWorld, data.getInt("SpawnX"), data.getInt("SpawnY"), data.getInt("SpawnZ"), respawnAngle, 0);
// Paper end

final float respawnAngle = data.getFloat("SpawnAngle");
net.minecraft.core.BlockPos respawnPos = new net.minecraft.core.BlockPos(data.getInt("SpawnX"), data.getInt("SpawnY"), data.getInt("SpawnZ"));
if (!load) {
return CraftLocation.toBukkit(respawnPos, spawnWorld, respawnAngle, 0);
}

final org.bukkit.World finalSpawnWorld = spawnWorld;
return ServerPlayer.findRespawnAndUseSpawnBlock(((CraftWorld) spawnWorld).getHandle(), respawnPos, respawnAngle, data.getBoolean("SpawnForced"), false)
.map(resolvedPos -> CraftLocation.toBukkit(resolvedPos.position(), finalSpawnWorld, resolvedPos.yaw(), 0))
.orElse(null);
}
return null;
}
Expand Down
17 changes: 7 additions & 10 deletions paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@
import org.bukkit.craftbukkit.map.RenderData;
import org.bukkit.craftbukkit.potion.CraftPotionEffectType;
import org.bukkit.craftbukkit.potion.CraftPotionUtil;
import org.bukkit.craftbukkit.profile.CraftPlayerProfile;
import org.bukkit.craftbukkit.scoreboard.CraftScoreboard;
import org.bukkit.craftbukkit.util.CraftChatMessage;
import org.bukkit.craftbukkit.util.CraftLocation;
Expand All @@ -181,7 +180,6 @@
import org.bukkit.event.player.PlayerHideEntityEvent;
import org.bukkit.event.player.PlayerRegisterChannelEvent;
import org.bukkit.event.player.PlayerShowEntityEvent;
import org.bukkit.event.player.PlayerSpawnChangeEvent;
import org.bukkit.event.player.PlayerTeleportEvent;
import org.bukkit.event.player.PlayerUnregisterChannelEvent;
import org.bukkit.inventory.EquipmentSlot;
Expand All @@ -194,7 +192,6 @@
import org.bukkit.plugin.messaging.StandardMessenger;
import org.bukkit.potion.PotionEffect;
import org.bukkit.potion.PotionEffectType;
import org.bukkit.profile.PlayerProfile;
import org.bukkit.scoreboard.Scoreboard;
import org.jetbrains.annotations.NotNull;

Expand Down Expand Up @@ -1556,16 +1553,16 @@ public Location getBedSpawnLocation() {
}

@Override
public Location getRespawnLocation() {
public Location getRespawnLocation(boolean load) {
ServerLevel world = this.getHandle().server.getLevel(this.getHandle().getRespawnDimension());
BlockPos bed = this.getHandle().getRespawnPosition();
BlockPos respawnPos = this.getHandle().getRespawnPosition();

if (world != null && bed != null) {
Optional<ServerPlayer.RespawnPosAngle> spawnLoc = ServerPlayer.findRespawnAndUseSpawnBlock(world, bed, this.getHandle().getRespawnAngle(), this.getHandle().isRespawnForced(), true);
if (spawnLoc.isPresent()) {
ServerPlayer.RespawnPosAngle vec = spawnLoc.get();
return CraftLocation.toBukkit(vec.position(), world.getWorld(), vec.yaw(), 0);
if (world != null && respawnPos != null) {
if (!load) {
return CraftLocation.toBukkit(respawnPos, world.getWorld(), this.getHandle().getRespawnAngle(), 0);
}
return ServerPlayer.findRespawnAndUseSpawnBlock(world, respawnPos, this.getHandle().getRespawnAngle(), this.getHandle().isRespawnForced(), false)
.map(resolvedPos -> CraftLocation.toBukkit(resolvedPos.position(), world.getWorld(), resolvedPos.yaw(), 0)).orElse(null);
}
return null;
}
Expand Down
5 changes: 2 additions & 3 deletions paper-server/src/main/java/org/bukkit/craftbukkit/util/CraftLocation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public static Location toBukkit(BlockPos blockPosition) {
public static Location toBukkit(BlockPos blockPosition, net.minecraft.world.level.Level world) {
return CraftLocation.toBukkit(blockPosition, world.getWorld(), 0.0F, 0.0F);
}

public static Location toBukkit(BlockPos blockPosition, World world) {
return CraftLocation.toBukkit(blockPosition, world, 0.0F, 0.0F);
}
Expand All @@ -40,16 +41,14 @@ public static BlockPos toBlockPosition(Location location) {
return new BlockPos(location.getBlockX(), location.getBlockY(), location.getBlockZ());
}

// Paper start
public static net.minecraft.core.GlobalPos toGlobalPos(Location location) {
return net.minecraft.core.GlobalPos.of(((org.bukkit.craftbukkit.CraftWorld) location.getWorld()).getHandle().dimension(), toBlockPosition(location));
}

public static Location fromGlobalPos(net.minecraft.core.GlobalPos globalPos) {
BlockPos pos = globalPos.pos();
return new org.bukkit.Location(net.minecraft.server.MinecraftServer.getServer().getLevel(globalPos.dimension()).getWorld(), pos.getX(), pos.getY(), pos.getZ());
return new Location(net.minecraft.server.MinecraftServer.getServer().getLevel(globalPos.dimension()).getWorld(), pos.getX(), pos.getY(), pos.getZ());
}
// Paper end

public static Vec3 toVec3D(Location location) {
return new Vec3(location.getX(), location.getY(), location.getZ());
Expand Down
Loading