Oredict doesn't work in 1.11 anymore

Question

Oredict doesn't work in 1.11 anymore

raoulvdberge opened this issue 7 years ago · 7 comments

raoulvdberge commented 7 years ago

Thanks to my optimization in 69d92c4 storages now use a multimap instead of a list. Thanks to that, we no longer have to iterate over every item in the storage.

This has an unintended side-effect that oredict extractions don't work anymore because when using the oredict the item will differ thus it won't be found in the multimap.

This only affects the StorageDiskItem class, since all other storage classes have to iterate over every item (for example the item handler external storage handler).

I have this patch laying around, but it appears to show weird behavior (unlimited extractions):

--- a/src/main/java/com/raoulvdberge/refinedstorage/apiimpl/storage/StorageDiskItem.java
+++ b/src/main/java/com/raoulvdberge/refinedstorage/apiimpl/storage/StorageDiskItem.java
@@ -5,12 +5,14 @@ import com.google.common.collect.Multimap;
 import com.raoulvdberge.refinedstorage.api.storage.AccessType;
 import com.raoulvdberge.refinedstorage.api.storage.IStorageDisk;
 import com.raoulvdberge.refinedstorage.api.storage.StorageDiskType;
+import com.raoulvdberge.refinedstorage.api.util.IComparer;
 import com.raoulvdberge.refinedstorage.apiimpl.API;
 import net.minecraft.item.Item;
 import net.minecraft.item.ItemStack;
 import net.minecraft.nbt.NBTTagCompound;
 import net.minecraft.nbt.NBTTagList;
 import net.minecraftforge.items.ItemHandlerHelper;
+import net.minecraftforge.oredict.OreDictionary;

 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
@@ -190,7 +192,27 @@ public class StorageDiskItem implements IStorageDisk<ItemStack> {
     @Override
     @Nullable
     public synchronized ItemStack extract(@Nonnull ItemStack stack, int size, int flags, boolean simulate) {
-        for (ItemStack otherStack : stacks.get(stack.getItem())) {
+        Collection<ItemStack> toAttempt = null;
+
+        if ((flags & IComparer.COMPARE_OREDICT) == IComparer.COMPARE_OREDICT) {
+            for (int id : OreDictionary.getOreIDs(stack)) {
+                Collection<ItemStack> ores = OreDictionary.getOres(OreDictionary.getOreName(id));
+
+                for (ItemStack ore : ores) {
+                    if (toAttempt == null) {
+                        toAttempt = stacks.get(ore.getItem());
+                    } else {
+                        toAttempt.addAll(stacks.get(ore.getItem()));
+                    }
+                }
+            }
+        }
+
+        if (toAttempt == null) {
+            toAttempt = stacks.get(stack.getItem());
+        }
+
+        for (ItemStack otherStack : toAttempt) {
             if (API.instance().getComparer().isEqual(otherStack, stack, flags)) {
                 if (size > otherStack.getCount()) {
                     size = otherStack.getCount();

way2muchnoise · Answer 1 · 2017-04-05T14:09:18.000Z

for (int id : OreDictionary.getOreIDs(stack)) and OreDictionary.getOres(OreDictionary.getOreName(id))

so you would like to trow performance out of your window? Kappa

raoulvdberge · Answer 2 · 2017-04-05T14:10:08.000Z

That will be cached

way2muchnoise · Answer 3 · 2017-04-05T14:10:41.000Z

It better be

Dykam · Answer 4 · 2017-04-05T18:01:10.000Z

You need to clone the list returned by ArrayListMultimap.get. Though I'm not entirely sure how it causes the bug you identified.
https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/collect/ArrayListMultimap.html#get-K-

Changes to the returned collection will update the underlying multimap, and vice versa.

Something else, am I seeing correctly that with the change, in case there are two or more stacks of different item types, but same oredict entries, it will stop retrieving them at the first, rather than continue looking for more when size > otherStack.getCount()?

@way2muchnoise The first looks slow, yeah, but getOres is O(1), it's a simple arraylist lookup.
https://github.com/MinecraftForge/MinecraftForge/blob/1.11.x/src/main/java/net/minecraftforge/oredict/OreDictionary.java#L542

way2muchnoise · Answer 5 · 2017-04-05T18:47:45.000Z

@Dykam I know it looks that way, but for some reason the oredict is very slow. This is speaking out of experience. Also even if that part is O(1), the whole thing will still be horrible performance wise.

raoulvdberge · Answer 6 · 2017-04-05T19:40:06.000Z

@Dykam Thanks, that was the cause.

getOreIds is the slowdown, but I'm caching it now.

raoulvdberge · Answer 7 · 2017-04-05T20:18:46.000Z

Something else, am I seeing correctly that with the change, in case there are two or more stacks of different item types, but same oredict entries, it will stop retrieving them at the first, rather than continue looking for more when size > otherStack.getCount()?

Yes, it has to do that or it will convert stacks of an other type to the equivalent oredict type, which is not allowed

Share to