Add ability to get cause in EntityTeleportEvent (#5767) #5861

Nyrok · 2023-06-27T00:07:25Z

Introduction

Relevant issues

Changes

API changes

The following method now accepts int $cause parameter. Defaults is 0 for EntityTeleportEvent::CAUSE_PLUGIN.

Entity->teleport(Vector3 $pos, ?float $yaw, ?float $pitch, int $cause) : bool;

Backwards compatibility

No BC-breaking changes, because there is a default parameter to this method signature change.

dktapps · 2023-06-27T17:07:34Z

src/event/entity/EntityTeleportEvent.php

+	public const CAUSE_ITEM = 1;
+	public const CAUSE_WORLD = 2;
+	public const CAUSE_RESPAWN = 3;
+	public const CAUSE_PATCH = 4;


This naming is very unintuitive, I have no idea what it's supposed to mean

Item: this is when an item cause the teleportation, like chorus fruit or enderpearl

World: this is when the world cause teleportation like changing world or being teleported in safe spawn due to world unload

Respawn: this is when the player respawn cause teleportation

Patch: I think we can merge it with "Respawn" as "Player"
So, I can suggest this changes:

public const CAUSE_ITEM = 1; public const CAUSE_WORLD = 2; - public const CAUSE_RESPAWN = 3; - public const CAUSE_PATCH = 4; + public const CAUSE_PLAYER = 3;

This is even less clear than the initial, and additionally provides less information.

JavierLeon9966 · 2023-06-27T17:13:51Z

src/entity/projectile/EnderPearl.php

@@ -40,7 +41,7 @@ protected function onHit(ProjectileHitEvent $event) : void{

 			$this->getWorld()->addParticle($origin = $owner->getPosition(), new EndermanTeleportParticle());
 			$this->getWorld()->addSound($origin, new EndermanTeleportSound());
-			$owner->teleport($target = $event->getRayTraceResult()->getHitVector());
+			$owner->teleport($target = $event->getRayTraceResult()->getHitVector(), null, null, EntityTeleportEvent::CAUSE_ITEM);


CAUSE_PROJECTILE seems better

JavierLeon9966 · 2023-06-27T17:14:29Z

src/item/ChorusFruit.php

@@ -76,7 +77,7 @@ public function onConsume(Living $consumer) : void{

 			//Sounds are broadcasted at both source and destination
 			$world->addSound($origin, new EndermanTeleportSound());
-			$consumer->teleport($target = new Vector3($x + 0.5, $y + 1, $z + 0.5));
+			$consumer->teleport($target = new Vector3($x + 0.5, $y + 1, $z + 0.5), null, null, EntityTeleportEvent::CAUSE_ITEM);


CAUSE_CHORUS_FRUIT seems better

JavierLeon9966 · 2023-06-27T17:15:57Z

src/player/Player.php

@@ -1339,7 +1340,7 @@ protected function processMostRecentMovements() : void{
 			}

 			if($to->distanceSquared($ev->getTo()) > 0.01){ //If plugins modify the destination
-				$this->teleport($ev->getTo());
+				$this->teleport($ev->getTo(), null, null, EntityTeleportEvent::CAUSE_PATCH);


CAUSE_SYNCHRONIZE seems better

CAUSE_PLUGIN would be sufficient.

Nyrok · 2023-06-28T10:28:43Z

This is specific, but let's see what @dktapps think about it

+ EntityTeleportEvent: Code style improve

ShockedPlot7560 · 2023-06-28T11:13:41Z

src/command/defaults/TeleportCommand.php

@@ -89,7 +90,7 @@ public function execute(CommandSender $sender, string $commandLabel, array $args
 					return true;
 				}

-				$subject->teleport($targetPlayer->getLocation());
+				$subject->teleport($targetPlayer->getLocation(), null, null, EntityTeleportEvent::CAUSE_COMMAND);


Suggested change

$subject->teleport($targetPlayer->getLocation(), null, null, EntityTeleportEvent::CAUSE_COMMAND);

$subject->teleport($targetPlayer->getLocation(), cause: EntityTeleportEvent::CAUSE_COMMAND);

Seems better

Refering to pmmp#5861 (comment) Co-Authored-By: ShockedPlot7560 <no-reply@tchallon.fr>

JavierLeon9966 · 2023-06-28T15:30:29Z

src/event/entity/EntityTeleportEvent.php

+		return $this->cause;
+	}
+
+	public function setCause(int $cause) : void{


Why do we need this?

Maybe it can be used for plugin conflicts 🤷‍♂️

dzolnowski · 2023-06-28T16:29:08Z

src/event/entity/EntityTeleportEvent.php

@@ -36,10 +36,10 @@ class EntityTeleportEvent extends EntityEvent implements Cancellable{
 	use CancellableTrait;

 	public const CAUSE_PLUGIN = 0;
-	public const CAUSE_ITEM = 1;
+	public const CAUSE_PROJECTILE = 1;


CAUSE_PROJECTILE doesn't tell us what type of projectile was used. Maybe CAUSE_ENDER_PEARL would be better?

You know, I think the cause should be a string so plugins can specify their custom causes like "morepearls:snowball". That way we can be specific while also allowing customization

Yeah agree, with ints its really easy to mess up if someone was to create custom one

Nyrok · 2023-07-01T09:40:25Z

What's now ? Should be the reason a string or a int (like seeing PlayerPreLoginEvent) ?

ShockedPlot7560 · 2023-07-13T07:09:10Z

What's now ? Should be the reason a string or a int (like seeing PlayerPreLoginEvent) ?

I think a string would allow greater flexibility. An integer is very restrictive and doesn't give direct information for plugins that want to get the values.

dktapps · 2023-07-13T12:09:43Z

What's now ? Should be the reason a string or a int (like seeing PlayerPreLoginEvent) ?

I think a string would allow greater flexibility. An integer is very restrictive and doesn't give direct information for plugins that want to get the values.

Plugins won't be able to interpret values they don't know about anyway. I think an integer is fine.

Really this should probably use an enum, but I don't want to introduce API inconsistencies.

dktapps · 2023-07-13T12:12:13Z

src/event/entity/EntityTeleportEvent.php

@@ -35,10 +35,18 @@
 class EntityTeleportEvent extends EntityEvent implements Cancellable{
 	use CancellableTrait;

+	public const CAUSE_PLUGIN = 0;
+	public const CAUSE_PROJECTILE = 1;
+	public const CAUSE_WORLD = 2;


I have no idea what this is supposed to mean.

Nyrok added 3 commits June 27, 2023 01:59

Add ability to get cause in EntityTeleportEvent

b97705a

EntityTeleportEvent: Code style fix

232294f

This CS Fixer is going to kill me

3d667cf

This comment was marked as outdated.

Sign in to view

dktapps reviewed Jun 27, 2023

View reviewed changes

JavierLeon9966 suggested changes Jun 27, 2023

View reviewed changes

Update constants of EntityTeleportEvent

cff6f97

+ EntityTeleportEvent: Code style improve

Nyrok requested review from dktapps and JavierLeon9966 June 28, 2023 10:46

ShockedPlot7560 reviewed Jun 28, 2023

View reviewed changes

Convert cause passed parameter into named argument

cb1334c

Refering to pmmp#5861 (comment) Co-Authored-By: ShockedPlot7560 <no-reply@tchallon.fr>

JavierLeon9966 reviewed Jun 28, 2023

View reviewed changes

dzolnowski reviewed Jun 28, 2023

View reviewed changes

jasonw4331 added Category: API Related to the plugin API Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Jun 29, 2023

dktapps reviewed Jul 13, 2023

View reviewed changes

dktapps added the Status: Waiting on Author label Jul 28, 2023

Joshy3282 added a commit to Joshy3282/PocketMine-MP that referenced this pull request Oct 2, 2024

add pmmp#5861

a01bc4d

dktapps closed this Nov 14, 2024

dktapps added Resolution: Abandoned PR has been abandoned by its author and removed Status: Waiting on Author labels Nov 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to get cause in EntityTeleportEvent (#5767) #5861

Add ability to get cause in EntityTeleportEvent (#5767) #5861

Nyrok commented Jun 27, 2023

This comment was marked as outdated.

dktapps Jun 27, 2023

Nyrok Jun 27, 2023

dktapps Jun 28, 2023

JavierLeon9966 Jun 27, 2023

JavierLeon9966 Jun 27, 2023

JavierLeon9966 Jun 27, 2023

dktapps Jun 28, 2023

Nyrok commented Jun 28, 2023

ShockedPlot7560 Jun 28, 2023

JavierLeon9966 Jun 28, 2023

Nyrok Jun 28, 2023

dzolnowski Jun 28, 2023

JavierLeon9966 Jun 28, 2023 •

edited

Loading

dzolnowski Jun 28, 2023

Nyrok commented Jul 1, 2023

ShockedPlot7560 commented Jul 13, 2023

dktapps commented Jul 13, 2023

dktapps Jul 13, 2023

	$subject->teleport($targetPlayer->getLocation(), null, null, EntityTeleportEvent::CAUSE_COMMAND);
	$subject->teleport($targetPlayer->getLocation(), cause: EntityTeleportEvent::CAUSE_COMMAND);

Add ability to get cause in EntityTeleportEvent (#5767) #5861

Add ability to get cause in EntityTeleportEvent (#5767) #5861

Conversation

Nyrok commented Jun 27, 2023

Introduction

Relevant issues

Changes

API changes

Backwards compatibility

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyrok commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JavierLeon9966 Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyrok commented Jul 1, 2023

ShockedPlot7560 commented Jul 13, 2023

dktapps commented Jul 13, 2023

Choose a reason for hiding this comment

JavierLeon9966 Jun 28, 2023 •

edited

Loading