Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Add ExplodeEvent and Improvement in Spigot Explode events

Open Doc94 opened this issue 1 year ago • 19 comments

~~This PR is a "copy" from a 2 years old PR in Spigot related to explosion, when this start the issue is how the gamerule for avoid mob griefing make the explosion event not being called, with recent changes now CraftBukkit pass the explosion behavior for make the user know what is the behaviour of this explosion.. with this in mind this PR takes that for well.. call the event for that also handle a issue where "explosions" created by Wind Burst call the block explosion with not data.~~

This PR implement a new "generic" event for any explosion generated by the server, this brings the same behaviour of the Spigot explode events (block,entity) this this event is intented for being called for all the explosions.. affects or not the environment.

Doc94 avatar Dec 27 '24 18:12 Doc94

PS: Very new with the new contribution system.. currently the ExplosionServer has unused CB imports but if remove that the fixupSourcePatches just fails

Doc94 avatar Dec 27 '24 18:12 Doc94

Then you mean a new event "ExplodeEvent" for handle all the explosions, like a merge of the current entity and block not? This require send a canceled state of this new event? PD in teory just need call this new event in the sane place but later of the bukkit?

Doc94 avatar Dec 29 '24 01:12 Doc94

Ok based in the comments here and in discord i rework this PR for keep the changes but also the new calls just being called in the new event called... i dont like "duplicate" the CB code for the paper event but dont wanna mess with strange behaviours for use the same method for call the paper event and make changes...

Doc94 avatar Dec 30 '24 15:12 Doc94

Exposed the DamageSource used in the explosion to the event... i feel this can help if consider the damage source can have many behaviours based in how datapacks can use the explode effect.

Doc94 avatar Jan 16 '25 19:01 Doc94

This pr break fire explosion done via world.createExplosion(location, 3, true, false) by filtering empty blocks i guess. Ideally should only affect the event but it's tricky cause block list is mutable

not sure how handle this.. i mean still being the "normal" behaviour with or not this PR i think...

Doc94 avatar Jan 18 '25 01:01 Doc94

With your PR that fire explosion doesn't spread fire on the ground which is an unintended side effect

Lulu13022002 avatar Jan 18 '25 17:01 Lulu13022002

With your PR that fire explosion doesn't spread fire on the ground which is an unintended side effect

i see the issue... not sure when i broke maybe the change from getType.isAir to isEmpty... i make a change for allow the empty things for the fire and looks is working. if its not a good "fix" can just revert the thing for the empty check

also i remove the shuffle in the KEEP behaviour because just make changes not related to the vanilla behaviour...

Doc94 avatar Jan 18 '25 22:01 Doc94

That change is completely irrelevant, i have already explained the cause of that issue in the original message. It's not only affect your new event but regular ones as well. So i will open a new issue about that (the fix you commited is a workaround). And you broke spigot further with your equivalent commit.

Lulu13022002 avatar Jan 22 '25 15:01 Lulu13022002

That change is completely irrelevant, i have already explained the cause of that issue in the original message. It's not only affect your new event but regular ones as well. So i will open a new issue about that (the fix you commited is a workaround). And you broke spigot further with your equivalent commit.

Yeah for the not explosion is handled by the keep maybe and how im now calm the event in that place the filter take all... Currently not sure why even need filter that... That block is in the list maybe just send all blocks and if user wanna they can filter that... Based in the first commit. Or not sure if you have another suggest for this thing

Doc94 avatar Jan 22 '25 16:01 Doc94

about resolve https://github.com/PaperMC/Paper/issues/11999 now the filter is gone for allow the event show the same blocks (from positions) used in vanilla and avoid the mess with reduction of the fire.

Doc94 avatar Jan 22 '25 18:01 Doc94

You can't remove the filter for the old events without updating the javadocs which is kinda a breaking change so not sure but the new one can hold empty blocks fine. Cancelling the events still spread fire it seems mainly because you removed the filter.

Lulu13022002 avatar Jan 22 '25 18:01 Lulu13022002

You can't remove the filter for the old events without updating the javadocs which is kinda a breaking change so not sure but the new one can hold empty blocks fine. Cancelling the events still spread fire it seems mainly because you removed the filter.

well already can consider a "breaking" change the thing about how now spigot fire the events for the keep behaviour and paper not (not in that events but yes in the new) but i make the rollback with the patch for the bukkit events.

the fire thing is maybe because just check the cancel for the keep behaviour and not in the main condition.. i move that for cover all calls.

Doc94 avatar Jan 22 '25 18:01 Doc94

Rebase notification D: This still need more improvements?

Doc94 avatar Feb 16 '25 21:02 Doc94

Uh, good question. I'll try to get to it again soon but I'll throw it at other peeps too.

lynxplay avatar Feb 16 '25 21:02 lynxplay

Any thing need to be make in this PR?

Doc94 avatar Apr 22 '25 11:04 Doc94

My first review is still relevant iirc, generally it's a bit tricky but you need to play with the lists a bit

Lulu13022002 avatar Apr 22 '25 11:04 Lulu13022002

This pr break fire explosion done via world.createExplosion(location, 3, true, false) by filtering empty blocks i guess. Ideally should only affect the event but it's tricky cause block list is mutable

This one? If not wrong currently only affects the legacy ay not? Because the new way not filter any what ws the issue in first place when you let that comment. Later im test that for see what can do with the list or just pass the empty blocks because i dont see a nice way to filter correctly and avoid mess that...

Doc94 avatar Apr 22 '25 12:04 Doc94

Ok i modify the filter based in the logic of fire method in NMS for pass in events the empty blocks used for that, the other option its just remove the filter and pass the whole blocks in the explosion and user can manage the whole explode expansion.

Doc94 avatar Apr 23 '25 23:04 Doc94

This also soft close https://github.com/PaperMC/Paper/issues/12441

Doc94 avatar May 19 '25 14:05 Doc94