Added target level to teleport events #1531
Conversation
|
|
Any opinions on the "api-ness" of those 3 fields? In my eyes, them being protected was a mistake (they have unrestricted getters and setters) and nobody should access them directly. |
sciwhiz12
added
enhancement
| @ApiStatus.Internal | ||
| public static EntityTeleportEvent.EnderPearl onEnderPearlLand(ServerPlayer entity, double targetX, double targetY, double targetZ, ThrownEnderpearl pearlEntity, float attackDamage, HitResult hitResult) { | ||
| EntityTeleportEvent.EnderPearl event = new EntityTeleportEvent.EnderPearl(entity, targetX, targetY, targetZ, pearlEntity, attackDamage, hitResult); | ||
| public static EntityTeleportEvent.EnderPearl onEnderPearlLand(ServerPlayer entity, GlobalVec3 target, ThrownEnderpearl pearlEntity, float attackDamage, HitResult hitResult) { |
There was a problem hiding this comment.
While it wasn't done by this PR, this method being internal whilst the other methods of its family are not internal strikes me as odd, and an inconsistency.
We should look into why this specific method is internal, and whether to remove its annotation or apply the same to the rest.
We should also factor in here the decision to mark as internal the constructor of those classes; I think we could leave those constructors unmarked and these marked as internal, but I'm not fully sure yet.
There was a problem hiding this comment.
I had a look at that and decided to leave it as-is because this one is fired from a very specific vanilla entity and I can see event subscribers assuming it is the only one that can fire it.
And adding Internal to the others would be breaking, so it's off-limits at the moment.
src/main/java/net/neoforged/neoforge/common/util/GlobalVec3.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/EntityTeleportEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/EntityTeleportEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/EntityTeleportEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/common/util/GlobalVec3.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/common/util/GlobalVec3.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/EntityTeleportEvent.java
Outdated
Show resolved
Hide resolved
src/main/java/net/neoforged/neoforge/event/entity/EntityTeleportEvent.java
Outdated
Show resolved
Hide resolved
Implements #1526
Added the target level to EntityTeleportEvent (and its sub-events). Changing it is supported where the teleporting code already supports it, although this feature could need some additional testing.
I decided to carry around a Level instead of a ServerLevel because the event wasn't confined to the server side before, even though it pretty much is a server-side-only functionality. Forcing a ServerLevel would be breaking for mods that fire the event for their own teleporting on both sides. Changing this in the next breaking phase may be desirable.
Adding this without breaking the old constructors adds a potential for mod incompatibility between mods that do dimension teleporting and fire the event with the old constructors and mods that handle the events and read/write the target level. This sadly is unavoidable when adding fields to an event that is fired by mods. I added a warning to the javadoc to reduce the risk.