Only use convert spawners tagged by the give command

[JRoy]

Marks all spawners made by EssentialsX with a special PDC tag that denotes if we should run "spawnerconvert" operations on it.

Tested on 1.8.8 & 1.16.5

Fixes #3811

[pop4959]

I've tested this code and (quite shockingly) this breaks the spawner convert feature. There's definitely a better way to go about fixing this.

I can explain this feature briefly since you're unfamiliar with it. In essence, it converts spawners being placed to their intended types when using Essentials commands to create them. It's probably easiest if I just give some example commands to reproduce the expected behavior.

If you run /i zombiespawner you get a "zombie Spawner" (not sure why the z is lower case, that's probably a separate bug as in older versions it was upper). Upon placement, the spawner is "converted" to a zombie spawner. With this code nuked, that no longer works properly and you get left with a pig spawner.

This worked properly in older versions of Minecraft, but from brief testing this clearly doesn't work well with vanilla spawners. If you use the vanilla equivalent command, /minecraft:give @p minecraft:spawner{BlockEntityTag:{SpawnData:{id:"minecraft:zombie"},SpawnPotentials:[{Entity:{id:"minecraft:zombie"},Weight:1}]}} 64, and place the spawner, you get a pig spawner because Essentials overreaches and tries to convert this instead of leaving it alone. PS: for the above command you will also need to be OP and in creative mode due to MC-105216.

Clearly this feature still needs an overhaul because it's not OK for it to do that. Ideally spawners made in Essentials should be the only ones touched by spawner conversion, and vanilla ones are left alone. This could probably be done by attaching a custom tag to the item stack on creation and checking for that before setting the spawned type (and of course, ignoring any that do not have the tag).

[JRoy]

@pop4959 am now using an enchantment as a way to differentiate between EssX spawners and normal ones. This is the best solution due to PersistentDataContainer being a 1.14 thing

[pop4959]

To be honest that's a solution I'd not have thought of, but I still have some worries about using enchantments here, given that they're not really intended for storing data. On top of that, it seems likely to me that there can easily be some unnecessary plugin conflicts created by this, since quite a few plugins touch enchantments.

The best solution in my opinion would be to leave the existing legacy handler as-is, and use PDC on the versions that support it. Of course that means that older versions would not receive the improvement, but these versions are not a priority and should not force us to use workarounds that create further maintenance burden.

[JRoy]

On top of that, it seems likely to me that there can easily be some unnecessary plugin conflicts created by this, since quite a few plugins touch enchantments.

The only possible plugin conflicts would be extremely rare and only in 1.8-1.12 as starting with 1.13 it uses a plugin namespaced key.

The best solution in my opinion would be to leave the existing legacy handler as-is, and use PDC on the versions that support it. Of course that means that older versions would not receive the improvement, but these versions are not a priority and should not force us to use workarounds that create further maintenance burden.

I apply the same logic to the reason in favor of this fix

[pop4959]

Just was concerned because I'm not familiar with any other plugins handling metadata that way. If we're absolutely sure this won't cause problems or have any other side effects, then it might be OK. I'll try to take a closer look later this week.

[JRoy]

Just was concerned because I'm not familiar with any other plugins handling metadata that way. If we're absolutely sure this won't cause problems or have any other side effects, then it might be OK. I'll try to take a closer look later this week.

Yeah no problem. Basically the only way this can conflict is with the id based enchantment registration which only occurs on 1.8 thru 1.12. I picked an offset which is good enough and no other plugins should occupy considering that barely any plugins register enchantments in the first place, I'd say we're fine. As for side effects there are none I can find.

[JRoy]

Marking as draft as this pends providers from #4143

[mdcfe]

#4143 has now been merged - will review when marked ready

outdated