Paper
Paper copied to clipboard
PaperMC
Add PreTeleportEvents
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.
- [x] Find all spots where the event needs to be called.
- [x]
EntityPreTeleportEvent - [ ] ~~Modifiability?~~
- [x] JavaDocs
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).
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
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 orsetSpectatorTarget(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.
can just always return a clone of the stored Location object
Yup good idea.
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?
The EntityTeleportEvent doesn't have a TeleportCause so I don't see why EntityPreTeleportEvent should?
Or am I misunderstanding?
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.
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.
Ok so i finished the initial work and this should be ready for testing and reviewing.
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.
Hello, do we have any updates regarding this issue and this pull request?
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.
Maybe I am just misunderstanding something but I don't really know why there is a
PlayerPreTeleportEventadded. I have a plugin which makes use of thePlayerTeleportEventand in order to make my logic work correctly I experimented a bit with that event and found out thatPlayer#getWorld()returns the same world asPlayerTeleportEvent#getFrom()which means that thePlayerTeleportEventis 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: https://github.com/PaperMC/Paper/issues/10168
If the player/entity has at least one passenger, the PlayerTeleportEvent is not triggered and teleportation is blocked.
The issue that this resolves is mentioned at the top of this PR: https://github.com/PaperMC/Paper/issues/10168
I mean, nevertheless, the name PlayerPreTeleportEvent is misleading then since the current PlayerTeleportEvent is called before teleporting.
The issue that this resolves is mentioned at the top of this PR: #10168
I mean, nevertheless, the name
PlayerPreTeleportEventis misleading then since the currentPlayerTeleportEventis 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?
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.
The issue that this resolves is mentioned at the top of this PR: https://github.com/PaperMC/Paper/issues/10168
I mean, nevertheless, the name
PlayerPreTeleportEventis misleading then since the currentPlayerTeleportEventis 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.
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.
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.
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
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 ;)
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.
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?