Teleportation API

[Owen1212055]

This expands the teleport API to allow for teleporting entities while keeping passengers and allowing to teleport the player while keeping the player mounted.

⭐ Teleporting player while keeping passengers and dismounting
https://user-images.githubusercontent.com/23108066/132114747-25568ff0-7f46-4186-84ce-064903781465.mp4

The idea of this PR is to essentially allow

1. Keeping passengers when teleporting entities

2. Allowing the entity being teleported to stay mounted when teleported

• This causes the player to teleport for a moment, but because they are still mounted it snaps them back. However because of the new packet we can now choose if we should keep the player mounted or dismount them. I am not sure if we should return false here OR allow this. See: https://bugs.mojang.com/browse/MC-202202

Behavior (keepPassengers, dismount):

• Teleporting entity (false, true) DEFAULT BEHAVIOR: If the entity has passengers it will return false (legacy behavior), else it will kick them off their vehicle.
• Teleporting entity (false, false): If the entity has passengers it will return false, but it will not kick them off the vehicle
• Teleporting entity (true, false): This will teleport the entity and most likely cause it to be teleported back next tick because it has a vehicle attached. (Passengers and vehicle will not be affected)
• Teleporting entity (true, true) This will teleport the entity and all passengers ontop with them. The entity teleported will be dismounted

So basically it will return false if keepPassengers is false and they have passengers

Tested with players and entities, seems to work just fine. Feedback appreciated!

[Owen1212055]

Added new API for support relative teleportation + look at api.
This has special behavior on players, where you can prevent resetting the players velocity while still forcing the player to be moved.

(Code for handling the position packet flags on client)

 boolean hasXFlag = packet.getFlags().contains(PlayerPositionLookS2CPacket.Flag.X);
      boolean hasYFlag = packet.getFlags().contains(PlayerPositionLookS2CPacket.Flag.Y);
      boolean hasZFlag = packet.getFlags().contains(PlayerPositionLookS2CPacket.Flag.Z);
      double newXVelocity;
      double newXCoord;
      if (hasXFlag) {
         newXVelocity = vec3d.getX();
         newXCoord = playerEntity.getX() + packet.getX();
         playerEntity.lastRenderX += packet.getX();
      } else {
         newXVelocity = 0.0D;
         newXCoord = packet.getX();
         playerEntity.lastRenderX = newXCoord;
      }

      double newYVelocity;
      double newYCoord;
      if (hasYFlag) {
         newYVelocity = vec3d.getY();
         newYCoord = playerEntity.getY() + packet.getY();
         playerEntity.lastRenderY += packet.getY();
      } else {
         newYVelocity = 0.0D;
         newYCoord = packet.getY();
         playerEntity.lastRenderY = newYCoord;
      }

      double newZVelocity;
      double newZCoord;
      if (hasZFlag) {
         newZVelocity = vec3d.getZ();
         newZCoord = playerEntity.getZ() + packet.getZ();
         playerEntity.lastRenderZ += packet.getZ();
      } else {
         newZVelocity = 0.0D;
         newZCoord = packet.getZ();
         playerEntity.lastRenderZ = newZCoord;
      }

This allows you to change a specific coordinate without causing the entire player's velocity to be reset. For example, setting the player's yaw/pitch while allowing the player to move freely.
https://user-images.githubusercontent.com/23108066/132130745-f8d22752-c8b2-42ed-9f47-96f492901f52.mp4

Unsure if this should be in the same patch, but this also includes look at api for players.
This exposes a new lookat packet (used in /teleport command), which causes the player to look at a location or entity.
https://user-images.githubusercontent.com/23108066/132131985-5a92ef5e-4bb2-4259-b779-37fad56bafae.mp4

[lynxplay]

If the concept of relative teleportation flags is introduced, should those also be mirrored (be modified by) the PlayerTeleportEvent ?

[Owen1212055]

If the concept of relative teleportation flags is introduced, should those also be mirrored (be modified by) the PlayerTeleportEvent ?

Looking into this a bit more, this would require a bit more changes than I would like here. As alot of methods just hide these parameters (see this one)

Additionally, it seems in some places the relative flags are removed completely. So, opinions would be appreciated.

[lynxplay]

So, opinions would be appreciated.

I agree, I don't think they are a 100% necessity especially with how incredibly niche the flags are anyway.

At least the usage of these should be documented better in terms of javadocs though. Reading "Coordinates of the location that should be relative instead of absolute" does not, at least to me, convey the fact that the following:

var finalLocation = new Location(player.getWorld(), 0, 1, 0, 0, 0);
player.teleport(finalLocation, PlayerTeleportEvent.TeleportCause.PLUGIN, false, false, RelativeTeleportFlag.values());

does still teleport the player to [0,1,0] and only has a client side effect in regards to cancelled motion. While event additions might not be required due to how few teleportations will actually use these (and want others to mutate them), documentation then becomes just the more important.

[Owen1212055]

So, opinions would be appreciated.

I agree, I don't think they are a 100% necessity especially with how incredibly niche the flags are anyway.

At least the usage of these should be documented better in terms of javadocs though. Reading "Coordinates of the location that should be relative instead of absolute" does not, at least to me, convey the fact that the following:

var finalLocation = new Location(player.getWorld(), 0, 1, 0, 0, 0);
player.teleport(finalLocation, PlayerTeleportEvent.TeleportCause.PLUGIN, false, false, RelativeTeleportFlag.values());

does still teleport the player to [0,1,0] and only has a client side effect in regards to cancelled motion. While event additions might not be required due to how few teleportations will actually use these (and want others to mutate them), documentation then becomes just the more important.

Ahhh, so perhaps I did not actually describe it well enough in my PR. This does cause the player to be moved relatively 0 1 0. The behavior can be seen by using the teleport command and using /teleport @p ~ ~1 ~

Perhaps then a new type of teleport like RelativeTeleportEvent could be added which includes these flags? This way we could prevent breaking any API by simply making it so the getLocation will be made absolute and then we can add a method to get the actual relative location and flags.

[lynxplay]

Ahhh, so perhaps I did not actually describe it well enough in my PR. This does cause the player to be moved relatively 0 1 0. The behavior can be seen by using the teleport command and using /teleport @p ~ ~1 ~

Did you actually test this behaviour then ? Looking at the internalTeleport it seems to expect the final location not the relative. The modifications based on the flags are only applied to the packet send, the actual location is based directly on the location passed in d0, d1,d2:

this.awaitingTeleportTime = this.tickCount;
this.player.moveTo(d0, d1, d2, f, f1); // Paper - use proper setPositionRotation for teleportation
this.player.forceCheckHighPriority(); // Paper
this.player.connection.send(new ClientboundPlayerPositionPacket(d0 - d3, d1 - d4, d2 - d5, f - f2, f1 - f3, set, this.awaitingTeleport, flag));

Your method still just passed through the then "relative" or partially "relative" location, resulting in the behaviour I described above.
Could you please validate that this isn't just something I somehow screwed up when checking out your PR?

Example listener showing the issue, teleporting the player to [0,0,0] instead of keeping them at their current position.

@EventHandler
public void on(final PlayerDropItemEvent event) {
    var relativeMotion = new Location(event.getPlayer().getWorld(), 0, 0, 0);
    event.getPlayer().teleport(relativeMotion, PlayerTeleportEvent.TeleportCause.PLUGIN, true, false, RelativeTeleportFlag.values());
}

[Owen1212055]

Ahhh, so perhaps I did not actually describe it well enough in my PR. This does cause the player to be moved relatively 0 1 0. The behavior can be seen by using the teleport command and using /teleport @p ~ ~1 ~

Did you actually test this behaviour then ? Looking at the internalTeleport it seems to expect the final location not the relative. The modifications based on the flags are only applied to the packet send, the actual location is based directly on the location passed in d0, d1,d2:

this.awaitingTeleportTime = this.tickCount;
this.player.moveTo(d0, d1, d2, f, f1); // Paper - use proper setPositionRotation for teleportation
this.player.forceCheckHighPriority(); // Paper
this.player.connection.send(new ClientboundPlayerPositionPacket(d0 - d3, d1 - d4, d2 - d5, f - f2, f1 - f3, set, this.awaitingTeleport, flag));

Your method still just passed through the then "relative" or partially "relative" location, resulting in the behaviour I described above.
Could you please validate that this isn't just something I somehow screwed up when checking out your PR?

Example listener showing the issue, teleporting the player to [0,0,0] instead of keeping them at their current position.

@EventHandler
public void on(final PlayerDropItemEvent event) {
    var relativeMotion = new Location(event.getPlayer().getWorld(), 0, 0, 0);
    event.getPlayer().teleport(relativeMotion, PlayerTeleportEvent.TeleportCause.PLUGIN, true, false, RelativeTeleportFlag.values());
}

You are totally correct here. I think I did some really botched testing, so I appreciate you double-checking me here.
Let me improve this documentation to make this a bit more clear.

Again, thank you very much.

[Owen1212055]

I tried changing the documentation, do you have any feedback?

[lynxplay]

I tried changing the documentation, do you have any feedback?

I like it 👍 could you reflect the additional information in the parameter description ?
E.g.:

-+ * @param teleportFlags Coordinates of the location that should be relative instead of absolute
++ * @param teleportFlags Coordinates of the location that the client should handle as relative teleportation.

Just to ensure no one actually reads it and believes the flags cause any form of mutation to the final location. Your addition already covered it but half of the people barely read the javadocs, let alone something after the first <p> tag 😅

[Owen1212055]

Fair, thank you for all ur feedback lynx. I do appreciate it.

[Owen1212055]

Rebased and resolved. Changed keep passengers to ignore passengers, added throw javadoc and added stuff as default interface methods.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Owen1212055]

Shoo

[Owen1212055]

rebased

[amaranth]

Is there a reason you can't teleport with a vehicle or passenger to another world? If there is no technical reason stopping this the extra work might be worth considering to make the API simpler to use. If that can't be done I think the API should at least just return false in this case instead of throwing an exception.

[Owen1212055]

Is there a reason you can't teleport with a vehicle or passenger to another world? If there is no technical reason stopping this the extra work might be worth considering to make the API simpler to use. If that can't be done I think the API should at least just return false in this case instead of throwing an exception.

I’ll have to play around, I didn’t try messing with that too much as generally looking at how other people did cross world teleportation it seemed to be quite a large task. So, ill see if it works nicely or not.

[Owen1212055]

Is there a reason you can't teleport with a vehicle or passenger to another world? If there is no technical reason stopping this the extra work might be worth considering to make the API simpler to use. If that can't be done I think the API should at least just return false in this case instead of throwing an exception.

The Minecraft method itself forcibly removes passengers.. and judging how there is no logic for handling them either I don't think doing this is very safe.

[Owen1212055]

Rebased

[Owen1212055]

Rebase

[Owen1212055]

Rebased, all methods and classes have been marked with @org.jetbrains.annotations.ApiStatus.Experimental

stale

[Owen1212055]

Rebased....

[Owen1212055]

Rebased!