Modified despawnEntity() in EntityLiving to allow forge event to be called in all ticks

[MrChoke72]

Modified despawnEntity() to allow forge AllowDespawn event to be called every tick, not just every 32nd one.

This is a fix to issues #4485 (closed) and #5148

[CLAassistant]

All committers have signed the CLA.

[MrChoke72]

Can someone tell me what next steps are for getting a pull request put in?

Thanks.

[liach]

Any use case @MrChoke72? I fail to see one.

P.S. It was added in 6da6e9d

[MrChoke72]

Any use case @MrChoke72? I fail to see one.

P.S. It was added in 6da6e9d

The use case is we are incapable of modifying default behavior as it relates to distance checks for despawn. Example 1, the 128 block away forced despawn. Example 2, the 32 block away random check for despawn. We can override idleTime to affect that varaible but that is it. With this fix, we can.

So the link you gave, is that coming in 1.13? I don't think its quite the same fix though is it?

P.S. I see you are modding Railcraft. Never heard of it but I am going check that mod out!

[liach]

After another glance, this 31 tick check is not necessary at all, because with this check mods can never prevent vanilla behaviors.

[liach]

That commit was an old one when the event was added

[MrChoke72]

Exactly. So I just removed the 31 tick check. Maybe it was there for performance but I don't think its that critical.

So how do pull requests work exactly? Are you admin type that can merge it in?

[liach]

I am not an admin. You are supposed to wait very patiently over a period of time (may be over one month). At the time for batch merging, LexManos will either point out issues with your code or merge it if this pr appears fine. By the way, you can go to forge discord to ask for reviews, too. Sometimes, mezz will come to review your pull request before Lexmanos does.

[MrChoke72]

I am not an admin. You are supposed to wait very patiently over a period of time (may be over one month). At the time for batch merging, LexManos will either point out issues with your code or merge it if this pr appears fine. By the way, you can go to forge discord to ask for reviews, too. Sometimes, mezz will come to review your pull request before Lexmanos does.

Ok, thanks.

[LexManos]

This will not go in with this event firing every tick. The limiter was added for performance reasons.
You have yet to provide a satisfactory example of what you're trying to do that you can not do.
End of story.

[liach]

I just want to say that after this if block, there is a piece of vanilla behavior.

https://github.com/MinecraftForge/MinecraftForge/pull/5150/files#diff-01fe66a544382ec9b519f85d84052c28R40

With the tick check limit, mods cannot override vanilla behavior. The vanilla behavior may let the mob despawn before the mod listening to the event could have made a choice.

[MrChoke72]

This will not go in with this event firing every tick. The limiter was added for performance reasons.
You have yet to provide a satisfactory example of what you're trying to do that you can not do.
End of story.

I am not sure how else I can explain it. If the despawn handler is not called every tick then whatever you do in it is overridden by the very next tick. My example is that of extending the 128 block distance for a mob to automatically despawn. You cannot do it. Let's say your event handler fires and returns DENY when the distance just exceeded 128. On the very next tick the default handler is going to fire and the mob is gone. And it is IMPOSSIBLE to stop it given the current design. It makes LivingSpawnEvent.AllowDespawn only partially usable.

If you don't believe this happens, try it out. I assure you it does. As far as performance, yes I understand why it is there. But to remove it is just part of what modders have to juggle all the time in their mods. Do not write slow code when its executed often (i.e. every tick).

[Alpvax]

Could we make it so that returning DENY causes the despawn to be delayed 32 ticks.
That way we keep the performance benefit, and only reduce a counter the remainder of the time

[ahall72]

Could we make it so that returning DENY causes the despawn to be delayed 32 ticks.
That way we keep the performance benefit, and only reduce a counter the remainder of the time

(I am the author of the pull, MrChoke72, on my work GIT), That is a great solution. As long as the event handler is given the opportunity to run BEFORE the default behavior that will work.

@LexManos, how about this? A DENY will force no execution of the event handler OR the default behavior for 32 ticks. I can implement this and do a new pull.