Skillet-Classic

Skillet-Classic

445k Downloads

Error: attempt to index local 'reagentsChanged'

matthewhively opened this issue ยท 39 comments

commented

The below error occurred after I did the following:
Crafted about 7 wool bandages
Learned Heavy wool bandages from trainer
Re opened skillet
Clicked heavy wool bandages
clicked create all

Message: Interface\AddOns\Skillet-Classic\SkilletQueue.lua:197: attempt to index local 'reagentsChanged' (a nil value)
Time: Tue Sep 28 00:55:18 2021
Count: 1
Stack: Interface\AddOns\Skillet-Classic\SkilletQueue.lua:197: attempt to index local 'reagentsChanged' (a nil value)
[string "*:OnClick"]:2: in function <[string "*:OnClick"]:1>

Locals: self = SkilletCreateAllButton {
 0 = <userdata>
 Text = SkilletCreateAllButtonText {
 }
 Right = <unnamed> {
 }
 Middle = <unnamed> {
 }
 Left = <unnamed> {
 }
}
button = "LeftButton"
down = false

I am not sure it is a reproducible error. I was able to get it to repeat with the current queued items by continuing to click "create all", but as soon as I cleared the queue, I could not get the error again.
I would presume the same could happen with linen bandages and heavy linen bandages, and I will test that since it is an easier case to test (since its likely this is only possible to do once for each new level of bandage)

commented

Well I haven't found any new errors with alpha9.
Thanks for adding the setting to turn off the clear queue when move/jump to cancel!

commented

If you have multiple items in your queue but are missing reagents, clicking Process will first skip the item, and then remove it if the option is set (which I made the default). This has to be fixed before I can release this code. I'm going to change the default in alpha10 and then work on fixing this for alpha11.

commented

I could have sworn the UNIT_SPELLCAST_SUCCESS event still comes through even after the frame is closed. If true, then hopefully the queue could be properly updated even after closed.

There is a UNIT_SPELLCAST_SUCCEEDED after the frame is closed, but the process of closing the frame has destroyed the context needed to decrement the queue.

commented

Skillet-Classic-1.37-alpha11 should be the final version built because of this issue. Let me know if you find any problems.

commented

Please enable debug logging with "/skillet debuglogging on" before you do any testing as I'll need that output (stored in the per character saved variables file). Debug logging is not resource intensive so I leave it on all the time on my characters. Github doesn't allow .lua files so either change the extension to .txt or better, zip the file before attaching here.

commented

When you learn a new recipe, any items in the queue for that profession will probably have an incorrect skillIndex. Skillet-Classic prints a warning message when that happens and attempts to clear the queue. "/skillet warnlog on" will make sure that warning is captured in the log.

I don't think this error is First Aid specific so Cooking might be an easier profession to test as I think a new recipe can be learned after crafting just a few items with vendor bought materials. Since Cooking (and First Aid) can't be unlearned, this may involve leveling a new character to 5 over and over to test. I wonder if the PTR might be helpful here.

commented

The PTR is probably useful here. A template level 70 doesn't know Cooking and you can level cooking to 10 with vendor bought reagents. You will not have reagents for the recipe you learn, however so some farming will be in order. Probably a better solution is to create a new character on the live server, collect the materials needed to perform the experiment, and then copy that character to the PTR.

Installing the PTR isn't very painful and doesn't take much space since Blizzard consolidated the majority of the data files. You will, of course, have to install Skillet-Classic on the PTR but that is pretty easy as well.

commented

Good ideas. I'll try that tonight

commented

Well, the first test I tried didn't work with cooking. I have the full log, but no error was found.
I'm going to see if one of my existing alts has similar circumstances and can try to reproduce it with firstaid.
If the SAME exact conditions don't work.... I dunno what else to try.

commented

It did not occur on the transition from linen -> heavy linen
But I did get it to happen with silk -> heavy silk.
It might be important to close, learn the skill, then open it again.
I've attached the log. (though I only see 1 log line with today's date)
I might try it on the PTR just for the hell of it.
log was borked

commented

Do I need to turn on the debugging every time I log in? I had assumed it was on for the entire session.

commented

Gah, it won't let me copy a level 1 character to PTR

commented

Ok, I think I got it!
This is with heavy linen -> wool
Skillet-Classic.zip

commented

As a note, I've only tried it with "create all" button. Haven't tried manually queuing X amount. Not sure if it matters though.

commented

You don't want to copy a level 1 character to the PTR because to learn a profession you need to be level 5 or higher. Here's what I would do:

  1. Create a new character on live BCC
  2. Level to 5
  3. Learn First Aid (and Cooking?)
  4. Gather enough materials to level First Aid to the trouble spot. (Note: you could level to somewhere below the trouble spot)
  5. Transfer this character to the BCC PTR.

Now you have everything you need. After passing the trouble spot you can delete the character and transfer again so you can repeat the experiment.

Let me make sure I understand the steps you took. Let me know if this is accurate.

  1. Character is at some skill level below 80
  2. Character has sufficient linen cloth to exceed 80.
  3. You press the Create All button for Heavy Linen Bandage.
  4. Character reaches skill 80
  5. You stop the process (How do you do this?)
  6. You close Skillet-Classic
  7. You go to the trainer and learn Wool Bandage
  8. You open Skillet-Classic
  9. You select Wool Bandage
  10. You click Create All
  11. Error occurs

I do see the error I expected in the log:

	"Wed Sep 29 01:00:23 2021(W): recipeIndex= 2 and verifyIndex= 3 do not match", -- [1056]
commented

Need to add a couple of extra debug commands. "/skillet tabledump on", "/skillet tracelog on" (and yes, the debug state is maintained across logins).

Skillet-Classic-1.36 updates the .toc for the latest release of Classic Era (1.14.0) and has a couple of unrelated changes.

I've added some code that I think will fix this problem in Skillet-Classic-1.37-alpha2. Try it and let me know.

commented

First, I definitely can train first-aid or cooking on a level 1 character and have on my bank alts before I decided to try testing on the PTR.
Second, I'll just delete and re-transfer my test character to the PTR. Then I can re-test the exact circumstances again. (good to know I have to delete it from the PTR first).
Third, So, I should grab 1.37-alpha2 for the PTR?
Fourth, your repo steps are accurate. For Step 5, usually I just start moving (or jump) to interrupt the crafting process.

commented

I guess I never tried to train professions on level 1 characters before. My level 1 toons are either level 4 or 5 before they encounter a trainer or they go directly from the starting area to a major city to assume their role as an "inventory manager".

When you interrupt the crafting process in that manner, there is still a partially completed entry in the queue. When you then train a new recipe, that partially completed entry is most likely going to fail because the skill index has changed.

When I'm micro managing my crafting to get to a specific skill level, I will only queue enough items to reach the goal assuming a skill up every craft. For example, If I'm at 75 and want to get to 80, then I queue 5. If that gets me to 78 so I queue 2. If that gets me to 79 I craft one at a time (smashing the "Craft" button) until I get to 80.

The only thing I could do is monitor the events that are fired when you learn a new recipe and clear the queue when that happens. I'll have do some research to determine if the events tell me which profession learned the new skill so I can clear only that profession from the queue. If the event doesn't tell me which profession, I'm not sure I want to clear the entire queue.

The only other thing I can say is if it fails when you jump to interrupt, don't jump to interrupt or clear the queue of any old entries before you start processing with your new recipe.

I think 1.37-alpha2 (or beyond) will work better, but I haven't tried to replicate the issue on my system yet. If you try it, leave debugging on (with the added commands) and post it for me to look at.

commented

I just tested 1.37-alpha2 with a jump interrupt and the changes made will remove the in process queue entry so that solves this specific issue. I think there is still a problem if you have multiple items queued for a profession and you learn a new skill in that profession, the remaining queue entries are now going to fail. More testing needed...

commented

I think the multiple items queued case is already handled correctly.

commented

I just tested 1.37-alpha2 with a jump interrupt and the changes made will remove the in process queue entry so that solves this specific issue. I think there is still a problem if you have multiple items queued for a profession and you learn a new skill in that profession, the remaining queue entries are now going to fail. More testing needed...

So, why would you want to clear the queue just because the character moved to interrupt crafting?
I mean, yes... this would probably solve this particular problem, but it seems like a sledgehammer since it would effect more than when new recipes are learned. It essentially side steps the issue.
How are you supposed to stop crafting without moving?
I guess closing the window works to stop crafting after the current one completes, but this seems kinda jenky.
When I click create all, the process button stays the same. If I click it, it changes to pause, but clicking it again doesn't actually pause the crafting queue either. It seems to actually only start the crafting process.

I can however confirm that in alpha3 whatever changes were made, it does appear to not generate the error anymore.
Tested steps:

  1. create all heavy linen bandages
  2. Let it process until skill level 80 is achieved
  3. close the skillet window to cancel crafting
  4. Open trainer and learn wool bandage
  5. Open skillet, linen bandages are still in the queue
  6. Select wool bandage and hit create all
  7. Queue is cleared (silently) and wool bandages are added to queue, but processing does not auto-start as expected.
commented

I hate trying to queue exactly the number needed to reach a skill level because the random chance not to skill up. It drives me nuts to do the system you describe, queue 5, then queue 2.... then keep clicking craft until you get the last point.

I'll definitely try the new alpha version tonight.

commented

Skillet has been around almost as long as World of Warcraft. Skillet-Classic is a fork of an older version of Skillet. Over the years, Blizzard has changed the API used by professions and they have added restrictions on spells and spell interrupts. In Skillet-Classic, DoTradeSkill (and DoCraft for Enchanting) are spells. Both have inputs of spell_index and count (Note: In Skillet for retail, this is changed to recipe_id and count which would be better).

While there used to be an API call to cancel a spell in progress, this has been removed. Moving or closing (API calls CloseTradeSkill or CloseCraft) is now the only way to cancel the spell. The Process / Pause button used to call the cancel spell function when the button was labeled Pause, but since that call was removed, clicking Pause currently does nothing. I guess I should change the button.

Since the classic API call(s) uses the skill index and learning a new recipe will change the skill indices, Skillet-Classic needs to do something to prevent an entry in the queue from crafting the wrong item. If the processing was interrupted by closing the frame, lots of things could happen behind Skillet-Classic's back that would make starting the queue again do more harm than good. Both closing the frame and moving result in a UNIT_SPELLCAST_INTERRUPTED event and there is no way to tell what actually caused it.

Code could probably be written to handle some of these cases better, but the current action of removing an entry from the queue is safe.

commented

That seems annoying that that blizzard isn't providing useful functions.
I guess there is a macro action for /stopcasting could skillet make use of that slash command? Or would printing to the console not activate other slash commands?

I hear you that there isn't a way to determine the cause of UNIT_SPELLCAST_INTERRUPTED, but closing the frame doesn't clear the queue. So why does it behave differently from movement?
Maybe I'm not understanding the exact problem, but I thought the queue was supposed to be cleared only when recipeIndex= 2 and verifyIndex= 3 do not match was encountered?

commented

I'm not saying your wrong about this, but based on what I read here, it looks like stop-casting should be allowed as long as it is initiated by the user mouse-click.

https://www.wowinterface.com/forums/showthread.php?t=42367 (maybe this is out of date?)

These actions require a hardware event, such as clicking a button on your mouse or pressing a key on your keyboard, and include:

    Casting a spell
    Stopping a spell cast

But then this
https://wowwiki-archive.fandom.com/wiki/API_SpellStopCasting
says it is only allowed from blizzard code.... but then allowed through this?
https://wowwiki-archive.fandom.com/wiki/SecureTemplates

commented

Try Skillet-Classic-1.37-alpha5. Instead of removing the display of "Pause", I have implemented an actual "Pause" button that executes "/stopcasting".

When this button is used, the queue is not cleared. Jumping/Moving will clear the queue. Closing the frame does whatever it did before which is probably wrong and I'll have to fix it.

I'm sure there are still cases that aren't handled correctly so let me know what you find.

commented

Skillet-Classic-1.37-alpha6 should behave better when the frame is closed during processing.

commented

But it didn't behave better so try Skillet-Classic-1.37-alpha7 instead.

commented

Skillet-Classic has no control over jumping/moving so I am reluctant to never clear the queue as I'm not sure I can test all the cases. You are a clever person so feel free to modify the code to suit your play style (comment out 3 lines in the StopCast function in SkilletQueue.lua and jump away)!

Please use the latest alpha in your testing. I found one more bug in alpha7 which is fixed in alpha8.

commented

I would personally prefer if jumping/moving didn't clear the queue since that is the primary way that I interrupt whatever I happen to be casting, but its not a huge deal.
I'll try out alpha 7 tomorrow evening.

commented

I'm not sure if its relevant, but it looks weird.
When I turn on traceshow, it shows the following events
UNIT_SPELLCAST_INTERRUPTED (with processingSpell != nil)
UNIT_SPELLCAST_STOP
UNIT_SPELLCAST_INTERRUPTED
UNIT_SPELLCAST_INTERRUPTED
UNIT_SPELLCAST_INTERRUPTED
This seems odd, I suppose the others could probably be skipped with no spell caught?

When I click the pause button (which works now) it triggers:
UNIT_SPELLCAST_FAILED_QUIET (with processingSpell != nil)
UNIT_SPELLCAST_STOP
UNIT_SPELLCAST_INTERRUPTED
UNIT_SPELLCAST_INTERRUPTED
UNIT_SPELLCAST_INTERRUPTED

commented

I don't seem to be understanding what the point of self.reagentsChanged is.

I must not be understanding something, but why isn't the solution to the original error adding a self.reagentsChanged = {} before lines 325 and 339 in SkilletQueue.lua ?
Or alternatively
Since I cannot find any place where its value is actually read by something (only written to), maybe it could just be deleted entirely?

commented

You are correct, reagentsChanged is gone in the next alpha. I don't claim to understand every line of code as I didn't write all of it (and there are pieces I would love to rewrite completely). Since it was Skillet.reagentsChanged and only appeared in one module it could have been a debugging aid (/dump Skillet.reagentsChanged).

As for the events (UNIT_SPELLCAST_*) those are generated by Blizzard so there's no "skipping". Why there are more than one is a question for Blizzard and they don't answer questions. The variable processingSpell allows Skillet-Classic to only process one of them.

Other tests make sure the event was actually caused by you. Hang out by the forge in a major city with traceshow on and you'll see what I mean.

commented

Use alpha8 + the following and see how that works. This version doesn't remove the queue entry when interrupted by jumping/moving. Let me know how much testing you do and, of course, any errors or other issues.

SkilletQueue.zip

commented

Just to be safe I've added an (appearance) option to clear the current queue entry on interrupt (jump, move, close frame, use Pause button). The default is true but you will, obviously, want to set it to false (unchecked).

My testing so far has shown that the current queue entry doesn't always reflect the correct count which could be confusing. Removing the entry assures that the queue will always be accurate.

Since there's a new option, localization entries had to be created and a new build, Skillet-Classic-1.37-alpha9.

commented

Technically true, it doesn't get to stopCast but the event handlers still do this (which is hopefully noop)

self.castSpellID = spellID
self.castSpellName = GetSpellInfo(spellID)

From my perspective maybe they could skip the debugging messages, or demote the debugging messages to a higher debug level when its a noop.


I could have sworn the UNIT_SPELLCAST_SUCCESS event still comes through even after the frame is closed. If true, then hopefully the queue could be properly updated even after closed.


yup, GIGO. Not expecting a fix. Just listing it because you were talking about inaccurate queued counts.

commented

As for the events (UNIT_SPELLCAST_*) those are generated by Blizzard so there's no "skipping".

What I mean by skip, is if processingSpell is already cleared there isn't really anything that needs doing, so the handler for that event could just return immediately.

My testing so far has shown that the current queue entry doesn't always reflect the correct count which could be confusing.

My biggest issues with queue inaccuracy are:

  • When the window is closed and the craft completes, the current queued item is not decremented
  • When pressing "create all" multiple times its increments the queue count multiple times. (so I have enough to craft 20 bandaids, If I press it 3 times it will queue 60) It would be nicer if it could check the queue and only add the difference between what is already queued and what you currently have materials for.
commented

What I mean by skip, is if processingSpell is already cleared there isn't really anything that needs doing, so the handler for that event could just return immediately.

It does return immediately. IMO, the debug messages don't count (unless I'm looking for something).

When the window is closed and the craft completes, the current queued item is not decremented.

Once the frame is closed, the Blizzard API functions don't return useful data (if they return any at all) so I'm not sure this one can be fixed. Not even sure I can remove the queue entry.

When pressing "create all" multiple times its increments the queue count multiple times.

GIGO. You are the first person to complain about this and I don't think I want to complicate the code trying to fix it.

commented

The event handlers mostly use DA.TRACE for debug output which is normally turned off. The code that is executed is minimal and provides useful information when something goes wrong.