Paper: Teleporting player on entity dismount has no effect

[Shevchik]

What behaviour is expected:

Player teleported to expected location

What behaviour is observed:

Player is teleported to vehicle dismount location

Steps/models to reproduce:

Compile and install https://github.com/Shevchik/Chairs, try sitting and dismounting after that.

Plugin list:

https://github.com/Shevchik/Chairs

Paper build number:

git-Paper-51 (MC: 1.14.2)

Anything else:

For whatever reason only player initiated dismount has this problem, dismounting via Entity#leaveVehicle and teleporting work properly.

[BillyGalbreath]

Does this affect Spigot as well? Reason I ask, is I've seen behavior before and I assumed it was because the dismounting fires its own teleport. Basically if you teleport on dismount you just get teleported back again when the event is done, from what I've seen.

[kickash32]

This is because while the event is being processed by listeners, the dismount hasn't happened yet. We could add a post dismount event to fix this. Alternatively you could schedule a teleport to execute on the next tick.

[Shevchik]

Yes, this does affect spigot.
The dismount even needs to provide target teleport location then, or do not schedule it's own teleport if player was already teleported by plugin.

Cancelling the dismount event and dismounting manually still produces the same issue.
(This most likely happens because current server just compares vehicles after the event processing (instead of stopping processing), and if they are different it does it own teleport shit).

Scheduling teleport is not really an option, since it will require quite complex tracking to not actually teleport player already teleported by another plugin.

[BillyGalbreath]

You can listen to the PlayerTeleportEvent and cancel it. That event fires after the EntityDismountEvent starts and before the current tick ends when a player dismounts naturally.

[Shevchik]

No reliable way to detect that.

[kickash32]

You listen to the EntityDismountEvent, and store information about the fact that the player is about to teleport due to dismounting by putting them into a set or something. Then in the PlayerTeleportEvent, check if they exist in the set and if they do, remove them from it, and continue accordingly.

[Shevchik]

Multiple teleport events can be fired due to other possible teleports including plugin initated ones.

[BillyGalbreath]

Plugin initiated teleport events use the TeleportCause.PLUGIN and the dismount event uses the TeleportCause.UNKOWN. There are a hundred ways you can solve this using existing API, which is what plugin devs have been doing for years already to work around these kind of issues. The great thing about this is it's single threaded, meaning the two events will fire right after another for the same user. Nothing else will happen between the two unless you have a plugin doing things its not supposed to do in an async task.

If you want a more proper fix I suggest taking it upstream to CraftBukkit and getting the dismount's teleport event to use a new TeleportCause.DISMOUNT or something. Paper cant change this kind of thing because it would deviate away from upstream too much and break someone else's workflow.

[ninja-]

I guess what we could do api-wise is add event.setTargetLocation - assuming server knows it when the event is fired?
I am against adding Post* events for every use case where people are fine with just scheduler.runNextTick(something)....I think a couple of these Post* were recently added

[BillyGalbreath]

I guess what we could do api-wise is add event.setTargetLocation - assuming server knows it when the event is fired?

This was discussed in Discord, and its not possible because the context on where the two events fire from is too far apart. The dismount event is unaware a teleport ever takes place.

[ninja-]

If we really want to do Post* kind of events, maybe a common event.addPostEventCallback( ... ) pattern? without adding Post* evenet themselves

[Shevchik]

Post event callback won't help there due to how shitty the dismount code is.
Also that is a regression from 1.7 i believe where it worked fine.

It is possible to workaround that shit (And i guess i will have to do it after all), yet it doesn't look like a correct thing to do.