-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ExplodeEvent and Improvement in Spigot Explode events #11840
base: main
Are you sure you want to change the base?
Conversation
PS: Very new with the new contribution system.. currently the ExplosionServer has unused CB imports but if remove that the fixupSourcePatches just fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, thank you for the PR!
We have reviewed it but are a bit (a bit too) hesitant to call these events when their javadocs do define them to not be called in these exact spots.
We could fire them pre-cancelled, but block break related events are obviously rather important for plugin data, and I'd rather avoid plugins dying/duping because they did not correctly check cancelled here.
Instead, Machine and I think it'd be best to use this API downside to finally create a better version of the "SomethingExplodeEvent" (better name pending) that covers both entities, blocks and nothing (like in the mace's case). Such an event can be called for these cases, specifying that no blocks will be broken etc.
The current events types could then be marked as Obsolete and the new event would be called after to allow people to take priority with the new API.
If you are up for implementing such a change yourself, feel free to just use this API, otherwise we can close it and open a feature request issue 👍
Then you mean a new event "ExplodeEvent" for handle all the explosions, like a merge of the current entity and block not? |
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... |
paper-api/src/main/java/io/papermc/paper/event/world/ExplodeEvent.java
Outdated
Show resolved
Hide resolved
paper-api/src/main/java/io/papermc/paper/event/world/ExplodeEvent.java
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
paper-api/src/main/java/io/papermc/paper/event/world/ExplodeEvent.java
Outdated
Show resolved
Hide resolved
paper-server/patches/sources/net/minecraft/world/level/ServerExplosion.java.patch
Outdated
Show resolved
Hide resolved
paper-server/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
Outdated
Show resolved
Hide resolved
paper-server/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
Outdated
Show resolved
Hide resolved
paper-server/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
Outdated
Show resolved
Hide resolved
not sure how handle this.. i mean still being the "normal" behaviour with or not this PR i think... |
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... |
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. |
paper-server/patches/sources/net/minecraft/world/level/ServerExplosion.java.patch
Outdated
Show resolved
Hide resolved
paper-server/patches/sources/net/minecraft/world/level/ServerExplosion.java.patch
Outdated
Show resolved
Hide resolved
+ else { | ||
+ io.papermc.paper.event.world.ExplodeEvent explodeEvent = CraftEventFactory.callExplodeEvent(this, list); | ||
+ this.wasCanceled = explodeEvent.isCancelled(); | ||
+ this.yield = explodeEvent.getYield(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The yield shouldn't be needed there no blocks explode anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but not make "damage" keep that.
about resolve #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. |
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. |
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. |
paper-server/patches/sources/net/minecraft/world/level/ServerExplosion.java.patch
Outdated
Show resolved
Hide resolved
paper-server/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
Outdated
Show resolved
Hide resolved
paper-server/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java
Outdated
Show resolved
Hide resolved
- Handle KEEP explosion behaviour (gamerule or explosion by Wind Burst) - Call custom event damager for allow CraftBukkit know who make the explosion by Wind Burst
…th moon cache reset
Need to figure out what we do with EnderDragon.
daa652b
to
4a47bd8
Compare
@@ -1,11 +1,14 @@ | |||
package org.bukkit.event.block; | |||
|
|||
import java.util.List; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is intended?
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.