Question on Forge's mod to getTileEntities() in WorldServer.java

[mikeprimm]

This is specific to https://github.com/MinecraftForge/MinecraftForge/blob/master/patches/minecraft/net/minecraft/world/WorldServer.java.patch#L171.

 

Over on Spigot, we developed our own version of this performance patch before realizing that the Forge team had already discovered and addressed it.  One debate we're still having is on the need for the 'isInvalid()' check that is being made in the Forge version - I've been on the page that you guys have had it for a while, so I'm inclined to trust that you had a good reason, but one of my counterpart is arguing that it doesn't appear to be necessary.  If anyone involved with this change can recall, was there a particular need or issue addressed by checking for the isInvalid()?  The cases my associate has identified appear to avoid the issue of the invalid TEs being on the chunk's list by the time the call is used, but I'm not confident that all the cases have been considered.

 

If you're interested, the discussion is on this PR - https://github.com/EcoCityCraft/Spigot/pull/21#discussion_r3192913.  Thanks for any insight that can be offered.

[LexManos]

Due to several changes where we defer removal until later times, the tile entities in the chunk can be invalid, so yes, this is needed.

[mikeprimm]

Due to several changes where we defer removal until later times, the tile entities in the chunk can be invalid, so yes, this is needed.

Super - thanks for the info.  So, just to be 100% clear, to your knowledge, it was needed in Forge due to other Forge-related changes, versus being needed to preserved "vanilla" behavior, correct?

[LexManos]

That, i haven't looked into for a long time, as far as I remember invalid entities should not be in that list in vanilla. So, in vanilla it is not needed.

[mikeprimm]

Great - thanks again!