maruohon / itemscroller

A client-side Minecraft mod that adds various convenient ways of moving items within inventory GUIs, such as scrolling over stacks to move single items to or from it
GNU Lesser General Public License v3.0
147 stars 69 forks source link

Crashing from using mouse scroll wheel in an Applied Energistics crafting terminal. #41

Open IchigoGames opened 3 years ago

IchigoGames commented 3 years ago

I am having an issue with trying to use the mouse scroll wheel in an Applied Energistics crafting terminal. Everytime I try to scroll while the mouse is in the middle of the screen the game crashes. Happens on server and single player. Happens on multiple computers.

This is from a player who plays my modpack after looking at the crash report i think its a compatibility issues with this mod and AE2

Minecraft 1.15.2 Forge 31.2.36 appliedenergistics2-7.0.1 itemscroller-forge-1.15.2-0.15.0-dev.20200412.215325 https://pastebin.com/KQT20GqL

maruohon commented 3 years ago

This looks like a bug in AE2, as it's crashing in the Forge SlotItemHandler code here: https://github.com/MinecraftForge/MinecraftForge/blob/5224533d45ad820121deb09f2419b8680f7b5f3b/src/main/java/net/minecraftforge/items/SlotItemHandler.java#L85

Meaning that they must be using the slots with a null IItemHandler reference, so any mod calling some of those methods on the slots would cause a similar crash. Maybe they didn't expect those methods to be called on the client side?

IchigoGames commented 3 years ago

ok thank you for looking at it ill report it on there page.

maruohon commented 3 years ago

Do you know if this was also an issue in 1.12.2? Or did something change in one of the mods since then so that it only now started happening?

shartte commented 3 years ago

I checked our code. I think when AE2 internally changed from IInventory over to IItemHandler, someone was a bit overzealous and also changed the base-class for ME Terminal slots from Slot to SlotItemHandler, even though we actually pass a null IInventory inventory to the Slot anyway, since slots in an ME Terminal are not backed by one.

The problem is though, getItemStackLimit (which we dont override) in SlotItemHandler does not just delegate to getSlotStackLimit (which we override) like the old vanilla Slots do. Instead it tries to query the handler (which is null).

That change was made in 2017 (1.12).

maruohon commented 3 years ago

Okay it looks like I had the ME terminal slots blacklisted back in 1.12.2, but due to all the changes in the later LiteLoader and then Fabric versions, (which the 1.14.4+ Forge versions are now based on) the current 1.15.2 Forge version also has an empty blacklist by default currently. I'll need to restore the default values to that list and add it back to the config screen too.

shartte commented 3 years ago

We'll fix the bug on our end, actually. Are you doing your versions cross-version? It's possible we won't release a 1.12 bugfix, but 1.15-1.16 should receive the fix. p.s.: Unless I am unaware of some other reason it should not work for ME terminal slots, but if your mod emulates what a player does (pick stuff up), I don't see why it should not work. The only problem is that you're not likely to be able to "insert" this way, since the slots return 0 as their max count.

shartte commented 3 years ago

Actually, after checking it again, you might as well blacklist them, we handle the extraction in the terminal screen, so the slots are probably not worth anything to you.

maruohon commented 3 years ago

AFAIK this should not affect the 1.12.2 version which is on CurseForge, ie. 0.12.0, it only affects the later versions based on the largely rewritten codebase in the LiteLoader branch and thus the versions that are based on that code base. There are 1.12.2 Forge versions with the LiteLoader branch merged back, but they are currently only on my own server, ie. not widely used. So basically don't worry about 1.12.2.

I can restore the GUI and slot blacklisting to the current Item Scroller versions. Ideally it would be nice if the slots don't cause an NPE when the normal public methods are called, but I'm not sure if you can easily do that in your case?

shartte commented 3 years ago

It's easy enough. I'll just change back to extending Vanilla Slot and it'll be fixed. But first I'll actually have to go back and check which classes are affected by this.