MineMaarten / PneumaticCraft

PneumaticCraft source
Other
84 stars 51 forks source link

NEI crash on recipe display when installed along side Rotarycraft. #675

Closed Starhero closed 9 years ago

Starhero commented 9 years ago

Just spoke to reike in TS3 and he stated the crash that follows has been identified as a mod interaction from Pneumaticraft.

The clues I have is something to do with fluids.

This breaks NEI completely.

http://skidpaste.org/mGyHfhIE

If this is double post, sorry, this just cripples my server at the moment.

Also, this seems to happen only in SMP?!? I tested this and thought i fixed it with a mod update run for my friend, yet he still crashed when on the server. I then tested this same thing and got the crash on the server, I haven't tested SSP again.

MineMaarten commented 9 years ago

After talks with @Starhero you sent him here @ReikaKalseki ? I don't get how I'm involved in this? Could you explain what the problem is? The line given in the stacktrace doesn't give much info.

ReikaKalseki commented 9 years ago

It is because of that remove-and-unregister you do to lubricant if BC is detected.

The RC drying bed has a recipe for drying lubricant into rubber. Internally, the recipe boils down to a specialized HashMap (approximately Map[Fluid, TreeMap[Integer, ItemStack]]). When you remove your fluid, and a different lubricant is registered, the FluidRegistry.get(name) calls return a different object but the map keys remain the original objects, causing things like map.get(fluid) to falsely return null. In this case, iterating over the map keyset, and then calling .get() on those keys, returns null, something that should never happen.

The correct way to fix this is not to do that - rather than removing your fluid, simply opt not to register it if BC is detected. You will probably need to change some handling logic to accommodate that.

MineMaarten commented 9 years ago

Right, gotcha. I indeed am not fully sure about my fluid implementation. For example, https://github.com/MineMaarten/PneumaticCraft/issues/634

I'll take a look to see how to solve this.

ReikaKalseki commented 9 years ago

I would also make it so that RC also triggers the "do not make custom lubricant" code, since PnC lubricant currently breaks several RC machines. As I said before, however, I will be changing that fluid name, so this would be somewhat temporary (i.e. a few months).

ReikaKalseki commented 9 years ago

Has there been a release since? The bug is still happening, and some logging I put in confirms that the fluid swapping is still taking place.

java.lang.IllegalStateException: Looking up recipe for 64xtile.pylon@0: 100xfluid.lubricant(Reika.DragonAPI.Instantiable.EnhancedFluid@1bcaabd6)[135]{null}, despite being in the keyset of the map, returned null on get()!

Map data: {fluid.tile.water(net.minecraftforge.fluids.FluidRegistry$1@795f8317)[1]={250=Reika.RotaryCraft.Auxiliary.RecipeManagers.RecipesDryingBed$DryingRecipe@b015f1a}, fluid.tile.lava(net.minecraftforge.fluids.FluidRegistry$2@67531e3a)[2]={1000=Reika.RotaryCraft.Auxiliary.RecipeManagers.RecipesDryingBed$DryingRecipe@1636ef44}, fluid.lubricant(pneumaticCraft.common.fluid.FluidPneumaticCraft@7fc6d2fb)[135]={100=Reika.RotaryCraft.Auxiliary.RecipeManagers.RecipesDryingBed$DryingRecipe@379a2030}, fluid.oil(net.minecraftforge.fluids.Fluid@62072869)[118]={200=Reika.RotaryCraft.Auxiliary.RecipeManagers.RecipesDryingBed$DryingRecipe@1e878b09}, fluid.honey(net.minecraftforge.fluids.Fluid@157f3d61)[8]={400=Reika.RotaryCraft.Auxiliary.RecipeManagers.RecipesDryingBed$DryingRecipe@1fffee4c}, fluid.for.honey(net.minecraftforge.fluids.Fluid@7c39b7a6)[7]={400=Reika.RotaryCraft.Auxiliary.RecipeManagers.RecipesDryingBed$DryingRecipe@3e926e38}, }

Note that ID 135 (lubricant) is one fluid type in the map data and another in the lookup (indicating a swap between mod init and game load completion).

Line 4938 of this log has the same thing.

In case it is not clear, the format of the bracket lines is:

And the map data is a HashMap<Fluid, TreeMap<Integer, RecipeObject>>.

See here for its class.

MineMaarten commented 9 years ago

So... After I updated the fluid handling hoping to fix this, I've asked CPW about it who gave me the 'seal of approval'. Afaik things are done correctly now.

Now just by references to your code I can't really see if (or what) I've done wrong. The log states it is using PneumaticCraft's Lubricant, which on its own isn't bad, that's what the fluid system should allow.

So let me instead link to how fluids are handled in PneumaticCraft nowadays, and you could tell me what I've done wrong. https://github.com/MineMaarten/PneumaticCraft/blob/master/src/pneumaticCraft/common/fluid/Fluids.java

It comes down to every fluid I add will be registered as well, and I let the FluidRegistry decide which fluid to use (which happens at world load). I can't see anything going wrong.

ReikaKalseki commented 9 years ago

Your fluid objects register themselves in their constructors, and since they are static fields you construct them at the moment your mod class is initialized (ie extremely early in the loading process).

Though I do not know for sure, I have never seen this done before and have reservations about its safety. Try moving it to init().

All I know for sure is that the return value for FluidRegistry.getFluid(name) is different at two points in the loading.

ReikaKalseki commented 9 years ago

I have changed all my fluid IDs to help avoid future issues.

MineMaarten commented 9 years ago

All I know for sure is that the return value for FluidRegistry.getFluid(name) is different at two points in the loading.

That's because that's what the FluidRegistry can do. It loads the world save (upon world load), and takes the fluid default of that world. So the default fluid can change on world load, which is part of the behaviour of the fluid system. (look at the NBT load method in FluidRegistry for a reference)

ReikaKalseki commented 9 years ago

That's because that's what the FluidRegistry can do.

What.

MineMaarten commented 9 years ago

look at the NBT load method in FluidRegistry for a reference

ReikaKalseki commented 9 years ago

There are no methods in FluidRegistry dealing with NBT.

MineMaarten commented 9 years ago

FluidRegistry line 299, public static void loadFluidDefaults(NBTTagCompound tag)

line 318, public static void writeDefaultFluidList(NBTTagCompound forgeData)

ReikaKalseki commented 9 years ago

I have no such methods. Presumably these were added since 1291 (which is what I use).

I also suspect that this change - from where fluids are effectively immutable to dynamic - is the problem.

Also, who the hell thought changing that was a good idea?!

MineMaarten commented 9 years ago

Presumably these were added since 1291 (which is what I use).

That could be very much the case, yes. I'm running 1388.

ReikaKalseki commented 9 years ago

Any idea why they added this, given how badly it breaks a previous contract?

MineMaarten commented 9 years ago

I don't I'm afraid. It's really not my call either really ;). I'm just here to inform you about what happened :P. For info why you'll have to contact them.

AEnterprise commented 9 years ago

as far as i know the change was made to prevent and fix issues caused when mods where added and the 'default' fluid in the world changed due to that, now each time the world loads the same fluid will be used (there where some other things about it as well but that's the main reason afaik)

ReikaKalseki commented 9 years ago

Right...well, hopefully the issue is now fixed given that I 1) changed my FluidHashMap to use strings, not fluid objects, and 2) changed all my fluid IDs.