AppliedEnergistics / Applied-Energistics-2

A Minecraft Mod about Matter, Energy and using them to conquer the world..
https://appliedenergistics.github.io/
Other
1.42k stars 651 forks source link

Autocrafting stuck at "sceduled" #4375

Closed fdevx closed 4 years ago

fdevx commented 4 years ago

Mod versions

AE2 version: rv6.stable build 7 JEI version: 4.15.0.291 Mekanism version: 1.12.2-9.8.3.390

Description

Unable to autocraft elite energy cube (and maybe more but thats not relevant atm) image

How to reproduce

Probable casue of the bug

When setting the recipes trough JEI the elite energy cube wants an advanced energy cube with the nbt tag "tier: 1". However when you craft an advanced energy cube it also gets another nbt tag "mekData: { security: 0 }" which very likely causes the system to not use it for autocrafting.

Expected behavior

As I see it you have 2 options

  1. When simulating the crafting also account for nbt tags and just say "missing advanced energy cube" (this will very likely cause many issues with users saying "I cant autocarft even thou I have the pattern")
  2. Do the crafting how the simulation works so there are no differences

Edit: I forgot to mention that if you edit the elite energy cube pattern by placing an crafted advanced energy cube in the center, autocrafting works with no problems. The reason I still consider this a bug is because creating the crafting plan always works as expected as it somehow ignores the nbt difference (yes the autocrafting pattern for the advanced energy cube also says that it will have 2 nbt tags after crafting)

yueh commented 4 years ago

Please report this directly to your modpack. In many it actually is the intended behaviour.

fdevx commented 4 years ago

If this is intended I would still expect the ae2 to complain about a missing energy cube instead of pretending that it can craft the item.

For example: This is my pattern for the elite energy cube: image

And this is my pattern for the advanced energy cube: image

I would expect ae2 to give me the following message when trying to autocraft the elite cube: image

Instead this happens image

To be clear here: In my opinion if ae2 tells me it is able to craft something it should also be able to do it. If ae2 cant craft an item (because nbt stuff) it also should not let me do it

yueh commented 4 years ago

There is no missing item.

Your modpack encodes the pattern saying it will give Item A as result while actually producing Item B, something completely different. That is the same as as encoding a pattern for Cobble -> Sand and attaching it to a machine which does Cobble -> Gravel.

The simulation can only work with the expected results not actual products. Say you want to craft glass from sand, but only have cobble stored. We can then simulate that it can use Cobble -> Sand -> Glass. But as no sand is every produced, it will wait forever.

Tahakk commented 4 years ago

With OreDict substitutes it actually work. Depends if you craft it in system or if put it in your inventory.

Same thing is with most of the TE capacitors or Simple Jetpack jetpacks. For some reason All capacitors in TE works, just not the Signalum one.

Technically any item crafting with 'durability' item in recipe works weird.

fdevx commented 4 years ago

@yueh you misunderstood me. I have a pattern that produces Item A and another patter that turns Item B -> Item C. Item A has more nbt tags than Item B but ae2 thinks Item A = Item B while creating the crafting plan.

Thank you @Tahakk this actually works.

yueh commented 4 years ago

It doesn't not think. It methodically follows what you have taught it. So if you tell it Item A = Item B it will stupidly use it, regardless of what the actual outcome will be as it is just a simulation.

Enabling substitution will only work on stored items. In case of Sand -> Glass it might find an oredict sand variant, which can be used and it no longer has to use the Cobble -> Sand recipe. E.g. when canceling the first request, the crafted energy will now exist as item and can be tried as replacement. The same would happen when encoding a pattern with Cobble -> Red Sand and Red Sand -> Glass, but actually producing normal Sand. The second time run it will be able to use the now crafted sand as replacement. But once this stockpile is consumed it will fall back to the pattern and fail again.

And ultimately your modpack is the one doing the Item A = Item B part.

fdevx commented 4 years ago

Okay maybe I'm bad at explaining (sorry english isnt my first language)

I'm gonna try again: (This is an analogy) I have a pattern that says dark oak log -> dark oak planks and I have a pattern birch planks -> sticks (ore dict substitution is disabled for all patterns)

My ME system only has these patterns and a bunch of dark oak logs Now I autocraft sticks and the system tells me: "sure no problem we can use dark oak planks instead of birch planks" but as soon as it wants to craft sticks from dark oak planks it gets stuck because the pattern for sticks wants birch planks instead of dark oak planks.

Would you say that the ME system did what it should have? I mean after all you can use dark oak planks to create sticks. But IF you make such a substitution the pattern also has to support it and since it does not have ore dict enable, the pattern does not support this substitution and therefore the ME system made a mistake.

Now replace dark oak planks with advanced energy cube and sticks with elite energy cube and you have what I tried to poorly explain above.

Yes the "modpack" (in this case just the mod Mekanism) says Item A = Item B but the ME system does not follow trough with that. It only replaces half of the crafting of Item B with Item A and then gets confused when it doesnt find Item B.

This means ME doesn not methodically follows what you have taught it.

PS: The ore substitution also works on craftable items (at least in this case) How do I know? Because I tried it

Tahakk commented 4 years ago

I would be more interested why does it work with OreDict enabled, does it ignore some NBT? If not why doesn't it work without that. I get that crafting items that have NBT tags you are searching for exact duplicates so that it satisfy the recipe.

@fdevx could you try this with the Thermal Expansion Capacitors? For some reason (not sure if it is only PO3 thing) I could craft only up to capacitor made from electrum (OreDict enabled) but not the signalum one. There is not any hard difference between those, yet the crafting stopped there so I had to use Crafter from RFtools to be able to craft them automatically. I think it is very similar case to the mekanism energy Cubes.

fdevx commented 4 years ago

@Tahakk same thing here. I had ore dict enabled for the capacitors but it only got up to signalium. A quick look at the nbt stuff also didnt show any differences between the stored capacitor and the one it needed for crafting.

Manually crafting the signalium one gives me a similar picture like in ticket #4372 image

Note: I didnt try to reproduce it with only ae2 and thermal expansion but in my opinion that is not needed (yet)

Edit: cropped the pic wrong Edit2: pic again

Edit3: I found another item from thermal expansion that has the same behavior the "satchel". Just like the capacitor it only goes up to signalium. But unlike the capacitor already crafted satchels of the level reinforced or higher are not recognized by the me autocrafting even tho they stack with the pattern. image

Crafting plan for resonant satchel: image as you can see the ME system ignores my reinforced and signalium satchel but for some reason it likes the hardened satchel.

But if I disable ore dict for the patterns it suddenly recognizes satchels of the level reinforced or higher but the crafting is just like before (stalling while trying to craft the signalium satchel) image

fdevx commented 4 years ago

@yueh you probably closed this ticked too quick. Do you mind reopening it until all this is resolved?

yueh commented 4 years ago

In terms of planks, it will never decide to use dark oak planks instead of birch planks if there are only patterns. The only case it will use dark oak planks in case the birch planks -> stick has substitutions enabled and there are real dark oak planks itemstack stored somewhere.

What your modpack is doing is bascially introducing a dark oak log -> dark oak planks recipe, which during the simulation will say birch log -> dark oak planks but then produces birch planks while AE expects dark oak planks.

In terms of ignoring ore dict with substitutions, yes it will do it for stored items. But we basically test any stored item with the same oredict entry (and potentially the same item id) until the recipe will accept one. Which is not ideal in terms of performance. Especially with mekanism, which is pretty much the prime example of "how to NOT do items and recipes".

In terms of reopening, that is a simple no. AE2 has to adhere to some limitations introduce by minecraft itself or some mods doing broken things, which pretty much contradicts any improvements we could make. With some mod packs then deciding they are above it and bypass it, it's basically a classic "fix your own shit".

In terms of the TE satchel. Imo that thing is broken by design. It is marked as damagable and has subtypes without overriden the default damage behaviour. Effectively sharing the type with the damage value. Or on other words repairing it would upgrade it and damaging downgrade. I doubt that would be the intentional behaviour. So for us we see a damageable item and query the stored items for their whole damage range. Which for the TE satchel is [0,1), which happens to translate to just [Satchel@0, Satchel@1], so the first two ones. Anything above is non existing for us. And there is no fix for it, because otherwise it would break in case someone has the great idea to stuff multiple different items into the same item id and use various offsets for subtypes and damage values. Like Item@0-99 is Type A with 0-100% Damage and Item@100-199 is Type B with 0-100% and so on. Totally possible and I would not be surprised if some mod actually does it. (Actually we are even using something similar with seeds. They are just not damageable, preventing this issue.) So I would recommend reporting it to TE, but 1.12 is no longer support by them. So there probably won't be a fix for it.

fdevx commented 4 years ago

Okay you made me check the code (fuck java 8.u242)

Again for the patterns: The one that creates the advanced energy cube looks like this image

We can conclude from that that the pattern indeed does what it says ( dark oak logs -> dark oak planks)

The other pattern expects somethign else ( birch planks ) image

But the test for "isValidItemForSlot" doesnt care about that instead it just tries to replace the brich planks with dark oak planks and notices that it works image

However while crafting you want the precise Item image which ultimately causes the cpu to run in an infinite loop trying to find the precise ingerdient

A fix would be to store which items you accepted and then ask for them precisely

Do you want me to implement this fix or do you want to do it yourself?

Tahakk commented 4 years ago

btw. if there is something wrong with recipes with TE recipes I'm sure that KL would be very willing to adjust them to work, he is very open minded guy as for Mekanism they are working on 1.15.2 versions now only, but I think they did big rewrite for lot of stuff so I wouldn't say it's beyond their capabilities to make the recipes working with 1.15.x versions as for the other mods I would say just redirecting them to this thread so they know 'what to fix' would be nice

Java code is like magic to me so @fdevx if you think it can be fixed I guess there won't be need for any of what I wrote. Depends if @yueh will accept your commit or fix it himself

yueh commented 4 years ago

Most likely not. We never supported the initial condition causing it. Even fixing some issues with it, would still not cause it to work correctly as the whole planning is designed with precise lookups in mind for performance reasons.

It is simply not worth to take the performance hit for a few slightly broken mods, when it otherwise works. Even with the broken ones in most cases.

fdevx commented 4 years ago

The only reason I consider this a bug is because: I can successfully create a crafting plan AND ME fails to execute it

so you either execute the plan or you dont create a plan at all

I will try to implement a way to execute it but if I have to convince you to add a few HashMap lookups because

It is simply not worth to take the performance hit

then I think I dont even want to make the pull request

And by the way before you complain about an O(1) lookup maybe remove a few of the O(n^2) ingredient checking loops (for example https://github.com/AppliedEnergistics/Applied-Energistics-2/blob/rv6-1.12/src/main/java/appeng/crafting/CraftingTreeProcess.java#L133)

yueh commented 4 years ago

It can only create the plan due to it being feed bogus data. And this part is the one not supported by us. You even confirm that it works just fine with using the crafting items, which is the intended way. This is certainly not a project which has to take fuzzing into account.

The O(n^2) there isn't really an issue. Or at least nothing compared to real ones. What I am actually worried about is the isValidItemForSlot(). This is a nightmare is it has the potential to reach out to Forge recipe handling. This basically checks the ingredients against every single recipe out there. And I already considered disabling it. But yet again there are some broken mods which register multiple recipe alternatives and with oredict or similar you kinda have to do it otherwise crafting would completely break.

In most cases to fix stuff on your side we would really need to have some tools in Forge. E.g. a way to know which nbt tags are needed for comparisons and which ones can be ignored. Or some real equals() for items, which mods can override. But good luck convincing Lex about adding something actually useful for performance or easier use to forge. And even then some mods will find a way to break it for everyone else again.

fdevx commented 4 years ago

The fuzzy search is somehow bugged (I dont blame you after reading this code - the stuff you have to check is horrible). I just found out that if you search item precisely can sometimes give more results than using fuzzy search ....