kreezxil / Lava-Dynamics

Adding Vulcanism to Minecraft via Block Update Detection (BUD)
3 stars 3 forks source link

Lava Dynamics making Storage Drawers not work #2

Closed purestone9 closed 6 years ago

purestone9 commented 6 years ago

https://github.com/jaquadro/StorageDrawers/issues/645

kreezxil commented 6 years ago

I've inspected Jaquadro's api and mod and my mod and I don't see where the conflict to cause this. Although our two mods conflict to cause this. I thought it was something to do with a right click mechanic. However, Primal Core and Primal Tech both use right click events to receive items into their machines (tile entities) and they work without the odd placement behavior.

So as far as my capabilities go I can't fix it and therefore won't fix it. However, if someone with sufficient skill is willing to fix it that would be great.

For now It is simply a mod incompatibility.

CplPibald commented 6 years ago

The problem is in this line. Passing null as the first parameter to getActualState() causes any block that has an actualstate distinct from its blockstate to throw an NPE when it calls state.withProperty() in response to this code, which fires on every block event. One such block is BlockGrass, which sends a block event when it attempts to spread. This cause a crash pretty much on world load.

The apparent fix in the code is to wrap the call in a try/catch block. Minecraft is throwing dozens of exceptions per tick (one for each block event), which are getting silently dropped, preventing the crash. Unfortunately, any other side-effects of these exceptions are not being cleaned up.

One such side-effect is in StorageDrawers, in which getActualState() loads the drawer tile entity to query its state. When the tile entity throws an exception, the tile entity gets deleted by Minecraft (or Forge? not sure). The exception continues out and is swallowed by the catch statement above, but the result is a storage drawer whose tile entity has been deleted.

The solution is to fix the incorrect blockstate call, then to remove the try/catch block that is silently swallowing exceptions. In this case, the call to getActualState() is unnecessary, because the only block the code cares about is lava, which does not have a separate actualstate.

I submitted a pull request which implements this fix. I tested it with Storage Drawers and they appear to work correctly.

kreezxil commented 6 years ago

Thank you, I'll merge this pull request shortly.

On Fri, May 18, 2018 at 2:09 PM, Sir Ryan Bemrose notifications@github.com wrote:

The problem is in this line https://github.com/kreezxil/Lava-Dynamics/blob/master/src/main/java/com/kreezcraft/lavadynamics/LavaLove.java#L66. Passing null as the first parameter to getActualState() causes any block that has an actualstate distinct from its blockstate to throw an NPE when it calls state.withProperty() in response to this code, which fires on every block event. One such block is BlockGrass, which sends a block event when it attempts to spread. This cause a crash pretty much on world load.

The apparent fix in the code is to wrap the call in a try/catch block. Minecraft is throwing dozens of exceptions per tick (one for each block event), which are getting silently dropped, preventing the crash. Unfortunately, any other side-effects of these exceptions are not being cleaned up.

One such side-effect is in StorageDrawers, in which getActualState() loads the drawer tile entity to query its state. When the tile entity throws an exception, the tile entity gets deleted by Minecraft (or Forge? not sure). The exception continues out and is swallowed by the catch statement above, but the result is a storage drawer whose tile entity has been deleted.

The solution is to fix the incorrect blockstate call, then to remove the try/catch block that is silently swallowing exceptions. In this case, the call to getActualState() is unnecessary, because the only block the code cares about is lava, which does not have a separate actualstate.

I submitted a pull request which implements this fix. I tested it with Storage Drawers and they appear to work correctly.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/kreezxil/Lava-Dynamics/issues/2#issuecomment-390304373, or mute the thread https://github.com/notifications/unsubscribe-auth/ADs0iDtT_FdmsZyl5mcNFX9OpJ0jK5tTks5tzxxhgaJpZM4T2sDM .