Potion ID generation generates invalid potion ids.

[chrisbecke]

Steps to reproduce: Install the Direwolf20 pack for MC 1.7.10 - I used Direwolf20 1.3.1 Add the AM2.jar file to the mods folder, but rename it to z_AM2.jar to ensure that forge loads it last (Forge loads mods in alphabetical order quite reliably). Launch Minecraft and examine the forge fml-client-latest.log after starting a game.

Everything worked correctly AM2 was loaded last and allocated itself Potion IDs in a unique, non conflicting range. HOWEVER: these lines are logged:

[20:06:08] [Client thread/INFO] [FML/arsmagica2]: Ars Magica >> Extending Potions Array
[20:06:08] [Client thread/INFO] [FML/arsmagica2]: Ars Magica >> injecting potions starting from index 352
[20:06:08] [Client thread/TRACE] [FML/arsmagica2]: Ars Magica >> Instantiating Items
[20:06:08] [Client thread/INFO] [FML/arsmagica2]: AMCore >> Altering definition of am2.armor.ItemMageHood to be thaumcraft compatible.
[20:06:08] [Client thread/INFO] [FML/arsmagica2]: Ars Magica 2 >> Attempting to set enchantment magic_resist to ID 100 (configured currently as 100)
[20:06:08] [Client thread/INFO] [FML/arsmagica2]: Ars Magica 2 >> Successfully registered enchanment!

Success - the potion id array was extended and there are no array out of bounds exceptions:

The problem here is that minecraft itself treats all potion ids as modulus 256, so when AM2 potion effects are invoked, the (effect_id %256) effect (if it exists) is invoked instead.

[chrisbecke]

A further observation is that the potion range extension calculation is done independently of the configured potion ids. So, even if I renumber the potion ids to be 128+, the log message still indicates that AM2 has extended the array starting from index 352.

[chrisbecke]

And a further wrinkle is the fact that simply extending the potion id range is not enough: when the potion id is actually applied it is treated as an int8 - a signed quantity, so

I:waterygrave=164

triggers an exception:

java.lang.ArrayIndexOutOfBoundsException: -92
at net.minecraft.potion.PotionEffect.func_76455_a(PotionEffect.java:105)
at net.minecraft.entity.EntityLivingBase.func_70679_bo(EntityLivingBase.java:524)

164, cast to a signed 8 bit int, is -92

[Mithion]

I came across the same thing a few weeks ago myself. I've re-tooled the potion ID support to be better.

[theProphet666]

When is 1.4.0.009 coming out? Any word?

[jjtParadox]

This may have been fixed by #1240, but it still needs to be confirmed. I'll try some testing and get back with results.

[jjtParadox]

Hmm, well at least AM2 is now reporting if a potion is being overwritten. It appears that AM2 still tries to expand the potions array (BuffList.extendPotionsArray()), so this may still be an issue. @Mithion and @DoomFruit, do you have any input on this?

[Mithion]

It just expands it to maximum length regardless. There is no problem with doing so, in fact there are other mods that do so regularly without issue.

[jjtParadox]

Looks like potion IDs are still treated as a signed 8bit quantity, but if they are over 127 they are re-allocated by potion allocation code. Is this still a bug?

[Mithion]

Let's just extend it to 127 then if it isn't already, instead of 255.

[DoomFruit]

In my fully modded test world (and the server that I run), most of AM2's potions are at 128 or above and I don't get any errors from it. Some of the other mods might alter the potions array (I don't know what Botania or Blood Magic are doing, but they both picked IDs well above the default size of 32) and I know for a fact that DragonAPI (pre-requisite for RotaryCraft) also extends it to 255.

Looking at the order of things in the log, DragonAPI seems to get its hooks into several potion-related things first, but it doesn't try to actually extend the array until after AM2 has already done it. The only other potion-related thing in my startup log is Sanguimancy going "OMG THE SKY IS FALLING" when it makes an array of length 128 and tries to batch copy the existing MC potions array into it - see lines 26 and 31 at https://github.com/Tombenpotter/Sanguimancy/blob/master/src/main/java/tombenpotter/sanguimancy/registry/PotionsRegistry.java

For reference, NEI's potions dump in my test world is: http://pastebin.com/YSiNc1F4 , the relevant parts in AM2's startup log is: http://pastebin.com/CBB0LRiL and the relevant entry in my AM2 config file is: http://pastebin.com/YbPC4dMZ

I'll test what happens in the dev environment with potion IDs forced to be above 128, but it doesn't appear to cause any issues in a heavily modded environment.

[DoomFruit]

OK, tested it and I do get a crash in the dev environment when using potions with an ID greater than 128:

Time: 23/09/15 09:39
Description: Ticking entity

java.lang.ArrayIndexOutOfBoundsException: -128
        at net.minecraft.potion.PotionEffect.onUpdate(PotionEffect.java:105)
        at net.minecraft.entity.EntityLivingBase.updatePotionEffects(EntityLivingBase.java:524)
        at net.minecraft.entity.EntityLivingBase.onEntityUpdate(EntityLivingBase.java:293)
        at net.minecraft.entity.Entity.onUpdate(Entity.java:318)
        at net.minecraft.entity.EntityLivingBase.onUpdate(EntityLivingBase.java:1561)
        at net.minecraft.entity.player.EntityPlayer.onUpdate(EntityPlayer.java:288)
        at net.minecraft.client.entity.EntityClientPlayerMP.onUpdate(SourceFile:63)
        at net.minecraft.world.World.updateEntityWithOptionalForce(World.java:2070)
        at net.minecraft.world.World.updateEntity(World.java:2034)
        at net.minecraft.world.World.updateEntities(World.java:1887)
        at net.minecraft.client.Minecraft.runTick(Minecraft.java:1994)
        at net.minecraft.client.Minecraft.runGameLoop(Minecraft.java:961)
        at net.minecraft.client.Minecraft.run(Minecraft.java:887)
        at net.minecraft.client.main.Main.main(SourceFile:148)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:606)
        at net.minecraft.launchwrapper.Launch.launch(Launch.java:135)
        at net.minecraft.launchwrapper.Launch.main(Launch.java:28)
        at net.minecraftforge.gradle.GradleStartCommon.launch(Unknown Source)
        at GradleStart.main(Unknown Source)

The easy fix would be to limit the size to 128, but I'd like to see if I can fix the underlying problem (using signed character types in Java... for array indices? Really, Mojang?) first.

[chrisbecke]

You'd need to be patching mojang code to fix that tho?

[DoomFruit]

Yes, but since we already do that to extend the potions array size, I don't think that fixing yet another of their braindead mistakes will cause too much difficulty.

[DoomFruit]

I've put together a fix that lets us keep a 256-long potion ID array, it's in pull request #1283.

[chrisbecke]

Could this, should this, not be pushed to forge?

[DoomFruit]

Servers/clients with this patch will not be able to connect to clients/servers which are not running the patch. While my information on this is somewhat out of date, I think that a Forge client can still connect to a vanilla server and vice-versa, provided that neither of them are running any game mechanics-altering mods (purely server-side and purely client-side mods such as server commands or render changes should be fine, for instance).

Adding this code as it is will break Forge-vanilla compatibility, and also break compatibility between the altered version of Forge and previous versions. Changing it to make it behave with vanilla-Forge mixtures will involve a crapton of effort and special-case checks - for instance, a new packet type might be used purely for Forge-Forge potion data, the acceptable packet types would vary depending on the versions of Forge (or not) which the server (or client) uses, Forge servers would need to either go full vanilla potion packets for everyone or convert them for different clients if you have a mixture and probably a whole lot more.

Yes, it would be nice to have in plain Forge. But I don't have the energy to try and get it incorporated and then to deal with the enormous quantity of inevitable bugs that result when you apply it to everything.

[jjtParadox]

This has been inactive for some time, is this still an issue?

[Mithion]

Yes, @DoomFruit was at one point working on a PR to allow AM2 to modify source via ASM to allow the expanded potion IDs. I believed he had it working and was waiting on some style changes when it was closed without comment.

I imagine someone else will likely need to take this on.

[jjtParadox]

Actually, looks like he might still be working on it, https://github.com/DoomFruit/ArsMagica2/commit/afec57de1b36c8f40e9dba82d0401796106328bd was updated 2 hours ago.

[Mithion]

Oh right on, that's even better! I was confused when the PR was closed.

[DoomFruit]

I am still working on this, it just took me a while to finally knuckle down and figure out how to use git (damn you, Warframe). The pull request probably got closed automatically when I was attempting to figure out the rebase command and made the entire source tree the same (without my changes), thus making Github go "oh, they've been incorporated already, nothing more to see here".

[Bogdan-G]

You have already added all the potion ID in the config? Another mod increase cap ID to 4096 and edit config AM2 and profit!

[Mithion]

that's the workaround for now, but I'd rather not have a dependency on another mod to fix a shortcoming in this one!

@DoomFruit how's the PR coming? Do you need any help?

[DoomFruit]

As far as I can tell, it's ready to go (PR #1322). I've tested all the situations I can think of and it seems to work fine.

[Mithion]

Fixed by PR #1322