[1.7.10][Suggestion] Change potion ID registering so it doesn't mess up if BoP is loaded after other mods that extend potion array
MC-Soupcup opened this issue ยท 10 comments
EDIT: Previously the title was about the Possession potion ID being set to 256, which caused the nether phantom to crash the client when it hit you. The third post has more info, but BoP's method is messing up after another mod loads before BoP that extends the potion array (In this case, it was AbyssalCraft). The fix, for now, is to rename the BoP jar to "aBiomesOPlenty" so it's at the top of the list.
Title, really. This crash only occurs when a phantom in the Nether hits you. After the crash, if I look at the player.dat file (or level.dat in single player), I see an active effect with ID 0. I used NEI to dump the potion IDs list and noticed that paralysis is set to 255, and possession is set to 256. Since the max is 255, I'm guessing either 256 is null or rounds over to 0. Either way, this is a pretty big issue, and there are several threads out there that talk about this. This is a copy of #388, just with more detail. Same crash reports.
Should be noted that with just BoP installed, the two potions register to 31 and 32 (I think, somewhere around that). So it could be a mod interaction, maybe. micdoodle8/Galacticraft#1365 suggests Blood Magic, but the removal of this didn't change anything, not to mention modpacks like FTB Infinity and DW20 do not have this issue (and have BM). The potions aren't even listed in the ID dump for those packs, I'm unsure how they did that. They use BoP version 1067. I've tried this one, with no luck. I've also tried a few of the latest, up to 1127.
Other references on this issue:
In case anyone's wondering, removing Galacticraft does not solve it, these were reported to GC's github simply because the reports use GC's player class thing instead of the normal vanilla one.
I think BoP used to have a configuration option to set potion IDs, though I know it used to have one to disable phantom spawning. This really needs to come back, so that this issue could be avoided in the future. For now, I'll be going through and removing mods until this stops happening, to determine a more exact mod interaction cause. I just wanted to bring this to your attention and hopefully get some insight on this. If you need any more information, let me know.
I've found that if the potion IDs are higher than 128, things get screwy. It's a bit messy at the moment but here's my potion effects extension: https://github.com/Lycanite/LycanitesMobs/blob/master/java/lycanite/lycanitesmobs/PotionBase.java
Also I'm not sure if it is obfuscated or not but I check for both potionTypes and field_76425_a when doing reflection.
Hi guys. I've been thinking about this issue. In BOP we're trying to be considerate to other mods by putting our potions in a new section of the potions array which no-one else is using (by expanding the array by 8). It seems like this won't work with abyssalcraft because they extend the array to 128 (which is way more than they need). So.... I think an approach which will work for everyone is:
- count the spare slots in the potions array
- figure out if we need to expand it
- figure out if we CAN expand it without increasing it over 128
- if it's already at 128, and there isn't room for our potions, crash and say that there's too many potions for BOP
I think the only way that this won't work, is if another mod loads after BOP and has hard-coded potion IDs which overwrite our dynamically assigned ones. But that's really the other mod's fault I think.
Any more comments?
Perhaps it would be better if a proper API for potion effects was built, it could be pushed to LexManos to implement as this should really be handled by forge IMO.
Well, yeah that would be even better, but in the meantime I think we can still fix this bug without waiting for Forge.
So my mod's logic is this:
If the ID list is less than 128, resize it to 128 (keeping all vanilla and modded entries in the list as they are). It then searches from the last entry (127) and counts down until a space is found for all effect IDs to fit.
It's a bit basic at the moment as it will simply check for an empty space starting from the end of the list and will then populate the list backwards when really it should search for an available range. So for example, if there is an entry in 126 but not an entry in 127, Lycanites Mobs will fill from 127 backwards overwriting 126. A more advanced approach that I want to make is to have it detect 126 and skip that ID.
I've just pushed a change which I hope will fix this issue. Our potions init code now looks like this:
getSparePotionId() scans for a free potion ID and expands the array only if necessary, and won't go beyond 128. It's not efficient, but who cares, it only runs once at startup.
Can anyone see a problem with this? Is it solving the clash you reported?
That code shouldn't cause any problems, as it goes through the entire array before expanding it. I might actually switch to auto-assigning potion IDs myself in AbyssalCraft to prevent potion ID conflicts instead of expanding the array to 128 and set configurable IDs (I could have a thing that forces free IDs unto the configurable ones, but then again, that would defeat the purpose of configurable IDs).
Can confirm that this issue is biting me as well. Began happening after I added a mod (AppleMilkTea) which also increases the max potion ID limit.
I've found that the removal of AbyssalCraft stops this from happening (for me, at least). Even in an instance with just BoP and AC, this issue occurs (doesn't seem to matter what versions, even older Forge versions do this), so removing it is the fix on my end. Not sure why that is and I've notified AC's author. Figured it was some mod interaction.
According to AC's dev, who is much more knowledgeable than I when it comes to Java,
"That would happen with any mod that adds potion effects (due to the way BoP adds theirs). Basically, the potion IDs are automatically set for BoP to the last ID (which would normally be 32, which they then would increase to 40, then assign IDs from the 32 - 40 range). This generally screws up if BoP loads after a mod that has extended the potion array, as it would still do the same thing (which apparently screws up in some way when the array's already been extended)."
He also linked to the method you use to extend the potion array.
This actually seems to be the case, I renamed the BoP jar to "aBiomesOPlenty" so it would load first (in my instance of just BoP, AC, and NEI), and it stopped happening. So that fixes that for now, but it would be nice if it could be avoided altogether.