Add PreTeleportEvents

[Leguan16]

This pull request adds a PlayerPreTeleportEvent. Resolves #10168

Ready for review. It is currently called where I think it makes the most sense. If we want to call it earlier, it needs some serious refactoring at some places and sometimes it really does not make sense to call it earlier.

• Find all spots where the event needs to be called.
• EntityPreTeleportEvent
• Modifiability?
• JavaDocs

[molor]

Modifiability

I think that it doesn't needed in this event, so just call it with a cloned Locations if necessary. IMO the main purpose of this event is to "notify" plugins that a player will be teleported, and they need to prepare him for this (for example, as in my case, remove all passengers or setSpectatorTarget(null) if he is in spectator game mode).

[OakLoaf]

EntityPreTeleportEvent

The EntityPreTeleportEvent would likely be useful for the same reasons that PlayerPreTeleportEvent is useful, obviously would be even more niche scenarios but, would make sense to exist

[Leguan16]

Modifiability

I think that it doesn't needed in this event, so just call it with a cloned Locations if necessary. IMO the main purpose of this event is to "notify" plugins that a player will be teleported, and they need to prepare him for this (for example, as in my case, remove all passengers or setSpectatorTarget(null) if he is in spectator game mode).

I agree here. This event should be solely for "preparation" purposes and to make stuff more compatible with other plugins.

[OakLoaf]

In situations like this I wish there was an Immutable Location

[electronicboy]

can just always return a clone of the stored Location object

[Leguan16]

can just always return a clone of the stored Location object

Yup good idea.

[OakLoaf]

Yeah, that works just thought it'd be handy for clarity

[Leguan16]

Ok so, I am working on the changes right now and the more I am working on the EntityPreTeleportEvent the more I get the feeling that this is bigger than just a few calls.
For example, the Event initially doesn't have a TeleportCause I can use. Should I create a new Enum, do we use the PlayerEvent.Cause enum (This would not really make sense) or do we just leave it?

[OakLoaf]

The EntityTeleportEvent doesn't have a TeleportCause so I don't see why EntityPreTeleportEvent should?
Or am I misunderstanding?

[Leguan16]

No it doesn't.

[Leguan16]

Ok so I started adding the EntityPreTeleportEvent. I have no clue why the workflow fails. I tried to apply patches locally and it works. Build also works.

[OakLoaf]

What's left regarding actual implementation? It looks pretty complete

[Leguan16]

Implementations of the EntityTeleportEvent aren't fully covered yet. Minor cleanup of the Patches are also necessary. Then I would hope someone in the team could take a look if that what I have done is alright. Did change quite a bit in the CraftEventFactory class 😅.

Also need to figure out why the gh action cries.

[Leguan16]

Ok so i finished the initial work and this should be ready for testing and reviewing.

[Will33ELS]

In my current scenario, I have a cosmetic plugin that adds a passenger to the player (a TextDisplay), and all teleportations are blocked because the player has at least one passenger, with no event being called to remove it. This forces the use of NMS (which is discouraged today). This event would allow plugin developers the opportunity to ensure compliance with checks before teleportation occurs.

[Will33ELS]

Hello,
do we have any updates regarding this issue and this pull request?

[DerEchtePilz]

Maybe I am just misunderstanding something but I don't really know why there is a PlayerPreTeleportEvent added.
I have a plugin which makes use of the PlayerTeleportEvent and in order to make my logic work correctly I experimented a bit with that event and found out that Player#getWorld() returns the same world as PlayerTeleportEvent#getFrom() which means that the PlayerTeleportEvent is actually called before teleporting now.
Again, I might just be misunderstanding something here, if so I'd love to know what.

[OakLoaf]

Maybe I am just misunderstanding something but I don't really know why there is a PlayerPreTeleportEvent added. I have a plugin which makes use of the PlayerTeleportEvent and in order to make my logic work correctly I experimented a bit with that event and found out that Player#getWorld() returns the same world as PlayerTeleportEvent#getFrom() which means that the PlayerTeleportEvent is actually called before teleporting now. Again, I might just be misunderstanding something here, if so I'd love to know what.

The issue that this resolves is mentioned at the top of this PR: #10168

[Will33ELS]

If the player/entity has at least one passenger, the PlayerTeleportEvent is not triggered and teleportation is blocked.

[DerEchtePilz]

The issue that this resolves is mentioned at the top of this PR: #10168

I mean, nevertheless, the name PlayerPreTeleportEvent is misleading then since the current PlayerTeleportEvent is called before teleporting.

[OakLoaf]

The issue that this resolves is mentioned at the top of this PR: #10168

I mean, nevertheless, the name PlayerPreTeleportEvent is misleading then since the current PlayerTeleportEvent is called before teleporting.

I think it's perfectly fine personally, PreTeleportEvent implies that is is called before the teleport event.
It's not my PR but if you don't like the name then maybe give alternative suggestions?

[DerEchtePilz]

I think it's perfectly fine personally, PreTeleportEvent implies that is is called before the teleport event.
It's not my PR but if you don't like the name then maybe give alternative suggestions?

Yeah, it does. Still, the existing PlayerTeleportEvent is also called before actually teleporting.
Alternative names? Sure, something like PlayerTeleportPrepareEvent, could also switch that up to PlayerPrepareTeleportEvent.
The name PlayerPreTeleportEvent, again, just doesn't fit because the existing one is fired before teleporting.

[Leguan16]

The issue that this resolves is mentioned at the top of this PR: #10168

I mean, nevertheless, the name PlayerPreTeleportEvent is misleading then since the current PlayerTeleportEvent is called before teleporting.

Do you have a better suggestion how to name the events? PrepareTeleportEvent (edit: made that up before I saw your comment), but that would cause people to come in the discord and ask if a PreTeleportEvent exists since they cannot find any. I know that the name is technically misleading but you always have to think about the users and make everything as much userfriendly as possible. Additionally many events are called before the actual thing happens. I don't know how it's handled with other "pre" events and if they are being called before the actual thing happens. Need to look into that.

[DerEchtePilz]

I am literally fine with any name. If it ends up being named like it currently is - so be it.
I thought about the names I suggested the minute I wrote that comment. Although, I still feel like there should be another name for the proposed event.

but you always have to think about the users and make everything as much userfriendly as possible

That's a good point, however I don't see how the name PrepareTeleportEvent could be misleading? I think it's accurate. It's called to prepare a player about to be teleported. You can make certain changes, remove passengers like the issue wants to, etc.
If this goes through, then the PlayerTeleportEvent can be called, which happens before the player actually is teleported.

Additionally many events are called before the actual thing happens

Yeah, probably to be able to cancel them. The PlayerTeleportEvent can also be cancelled so it's called before anything happens. Still, it is called before anything happens which is why I think the name PlayerPreTeleportEvent doesn't really fit.

Again, if it stays at PreTeleportEvent I am good with that too but I'd also like to see the name being changed. I don't have any other name sugestions, unfortunately. Maybe someone else comes up with another name.

[Leguan16]

That's a good point, however I don't see how the name PrepareTeleportEvent could be misleading

Didn't say it will be misleading. I'm saying that the first name that comes into my mind if I want to search an event that happens before the teleport event I would search for a PreTeleportEvent. But honestly I am fine with PrepareTeleportEvent too. I'd just wait for maintainers to give input on this.

[molor]

Now I'm also think that PrepareTeleportEvent is sounds better.

[OakLoaf]

It seems like both are used in Paper, as the purpose of the event is to prepare I agree that PrepareTeleportEvent sounds nicer but they both make sense to me.

Examples of current events:

• PreLookupProfileEvent
• PreFillProfileEvent
• PreCreatureSpawnEvent
• PreSpawnerSpawnEvent
• PrePlayerAttackEntityEven
• PrepareGrindstoneEvent

[Will33ELS]

Hello,
I'm resending a message for this pull request, do we have an ETA for this merger? This addition is eagerly awaited on our side.
Thank you ;)

[TonytheMacaroni]

Would it make sense to make the flags present in the event mutable? That way plugins can directly modify the behavior of teleporting passengers/vehicles by adding/removing the flags.

[Leguan16]

Would it make sense to make the flags present in the event mutable? That way plugins can directly modify the behavior of teleporting passengers/vehicles by adding/removing the flags.

Is there a use case you can provide where this makes sense?

[TonytheMacaroni]

I'd say a major reason the pre teleport events are useful is for plugins that wish to have display entities or armor stands ride on players, but of course doing so breaks if another plugin attempts to teleport without using the passenger flag. You could work around it using these pre teleport events and temporarily dismounting and remounting, but allowing them to add the flag in would make the process much smoother. Not really sure if there's really a reason against allowing flag modification - its on a similar level to changing where the teleport is from/to on the teleport events.

[Leguan16]

I mean I am generally waiting for maintainers to review the diff and whether we should rename it to PrepareTeleport event. I can make the Flags modifiable but I think there it would also be nice if a maintainer could give their opinion about it.

[bridgelol]

This would be extremely useful, PlayerPrepareTeleportEvent sounds like a better fit however

[Will33ELS]

Hello,
This PR is eagerly awaited on our side, do we have an ETA concerning the merger? Today we have no other solution to this problem.

[SnowzNZ]

Hello, This PR is eagerly awaited on our side, do we have an ETA concerning the merger? Today we have no other solution to this problem.

You can just build this fork if it is that important

[Will33ELS]

We've been waiting for over 3 months... so we're doing without. It's really silly when the solution is right here. We're not going to have fun making forks of forks

[Leguan16]

I'm on vacation for another 4 weeks so I won't be able to update the branch unfortunately.

[Villagers654]

Joining in on the chorus that I would love to see this merged so we can mount passengers to players without breaking every plugin using Player#teleport

[NonSwag]

Wouldn't something like PlayerFailTeleportEvent make more sense
It would only be called if the player is not teleported unlike a pre-event
Also it would match the naming of the PlayerFailMoveEvent

[Villagers654]

Wouldn't something like PlayerFailTeleportEvent make more sense
It would only be called if the player is not teleported unlike a pre-event
Also it would match the naming of the PlayerFailMoveEvent

Purpur already has an event like that and for my use case, it would work too, but I don't see why we couldn't also have this event

[NonSwag]

A pre event in this case literally makes no sense when the teleport event is triggered anyway
Just calling an event in case of failure makes more sense

[Leguan16]

I think that both events would make sense here. I don't know what maintainers would prefer.

[Leguan16]

Ok so i finally got time to have a look at the PR again.

Looking at the code i realized that it's kind of impossible to call the Pre(pare)TeleportEvent for PortalEvent at a time where it makes sense since it requires a DimensionTransition object for both the destination coordinates and the teleport cause. Unfortunately, until we actually get that object it will not just call PreparePortalEvent and PortalEvent but it will also create a portal in the process. And since the PortalEvent is cancellable it would not make sense to create the portal.

Now i could add boolean parameters that handle whether to call the events or create the portal, but I don't know if we want this. Maybe i am overseeing something and there is another way to do this, but I don't see any other way.

Maybe it is actually better to create a TeleportFailEvent. But I would like to have different opinions on this if that is possible.