[CRITICAL] teleport {_player} to {_loc} DELETES ALL LOCAL VARIABLES!

[ghost]

Description

using teleport {_player} to {_loc}, when at the target location the chunks aren't loaded (but are generated), will delete all local variables in the code. (It may be 1.12.2 only bug, as it has no async teleportation)

Steps to Reproduce

1. have 0 chunks loaded in the "testworld"
2. use this code:

command /asynctp:
	trigger:
		set {_game} to "no bug"
		set {_loc} to spawn point of world "testworld"
		set {_player} to player
		wait a tick
		teleport {_player} to {_loc}
		send "%{_game}%"

3. observe it outputting <none>

Expected Behavior

To not delete local variables, as it was before.

Errors / Screenshots

None

Server Information

• Server version/platform: 1.12.2 Paper (maybe it is 1.12.2 only bug?)
• Skript version: 2.5.2

Additional Context

This is very likely caused by async teleportation update.

[TPGamesNL]

PR: #3478
I have no clue how this wasn't seen by any reviewers (other reviewers were requested, none responded, but it was still merged). Most of the things you should keep in mind when creating async effects in Skript were not used, mainly variable backup and re-setting those variables. See AsyncEffect for reference.

[APickledWalrus]

If you're able to, please let me know if the build from that PR fixes your issue. @Govindass

[TPGamesNL]

It should probably be documented that EffTeleport is now a delayed effect, as that has a big impact on plenty of scripts (breaking change)

[JRoy]

It should probably be documented that EffTeleport is now a delayed effect, as that has a big impact on plenty of scripts (breaking change)

Line of execution isn't broken, the next TriggerItem still executes after all teleports have occurred and the chunk has been fully loaded (async on paper or sync on spigot)

Skript/src/main/java/ch/njol/skript/effects/EffTeleport.java

Lines 93 to 106 in da691e2

	// This will either fetch the chunk instantly if on spigot or already loaded or fetch it async if on paper.
	PaperLib.getChunkAtAsync(loc).thenAccept(chunk -> {
	// The following is now on the main thread
	for (final Entity entity : entityArray) {
	entity.teleport(getSafeLocation(loc));
	}
	// Re-set local variables
	if (localVars != null)
	Variables.setLocalVariables(e, localVars);
	// Continue the rest of the trigger if there is one
	continueWalk(next, e);
	});

[Pikachu920]

It should probably be documented that EffTeleport is now a delayed effect, as that has a big impact on plenty of scripts (breaking change)

Line of execution isn't broken, the next TriggerItem still executes after all teleports have occurred and the chunk has been fully loaded (async on paper or sync on spigot)

Skript/src/main/java/ch/njol/skript/effects/EffTeleport.java

Lines 93 to 106 in da691e2

	// This will either fetch the chunk instantly if on spigot or already loaded or fetch it async if on paper.
	PaperLib.getChunkAtAsync(loc).thenAccept(chunk -> {
	// The following is now on the main thread
	for (final Entity entity : entityArray) {
	entity.teleport(getSafeLocation(loc));
	}
	// Re-set local variables
	if (localVars != null)
	Variables.setLocalVariables(e, localVars);
	// Continue the rest of the trigger if there is one
	continueWalk(next, e);
	});

does that mean functions can now be delayed? because I teleport inside a function

They are the same as before - the function can have delayed execution however the calling code will continue before the delayed code finishes + it can't be used as an expression anymore (or at least, the return value will always be none if the return comes after a delay)

[TPGamesNL]

Making EffTeleport async would break the following (and more):

• line of executions in functions not used as expressions, as the calling code does not wait for the function to finish delays.
• functions with return values (used as expressions), since no value would be returned when EffTeleport is used before EffReturn.
• triggers that influence events (cancel event, change event values, etc) after calling EffTeleport.

The least that can be done is for it to be documented.

[ham1255]

@Govindass async teleport player

[ShaneBeee]

Unless I am missing something here, I don't see the issue.

Currently testing this on 1.12.2 (because Govindass I know you use that)
code:

function moveTo(p: player, w: world) :: world:
	teleport {_p} to spawn of {_w}
	return world of {_p}

command /world <world>:
	trigger:
		send "World before TP: &a%world of player%"
		set {_w} to moveTo(player, arg-1)
		send "World after TP: &b%{_w}%"

Result:

Needless to say, it works totally fine for me.

Prior to this change, when you used the teleport effect, the chunk was force loaded, if there was any delay in that chunk loading, the code after the teleport would all be delayed also, just like this is.

So unless there is more info you can give us, I don't see any issue here.
Josh set it up, so all code after the teleport, is executed after the teleport is complete (ie: its kinda put on hold waiting for the TP to finish)

EDIT
wait, hold on, I had "keep spawn loaded" set to true, so that sped things up, if I set that to false, I now see the issue.
when using the same code as above, I now get a return of null.

On that note, that issue is different than this one, and a new issue should be created so we can properly address it.

[TPGamesNL]

That code shouldn't even parse, but the trigger isn't marked as delayed in the init section (which is also a bug with AsyncEfect / EffLoadServerIcon, the only AsyncEffect currently in Skript), so it won't error.

My suggestion is making teleportation optionally async, as was suggested by #3355.