Modular Routers

Modular Routers

33M Downloads

[?~1.16.4] Mimic argument cause client crash when used with immersive engineering

mmis1000 opened this issue ยท 6 comments

commented

Minecraft Version

1.16.4

Forge Version

net.minecraftforge:35.1.36

Mod Version

1.16.4-7.3.0-47

Describe your problem, including steps to reproduce it

  1. Install this mod with immersive engineering
  2. Put the router on ground
  3. Config the Extruder MKII with mimic argument
  4. Try to break the block
  5. Client crashed (server is fine, but probably just because client crash first?)

Any other comments?

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

The modular router invoked the getDigSpeed with a null position(because it did not matter here?)

event.setNewSpeed(event.getPlayer().getDigSpeed(camoState, null));

But the immersive engineer listens the BreakSpeed event and tries to read the entity info with given location

# The function in the stack
MC 1.16.3: net/minecraft/world/World.getTileEntity
Name: c => func_175625_s => getTileEntity

Hence crash the client (The pos is null).

I am not sure which is the fault here.
Neither do I know whether that field is supposed to be null-able.
Either the Modular Router pass null as a non null-able parameter, or the Immersive engineer use a null-able parameter without check first?

The code is conflicting that, the pos in EntityPlayer.getDigSpeed is indeed @Nullable.
But the BreakSpeed event says you should use a BlockPos that has y == -1 for unknown location

commented

Probably Modular Routers at fault here (although as you say, the blockpos is explicitly marked as @Nullable). Can't remember why I chose null there, probably because originally I used the method variant just taking a BlockState (back before it was deprecated):

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

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

However, I think it should be OK to pass event.getPos() in there, or failing that a pos with Y == -1. I'll do some testing with that anyway.

commented

@desht
I also notices that there is a static field ZERO in the BlockPos class.
And the comment says that is intended to be used as a unknown position.
(But that is probably unsafe in a mod server or even in vanilla? That said [0, 0, 0] is a valid location)
I am actually ultimately confused which is the proper value for an unknown position.
The codes conflicts with itself everywhere as far as I see.

commented

After a bit of investigation, I'm settling on passing a (0,-1,0) blockpos, which as the event javadoc states, is an "unknown" position. I definitely can't pass getPos() again, since that would lead to an infinite event loop...

This should fix the crash, since world.getTileEntity() (as IE does in its handler) will just return null for an out-of-bounds Y pos, which is fine here.

The fix is in build 49 from https://jenkins.k-4u.nl/job/Modular%20Routers%201.14+/ if you'd like to try it out.

commented

Well, BlockPos.ZERO would be generally invalid in regular worlds, since that location would contain bedrock and shouldn't be interacted with. But it could be valid in a void world, so it's not ideal to use as a special marker value...

commented

Fixed in 7.3.1 release

commented

Seems no longer a client crash.

Server upgrade seems required.
This logic also affects server and prevent anyone from breaking the fake block.
(However it won't crash the server, just log spam)