AbstractMutableLootEntry shouldn't crash with null LootCondition[]
yeelp opened this issue ยท 4 comments
Describe the bug
Right at:
LootEntryAccessors.getConditions()
has no issue returning null if the underlying LootCondition[]
is null, but Lists.newArrayList()
throws a NPE if the passed varargs are null. This, as you might expect, causes a game crash.
Small snip of a log from a crashed game:
[12:57:15] [Client thread/ERROR] [FML]: Exception caught during firing event net.minecraftforge.event.LootTableLoadEvent@3dae1201:
java.lang.NullPointerException: null
at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:770) ~[guava-21.0.jar:?]
at com.google.common.collect.Lists.newArrayList(Lists.java:112) ~[guava-21.0.jar:?]
at leviathan143.loottweaker.common.mutable_loot.entry.AbstractMutableLootEntry.<init>(AbstractMutableLootEntry.java:22) ~[AbstractMutableLootEntry.class:?]
(That's about all you need to see the problem.)
Regardless if it's "right" or "good practice" to allow a null LootCondition[], vanilla Minecraft handles it and expects it, so LootTweaker ought to as well I figure.
A little snip from vanilla's LootConditionManager
reveals this fact:
public static boolean testAllConditions(@Nullable LootCondition[] conditions, Random rand, LootContext context)
{
if (conditions == null)
{
return true;
}
else
{
for (LootCondition lootcondition : conditions)
{
if (!lootcondition.testCondition(rand, context))
{
return false;
}
}
return true;
}
}
You could create your own implementation of LootCondition where LootCondition.testCondition()
always returns true and default to that LootCondition (obviously wrapping it in an array) if a null LootCondition[] is encountered (alternatively you could just use the RandomChance(1.0f) LootCondition which is guaranteed as well). Then you're nicely mimicking how Minecraft handles null LootCondition arrays without too much extra work.
To Reproduce
The crash is obviously 100% reproducible. All you need is a mod that injects loot into loot tables and uses a null LootCondition[]. I have such a mod, here's the relevant file and line, and that's how I ran into this issue. Obviously I can fix it on my end by doing exactly what I suggested to you (and I plan to), but that won't stop other modders from making the same decision I made which again, is perfectly valid.
Expected behavior
Title
Version Info (Exact versions only):
I think you agree that isn't super relevant here, since your code I linked and referenced is from the default branch. However, for do diligence and completeness
LootTweaker: 0.2.0
CraftTweaker/Minetweaker: 4.1.20
Forge: 14.23.5.2855
Minecraft: 1.12.2
If vanilla expects it LT should too. That's easily a good reason for me to fix this.
Note: Vanilla's loot entry serialiser will throw an NPE if asked to serialise a loot entry with a null conditions array.
Closed by 719e0dc
Note: Vanilla's loot entry serialiser will throw an NPE if asked to serialise a loot entry with a null conditions array.
So the serializer would throw an NPE but the LootConditionManager wouldn't. That seems... flawed. If the NPE is caught by the caller in vanilla, then I guess it's not a problem. But it would make more sense to me if they were written with the same input expectations imo. Either way, this is good to know so I don't use null arrays in the future ๐