Bug: Nested Materials
KevinTyrrell opened this issue ยท 31 comments
Description: Crafts which contain materials both in the primary crafted item AND a nested prerequisite item are not properly added to the queue.
How To Reproduce:
* Open Skillet crafting window.
* Select a craft which has materials which also need to be crafted recursively. It's not immediately clear from here exactly what's causing the bug, but it may require a situation where the primary item being crafted has a material which also needs to be created and that material has a crafted material which is also shared by the primary item.
* Goblin Sappper Charge experiences this bug. Its material, 'Unstable Trigger', requires a 'Solid Blasting Powder', which is also shared with the primary item.
* Queue any number of 'Goblin Sapper Charge' crafts.
* Notice that it will queue 3n solid blasting powder, where n is the number of queued Goblin Sapper Charges. It does not include the 'Solid Blasting Powder' needed for the 'Unstable Trigger'.
Version: Skillet-Classic 1.21
I have a level 30 toon with engineering skill 200 on Burning Crusade Classic and I'm about to do the Goblin Engineering quest. Stay tuned for more...
This issue should be fixed in Skillet-Classic-alpha8.
Okay it appears like the issue is that QueueCommandIterate(Solid Blasting Powder, 1) is never called after QueueCommandIterate(Unstable Trigger, 1) is called. Line 184
I believe the edge case centers around that if queueCraftables and need > have fails, due to Skillet thinking it already has Solid Blasting Powder x3. The if statement likely evaluates to if true and 1 > 3, which evaluates to false.
Perhaps the need parameter is not being incremented as it goes along? It's hard to tell.
Unfortunately, I don't have a classic engineer so I can't test this myself. I do have a retail Gnomish engineer and a "Mithril Frag Bomb" has a similar pattern. It uses an "Unstable Trigger" and "Solid Blasting Powder". In the Skillet (retail) case, it works correctly.
I will need you to enable Skillet-Classic debug mode by typing "/skillet debugshow", "/skillet debuglevel 4", and "/skillet tabledump". Open Skillet-Classic, queue one Goblin Sapper Charge, close Skillet-Classic, and logoff. Upload your global saved variables file, Skillet.lua, and the engineer character's saved variables file, Skillet.lua. You can rename the per character file and zip them together and uploading (drag and drop) the file(s) here.
Thanks, I'll take a look at the source and see if I can spot it as well. Though I'm not as familiar with the codebase.
Thanks for the data. Since Skillet works and Skillet-Classic doesn't. I need to look at the differences between the two.
I knew this problem sounded familiar. The same issue was reported against Skillet 4.20 and fixed in Skillet 4.21.
I have copied the fix from Skillet 4.21 into the Skillet-Classic\SkilletQueue.lua in the attached .zip file (SkilletQueueOld.lua is a copy of the original file). Since I can't duplicate the issue on my end, I'll need you to test this for me. This fix may fall flat on its face as I've only done a minimum of testing. Use the command "/skillet cleardebuglog" before you open Skillet-Classic and try the experiment. Let me know how it goes, please.
One of those issues was definitely opened by me. I believe it was a few months ago and I had forgotten to follow-up. If those issues were made on the Skillet retail version then that would have been a mistake on my part, as I had only found the bug on classic.
Anyhow I've replaced my file with the new one you provided, cleared the debug log, then queued one Goblin Sapper Charge and logged out. This time when the sapper was queued, no solid blasting powder was queued. I do not have any relevant items in my bags, so four solid blasting powders being added to the queue is still what we would expect. I opened the materials window to double check what was going on and sure enough Skillet considers Solid Blasting Powder/Unstable Trigger to not be craftable, instead classifying it as a reagent.
Here is the debug log.
Skillet-SavedVariables.zip
Issue is on line 79, if skillIndex then. skillIndex for Solid Blasting Powder is nil despite me knowing the recipe to create it, and thus it does not consider it to be a craftable item.
The definition of SkillIndexLookup was wrong (line 63), I really need to create an engineering character on Classic.
Try this version and see if it works any better...
BTW, thank you for your input and analysis. Remote debugging can be hard but you are making it much easier!
You may have reported against Skillet-Classic back then. What is your username on CurseForge?
Since my ability to debug this issue on Skillet-Classic was (and still is) limited, I tested against Skillet and found that the problem existed there as well. I fixed it on Skillet and forgot to move the fix to Skillet-Classic!
I have a theory. In the Goblin Sapper Charge, the Unstable Trigger is after the Solid Blasting Powder and in the Mithril Frag Bomb, it is before the Solid Blasting Powder.
Please change line 113 from:
for i=1,#recipe.reagentData do
to:
for i=#recipe.reagentData,1,-1 do
and let's see if it fixes that recipe. If so, then I may need to process the craftables before the non-craftables.
That looks good... How about this version of SkilletQueue.lua?
Skillet-Classic.zip
No dice, that version of the queue has the original error.
In the log, line 2335
isReagentCraftable: recipeSource= { ['Solid Blasting Powder'] = true }"
It's iterating over the materials of Unstable Trigger and sees Solid Blasting Powder as craftable, but does not seem to call QueueCommandIterate on it.
You may have reported against Skillet-Classic back then. What is your username on CurseForge?
Sorry. To answer this question, my curseforge name should be Hatefiend. I may have reported it late last year and forgot to check my curseforge inbox. It may have been on Skillet's retail page by mistake.
By the way, I noticed that in this repo's documentation it says:
To report bugs and request new features check: https://www.curseforge.com/wow/addons/skillet-classic/issues
That link leads to a 404. But if you actually do go to the skillet-classic curseforge page and click the issues tab, it refers you back here on GitHub. No worries if you already were aware of this.
Alright, that corrected the above problem but we're back to the original issue.
I have fixed the documentation.
I am out of ideas on how to fix this issue. The Unstable Trigger has to be processed before the Solid Blasting Powder or this section of code needs to be completely redesigned.
I really need my own Goblin Engineer and that will take a while. I will keep this issue open until I figure something out.
The Unstable Trigger has to be processed before the Solid Blasting Powder or this section of code needs to be completely redesigned.
Can you elaborate on why this is? I would imagine the ideal scenario where no matter which order it iterates over the materials, it would arrive at the same materials list/queue. I'll review the code myself and see if I can spot an easy way out that doesn't disrupt the rest of the codebase. If it involves the have < need part, then perhaps it needs a way of updating the need value as more calls are made? For example if I was writing this from scratch, I would imagine a situation where I would have a recursive function (or iterative solution, using a stack or queue) where the parameters would include have and need, and are updated as I descend downwards.
Thankfully there's so little recipes that have nested crafts -- Sapper Charge may be the only one in classic specifically that has this issue. Sadly it's one of the most commonly crafted items. There may be more examples like this in TBC as well.
Appreciate the information. I concur with the two pass approach.
One thing I don't understand though is your earlier comment mentioning that this issue doesn't exist on Skillet-Shadowlands. How can that be?
Obviously, this code is sensitive to the order of processing because it doesn't recurse through the recipe correctly. It works when the Unstable Trigger is seen first and it fails if the Solid Blasting Powder is seen first.
I believe the proper implementation is something like you describe. First, determine the needs for the entire recipe, then compare to what you have adding the craftable recipes to the queue as necessary.
You are also correct that the number of recipes that have the same craftable used by both the recipe and its reagents is small. I've only found the "Goblin Sapper Charge" and the "Mithril Frag Bomb".
I would happily accept a rewrite of this code.
I was wrong. It has never been reported on Skillet (retail). Your previous report against Skillet-Classic caused me to test the closest recipe I could find and that recipe failed. Skillet changes between 4.20 and 4.21 fixed that test case, the Mithril Frag Bomb. Given what we have discovered here, I'm pretty sure Skillet (retail) will barf on Goblin Sapper Charge. Unfortunately, it will take a major undertaking to level a Goblin Engineer to test that theory.
I will keep staring at the code and if I have an epiphany, I'll post it here.
No problem. I'm enrolled into a programming bootcamp at the moment but I actually would like to try to take a stab at a pull request here. Feel free to beat me to it. Thankfully it looks like the issue is only limited to just the Queue.lua, at least from what I can see, so it shouldn't be too bad.
I believe you are correct, the changes should be limited to SkilletQueue.lua only. If you get something to work, you can just post it here. If you go the pull request route, I'll have to figure out how to deal with that, lol (GIT novice here).
I've made some small changes and adjusted the debug statements. I don't expect it to work but I'd like to see the debug output.
Saw two files in the directory so I tried them both (renaming SkilletQueue1 to SkilletQueue). Both had the same failure as the original issue (3 solid blasting powder added to queue). Logs for SkilletQueue.lua are in the SkilletQueue folder, etc.
Thanks for all your help with this issue but I'm afraid fixing this is going to have to wait until I get my own toon leveled up (at level 19, engineering 121 so far).
SkilletQueue1.lua is the original version so I wouldn't expect that one to work. I probably need to figure out how to properly use GIT branches so I better keep track of my experimental changes.