RFTools Crafter is not Supporting MetaItems + Duplication Bug with IC2C
Speiger opened this issue · 65 comments
Well i think that sounds ruff first.
But RFTools Crafter doesnt support Special Meta Items in the crafter.
https://github.com/McJty/RFTools/blob/1.10/src/main/java/mcjty/rftools/blocks/crafter/CrafterBaseTE.java#L373
The function you are doing here is checking ItemDamage. Not Metadata.
Now for the most default cases thats fine. But as soon someone tries to do meta damage items (which are possible with 1.9+) you get into seriouse issues when comparing metadata. On this call i think forge is outdated in the system. (And i got banned by requesting proper meta checking)
There is a function which calls itself getMetadata which asks for the proper metadata instead of the ItemDamage.
And if you compare ShapedRecipes with ShapedOreRecipe you see that vanilla checks metadata & forge checks itemDamage and i would suggest to switch over to the metadata since a lot of IC2 items are metadamage item (in other words. getItemDamage returns in 99% of the cases something differend then getMetadata())
to save room. Else i would have to add over 100 new ItemIDs which is a lot.
Sadly on top of that IC2Classsic Recipes (Repair Recipe Only it seems)
causes a Duplication bug in the RFTools crafter
And i wonder why... I checkt your source but sadly besides this "Special Meta Support" i found not really much...
If you want i can provide the recipe class source to help out since classic is not released yet but it is basicly on the edge of beeing released... (Just testing and trying to fix bugs like these)
@tterrag1098 we talk here about getRemainingItems. That is the only part where i remove items out of the crafting inventory. Its the function which is allowed to do that. Since there the item removing gets handleded there...
What you think is: I remove the items when getCraftingResult... which is wrong but if you change items in getRemainingItems thats totally fine. Because its made for the "RemoveInputs call" so thats when the player already took the items out of the CraftingResult Slot.
I don't understand your solution at all and the fact remains that it is simply invalid. I'm afraid you're not going to get much support for your recipe this way. Note that there are also recipes that DO return remaining items and still don't touch the input inventorycrafting. Then I would still not know what to do.
Are you seriously trying to argue that because you're using a different accessor method, it makes this valid somehow?
Well basicly:
You put in the Items for the Recipe into the workInventory (keep a copy of what you put into it into the Undo) and then get the output & Remaining items. And then you process remaining items like SlotCrafting does thats it.
Its really really simple.
Also Modifying it has no problems on vanilla crafting. Its using what vanilla provides.
Its not a Invalid System. Its a equal system like Autocrafting does.
I mean Autocrafting creates a fake Inventory that allows you to put items in and then process the recipe things.
thing is: My system works fine with vanilla. No Issues at all (Besides vanillas crappy syncing) and the only issue i have with my system is with things that try to autocraft aka hack into vanillas recipe system
If we go to the Invalid Things route now.
The undo operation is only used when the crafting fails. I need a solution for when the crafting does NOT fail. What if crafting does not fail? How do I know what items to put back? The only items that I know result from the craft are the remaining items (getRemainingItems()) and the craft result. The work inventory I cannot use as I don't know which items are consumed and which are not. Most recipes never touch that inventory (as it should be) so all items would still be there.
Don't talk about undo. That's not relevant in this discussion as that is only called if the recipe didn't work.
there is the thing: Your crafting can only fail if there is no room for the items...
What you could do: My suggestion: Do the recipe no matter what. Look if the Items (that come out) can be inserted all together if that fails you dont insert the items (boolean simulate function a lot of things exist already) and put back the items which are stored inside of Undo.
If the things can be inserted then you just insert stuff since you removed the components already for recipe check/getting result.
Anyway i have now a meeting too i am back in about 1-1.5hours.
Here is how the algo works. I will make it more general since I will also talk about how the crafting grid for the modular storage works since that can get inputs from different inventories at the same time:
- Crafting system gathers materials to craft. In case of crafter block it gets ingredients from the internal inventory. In case of storage scanner system it gets ingredients from all connected inventories. So the items are already consumed from the inventory/inventories
- A CraftingInventory is made from those items
- The recipe is tried. If it works then I assume all items in the working inventory are consumed (I have no way of knowing that they are not)
- The items from getRemainingItems() and getCraftingResult() are put in the output. In the case of the crafter that means to the output slots reserved for the crafter. In the case of the remote storage that means the items are sent to the storage system and they get distributed to whatever inventory is ready to accept the items
i.e. my crafters don't put back ANYTHING in the 3x3 crafting grid. The output is given directly to whatever requested the crafting. I still have no way to know which items from the input inventory should go back or not
Give up please. Your recipe will never work with modded auto crafters if you keep doing it like that. The fact that it happens to work for vanilla is just luck and is not relevant at all
@McJty stupid question can we talk this out in TeamSpeak or something?
Because text messages are to slow and often have missunderstanding in it because of all the delay...
Edit: or discord?
Because we talk here away from each other or we miss each other.
If we still come to no conclusion after that then i give up. I promise ^^"
No problem just tell me when you would be ready?
We talk here about a 10 minutes thing vs a lot of chatmessages until we know what each others problem is...
Well I know what the problem is. You modify stuff in getRemainingItems(). That's a getter. It is not supposed to change stuff. People can call getRemainingItems() multiple times and expect to get the same result and also expect nothing to change when you call a get function
I am not modifying the getter...
I am modifying the inventoryCrafting. Thats the issue which causes at your side problems
yes, but the point is that calling your getter multiple times will give weird results (because the second time it will have a different inventory). And also a getter should not modify anything.
ok. here is the point:
First of all i am doing this remove items stuff only if there is more then 1 item per slot in the recipe.
Second: You have to account for that inventory needs to be cleared everytime you craft something.
Again when we talk i can explain you what the issue is in detail, i cant tell you that in type of chat.
Becuase i cant type it in words... (Since my explaining also has questions)
My work inventory does not have to be cleared at all. I just throw it away. It is a temporary structure that only exists while I craft. When the crafting finishes it gets removed.
Anyway, I'm not going to continue further with this. I'm afraid you will have to fix this in your recipe because even I would add support (which I will not because I think it is impossible) then you still have all the other auto crafters out there and good luck getting all of them to work with this
Nope. Leave working inventory alone. And just put anything that you didn't consume (and that you modified) back in getRemainingItems(). It is up to the crafting mechanic (and vanilla handles that fine) to put those items back (or throw them on the ground if they don't fit, which vanilla also does)
thats what i am doing...
and in this case you could be right that i made for repair recipes a mistake...
ok sry what i said. I found the issue. I let my tester do some further tests with RF Tools to validate that it was my & userinput mistake if i find anything else then i come back. In this case i oversaw 1 important line...
Stupid me. Sry. And thanks for listening^^"
Edit: We basicly found only a rare edge mistake that can only happen with other mods...
Luckly its fixable...
Just to give my OCD a releave. The system i showed you is still Valid it works fine i just made 1 small mistake which bugged it out xD
So Stacksize Crafting should be possible with any mod xD
xD Yeah :3 i hope IC2C can be released soon enough and i hope it works fine with RFTools xD
Note that for items: meta and damage are the same thing. Aren't you confusing with NBT data?
Em getItemDamage can be overriden and thats what i am doing and thats refering to NBTData. getMetadata is refering to: ItemStack.itemDamage...
Since Metadata & ItemStack.itemDamage should be equal. If you change that its causing bugs but when you change getItemDamage its fine.
To give a Explample @McJty
@Override
public int getMaxDamage(ItemStack stack)
{
switch(stack.getMetadata())
{
case 0: return 10000;
case 1: return 40000;
case 2: return 320000;
}
return super.getMaxDamage(stack);
}
This is a function in my reflector class when its asking for the maxDamage...
Meta Damage Items... (Metadata defines the max damage if itemDamage & Meta would be equal this would not be possible...
Well check your facts. Meta and damage are the same for items. Check how ItemStack works. There is only the field 'int itemDamage'. There is no 'meta' variable or similar in ItemStack. For example. Here is the constructor of ItemStack:
public ItemStack(Item itemIn, int amount, int meta, @Nullable NBTTagCompound capNBT)
{
this.capNBT = capNBT;
this.item = itemIn;
this.itemDamage = meta;
this.stackSize = amount;
if (this.itemDamage < 0)
{
this.itemDamage = 0;
}
this.updateEmptyState();
this.forgeInit();
}
yes in that part you are right but check getMetadata function in the ItemStack class and also getItemDamage function. It allows you to Split Metadata from ItemDamage which i did. The thing is when you use getItemDamage for meta comparing you get wrong results. Thats why getMetadata exist. Look into ShapedRecipes class which is Minecraft and ShapedOreRecipe for forge you see the difference!
Forge uses ItemDamage for meta comparing and getMetadata for damage comparing which is a bug!
Note that ItemStack is final so you cannot override from that. That means that you cannot change the behaviour of getItemDamage() and getMetadata(). They will always return the same thing.
You can change getItemDamage & getMetadata since they call ItemFunctions which are 2 seperate functions.
@Override
public int getDamage(ItemStack stack)
{
return getCustomDamage(stack);
}
@Override
public int getCustomDamage(ItemStack stack)
{
return StackUtil.getNbtData(stack).getInteger("advDamage");
}
@Override
public void setDamage(ItemStack stack, int damage)
{
if(damage < 0)
{
damage = 0;
}
setCustomDamage(stack, damage);
}
@Override
public void setCustomDamage(ItemStack stack, int damage)
{
StackUtil.getOrCreateNbtData(stack).setInteger("advDamage", damage);
}
While getMetadata Stays like this:
public int getMetadata(ItemStack stack)
{
return stack.itemDamage;
}
Still this is very complicated for me to fix so I'm not sure I'm going to bother with this. That seems to be very weird and suspicious behaviour to do this and it will very likely cause a lot of issues with other mods that work with item metadata
well the only thing that happens is that your crafter wont work with my items...
Its a easy fix. For meta comparing you use stack.getMetadata() instead of getItemDamage.
But this is not the main bug. i want to talk about.
A MetaDamage item has a duplication bug with your crafter. and i do not know why.
Edit: Its a repair Recipe that uses Lapis Lazuli which starts to fill your crafter with lapis instead of using it..
Not sure what you mean.
Here is the class..
The AdvRecipe getContainer is just a forge fix.
Edit: Removed because of IC2License that i broke just to show you the problem.
The thing is it works fine with vanilla and i made it sure with a lot of crafting tables...
RFTools crafter just uses the regular crafting system and calls getRemainingItems() to fetch possible items that are left over. Your implementation is a bit complex to follow for me but you are not returing the lapis there are you?
Em i got the bugreport from someone.
He tried it with Extra Utils one that workt to a certain extend but had no duplication bug.
EnderIOs didnt work at all...
That is what i have.
And to answer your other question:
I am removeing the items from the crafting inventory reducing the amount and return the results
and this is how you do it. Else it would not work with inventories.
That could be your issues you do not account for recipes that handle their "ItemRemoval themselves"
Maybe you should think why your recipe isn't working on other auto crafters as well. You can't manipulate the crafting inventory as that is just input. It is not guaranteed to go back to whatever is crafting. That is wrong. You have to keep the items untouched and just return everything that is not consumed by your recipe in getRemainingItems(). Your recipe is just broken.
Your recipe will fail for Refined Storage, AE2, ...
Check how CraftingManager.getRemainingItems() works and how that is called from SlotCrafting.onTake() to place back the items on the crafting grid.
That would fit for normal recipes that eat 1 item per slot, but my recipe eats differend amount of Items per slot. There are recipes which eat 4 items to repair stuff thats why i need to do it like that.
And thats the only way to handle it. Thats the code in the slot. I made my research but this is the way how getRemainItems is supporting that feature, i may made a mistake but then it would also fail with vanilla which it doesnt! only some autocrafters fail
net.minecraftforge.common.ForgeHooks.setCraftingPlayer(playerIn);
ItemStack[] aitemstack = CraftingManager.getInstance().getRemainingItems(this.craftMatrix, playerIn.worldObj);
net.minecraftforge.common.ForgeHooks.setCraftingPlayer(null);
for (int i = 0; i < aitemstack.length; ++i)
{
ItemStack itemstack = this.craftMatrix.getStackInSlot(i);
ItemStack itemstack1 = aitemstack[i];
if (itemstack != null)
{
this.craftMatrix.decrStackSize(i, 1);
itemstack = this.craftMatrix.getStackInSlot(i);
}
if (itemstack1 != null)
{
if (itemstack == null)
{
this.craftMatrix.setInventorySlotContents(i, itemstack1);
}
else if (ItemStack.areItemsEqual(itemstack, itemstack1) && ItemStack.areItemStackTagsEqual(itemstack, itemstack1))
{
itemstack1.stackSize += itemstack.stackSize;
this.craftMatrix.setInventorySlotContents(i, itemstack1);
}
else if (!this.thePlayer.inventory.addItemStackToInventory(itemstack1))
{
this.thePlayer.dropItem(itemstack1, false);
}
}
}
by the way thats the SlotCrafting onCrafting code...
And returning negative Items to help with that feature is a really bad idea...
I think the problem here is that, in getRemainingItems, you modify the items present in the crafting inventory that you defined as inv by using removeStackFromSlot, but the usual procedure here should be that the input crafting inventory stays unchanged and the changes are only returned in the array.
Sorry but what you're doing is simply not legal. The fact that it works in vanilla is just pure luck. Double check that it actually still works in 1.11 as there are a lot of itemstack changes there.
I will change the crafter to use ItemStack.getMeta instead of getDamage though. But I will not fix the crafter for your duplication bug as you're simply doing the recipe wrong. If you absolutely need that mechanic then I think you'll have to come up with some other system
@Ellpeck no then i would start duplicating items because Minecraft tries to stack these items together...
There will be no 1.11 IC2Classic. There will be a 1.20 IC2Classic...
@McJty and i have no doubt that this will change...
The function is exactly the same in 1.11x
I just checkt it. the only difference is that you return != null items.
public ItemStack func_190901_a(EntityPlayer p_190901_1_, ItemStack p_190901_2_)
{
net.minecraftforge.fml.common.FMLCommonHandler.instance().firePlayerCraftingEvent(p_190901_1_, p_190901_2_, craftMatrix);
this.onCrafting(p_190901_2_);
net.minecraftforge.common.ForgeHooks.setCraftingPlayer(p_190901_1_);
NonNullList<ItemStack> nonnulllist = CraftingManager.getInstance().getRemainingItems(this.craftMatrix, p_190901_1_.worldObj);
net.minecraftforge.common.ForgeHooks.setCraftingPlayer(null);
for (int i = 0; i < nonnulllist.size(); ++i)
{
ItemStack itemstack = this.craftMatrix.getStackInSlot(i);
ItemStack itemstack1 = (ItemStack)nonnulllist.get(i);
if (!itemstack.func_190926_b())
{
this.craftMatrix.decrStackSize(i, 1);
itemstack = this.craftMatrix.getStackInSlot(i);
}
if (!itemstack1.func_190926_b())
{
if (itemstack.func_190926_b())
{
this.craftMatrix.setInventorySlotContents(i, itemstack1);
}
else if (ItemStack.areItemsEqual(itemstack, itemstack1) && ItemStack.areItemStackTagsEqual(itemstack, itemstack1))
{
itemstack1.func_190917_f(itemstack.func_190916_E());
this.craftMatrix.setInventorySlotContents(i, itemstack1);
}
else if (!this.thePlayer.inventory.addItemStackToInventory(itemstack1))
{
this.thePlayer.dropItem(itemstack1, false);
}
}
}
return p_190901_2_;
And calling my System illegal just because you dont understand it doesnt mean it is...
It works not by pure luck it works because i did my research!
Edit:
And sooner or later you have to adapt that system because people will start to copy it sooner or later.
BTW. Imagine how your recipe would have to work for Refined Storage or AE2. If you manipulate the InventoryCrafting input then how are these mods supposed to move that back into their storage system? How would they even know that some items are supposed to go back there? Currently these mods just prepare an InventoryCrafting instance with items from storage. They give that to the recipe and then they call getRemainingItems() to push that back into their system. But the problem is that it cannot be known what items from the input InventoryCrafting should be put back and which not because most recipes simply don't modify that. So if RS or AE2 would push back those items then you would get a LOT of duping
Oh that point its actually really simple:
Its what you are doing:
You fill your custom Inventory Crafting class with items and then call your craft methode (thats what you are doing)
What you could do is instead of handling the decreasing yourself all the time allowing other mods to handle it too. Minecraft leaves that option for you. Since you get Remaining Items (that have changed) back that should not be an issue at all... Its actually not much code to add/change and also a really easy support.
I mean SlotCrafting checks if the CraftingInventory Slot != null then it removes 1 Item.
If the Remaining Items in that slot != null it adds it to that slot. So allowing to handle Consuming of items in the recipe. Whats so hard to allow that feature in the AutoCrafters?
This is nothing hard. Its just something outside of the "TextBook/StandartTutorial" Area...
Beeing Creative you know?
Check ShapedRecipes.getCraftingResult(). That is the standard recipe. It does not set the inputs in the InventoryCrafting to null or empty. So how is an autocrafter supposed to know that those items should not be put back in the storage?
Just thanks for listening. A lot of other devs would have called me an ideot and lockt the issue and banned me.
This issue is really simple. getCraftingResult is an accessor and should not mutate any state. Manipulating the passed inventory is completely invalid, just by basic java logic. No vanilla recipes require more than one item in the slot, this mechanic is completely at odds with every recipe in the game (modded or otherwise). I suggest you think more about the actual design of this feature, rather than hounding other mod devs because they don't handle your hacky impl.
Ok i give you a Example:
Lets say the Items inside of the InventoryCrafting.. That means you moved them over.
But you keep track of what you put in. If the Recipe is failing.
then you run into this:
CraftingRecipe.CraftMode mode = craftingRecipe.getCraftMode();
if (result != null && placeResult(mode, result, undo)) {
ItemStack[] remaining = recipe.getRemainingItems(workInventory);
if (remaining != null) {
CraftingRecipe.CraftMode remainingMode = mode == EXTC ? INT : mode;
for(int i = 0;i<workInventory.getSizeInventory();i++)
{
ItemStack invItem = workInventory.getStackInSlot(i);
if(invItem != null)
{
workInventory.decrStackSize(i, 1);
invItem = workInventory.getStackInSlot(i);
if(invItem != null)
{
if (!placeResult(remainingMode, s, undo)) {
// Not enough room.
undo(undo);
return false;
}
}
}
ItemStack recipeItem = remaining[i];
if(recipeItem != null)
{
if (!placeResult(remainingMode, s, undo)) {
// Not enough room.
undo(undo);
return false;
}
}
}
}
return true;
} else {
// We don't have place. Undo the operation.
undo(undo);
return false;
}
To Explain it:
If there is a Inventory Item its removing 1 (default behaivor) and adds the remaining into the Inventory.
If there is none then it doesnt add anything.
If there is a RemainingRecipe Item it adds that because the recipe already handled that.
(ContainerItems is by the way also the same thing just that the bucket gets removed since stacksize always maximum of 1)
The only difference between my system and Minecrafts is that i take Stacksize into account. If my system would use the same things like buckets the only difference is that MC doesnt need to remove the items because i already did. Again: SelfHandling. Minecraft added that feature basicly. Its not a hack or invalid thing its something that is not really obivouse but supported...
Edit: Sry that was for @tterrag1098 because he missunderstood something.