search

BrassGoggledCoders / Transport

Transport of all kinds in Minecraft
MIT License
4 stars 4 forks source link

[1.16.5] Mod interaction crash with Project Red #46

Closed ArcanoxDragon closed 3 years ago

ArcanoxDragon commented 3 years ago

When Transport and Project Red are both installed, any attempt to open the GUI for a Project Red gate will crash the game with the following stack trace: https://hastebin.com/cenisecuku.properties

This appears to be due to the fact that Project Red uses -1 for a windowId (containerId using Mojang mappings) for their NodeContainer GUI, and Transport also uses that for the EMPTY instance of ModularScreenInfo. This allows the first check here: https://github.com/BrassGoggledCoders/Transport/blob/develop/1.16.x/src/main/java/xyz/brassgoggledcoders/transport/screen/ModularScreenInfo.java#L49 to pass, and fall to the second check using getType() which (for some reason, thanks to Mojang) throws an exception for Container instances with a null containerType (menuType w/ Mojang), which is the case for PR's NodeContainer. It's obviously a multi-layer problem with no clear culprit, but logically it seems like the best workaround is on Transport's part by adding a preliminary check to the ModularScreenInfo.matches method which immediately returns false if current == EMPTY. I haven't tested this fix, but this should prevent Transport from attempting to call into any GUIs at all from other mods.

I managed to work around this temporarily on Project Red's end by changing the NodeContainer containerId to -2 instead of -1, but I don't think this is the appropriate permanent fix, because there's always a possibility that another mod may do the same thing. I would argue that Transport should not be touching any other mod's GUI/Container classes unless that mod has a dependency on Transport.

This can be reproduced as follows:

  1. Install Transport 3.6.0
  2. Build Project Red from source using this branch (has some necessary fixes I've made recently) and install all 6 sub-mods for PR
  3. Place down a gate with a GUI from Project Red such as a Timer
  4. Right-click the gate to open the GUI
  5. The client should crash with a similar stack trace to the one I posted above
SkySom commented 3 years ago

I'm curious why Project Red is setting their own windowId, since that generally comes from Minecraft itself? Regardless, I can be a little bit more defensive against mods doing silliness like that.

SkySom commented 3 years ago

And the reason it's checking that is like in the case of Chest carts, that is Minecraft's chest gui, I'm merely overlaying it when it was opened by my carts, so that'd check wouldn't work.

SkySom commented 3 years ago

Honest answer looking at this a bit, ProjectRed is being dumb by not having a ContainerType. MC expects it to not be null. I can only assume you're manually creating your containers or you'd be crashing when trying to use MC's code to open them.

ArcanoxDragon commented 3 years ago

I'm not entirely familiar with PR's GUI system, but it seems they use a generic system that inherits from ContainerScreen even for GUIs that are not associated with a container, so that some of their GUIs can have a container. For ones that do not, they get a Container with a null type (a sort of dummy container); I'll discuss what to do with the main Project Red devs to see if there's a better way to handle this on their end

SkySom commented 3 years ago

I'd definitely suggest just registering a type, even if it's just a dummy one. While I can fix stuff on my end, having it null is just asking for potential issues to pop up everywhere like this.