mekanism / Mekanism

A mod for Minecraft
MIT License
1.4k stars 531 forks source link

Fixed render crash when placing Universal Cables on ValkyrienSkies ships #8260

Closed Andy608 closed 1 week ago

Andy608 commented 1 week ago

Changes proposed in this pull request:

Now with this fix, the client no longer crashes and players are able to place cables on VS ships.

Andy608 commented 1 week ago

This fix is in response to the open issue on the ValkyrienSkies github page: https://github.com/ValkyrienSkies/Valkyrien-Skies-2/issues/558

thiakil commented 1 week ago

Valkyrien Skies should be checking net.minecraft.client.renderer.blockentity.BlockEntityRenderer#shouldRender which runs through mekanism.client.render.transmitter.RenderUniversalCable#shouldRenderTransmitter and would not return true if there's no network.

Andy608 commented 1 week ago

Sounds good, thank you! I have been trying to get ValkyrienSkies to compile with Mekanism as a runtime mod in their repo, but I continue to get this error when running the game:

Caused by: java.lang.NoSuchMethodError: 'java.lang.String net.minecraft.Util.m137492(java.lang.String, net.minecraft.resources.ResourceLocation)'

image

Has anyone run into this error before? I would love to investigate this issue from the VS side of things, but I can't seem to get it to compile properly. I had no issues getting ValkyrienSkies running in mekanism's build.gradle, so I'm not quite sure what I'm missing. Any help is appreciated. Thanks!

Andy608 commented 1 week ago

Okay figured it out, and have submitted a fix on the VS side here: https://github.com/ValkyrienSkies/Valkyrien-Skies-2/pull/1006

Andy608 commented 1 week ago

After further investigation, it turns out the VS side change isn’t the ideal fix. Using shouldRender() causes beds and other uniquely shaped objects to become invisible. I believe adding a proper null check here would be sufficient in this case, though I’m definitely open to other suggestions if anyone has ideas for a better solution.

I also see render() calls shouldRender() so I'm still uncertain how the cable's render is even getting called. I'll need to do some more investigating on this.

For now, I’m using a custom-built version of Mekanism with the null check locally to prevent crashes.

thiakil commented 1 week ago

can it actually be reproduced with only Mekanism and VS? They're right that it shouldn't be needed, but its either not being called or there's a race condition somewhere

Andy608 commented 1 week ago

Yee it occurs with only VS and Mekanism installed together. I found that adding shouldRender() to MekanismTileEntityRenderer fixes the crash (see most recent commit)

thiakil commented 1 week ago

Can you send a crashlog with just Mek and VS?

Andy608 commented 1 week ago

Sure thing, here's the crash log with Mek/VS/KotlinForForge crash-2024-11-18_11.41.28-client.txt

thiakil commented 1 week ago

should be fixed in 3d95fbab (your PR contains too many extras and is targeted at the wrong branch)

Looks to indeed be a weird race condition that only happens with VS.