Mantle

Mantle

157M Downloads

Oredict lookup in RecipeMatch.OreDict creates empty ore entries

phantamanta44 opened this issue ยท 4 comments

commented

Minecraft Version
1.12.2

Mantle Version
1.3.3.56

Versions of Mantle dependant mods
Tinkers' Construct 2.13.0.184

Forge version/build
14.23.5.2847

Versions of any mods potentially related to the issue
CodeChickenLib 3.2.3.358

When a RecipeMatch.OreDict instance is created (e.g. by registering a match for a Tinkers' Construct material), the constructor looks up matching items using a call to OreDictionary::getOres(String name). However, this function will always register the ore name in the ore dictionary regardless of whether the caller intends to register items under that name or not. This causes issues such as phantamanta44/tinkers-evolution#8, where other mods check for the existence of an ore by the existence of the ore name; see TheCBProject/CodeChickenLib#303.

I propose replacing the call with OreDictionary.getOres(oredictEntry, false) to prevent the creation of the ore name on lookup. I'll also open a separate issue on the Tinkers' Construct repository to address the same problem with ore dictionary lookups there.

commented

Your proposed fix will not work, we create those recipe match entries often before the ores are register in the ore dictionary, and as a result creating the entry ensures that we have the same list as the one ores are later registered into.

This is a problem with the mods that check for "existence", that is bad. If mods want to know if anything is registered, they should check if the entry exists and is not empty. According to the authors of Forge, you should treat the ore dictionary as though it could change any time, people can register ores as late as postInit which is way after our recipes need to register.

commented

I might also suggest lazily looking up the oredict entry. The constructors that directly take the ore list would still be a problem, as one would either have to make an api-breaking change to accept Supplier<List<ItemStack>>, or wrap the list in () -> oredictEntry, which would just move the issue up to the caller.

I agree that it's on the mod checking for existence to properly make that check, but it's still not terrible to avoid registering unnecessary oredict entries if possible.

commented

Lazily loading it would not exactly solve the problem, as we cannot guarantee when the recipe will first be called. We have events that can cancel registering recipes in most mods that use recipe match, so mods like crafttweaker can cancel the recipes. Since they will fetch the lazy list not long after registering, we still end up calling it too early. We would have to entirely remove the cache, which is not not worth it for 1.12 as its EOF.

As I said in my previous comment, it is wrong to base game logic on "existence" of an oredictionary tag, that statement comes from LexManos who wrote the system (and has yelled at us for mishandling oredict before). This is a big with the mod using existence first and foremost. We can only do so much protection against mods misusing the system before it hurts performance of this mod.

commented

Going to close this, since in 1.15 ore dictionary no longer exists and recipe match is deprecated. Unlikely to change anything in 1.12 as finding a system that lazy loads it will more than likely break more stuff than I want to fix on 1.12.