Skillet-Classic

Skillet-Classic

445k Downloads

SkillLevels DB issues

matthewhively opened this issue ยท 29 comments

commented
  1. Apparently Skillet hard caps skill levels at 375, so skills that are orange at 375 and would go gray at 405 (for example: https://tbc.wowhead.com/spell=28588/flask-of-mighty-restoration) instead incorrectly display 375/375/375/375

  2. Enchanting uses spell_ids whereas it appears all other crafting uses item_ids. This leads to conflicts when defining the recipe in SkillLevels where the same numeric ID for an enchanting recipe overwrites an ID for another tradeskill.
    If it is possible I would recommend that skillet always uses spell_ids for all tradeskills to negate this issue.
    Using item_ids might also be problematic for items that are crafted by multiple recipes. For example:
    https://tbc.wowhead.com/spell=28580/transmute-primal-shadow-to-water
    https://tbc.wowhead.com/spell=28567/transmute-primal-earth-to-water
    I cannot test this, as I haven't discovered shadow => water yet

commented

If I had to make a guess, its these lines:

        local skillRanks = self:GetSkillRanks(self.currentPlayer, self.currentTrade)
	if skillRanks then
		rank,maxRank = skillRanks.rank, skillRanks.maxRank
	end
...
	for c,s in pairs(SkilletRankFrame.subRanks) do
		s:SetMinMaxValues(0, maxRank)
	end

It looks like maxRank is the maximum skill level for the current profession, and skillet assumes that the player cannot rank up beyond the maximum. Which is correct... currently, but obviously the recipe could still be valid above the current rank when that profession is trained to the next level.
For example. On my level 150 max blacksmith, I see the pattern:
Rough Bronze Shoulders (3480) is displayed as: 110/140/150/150 when in reality it is 110/140/155/170
As this character is level 17, I cannot yet train the next rank. But I am 95% certain that when I do and the maximum increases to 225, it will display the corrected values.

commented

This actually dovetails into a suggestion I had for a UI improvement.
Currently the scale of the entire tradeskill bar in the skillet UI to 1-375 (for a level 70) is such that the level ranges (orange/yellow/green) on the bar will continually be compressed down until they are quite small (only a tiny sliver each) and difficult to productively make use of.
What I would propose is to keep the bar set to say 100 units, and have the midpoint move as the character skills up. In this way, there would not be horizontal compression of the color ranges. Maybe 25 before and 75 after the current skill level? I'm not entirely sure how to present the blue skill level bar in a way that is coherent with this design though.
But you could think of it as automatically zooming in on the relevant part of the bar.
I think its not unreasonable to show potential skill ups beyond the current maximum skill level, though not terribly useful given the cap in TBC is 375. At least it avoids displaying incorrect information.

commented

Skillet-Classic-1.31-alpha2 contains changes to SkillLevelData.lua and UI\MainFrame.lua. Changes are:

  1. The actual values obtained from GetTradeSkillLevels are saved in the display frame and are used to build the tooltip. The graphical display will still be truncated at maxRank but the tooltip will contain the actual values.
  2. The entries in the table Skillet.db.global.SkillLevels can now be either a string or a table. If a table, they are [itemID][spellID]
  3. AddTradeSkillLevels now takes an (optional) sixth parameter (to create the above entry).

For example, the table entries for your transmute case would be [21885][28580] = "0/385/392/400" and [21885][28567] = "350/365/372/380".

commented

I was documenting the changes while you were entering your proposal (and I have to actually build alpha2).

commented

I'm curious what you think of my suggestion?

The modifications you are making to SkillLevels data format look reasonable. Would that also address enchanting, or are those still negative?

commented

Skillet-Classic-1.31-alpha2 is now available. Note: that the tooltip contains a percent chance to get a skill point and I did not change the calculation, it is still based on the graphical values, not the actual values. I'll think about what changing it would mean.

Play with this version and let me know what you think. I read your proposals and I think what I have implemented is a reasonable compromise. I don't want to drastically change the bar graph as that may be confusing to the rest of the users but actual values in the tooltip addresses your desire to see the real values. Again, Skillet-Classic is only the messenger so what values are fetched are (mostly) out of my control.

Enchanting values in my table would look like #32 (comment) (i.e. they are negative).

commented

Updated to alpha2

To test the new format of the SkillLevels table data I manually put in this:

function Skillet:InitializeSkillLevels()
	self.db.global.SkillLevels = {
		[0] = "orange/yellow/green/gray",
        -- alchemy
-- elixir-of-lions-strength
[2454][2329] = "1/55/75/95",
	}
end

And when I fired up the game I got this:

Message: Interface\AddOns\Skillet-Classic\SkillLevelData.lua:59: '=' expected near '['
Time: Sat Jul 24 09:35:35 2021
Count: 4
Stack: Interface\AddOns\Skillet-Classic\SkillLevelData.lua:59: '=' expected near '['

Locals: <none>

Am I doing it wrong?

commented

The syntax here is a bit strange but the correct way to express this is:

		[2454] = {[2329] = "1/55/75/95"},

I misled you with my example above which should have been:

[21885] = { [28580] = "0/385/392/400", [28567] = "350/365/372/380" }

and a reference would be:

levels = Skillet.db.global.SkillLevels[21885][28580]

The chat command /dump Skillet.db.global.SkillLevels might be helpful.

Here's an updated version of InitializeSkillLevels:

function Skillet:InitializeSkillLevels()
	self.db.global.SkillLevels = {
		[0] = "orange/yellow/green/gray",
		[6338] = "100/105/107/110",  -- Silver Rod
		[21885] = { [28580] = "0/385/392/400",  -- Transmute primal shadow to water
			[28567] = "350/365/372/380",  -- Transmute primal earth to water
			},
	}
end

I use Notepad++ and I have LuaWoW installed (https://www.wowinterface.com/forums/showthread.php?t=57031) so I get syntax highlighting and the ability to compile within Notepad++ to catch syntax errors like this one.

commented

Thanks, I'll grab that NP++ plugin.
I suspected it was broken lua syntax, but since I don't touch lua often, I didn't want to poke around with it invade your lane.
I'll re-test with the correct syntax.

commented

You aren't invading my lane! Happy to help. Its been a while since I setup Notepad++ to do the Lua thing so I'm not sure if all the pieces are still available but its a start and if you get into trouble, let me know and I'll try and help.

commented

Definitely still a bug somewhere
Using exactly this:

function Skillet:InitializeSkillLevels()
	self.db.global.SkillLevels = {
		[0] = "orange/yellow/green/gray",
                -- alchemy
                [6338] = "100/105/107/110",  -- Silver Rod
		[21885] = { 
                       [28580] = "0/385/392/400",  -- Transmute primal shadow to water
			[28567] = "350/365/372/380",  -- Transmute primal earth to water
		},
	}
end

when I click on "primal earth to water"
I receive the following parser error:

Message: Interface\AddOns\Skillet-Classic\SkillLevelData.lua:93: bad argument #2 to 'split' (string expected, got nil)
Time: Sat Jul 24 14:33:43 2021
Count: 1
Stack: Interface\AddOns\Skillet-Classic\SkillLevelData.lua:93: bad argument #2 to 'split' (string expected, got nil)
[string "@Interface\AddOns\Skillet-Classic\UI\MainFrame.lua"]:882: in function `UpdateTradeSkillWindow'
[string "@Interface\AddOns\Skillet-Classic\UI\MainFrame.lua"]:2373: in function `ScrollToSkillIndex'
[string "@Interface\AddOns\Skillet-Classic\Skillet.lua"]:1514: in function `SetSelectedSkill'
[string "@Interface\AddOns\Skillet-Classic\UI\MainFrame.lua"]:2294: in function `SkillButton_OnClick'
[string "*:OnClick"]:2: in function <[string "*:OnClick"]:1>

I'm not sure what to make of this yet.
But it looks like there might potentially be a bug here:

if type(levels) == 'table' then
  if spellID then
    levels = skillLevels[itemID][spellID]
  end
end

In the case that it is a table, but the spellID is not found in that table, it'll return nil and thus the split will fail.
Should this result in a "missing" entry, or it should just skip down to one of the other libraries?

commented

I wonder if spellID is a string somehow?
Or is it somehow being negated like with enchanting?

commented

Here's where you have me at a disadvantage. I can't debug because I don't have a character high enough. That means I'll have to teach you how to collect debug output so we can see what's happening. I think you may be right about spellID so I created a new version. Use the SkillLevelData.lua from this zip file:
SkillLevelData.zip

Type "/skillet debugshow on" in chat and then open Skillet-Classic and do your testing.

The debug output goes two places, the chat window and to the character specific saved variables file, Skillet-Classic.lua. Let me know if you don't know how to find it. You should be able to see what's going on in chat but if you want me to look at it, then zip the character specific saved variables file (rename to character.lua if more than one) and post it here. GitHub doesn't take .lua files but you can rename it to Skillet-Classic.txt and then you can post it.

The basic problem is that given a recipe name, there isn't a Blizzard API function to convert that to a spellID in Classic (there is in Retail). This is the major reason why there is Skillet for Retail and Skillet-Classic for Classic Era and Burning Crusade Classic. There is an API function to convert a spellID to a name in Classic. Since I work on both, I sometimes get confused myself.

commented

The newest version (from the zip file) appears to be working without any known issues.

commented

I opened a pull request to import all skill level data from wowhead.

commented

Skillet-Classic-1.31-alpha3 contains the pull plus my modifications.

commented

Fantastic work!!!

Apparently my huge comment got lost as I fumbled through getting to this point. I did have one question, which Wowhead site (URL) did you scrape the data from?

  1. I have moved the function InitializeSkillLevels to the end of SkillLevelData.lua.
  2. I have updated comments to reflect the current state.
  3. I have changed the indentation of the table entries and converted spaces to tabs.
  4. I have added your name as a credit. Feel free to update it (but not remove it)!

I would like you to volunteer to maintain the .js script and the table in SkillLevelData.lua. I can add you as a maintainer on the Skillet-Classic project or we can handle it with pull requests. Hopefully, I'll figure out how to do them correctly.

Probably should consider these changes for Skillet as well. A few changes to the code but the table will be the same.

commented

I lost an edit to MainFrame.lua for issue #36 while fumbling through the merge of your pull request. Skillet-Classic-1.31-alpha4 recovers it.

commented

I was scraping from https://tbc.wowhead.com

commented

The only thing I wasn't able to put into a loop, was iterating through the profession pages themselves.
wowhead won't allow to be loaded through an iframe, and any javascript inserted in the console is lost when the page changes. So... I'm out of ideas how to automate that part. But its pretty quick to just open every tab, then paste the code 10 times, then ctrl+c & ctrl +v 10 times.

I'm hoping that 2 things are true:

  1. that the profession recipes don't change often. I think even the later phase recipes are already there, so that shouldn't be a big issue.
  2. That the html layout of wowhead doesn't change often. However, this is only really a concern if point 1 happens, and we need to run it again.

I'd say there is a good chance this won't need to be run again until WotLK classic is out.

The javascript should work as is for classic.wowhead.com assuming recipes have any differences... and assuming anyone still plays vanilla classic.
I can take a stab at making a version of the javascript for retail, but I have little to no interest in retail anymore.

If for any reason something needs fixing in the javascript, ping me. You can add me as a maintainer if you want, but optional.

commented

I don't think things change very often. New releases most likely will just be additions. Since the Skillet-Classic sources are currently identical for both Classic Era and Burning Crusade Classic, I don't see any reason to "poke the bear" by looking at classic.wowhead.com.

If you wouldn't mind, I'd like to get a version of the javascript for retail. While I'm not playing retail much, I am still maintaining Skillet and the more code I can make common, the easier it will be for me and I like what we have done here.

commented

Gahhhh, I just noticed 2 more recipes that are missing their correct orange bound. 2 starting cooking recipes.
I tried downloading the wowhead app, but it doesn't seem to properly detect those recipes.... or the updates go through REALLY slow.... not sure. Either way, its NOT a problem with all starter recipes... just some.... for some reason. Infuriating. I'll make a scan for any others that I missed and add those as well.

Either way... sure, I'll poke around see if I can make something for retail reasonably quickly.

commented

New PR up

commented

SkillLevelData.lua has been updated with changes found while debugging retail (both files are now identical).

commented

To address your bugs:

  1. Skillet-Classic is only the messenger here. It displays the data it receives. If you don't like the answer, you can add an entry to the Skillet.db.global.SkillLevels with values you like better.
  2. The function Skillet:GetTradeSkillLevels(itemID) (defined in SkillLevelData.lua) accounts for Enchanting by negating the itemID when the current profession is Enchanting. Again, Skillet-Classic is only the messenger so I have to conform to the convention used by the external addon sources that are used. If I make the change you suggest, then Skillet-Classic now owns the entire database which I'm not going to do.
  3. The example you point out is probably going to give the wrong answer and the current structure doesn't allow for that.

Skillet-Classic-1.31-alpha1 contains an enhanced SkillLevelData.lua. The main function now has two inputs, Skillet:GetTradeSkillLevels(itemID, spellID) and the call in MainFrame.lua has been changed to pass both arguments (other calls still pass only one argument). The input, spellID, is currently not used but the function (and data structures) could be modified to resolve the transmute issue you identified. Note that if the current profession is Enchanting, the itemID is negated (I believe -itemID is the spellID).

A new function, Skillet:PrintTradeSkillLevels(itemID, spellID), has been added. This can be manually called with /run .

A new chat command, /skillet printskilllevels itemID, will print the skill values from the table along with the source information. I don't believe you can input two values with the current implementation, use the /run option if you need to input both.

--
-- Print the TradeSkillLevels(itemID) result including the actual index and the source.
--
-- index will be itemID or if the current profession is Enchanting, -itemID
--
-- source will be:
--    1 if from Skillet.db.global.SkillLevels
--    2 if from TradeskillInfo
--    3 if from LibPeriodicTable
--    4 if itemID was missing or not a number
--    5 if it wasn't found (and was added to Skillet.db.global.MissingSkillLevels)
--

Since you seem to have an interest in enhancing Skillet-Classic's accuracy in this area, I'm making changes to assist your efforts. If you come up with a better version of SkillLevelData.lua, I will include it in future builds.

commented

Note: The chat command /skillet debugshow will turn on debug output to chat which will show the input(s) to GetTradeSkillLevels. You can uncomment other debug statements in SkillLevelData.lua if you wish to see more information.

commented

To be clear, Bug 1 is with a manual entry in SkillLevels DB. I had manually set "350/390/397/405" in SkillLevels DB to match wowhead, and it displayed the 350 correctly, but the other 3 numbers were rounded down to 375.

And thanks for making all enhancements you've done so far. I'm working on a reasonably easy to use scraper for wowhead profession data into a format that Skillet-Classic can read. Working on edge cases at the moment.
I'll poke around with your changes to GetTradeSkillLevels and the debug commands and see if I can make any useful contributions.

commented

OK, Bug 1 now makes sense but I'm not sure how to fix it since its a graphical thing. The code is in ...\UI\MainFrame.lua. Lines 941-955 set the limits of the bar and lines 1687-1697 put the values in.

What does the Blizzard UI (shift-click a profession button) do?

If you figure out something other than "maxRank" to use in the setup, let me know.

commented

Skillet-Classic-1.31 has been released.