Removal of onTNTExitPortal #2124
Conversation
| { | ||
| if (event.getEntityType() != EntityType.PRIMED_TNT) | ||
| return; | ||
| if (event.getTo().getWorld().getEnvironment() != Environment.THE_END) | ||
| if (event.getLocation().getBlock().getType() == Material.END_PORTAL) |
There was a problem hiding this comment.
Would this handle cases of end portal blocks that don't actually end up teleporting an entity/are plugin-handled/end portals in non-default worlds?
There was a problem hiding this comment.
Yes, will deal with all of the above.
Using the EntityEnterPortalEvent, you can only determine the location of entry and what block it is.
It shouldn't interfere with any portal plugins, however would probably be best to be configurable for future references if the server admins would like it to be close to the vanilla state.
There was a problem hiding this comment.
I think Robo's more asking "will this result in TNT that is not teleporting being removed" and I believe the answer is yes, though I'm not sure that it matters much. We could swap to the EntityTeleportEvent instead and see if it's from an end world to a non-end world with the correct cause.
There was a problem hiding this comment.
Yes, it would although as you said I don't believe there are many problems that could arise as a result of this change. In most cases, if the End hasn't been generated, the TNT will just drop into the lava at the end portal structure.
In terms of clashes with other possible plugins which use portals, they would probably there own instances to remove other entities from teleporting.
I've attempted the EntityTeleportEvent before this and it would still crash the server.
There was a problem hiding this comment.
It crashes even when cancelled? That seems shocking, honestly. Either way, yeah, I don't see this implementation as being problematic.
There was a problem hiding this comment.
Yeah, I tried that method first before attempting this one. Obviously the destination would be a lot better to use but it would also crash the server.
|
My modification of Portalstick (well, an addon) uses the gateway portal blocks. I know it isn't the same portal block being used here (though I did initially use this), but if I were using that block in another plugin, I wouldn't want to have teleporting entities just get deleted on me. |
That does make sense, but I think the key part here is that the entity is TNT - teleporting TNT smells of some weird niche grief attempt. Could add a config option for it, i.e. "BlockTeleportTNT" and just deny TP rather than remove? |
|
If you just cancel the event, wouldn't it just be the same as removing the entity? I also agree that if it's TNT, it would only cause harm and would probably be disabled for most plugins regardless due to its functionality only being for excavating, grief or farms. I'll have a go at the configuration as well. |
…iefPrevention into fixPrimedTNTCrash
…TTeleport Added Configuration Option (set by default to false) to prevent teleport of TNT to the end.
|
I've chucked it in. Let me know if it's alright. Also let me know if you want me to revert back to the other method (even though it isn't efficient in the slightest to do it like it was) |
|
The more I think about this the more I see that it's better to just remove. This does appear to be a weird niche, where it could be attempts to cause issues with overworld or the_end spawn. Given the comments, it's probably about the latter. Either way, I don't really see much of a use case for either situation - if the end spawn is desired to be protected, then a claim will solve that immediately. The only thing then I could think of is explosion damage. The config is a bit ambiguous, and would require looking at documentation to determine what it is. I'm more in favor of just removing this for now. However, since it was reported - I do wonder if it's still a concern for grief... who else has encountered this bug? I want more input on this. Also it seems that Paper has pushed a fix to this. PaperMC/Paper@408a905 |
I encountered the crash in testing but I honestly think it is completely redundant. It can honestly just be admin claimed and you'll be protected on the platform spawn. I'll edit the PR to remove it entirely if so. |
|
ya, I don't have much feedback on this (idk who else I should ask) but I see no need to handle it, so can just remove it ye. |
Retire Event - Redundant
TreemanKing
changed the title
Removes the entity preemptively rather than reactively.Edit: Removes the event entirely.
Fixes #2119