MinecraftForge/MinecraftForge — pull request #601 by agaricusb (2013-06-08 05:13:45+00:00)
Title: Fix NPE from item damage delegating to non-Item blocks in ItemStack

https://github.com/MinecraftForge/MinecraftForge/commit/b56d05ef9d6be5f3413cb62f2940f5b9cf3a8549 ItemStack delegation to Item for damage values

assumes ItemStack is only used to represent item IDs with a valid Item instance. However, ItemStacks can also be used for block IDs with no Item — but after Forge 1.5.1-7.7.1.672 (with this commit), attempts to access the damage will NPE in ItemStack since getItem() is null.

Arguably, "blocks with no items" is a bug in the mods, but item stack damage *used* to work on such blocks before the breaking commit. So this is a regression breaking backwards-compatibility with existing mods. It also affects two sets of mods, those that 1) register non-item blocks, and mods that 2) manipulate arbitrary blocks as item stacks. All of the mods in either category would have to be updated to thoroughly workaround this issue.

Known mods with non-item blocks:

* RedPower2 - flax cropblocks
* Mystcraft - writing desk, star fissure
* Forgotten Nature - fruit, nuts, crops blocks
* Forestry - forestry.farming.gadgets.BlockMushroom
* MFR - fake laser
* Thaumcraft - goo
* EE3 - redwater
* Natura - doors
* probably more

Tool to scan for these blocks: https://github.com/agaricusb/IncompatiblePlugin/commit/667d71da0100de86724d95d151536dd0b80431ec + raw results on 116 mods: https://gist.github.com/agaricusb/5733692

(Note these blocks are only meant to be placed in-world, not meant to be obtained in inventories)

Known mods accessing item stack damage on arbitrary blocks; non-exhaustive, but particularly impacted:

*Equivalent Exchange 3*: minium stone - clicking to transmute arbitrary blocks, will crash your client if the block has no item. Worse yet, the crash persists even after relog, so you'll likely have to delete or edit your player.dat file to recover. Or patch Forge, like I did :p. This issue is known all over their bug tracker: https://github.com/pahimar/Equivalent-Exchange-3/issues/348 https://github.com/pahimar/Equivalent-Exchange-3/issues/335 https://github.com/pahimar/Equivalent-Exchange-3/issues/340 - and there is a PR to have EE3 ignore non-item blocks https://github.com/pahimar/Equivalent-Exchange-3/pull/378 but this is a possible loss of functionality (what if I want to transmute Forgotten Nature fruit blocks? with a custom transmutation recipe, etc.). As of pre1h + Forge 725, this serious crash remains unfixed.

*ExtraBees*: noticed this interaction while updating MCPC+, wasn't sure where the problem was so added a temporary workaround in https://github.com/MinecraftPortCentral/MCPC-Plus/issues/854 - not sure if the mod has applied their own fix by now, but it definitely was affected at one point

*CraftGuide*: a recipe browsing mod, of course interacts with arbitrary item stacks - would've crashed with the above mods if the author hadn't implemented a workaround: https://github.com/Uristqwerty/CraftGuide/commit/d7fdcb02250c7ad56c5d29ed5b79fc357c33298b

> - Added CommonUtilities.getItemDamage(): Unlike ItemStack.getItemDamage, doesn't NPE if the stack's Item is null, instead using reflection as a fallback to read the field directly

which, imho, should be implemented in Forge instead of special-cased in each mod (plus, then reflection wouldn't be needed).

So that's what this PR does - falls back to itemDamage instead of delegating to Item if no instance is available. This restores the pre-672 behavior for non-item blocks, fixes all related getItemDamage() NPE's (NullPointerException at net.minecraft.item.ItemStack.func_77960_j(ItemStack.java:268))