Skinning mode deletes starlight rose
LyraelRayne opened this issue ยท 19 comments
I have hotloot set to collect herbs, however skinning mode is deleting my starlight roses when I herb. Skinning mode should probably not trigger on gathering/opening events (only looting corpses) if possible and starlight rose should definitely be included in the list of things to keep!
Skinning mode will delete anything not picked up by the filter. Is it looting other herbs ok? I'm trying to find out why that item would be different... the subtype appears to be "Herb" so im not sure why it wouldn't. Would it be possible for you to paste a screen shot of your filter settings as they were when it happened?
P.S. You can set a modifier key to Enable or Disable skinning mode so you could always use that. Skinning mode has a warning on it because anything not caught gets deleted.
Hmmmmm... I ran the item id through a test filter to see if it was caught and it worked and caught it in the "Loot Herbs" section. The only things I can think of is either you had it in the Exclude List or there is some weird bug in skinning mode that is messing it up... That wouldn't make much sense though since skinning mode happens after stuff is looted.
There has to be something I'm missing... I'll keep looking. In the meantime double check everything and keep skinning mode off and see if it loots the item.
There is a good chance this is fixed as of v3.0.3 although I am still unable to reproduce it. I am closing this issue for now. If you still notice it happening in the future please comment again and I'll reopen it. Thanks!
I'll test with the later version, but interestingly enough, selecting "loot cooking materials" causes them not to be deleted. I suspect the issue is that the herb is BOTH a cooking material and a herb. This means other similar items might have the same issue.
Testing also indicates that starlight roses are looted with skinning mode turned off.
Tested with 3.0.3 and it still happens. Also I got an error message but I think it's related to the issue you already created regarding empty thresholds ;)
Hmmm actually this happened again when i had loot cooking ingredients selected. I've commented out the line which actually does the deleting (but still prints out the message) and I'll do further testing (without the lost profits!)
At the moment I suspect this is happening when starlight rosedust is present but not happening otherwise.
Yep. Happens when starlight rosedust is in the loot but doesn't happen otherwise.
I think I found the bug.
You're creating a space separated list of items to be deleted as a string. For each item in the bag you search for any item whose name is contained in that string. This means that you will delete any item whose name matches or is contained within the name of any item which is found.
This will cause all manner of bugs, the least of which is deleting any item in your bag whose name is a substring of the name of an item to be deleted. It'll also cause trouble if you have two items in the "to be deleted" list and their names put together happen to match another item, creating an even more obscure and difficult to find bug.
I'd suggest something like the following:
local toDelete = {}
for do
toDelete[name_of_item] = true
end
then when doing your deletion you can simply go
if(toDelete[name_of_item_in_bag]) then { -- delete it)
My apologies and thank you for finding the bug. Most of the add-on was written a long time ago when I was relatively new to programming concepts. Ever since I have recently resumed development of the add-on I have been going through and redesigning everything the proper way only I have not gotten to that part as of late. I will make this my new priority and will have a new version out within a day or two. I'm also reopening this issue. Again, thank you for your help.
Honestly, it was like 5 years ago when I wrote that so I have no idea why I did any of it that way... I definitely would have done it differently nowadays.
hmmmmm.... I want to commit the pr to the develop branch since I only use the master for releases. Would it be better to just edit the pr and change the branch to develop or what?
I'm not that familiar with PRs. If that's an option then go ahead. Otherwise, it's a 3 line change so you could probably just copy what i did.
Thanks! The changes have now been pushed to develop. I am going to polish up the whole skinning function then release a new version tomorrow.
Oh btw if you ever have any other good ideas feel free to make another pull request. I am currently using the git flow branching model so you can either push to the develop branch or create a feature branch and merge it with develop.