Immersive Engineering

Immersive Engineering

134M Downloads

[? ~ 1.16.4] crashed when break virtual block generated with mimic argument from ModularRouters

mmis1000 opened this issue ยท 5 comments

commented

Description of the issue:

When used with ModularRouters's mimic argument.
Try to break fake block generated with ModularRouters crashed the client

Crashlog:

https://gist.github.com/mmis1000/671d05966ebbe60cae980a98cbf584db

Versions & Modlist

FTB Presents Direwolf20 1.16 - 1.4.1
https://www.feed-the-beast.com/modpack/ftb_presents_direwolf20_1_16

Lastly

The ImmersiveEngineering assumes BreakSpeed.getPos() to be non-nullable.
And try to use it on World.getTileEntity(BlockPos pos)

But it seems it isn't the case.
It turns out the pos argument in EntityPlayer.getDigSpeed(BlockState state, BlockPos pos) is indeed nullable.
And thus the BreakSpeed.getPos() be nullable.
(Is the inconsistent doc here forge's fault?)

More details in
desht/ModularRouters#113

commented

Shouldn't be necessary to change anything in IE... I'm changing Modular Routers to pass (0,-1,0) as the blockpos instead of null (a Y==-1 value is documented in the BreakSpeed event as a "don't know" value). Your getTileEntity() call will thus return null, which is fine for this case.

(More information about the background of this in the linked issue)

commented

Sounds like not us then

commented

While my change to Modular Routers fixed this particular crash, it would probably be wise to verify event.getPos() isn't null before using it to get a tile entity (https://github.com/BluSunrize/ImmersiveEngineering/blob/1.16.5/src/main/java/blusunrize/immersiveengineering/common/EventHandler.java#L324)

The problem being the getDigSpeed() methods in PlayerEntity:

@Deprecated //Use location sensitive version below
public float getDigSpeed(BlockState state) {
   return getDigSpeed(state, null);
}

public float getDigSpeed(BlockState state, @Nullable BlockPos pos) {
  // ...
}

Since the pos parameter is nullable, mods are within their rights to believe null can be passed here, and that will cause your event handler to crash. (The javadoc for PlayerEvent.BreakSpeed should probably note that pos can be null here).

commented

Ah, I had seen something about @Nullable when I glanced over the PNC issue, but only checked the event itself. I can add a null check later, and I'll probably open a Forge issue for it as well.

commented

I think Curle mentioned an omnibus PR on Discord which would probably be a good place to get this in.

Update: aha - MinecraftForge/MinecraftForge#7612