Shuffle Positions, does not shuffle. Instead teleports you to the boss (Overpowered)

[SoraShiunin]

Bug report

Short description

Shuffle Positions doesn't work properly.

Reproduction steps

1. Join an arena with /ma join
2. Wait for shuffle positions
3. You are only teleported to the boss, not shuffled.

Details

MobArena version: MobArena v0.108 Server version: Paper 1.20.4

https://github.com/garbagemule/MobArena/assets/23641964/84f0aca3-e15d-45fb-a08d-6b91cabf2383

[garbagemule]

Just gave this a whirl.

If monster-teleporting is set to false (default), the entity teleport handler cancels the teleport event. If you set it to true instead, the teleport goes through, and the positions shuffle randomly* as intended. The issue can just as easily be considered a "user error", because two conflicting parts of the plugin are used at the same time. If you don't want mobs to teleport around in the arena, it will affect shuffle-positions (and probably other things).

I only know of two ways to work around this problem if we want the shuffle ability to work while allowing monster-teleporting to be false:

1. We can try to keep track of the impending teleportees in some sort of collection and share that collection with the entity teleport handler. That way, it can check whether or not the given teleport event is for an entity that is currently being "legitimately transported" somewhere by the plugin itself. This is the same kind of approach we use for players being teleported between the lobby/arena/spectator warps. The major downside of this is that it's a type of global state sharing issue that is really messy to implement in the current code base, because the only access to global state we have is via the arena (and then arena master and plugin instance). For that reason, I haven't done any tests with this approach.
2. We can attach some sort of metadata to the entities as they are being teleported. This essentially solves the same problem in a similar way, but the "collection" is replaced by per-entity properties that "stick" to the entities. The major downside here is that volatile metadata values like this have a potentially significant performance cost. Across a couple of runs in the same arena with as many variables as possible locked, I got a 40-50% increase in time taken to complete the ability execution. It's less than 1 ms difference, but it is consistent, and it's just with one boss and one player. I'd like to see the effects on a real server with multiple players before I consider this a viable option.

There may be more ways about it, but I haven't been able to find a "teleport reason" kind of property on the entity teleport event, so I think it's necessary to implement some "hand holding".

If anyone wants to try out a debug build with the metadata on entities solution and send me their numbers, I can provide such a build on Discord.

---

* It's worth mentioning also that the way the shuffle works is that MobArena just adds the locations of all players (as well as itself) to a list, then shuffles that list using the standard library shuffle method, and finally "assigns" a location from that shuffled list to each player and the boss. In the 1v1 scenario, the random number generation might result in the list not getting shuffled at all, but that's just the nature of randomization.

[NightmareFox]

Just gave this a whirl.

If monster-teleporting is set to false (default), the entity teleport handler cancels the teleport event. If you set it to true instead, the teleport goes through, and the positions shuffle randomly* as intended. The issue can just as easily be considered a "user error", because two conflicting parts of the plugin are used at the same time. If you don't want mobs to teleport around in the arena, it will affect shuffle-positions (and probably other things).

I only know of two ways to work around this problem if we want the shuffle ability to work while allowing monster-teleporting to be false:

1. We can try to keep track of the impending teleportees in some sort of collection and share that collection with the entity teleport handler. That way, it can check whether or not the given teleport event is for an entity that is currently being "legitimately transported" somewhere by the plugin itself. This is the same kind of approach we use for players being teleported between the lobby/arena/spectator warps. The major downside of this is that it's a type of global state sharing issue that is really messy to implement in the current code base, because the only access to global state we have is via the arena (and then arena master and plugin instance). For that reason, I haven't done any tests with this approach.
2. We can attach some sort of metadata to the entities as they are being teleported. This essentially solves the same problem in a similar way, but the "collection" is replaced by per-entity properties that "stick" to the entities. The major downside here is that volatile metadata values like this have a potentially significant performance cost. Across a couple of runs in the same arena with as many variables as possible locked, I got a 40-50% increase in time taken to complete the ability execution. It's less than 1 ms difference, but it is consistent, and it's just with one boss and one player. I'd like to see the effects on a real server with multiple players before I consider this a viable option.

There may be more ways about it, but I haven't been able to find a "teleport reason" kind of property on the entity teleport event, so I think it's necessary to implement some "hand holding".

If anyone wants to try out a debug build with the metadata on entities solution and send me their numbers, I can provide such a build on Discord.

• It's worth mentioning also that the way the shuffle works is that MobArena just adds the locations of all players (as well as itself) to a list, then shuffles that list using the standard library shuffle method, and finally "assigns" a location from that shuffled list to each player and the boss. In the 1v1 scenario, the random number generation might result in the list not getting shuffled at all, but that's just the nature of randomization.

Maybe the shuffle method can bypass teleporting: false, I don't really know all intentions and purposes so I'm just suggesting.

[garbagemule]

Maybe the shuffle method can bypass teleporting: false, I don't really know all intentions and purposes so I'm just suggesting.

The way the shuffle ability works is by calling the teleport() method on the relevant entities (the players and the boss). This method is how we move entities in the Bukkit API, and it always triggers a teleport event, which is then passed to all teleport event listeners. There is no way to "hide" the event from the event listeners, which means MobArena's own event listener needs to know about the intent behind the teleport if we want it to distinguish between, say, an Enderman teleporting and the shuffle ability.

As alluded to in my comment on issue #789, the event handling is single-threaded and synchronous, so it certainly is possible to "do something" before and after the teleport, but sharing that state is a bit messy in the current code base.

It would be very useful if we had a way of either teleporting without triggering events or providing some sort of teleport reason. We could attach metadata to the entities, but like I said, it's not a free operation, and I'm worried about the potential performance impact. It would be massively helpful if we could get someone with a larger player base (e.g. 10 players) to try out a test build that profiles the performance impact of this type of solution, as well as a build that profiles the "lack" of a solution (but switching to monster-teleport: true in the config-file so everything works identically).