Paper icon indicating copy to clipboard operation
Paper copied to clipboard

`PlayerTeleportEvent` not called if player has passengers

Open OakLoaf opened this issue 1 year ago • 9 comments

Is your feature request related to a problem?

If a plugin adds an entity as a passenger to a player and then a plugin tries to run Player#teleport (with no TeleportFlags) the player is not teleported and the PlayerTeleportEvent is not called.

The context of my issue is that I have chat bubbles which use Text Display packets to show player's messages above their heads - however due to the issue they are unable to teleport until the chat bubbles disappear automatically.

Describe the solution you'd like.

The PlayerTeleportEvent could be defined before the Player#isVehicle check is ran, in the case that the player has passengers then it would set the event as cancelled prior to calling the event. This would then allow plugins to remove their entities as passengers if they wanted and then set the event as not cancelled

I am fully open to alternate solutions

Describe alternatives you've considered.

Alternatively some form of "PlayerPreTeleportEvent" could allow plugins to remove their entities prior to teleportation, however I feel like this would be a bit over the top

Other

No response

OakLoaf avatar Jan 14 '24 23:01 OakLoaf

~~Why is it not possible to teleport a player that has a passenger in the first place? I mean the only thing I can think of is because of how the passenger logic works but there has to be a better solution to this or not?~~ Nevermind. I only checked the TeleportCause enum for valid flags. There is infact a RETAIN_PASSENGERS flag that covers your case.

Leguan16 avatar Jan 16 '24 16:01 Leguan16

The intended solution for your case is to just use the TeleportFlag.EntityState.RETAIN_PASSENGERS flag. Why don't you want to use the specific flag for this case? Another solution to your suggestion would be to add new flags called DISCARD_PASSENGERS, DISMOUNT_PASSENGERS etc. to teleport the player and either dismount or remove the mounted entities in general.

Leguan16 avatar Jan 16 '24 16:01 Leguan16

The intended solution for your case is to just use the TeleportFlag.EntityState.RETAIN_PASSENGERS flag. Why don't you want to use the specific flag for this case? Another solution to your suggestion would be to add new flags called DISCARD_PASSENGERS, DISMOUNT_PASSENGERS etc. to teleport the player and either dismount or remove the mounted entities in general.

The issue is that I have no way of guaranteeing that every other plugin is going to add the RETAIN_PASSENGERS flag.

I personally do not teleport the player at all in my chat bubbles plugin, but the existence of those text displays as passengers means that the majority of other plugins will fail to teleport the player. For my use case it would be as easy as removing the entities entirely, but there is no way of me knowing when that needs doing to allow the player to successfully teleport.

OakLoaf avatar Jan 16 '24 17:01 OakLoaf

Yeah that's what I realised later as well. I think a PlayerPreTeleportEvent can be useful for such cases. The next question would be what methods we expose in that event and do we already allow modification of e.g. locations. Also does it make sense to extend PlayerMoveEvent as it's done with the normal PlayerTeleportEvent?

Leguan16 avatar Jan 17 '24 10:01 Leguan16

Time to remember https://github.com/PaperMC/Paper/issues/1285...

molor avatar Jan 17 '24 10:01 molor

PME assumes a movement is occuring, not that a movement will occur at some point in the future, so, generally, no, it should not, as otherwise plugins would be processing effectively the same general intent twice

Having a pre event was always somewhat of a consideration, a hindered event as was proposed elsewhere gets a bit weird when all of the existing API around this stuff just returns a boolean with no real pre-existing causes. This was something that was on my todo list elsewhere, I think I started something with worlds but was too sick to finish

electronicboy avatar Jan 17 '24 11:01 electronicboy

A pre event would definitely work, presumably the content of the event would be mostly identical to that of the PlayerTeleportEvent?

The next question would be what methods we expose in that event and do we already allow modification of e.g. locations.

I think modifying locations here would be fine

OakLoaf avatar Jan 17 '24 11:01 OakLoaf

This was something that was on my todo list elsewhere

@electronicboy are you still planning to do it or can I work on it?

Leguan16 avatar Jan 17 '24 13:01 Leguan16

it would generally be better if somebody else worked on it, my todo list is generally swamped right now

electronicboy avatar Jan 17 '24 13:01 electronicboy