Fix inconsistencies between offline/online spawn position getter #11960
Conversation
| */ | ||
| @Nullable | ||
| public Location getRespawnLocation(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As far as I know, it was an unnecessary rule enforced by spigot.
Warriorrrr
added
type: bug
| /** | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
The Javadocs have been removed on this method. Is this intentional, or are you going to be adding different Javadocs soon?
There was a problem hiding this comment.
iirc it should now inherit from offline player?
There was a problem hiding this comment.
I don't think JDs are inherited by default?
There was a problem hiding this comment.
oh, no, nvm, they are, ineritDoc is for additive
Lulu13022002
force-pushed
the
fix/off-loc
branch
from
c02f0ef to
d7b2c43
Compare
|
This also fix Player#getRespawnLocation that consume the respawn anchor in the nether if binded |
| default Location getRespawnLocation() { | ||
| return this.getRespawnLocation(false); // keep old behavior for offline players | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Lulu13022002
force-pushed
the
fix/off-loc
branch
from
333c255 to
bce6c7c
Compare
Original: #8243
Also deprecated the old HumanEntity#getPotentialRespawnLocation since it doesn't track the respawn angle.