Skip to content
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 EntityExtinguishEvent #5242

Closed

Conversation

aurorasmiles
Copy link
Contributor

Closes #5143
Replaces #5144
I tried to see if I could manage to fix that issue and add that event. After finishing, I noticed that there already way a PR for this, but as it was made by Ivan and he is banned from the Org, I figured to open a PR anyway.
I haven't implemented cancellable, because I figured this would cause a lot of issues in the long run, and I wasn't sure how to properly address that.
Other than that there is another extinguish called in EntityHuman line 1211, but I decided to not call the event there, since the entity was never technically on fire

@aurorasmiles aurorasmiles requested a review from a team as a code owner February 24, 2021 02:13
@Proximyst Proximyst mentioned this pull request Feb 24, 2021
@aurorasmiles
Copy link
Contributor Author

Changed :)

Copy link
Contributor

@Proximyst Proximyst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note, otherwise LGTM. The note can be handled on merge.

+ // Paper start - add EntityExtinguishEvent
+ EntityExtinguishEvent event = CraftEventFactory.callEntityExtinguishEvent(entity, EntityExtinguishEvent.Cause.WET);
+ if (!event.isCancelled()) {
+ entity.extinguish();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't indent

@SoSeDiK
Copy link
Contributor

SoSeDiK commented Mar 15, 2021

Do you mind separating the extinguish cases a bit more, please? The previous PR had 8 cases versus 4 in this one.
That would make my life easier 😅
Like for the cauldron, I'd otherwise have to use the CauldronLevelChangeEvent with ChangeReason.EXTINGUISH and getEntity(), and that may be way easier with ths merged.
(or listen for this and check if the entity is inside the cauldron, and if it has water (which may not work since changeLevel() is called before extinguishing, so there will be no water for the lowest water level?))
I'd use that for my temperature module where I trigger some additional effects upon extinguish (different for every case).

@aurorasmiles
Copy link
Contributor Author

Added more reasons, removed the indents and resolved conflicts

@aurorasmiles
Copy link
Contributor Author

"rebased"

Copy link
Contributor

@Trigary Trigary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from the unused BUBBLE cause.

+import org.jetbrains.annotations.NotNull;
+
+/**
+ * Is called when a burning {@link org.bukkit.entity.Entity} is extinguished.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I would specify what "burning" and "being extinguished" means. Meaning that I would add JavaDoc links to the fireTicks property.

Spigot-Server-Patches/0696-Add-EntityExtinguishEvent.patch Outdated Show resolved Hide resolved
Spigot-Server-Patches/0696-Add-EntityExtinguishEvent.patch Outdated Show resolved Hide resolved
import net.minecraft.world.phys.shapes.VoxelShapeCollision;
import net.minecraft.world.phys.shapes.VoxelShapes;

+import org.bukkit.craftbukkit.event.CraftEventFactory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that isn't needed.


// CraftBukkit start
import net.minecraft.world.entity.item.EntityItem;
+import org.bukkit.craftbukkit.event.CraftEventFactory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that isn't needed.

@stale
Copy link

stale bot commented May 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@TetraTheta
Copy link
Contributor

As mentioned in #5517, I think if this pull request want to include 'FireTickChangeEvent' event stuff, then its event name should be altered, so that that event can include both onFire(from 'FireTickChangeEvent'), onExtinguish(from 'EntityExtinguishEvent'). Just food for thought.

@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically closed because it has not had activity in a long time. If the issue still applies to the most recent supported version, please open a new issue referencing this original issue.

@stale stale bot closed this Jun 2, 2021
This was referenced Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EntityExtinguishEvent
7 participants