[B+C] Add and implement an EntityDespawnEvent. adds BUKKIT-5645

[mcmonkey4eva]

The Issue:

There was not any event fired for when an entity despawns for reasons other than death, EG when a non-persistent entity is removed because no player is nearby (RemoveWhenFarAway).

Justification for this PR:

An event to listen for entity despawns is incredibly useful for plugins.
One example of when this might be used is a plugin that needs to store information on the entity (and save it to file), but does not wish this data to persist if the entity no longer exists.
With this PR, a plugin could record the entity's UUID and map some information to it, then simply delete it if the EntityDespawnEvent is called for an entity with a matching UUID.

PR Breakdown:

Bukkit-side:

Simply adds an EntityDespawnEvent with base functionality for plugins to listen to.

CraftBukkit-side:

Fire the EntityDespawnEvent added in Bukkit whenever an entity is removed from the world. Additionally, as there was a fair bit of code exactly duplicating the functionality in the removeEntity() function, change all other entity remove calls to fire the function rather than duplicate all related code.

Testing Results and Materials:

Any default plugin or pre-existing plugin can listen to EntityDespawnEvent.
For my initial testing, I had:

    @EventHandler
    public void onEntityDespawn(EntityDespawnEvent event) {
        getLogger().info("Despawn: " + event.getEntity().getType().name() + " is " + event.getEntity().getUniqueId());
    }

This showed a fair number of regular despawns, that appeared to roughly match up to EntitySpawnEvent's (eyeballed only - obviously many entities were spawned and didn't despawn, or exist in the world prior to testing and despawned during the time)

I also integrated it into an entity-data storage system described in the justification, however, that is part of a larger system that isn't so easily copy-pasted. The stored data was confirmed as removed after the entity despawned (Entity despawn was induced by leaving a zombie in a box with a roof, then running far away, then running back)

... Also, I noticed the removeEntity() function was called fairly often with invalid / unspawned entities... I've placed the event firing inside an if block that ensures it only fires the event if the entity was spawned.

Relevant PR(s):

CraftBukkit: Bukkit/CraftBukkit#1386

JIRA Ticket:

BUKKIT-5645 - https://bukkit.atlassian.net/browse/BUKKIT-5645

[turt2live]

Fired not Thrown

[mcmonkey4eva]

@turt2live - Oh... I just copied that off EntityDeathEvent... it appears that outside a few Entity events, Called is the official term. I'll fix it to that. Should I patch the other events that say Thrown, or just this one? (And leave the others to updated in a different commit/PR)

[turt2live]

The others would be fixed in the JavaDoc repo, not here. So don't worry about them :)

[Wolvereness]

Your use-case/justification is seriously lacking. The idea of a despawn is extremely broad in comparison to the problem. So much so, that it appears to take the form of a band-aid instead of a solution. Despawning includes chunk unloading, which renders your entire concept of storing data irrelevant to the solution.

You want to store data? This doesn't solve that problem, therefor, this is not the proper solution.

[Black-Hole]

How about renaming this Event to EntityRemoveEvent? So it's more precisely describing when it's called.

[desht]

What if the despawn/remove event included more information about why the despawn is occurring? Chunk unloading is a fairly important case (more of a suspension than a despawn) - plugins would certainly need to distinguish this if they're tracking data for entities.

The general idea of entity removal notification is a useful one - another use-case is raised in https://bukkit.atlassian.net/browse/BUKKIT-4402 - knowing reliably when falling block entities are actually removed.

[Wolvereness]

The general idea of entity removal notification is a useful one - another use-case is raised in https://bukkit.atlassian.net/browse/BUKKIT-4402 - knowing reliably when falling block entities are actually removed.

Sounds like a different use-case with a different solution. In the case of falling blocks, they spawn an item, which is far more important than the simple fact that they are no longer in the world.

[desht]

They might spawn an item, and if they do, there's no reliable way to link the spawned item to the falling block that caused it to spawn - you can scan for all falling block entities near the newly-spawned item and if there's one of the same type then it might be the one, but that's a) not 100% reliable, and b) wasteful of CPU resources.

A way of knowing when falling blocks are removed, regardless of whether they form a block or drop an item would be extremely useful.

[Wolvereness]

A way of knowing when falling blocks are removed, regardless of whether they form a block or drop an item would be extremely useful.

Stop saying "useful" and start saying why.

[desht]

I've provided two use-cases in the JIRA ticket.

[Wolvereness]

I've provided two use-cases in the JIRA ticket.

Well, I guess it's my fault for assuming you had reasons independent of them being "falling blocks" and instead for entities in general. The first use case you had provided (for handling falling blocks without proper placement) can be solved with an event designed to solve it. The second use case (for tracking number of entities) doesn't sound useful when considering you need to be state-tracking your buffer anyway.

Both of those are tangent to this PR though - I want to see reasons why this PR, specifically, is useful.

[desht]

The second use case (for tracking number of entities) doesn't sound useful when considering you need to be state-tracking your buffer anyway.

That's kind of the point, though - without a way to know when an entity is properly removed, I can't fully state-track the entity buffer. All I can do is rate-limit the creation of new entities, not put a hard cap on the number of existing entities. This is somewhat tangential to this discussion, but not entirely - falling block entities are entities nonetheless, and I'm just providing a specific use-case which this particular PR would help with.

[Wolvereness]

I can't fully state-track the entity buffer

I do it using isValid()Z - what's your aversion to that?

[desht]

I do it using isValid()Z - what's your aversion to that?

I don't (and shouldn't need to) maintain a reference to every single falling block I created - I just need to know how many are in force. I can count when they're created, but not when they're destroyed.

If I were to maintain a list of all the falling blocks I created, I'd have to scan potentially thousands every few ticks/seconds just to check if they're still valid, which is really not very efficient or pretty - it would be more efficient just to decrement a counter when I get a hypothetical despawn event.

[Wolvereness]

If I were to maintain a list of all the falling blocks I created

You won't know which ones you created or not. Your argument now is becoming one of off-loading responsibilities.

[desht]

You won't know which ones you created or not.

I don't need to. Entities are Metadatable, so I could tag the ones I create. If/when I get a removal event, I can decrement my counter if the entity is so tagged. And I don't see that as offloading responsibilities. I guess we may have to agree to disagree about that.

[mcmonkey4eva]

@Wolvereness - Good question (or complaint or issue or ... whatever you call what you said), I didn't think about chunk unloading.
I like @desht 's solution of a despawn reason and have added an initial implementation of that. ATM it's just CHUNK_UNLOAD and DEFAULT, which is sufficient to fix the issue with data being lost when an entity is despawned by a simple chunk unload.
Tested and confirmed that an entity removed by chunk unload is marked as CHUNK_UNLOAD, and an entity removed otherwise is not.

    @EventHandler
    public void onEntityDespawn(EntityDespawnEvent event) {
        if (event.getDespawnReason() == EntityDespawnEvent.DespawnReason.CHUNK_UNLOAD) {
            if (!(event.getEntity() instanceof LivingEntity) || ((LivingEntity)event.getEntity()).getRemoveWhenFarAway())
                unlinkEntity(event.getEntity());
        }
        else {
            unlinkEntity(event.getEntity());
        }
    }

A zombie and a pig were placed in a container, I flew away to unload the chunk - the zombie was unlinked, the pig was not.
(To retest the code, you could implement unlinkEntity as a simple debug message of the UUID)

Other despawn reasons might be useful (EG living entity death, item pickup), but I don't want to go around adding all other possible reasons and trying to implement them correctly unless it's confirmed that would be useful.
(Could even go in a separate PR, to enhance the detail of the EntityDespawnEvent after it's added)

Also, I based my DespawnReason enum off the SpawnReason enum for CreatureSpawnEvent.

[feildmaster]

I don't need to. Entities are Metadatable, so I could tag the ones I create. If/when I get a removal event, I can decrement my counter if the entity is so tagged. And I don't see that as offloading responsibilities. I guess we may have to agree to disagree about that.

Then you lose your meta at server shutdown AND you have no way of finding them again on start because there's no spawn event...

When doing things that involve entities it's 100% required that you manage a UUID list at the very least.

[desht]

@feildmaster given that FallingBlock entities are by their nature very short-lived, it's not a major issue in this particular case. Lack of tracking of any in-flight falling blocks when the server restarts will at worst result in a brief interval where the number of falling block entities created by the plugin exceeds the limit configured by the plugin config.

However, that's all pretty academic. Assuming that I was to track the UUID for every single falling block that my plugin created: without an EntityDespawnEvent, I need to check that buffer (which could contain hundreds or even thousands of entity UUID's) ideally every single tick, and remove any non-valid entities. With a despawn event, I could just remove entities from the buffer as they despawn.

Surely it's better to have plugins notified of entities despawning than require them to regularly poll for despawning?

[feildmaster]

This PR isn't just about FallingBlocks though... In the case of FallingBlocks, yes, they are short lived.

You could totally just store the entity itself and check isValid.

[desht]

@feildmaster yes, I appreciate that. As I mentioned before, I'm just discussing one particular use-case of despawn events. But the principle of being notified about the despawn of tracked entities vs. having to poll for valid entities applies in general, especially if a plugin is tracking a large number of entities.

[turt2live]

This is not just fired for creatures - is it?

[mcmonkey4eva]

@turt2live - Corrected everything you said.