Mekanism Additions

Mekanism Additions

21M Downloads

Formulaic assemblicator doesn't work right with ic2 classic stacked crafting

Trinsdar opened this issue ยท 17 comments

commented

Issue description:

the formulaic assembler can craft stacked recipes but will only use 1 item instead of the proper amount, and it seems like it can't store the stacked recipes, though I don't really know how it stores recipes.,

Steps to reproduce:

  1. setup recipe
  2. craft 1
  3. watch

Version (make sure you are on the latest version before reporting):

Forge: 14.23.5.2847
Mekanism: 8.2.389
Ic2 Classic: 1.5.4.5
Jei: 4.15.0.292

mek

commented

I don't remember for certain what all the buttons in the formulaic assemblicator do given I haven't used it much, but I think the button with the two arrows is to auto refill the input (or maybe its stock control that does that, I don't remember). But also in the video you posted, you are clicking "craft single item" so even so it is doing what you are asking it to do, as it makes a single recipe.

I also don't know for certain what you mean by a "stacked" recipe, but if it is that it actually takes more than one per input slot, from what I understand/have heard, most modded crafting tables/auto crafters do not support that.

commented

but it's only consuming 4 of each

commented

yah, a single recipe is supposed to consume all 16 advanced alloy and all 8 magnets

commented

this is how it should work:
stacked

commented

also most modded regular crafting tables do support it, I can't say for sure about autocrafting, but I know ae2 doesn't allow anymore then 1 at all, so this bug doesn't exist with their autocrafters.

commented

Issue is:
After the craft this needs to be executed in reverse because you don't sync the final crafting grid result backwards. So if any "set calls" come in they lead to bugs with your crafter.
Requested by @pupnewfster
https://github.com/mekanism/Mekanism/blob/1.12/src/main/java/mekanism/common/tile/TileEntityFormulaicAssemblicator.java#L245
(Also this would lead that 1 craft will drop all the overflow into the output? so maybe a patch that reinserts the rest into the crafting grid?)

commented

@pupnewfster any eta on a patch?

commented

I think you misunderstood me there. I'm talking about recipe classes, not the inventory

There is no reason we should need to copy the dummyInv back anywhere as it is only used to check recipe matches and the crafting result.

commented

You are aware that mc is doing the same thing directly after the getRemainingItems function is called in SlotCrafting? So if I do it inside the crafting recipe or the CraftingGrid does it on its own makes no difference. The only thing I do is take over the consumtion logic by using what MC provides me.

commented

or the CraftingGrid does it on its own makes no difference.

If that were the case this wouldn't be an issue. Logic dictates that it does matter.

We decrease the stack size by one, just like vanilla does. The only difference is the InventoryCrafting we pass is a copy.

image

Just because something works in one specific instance, doesn't mean it's the right way.

commented

because you don't sync the final crafting grid result backwards. So if any "set calls" come in they lead to bugs with your crafter.

Recipes should not modify any inputs in a getter.

commented

@thiakil InventoryCrafting is a getter only class? Could you show me where? because there are clearly setter functions? If it should be read only make a class that is read only.

commented
  • I never said we wouldn't fix it
  • Please stop with the elitist attitude like you understand MC code better than everyone else
  • I am being factual, not "lazy"
  • Stop being rude, or I will block you from the repo & discord
  • We've not "fucked up" anything, ours is a perfectly valid implementation

Because you are not supporting the Crafting Slots implementation that is a minimum expectation.

Except we are, since we call the IRecipe method with a valid InventoryCrafting - the bug is that you are assuming things about the inputs to the method and turning it into an impure function.

commented

Simple solution: Dont provide me with the feeling that you want to dodge this.
About the "I know it better". I usually dive very deep into my projects code so I spend more time with this kind of thing then I like to admit, so having seen a bit more angles is my standpoint here.
There are a lot of other things that I do not know about at all, so here I know what I do.

About being rude, yeah that comes with me getting the feeling someone wants to push his issues to me. (Ignoring now the fact if you do want to or not. Just what comes to me ignoring the intention)
So if I react defensive is only a reaction to that statement you provided.

Last point: Exactly the reason why I think you want to dodge this. Yes I agree its a valid implementation, but so is mine and I base mine on what MC does, not what you do or forge does.
It is as valid as yours, so if you want to validate yours and invalidate mine, then you can only expect me to do what I did.

Except we are, since we call the IRecipe method with a valid InventoryCrafting - the bug is that you are assuming things about the inputs to the method and turning it into an impure function.

Your IRecipe implementation is correct. Just that the thing that goes around IRecipe is wrong, thats my point. I mean to quote pupnewfster: You already had issues with the autocrafter because of not supporting get remaining items properly (which is the new function that is not handled properly on your side)

Remember 1 fact: I didnt make this bugreport, I just offered my help to find the issue and even provided my implementation to make it easier to find out what exactly i use in the existing system.

Edit: The first line you provided was: You are doing this wrong. Remove the feature.
You didnt say that directly but It came over like this.

commented

I'm done with your mental gymnastics to justify your ego, despite multiple people saying your recipe is flawed.

Ours is not "wrong" by the exact same measure.

commented

Do you want to cause a duplication bug because you were not supporting vanilla mechanics?
Vanilla has a override system where you can replace items in the result because i am just "clearing" slots I am not inserting new items in the inventory itself. MC checks if the crafting slot is empty if the case and the remaining item is present at that slot it just puts it back disabling the consumtion method.

So if you want to argue about if you should fix a duplication bug that you guys cause (because besides TE & Thaumcraft crafting table) any other crafting table & automated recipecrafter supports this feature without any issues.
It is directly a mekanism bug that my feature triggered. Because you are not supporting the Crafting Slots implementation that is a minimum expectation.

commented

Extra note:
there is 2 ways fixing this bug:

  • 1: Disable stacksize in the recipe slots that way my recipe will never get valid. (the AE aproach by default)
  • 2: Fix the broken implementation and fix this duplication bug.

So far @pupnewfster was really nice about it and understanding that they did a fuckup. @thiakil you dont need to fuck this up now for being lazy