nikkisaurus / FarmingBar

4 stars 1 forks source link

Alpha testing #30

Closed niketa-wow closed 3 years ago

niketa-wow commented 3 years ago

Do you want other issues I find? (with the proper alpha package now 😋 )

Custom condition method seems to have gone missing

1x ...rface\FarmingBar\Options\ObjectiveBuilder.lua:168: attempt to call method 'ValidateCustomCondition' (a nil value)
[string "@FarmingBar\Options\ObjectiveBuilder.lua"]:168: in function <...rface\FarmingBar\Options\ObjectiveBuilder.lua:166>
[string "=[C]:"]: ?
[string "@Ace3\AceConfig-3.0-3\AceConfigDialog-3.0\AceConfigDialog-3.0-81.lua"]:51: in function <...nfig-3.0\AceConfigDialog-3.0\AceConfigDialog-3.0.lua:49>
[string "@Ace3\AceConfig-3.0-3\AceConfigDialog-3.0\AceConfigDialog-3.0-81.lua"]:747: in function <...nfig-3.0\AceConfigDialog-3.0\AceConfigDialog-3.0.lua:668>
[string "=[C]:"]: ?
[string "@Ace3\AceGUI-3.0\AceGUI-3.0-41.lua"]:72: in function <Ace3\AceGUI-3.0\AceGUI-3.0.lua:70>
[string "@Ace3\AceGUI-3.0\AceGUI-3.0-41.lua"]:306: in function 'Fire'
[string "@Ace3\AceGUI-3.0-41\widgets\AceGUIWidget-MultiLineEditBox.lua"]:61: in function <...AceGUI-3.0\widgets\AceGUIWidget-MultiLineEditBox.lua:58>

I should make a separate ticket probably

Originally posted by @Road-block in https://github.com/niketa-wow/farmingbar/issues/29#issuecomment-882160932

niketa-wow commented 3 years ago

I was actually planning on restructuring the custom condition and completely forgot about the fact that other people may have things using them at the moment. Let me check if it's broken because of rewrites or just commented out while I was trying to rethink it.

Road-block commented 3 years ago

No rush on my behalf, I can actually stay on alpha.7 this addon has been a life saver and I have a few really nice objectives setup with that version.

Primal Might goals that count materials down to motes in bank + alts, it's been great.

Edit: That said the option to include bank + alts seems to be missing from tracker options in latest alpha, is that getting removed?

niketa-wow commented 3 years ago

No rush on my behalf, I can actually stay on alpha.7 this addon has been a life saver and I have a few really nice objectives setup with that version.

What all is missing from the latest release (besides the custom condition) that you feel is a necessity before you upgrade? Summer classes are gonna be over in about 2 weeks and I'm hoping to spend some time doing fun stuff like this.

Edit: That said the option to include bank + alts seems to be missing from tracker options in latest alpha, is that getting removed?

I reworked the way include bank/alts works, so that instead of setting it per tracker, it's set on the bar itself. Basically the rework is that the objective builder essentially creates a template and when you put it on a bar, it creates a copy. So objective builder templates are no longer linked to the builder. I realize this may be slightly annoying if you want to make a change, but it is a lot more complicated to deal with linking them to bars and deciding how things should or should not be updated. Anyway, as a result, the objective builder template is a global setting that can be used on any toon whereas the objectives you add to your bar are character specific. Hence, include bank and alts seemed to make more sense as a character specific setting and cannot be edited in the objective builder. They can be toggled on* items with only one tracker. The default keybinds are alt+left+click to include bank and alt+right+click to toggle alts. I do have plans to create a separate editor for character specific trackers to allow you to do this with multiple trackers, which is what I imagine you are waiting for.

Road-block commented 3 years ago

Yeah I have stuff like this with alpha.7 image

Common scenarios I've implemented that I would like a way to keep doing going forward. (off the top of my head so this might not be all inclusive)

Probably other use cases I'm forgetting but the breakdown or combination tracking is a recurring theme for me. (I do also use the occasional quick objective that essentially saves me dragging an item to one of the game actionbars)

Road-block commented 3 years ago

Forgot to say I don't use Altoholic but I had installed the requisite DataStore* libraries that FarmingBar uses as a backend.
DataStore_Characters is missing from ## OptionalDeps: but is required for the item tracking to work.

niketa-wow commented 3 years ago

https://github.com/niketa-wow/farmingbar/releases/tag/v3.0-alpha10.1

v3.0-alpha10.1 should have custom conditions as well as the ability to add includeBank and includeAllChars for multi-tracker objectives. Alt+left click a button to includeBank and alt+right click to includeAllChars. For single trackers it will toggle; otherwise it opens a window for you to choose which trackers to include.

And I added DataStore_Characters as an optional dependency.

Feel free to open another ticket when you find more errors or have other suggestions. Thanks.

niketa-wow commented 3 years ago

Since you are familiar with lua and are using the custom conditions frequently, I thought I'd share with you my plan for a revamp. If you want to look over it and see if you have any thoughts/requests, that'd be cool. You don't have to, just wanted to give the opportunity while it's still just an idea. Mind you none of this is tested and the behavior section probably is flawed until I can actually try to implement it and troubleshoot.

I'm hoping doing it like this might make it easier to write/read and give a little bit more flexibility and control.

--[[
    * required, ** required if parent is provided
    conditions:
    {
        *priority = int, -- Condition has this priority in using available items/currencies
        *tracker_condition = str, -- "ALL" or "ANY"
        *trackers = {
            *[tracker.order] = {
                *objective = int, -- Required number of this tracker
            },
        },
        equivalencies = {
            [tracker.order] = {                
                distribute_evenly = bool, -- if true, ignores equivalencies.tracker.priority
                **equivalencies = {
                    [tracker.order] = {
                        priority = int, -- the whole count may be used on the first item, but this may be helpful if for example priority uses up as much as possible and there's leftover in a ratio usable by a later priority
                        **ratio = int, -- numInitialTracker / numEquivalentTracker
                    }
                },
            }, -- cannot be in condition.trackers
        },
    }
]]

--[[
    Example:
    Cooking recipe for Azshari Salad:
    1 Suramar Surf and Turf, 5 Fjarnskaggl, 5 Dreamleaf, 5 Foxflower, 5 Aethril

    Trackers:
    ["trackers"] = {
        ["ITEM:133566"] = { -- Suramar Surf and Turf
            ["order"] = 1,
        },
        ["ITEM:124104"] = { -- Fjarnskaggl
            ["order"] = 2,
        },
        ["ITEM:124102"] = { -- Dreamleaf
            ["order"] = 3,
        },
        ["ITEM:124103"] = { -- Foxflower
            ["order"] = 4,
        },
        ["ITEM:124101"] = { -- Aethril
            ["order"] = 5,
        },
        ["ITEM:124124"] = { -- Blood of Sargeras
            ["order"] = 6,
        },
    },
]]

-- conditionInfo table to be returned:
return {
    {
        priority = 1,
        tracker_condition = "ALL",
        trackers = {
            [1] = { -- Suramar Surf and Turf
                objective = 1,
            },
            [2] = { -- Fjarnskaggl
                objective = 5,
            },
            [3] = { -- Dreamleaf
                objective = 5,
            },
            [4] = { -- Foxflower
                objective = 5,
            },
            [5] = { -- Aethril
                objective = 5,
            },
        },
        equivalencies = {            
            -- This will error out, because we can't use an initial tracker as an equivalency tracker:
            -- [1] = { -- Suramar Surf and Turf
            --     ...
            -- },

            -- This tracker is used only for the equivalency:
            [6] = { -- Blood of Sargeras
                distribute_evenly = true,
                equivalencies = {
                    [2] = {
                        -- priority = 1,
                        ratio = 10, -- 10 herbs / 1 blood
                    },
                    [3] = {
                        -- priority = 3,
                        ratio = 10, -- 10 herbs / 1 blood
                    },
                    [4] = {
                        -- priority = 2,
                        ratio = 10, -- 10 herbs / 1 blood
                    },
                    [5] = {
                        -- priority = 4,
                        ratio = 10, -- 10 herbs / 1 blood
                    },
                },
            }
        },
    }, -- condition1
}

--[[
    Behavior:    
    1. Get counts of trackers 1, 2, 3, 4, and 5
        246/1 = 246 Suramar Surf and Turf, 18/5 = 3 Fjarnskaggl, 47/5 = 9 Aethril, 22/5 = 4 Foxflower, 512/5 = 102 Dreamleaf
    2. Find the minimum of the counts to get the actual amount for "ALL" or sum the counts for the actual amount for "ANY"
        "ALL": 3
        "ANY": 364
    3.a. Get the counts for each equivalency
        12 Blood of Sargeras
    3.b. Distribute:
        equivalency_count = 12
        while equivalency_count > 0 do
            for k, v in addon.pairs(equivalencies, function(a, b) return a.priority < b.priority end) do
                if distribute_evenly then
                    -- distribute one item and decrease equivalency_count by 1
                    -- 1 to [2] (+10), 1 to [3] (+10), 1 to [4] (+10), 1 to [5] (+10), 1 to [2] (+10), 1 to [3] (+10), 1 to [4] (+10), 1 to [5] (+10), 1 to [2] (+10), 1 to [3] (+10), 1 to [4] (+10), 1 to [5] (+10) break
                    -- Each will gain 30 herbs
                else
                    -- distribute everything possible
                    -- be sure to decrease equivalency_count for each item distributed
                    -- 12 to [2] (+120) break
                    -- [2] will gain 120 herbs
                end
            end
        end
    4. Add the additional counts to the original totla
        if distribute_evenly:
            "ALL": Lowest num is 3; each herb gained 30 and 30/5 = 6, so the new minimum/total count is 9
            "ANY": Each herb gains 30/5 = 6, so the new sum is 388 ((6 * 4 herbs) + 364 )
        if not distribute_evenly:
            "ALL": [2] was previously the lowest herb and it's now gained 120/5 = 24 which makes the new minimum/total count 4 because the new lowest is Foxflower
            "ANY": [2] still gains 24, so we add to the sum: 388
]]
Road-block commented 3 years ago

Just a note that I saw this and I'm happy to stay on the alpha channel and help test.

It might be a couple days before something shows up here though, as I have to slot this in among other things 😅

From a quick look the custom conditions system looks like a version of what we had with much less hardcoded assumptions and a lot more parameterized.
Since I would consider it a "power user" feature anyway I don't see a problem with that.
Maybe some time in the future there is a way to export or import objectives, including their custom trackers so more advanced users can share examples for others to tweak.

Road-block commented 3 years ago

[New] button on the objective builder seems to be silently ignored (no Lua errors) after an objective has already been built.
ie. first objective creation worked, second seems to not work (alpha.10.1)

It's at least in part a visual bug with refreshing the UI.
After a /reloadui all my attempts to create objectives show up in the dropdown.

image

niketa-wow commented 3 years ago

Yes I was already planning on some sort of import/export, especially because of the fact that you can create complex objectives.

My thought on the changes to the custom syntax is that the parameters would make it easier to see what's what and allows you to do more stuff (like the equivalency).

I also had thought about doing some sort of thing like WeakAuras to make a more user friendly custom condition but it was gonna be a lot of work put into figuring out how to organize and implement it so it went on the back burner. I may revisit it in the future.

Definitely no rush on any suggestions or anything like that. Help is appreciated but never necessary.

As far as the second bug, I did briefly come across it after the release but I didn't have time to fix it and release another. I did log it on my to-do list though.

I think it's an issue with creating a new table from another but not cloning it properly. Can't confirm but that's my best guess blind. Like if there's a table template and a new objective is created by new = template. I've seen issues with that before. I do have a clone method though.

Road-block commented 3 years ago

Yeah array variables in Lua are like "by reference" variables in other languages, unlike "primitives" (strings, numbers, functions etc)

local a = {}
local b = a
a[1] = "first item"
print(b[1]) -- prints "first item"

table variables end up pointing to the same memory address, so assignment operator is not very intuitive to use.

Road-block commented 3 years ago

I'm having some strange side-effects of that table bug.
Objectives that have their condition check to Any but tooltip reporting it as All.
Alerts when a specific objective is complete that instead print the title of another objective.
It looks like the database gets corrupted by objective edits/renames.

fb-bug1-01 fb-bug1-02 fb-bug1-03

niketa-wow commented 3 years ago

I won't be home for another 9 hours or so to be able to work on it but if you want to try something you can. It might not do anything though. Lol

Anywau

Modules/Objectives.lua line 211 Currently is local objectiveTemplate = self:GetDBValue("global", "objectives")[newObjectiveTitle]

Try changing it to local objectiveTemplate = self.db.global.objectives[newObjectiveTitle]

I'm not sure if it'll fix it or not because it's not necessarily copying a table and it is using acedb's ** key for missing values. Hence why I'm confused why it may need a more direct reference. You probably know better about that. If that's not it then it'll have to wait until I can get to the computer and mess around with it.

Road-block commented 3 years ago

You said you don't have time to work on the addon right now, don't get sucked in 😝

It'll keep; I'm just dumping this here lest I forget, there's no urgency whatsoever.

Edit: Also timezones are a 🐶 .. I'll be asleep next 9hrs ..

niketa-wow commented 3 years ago

Lol I can't help it. Once I get going I can't stop.

I actually work second shift so it's really 2:30am when I'ma get home. ><

Road-block commented 3 years ago

Question Would the current table structure for custom check (or the planned implementation) be able to support a 3-tier breakdown?

(ie would something like return {{t1=1},{t2=1,t3=1},{t4=1,t5=1}} work)

Use case example: 1 Felsteel Bar (objective) = 1 Felsteel Bar (item tracker) -- the finished product counts as one OR (3 Fel Iron Bar AND 2 Eternium Bar) -- the materials for the finished product in bar form count as one OR (6 Fel Iron Ore AND 4 Eternium Ore) -- the raw materials to bars to finished product count as one

Bars and Ores are essentially sub-objectives conceptually (ALL condition), combined with logical OR (addition) for the master objective.

niketa-wow commented 3 years ago

Question Would the current table structure for custom check (or the planned implementation) be able to support a 3-tier breakdown?

(ie would something like return {{t1=1},{t2=1,t3=1},{t4=1,t5=1}} work)

Use case example: 1 Felsteel Bar (objective) = 1 Felsteel Bar (item tracker) -- the finished product counts as one OR (3 Fel Iron Bar AND 2 Eternium Bar) -- the materials for the finished product in bar form count as one OR (6 Fel Iron Ore AND 4 Eternium Ore) -- the raw materials to bars to finished product count as one

Bars and Ores are essentially sub-objectives conceptually (ALL condition), combined with logical OR (addition) for the master objective.

It should work in the currently implemented version.

Also, btw, sorry I didn't have a chance to look at the objective builder bug last night, homework took longer than expected. I'll get to it at some point this week. Most likely tonight as I usually don't do much school on raid nights. lol

niketa-wow commented 3 years ago

Can you walk me through the steps to recreate the bug on the objective builder? It's possible that it's a BCC issue because I think I noticed it when I was bug checking over there, but I couldn't replicate it in retail.

Basically, walk through the steps of the first objective you set up before trying to add a new one, so I know which buttons/functions may have been called.

Road-block commented 3 years ago

I have a feeling it might not be an actual logic bug but more so non-intuitive user experience.

What I mean is that currently (in contrast with how older versions worked) you said when you make or change an objective you are essentially making changes to the template (class) used to make copies for buttons (instances of that class).

So what I think is happening is that after you create the objective, if you make changes to it (change the condition logic, update the custom condition or anything really) but you have already dragged the previous version of the template on a bar, that instance of the objective does not update.

There's probably some confusing "bug like" things happening with objective renames (at the template level) if they match an already dragged previous version of that or another instance of an objective that's on a bar.

TL;DR: User might expect that when they edit an objective in the objective builder, instances of that objective that are on bars are also updated, which doesn't appear to be the case at the moment.

Re-dragging the updated objective to the bar (or bars it was on) setting bank/alts again etc, ie manually updating instances of the edited objective would be the "fix" but it would have to be pointed out to the user, I don't think it's obvious.

Road-block commented 3 years ago

For the other part of it, where you hit the [New] button and nothing shows up on the UI until a /console reloadui I have a feeling it might be some kind of race condition, because I've had it happen intermittently not just on the objective level but also when adding a new id for a tracker (more rarely)

  1. Hit New, nothing happens, reloadui and it's there with the default "New" name (or if you made multiple attempts New 2, New 3 etc) Other times it works to populate the objective tab with default values and show the new entry in the dropdown top right.

  2. Similarly I've had it type in an item id into the tracker editbox, hit Okay and the list of tracked items does not update (it is not a misstyped item id as it shows up after a reloadui as well)
    This happens more infrequently than 1 but I've seen it as well. (no Lua errors in either case)

niketa-wow commented 3 years ago

I have a feeling it might not be an actual logic bug but more so non-intuitive user experience.

What I mean is that currently (in contrast with how older versions worked) you said when you make or change an objective you are essentially making changes to the template (class) used to make copies for buttons (instances of that class).

So what I think is happening is that after you create the objective, if you make changes to it (change the condition logic, update the custom condition or anything really) but you have already dragged the previous version of the template on a bar, that instance of the objective does not update.

There's probably some confusing "bug like" things happening with objective renames (at the template level) if they match an already dragged previous version of that or another instance of an objective that's on a bar.

TL;DR: User might expect that when they edit an objective in the objective builder, instances of that objective that are on bars are also updated, which doesn't appear to be the case at the moment.

Re-dragging the updated objective to the bar (or bars it was on) setting bank/alts again etc, ie manually updating instances of the edited objective would be the "fix" but it would have to be pointed out to the user, I don't think it's obvious.

This was actually an intentional change because I was having a hard time thinking of a nice, clean way to create instances.

The database is set up so you have a character database that has the items that are on your bar, similar to how each character has different things on their actual action bars. The objective templates are global. So to keep them up to date, would have to find some way to keep a manual link on them. I had started coding some sort of link before I took a long hiatus, but upon coming back I decided it wasn't worth the effort. I don't remember why, but I think there was something I was trying to figure out how it should function and couldn't decide the best way so I said screw it.

Do you think keeping it this way, with no link, would be detrimental to the experience? Or would better explanation of its behavior suffice?

Unfortunately right now, I'm at the stage of getting things working so a lot of my code is a mess right now. I did have some notes about how I could approach it in my todo file, but it wasn't very clear. I gotta get better with stuff like that. lol

For the other part of it, where you hit the [New] button and nothing shows up on the UI until a /console reloadui I have a feeling it might be some kind of race condition, because I've had it happen intermittently not just on the objective level but also when adding a new id for a tracker (more rarely)

  1. Hit New, nothing happens, reloadui and it's there with the default "New" name (or if you made multiple attempts New 2, New 3 etc) Other times it works to populate the objective tab with default values and show the new entry in the dropdown top right.
  2. Similarly I've had it type in an item id into the tracker editbox, hit Okay and the list of tracked items does not update (it is not a misstyped item id as it shows up after a reloadui as well) This happens more infrequently than 1 but I've seen it as well. (no Lua errors in either case)

So it sounds like maybe just an issue with reloading the config and not problems creating things?

niketa-wow commented 3 years ago

Btw, here was what I started with as far as instances of the objective:

  1. Add a value in the char db of the button's item that has a reference to the template it was made from
  2. Add the character->bar->button IDs to the objective template itself
  3. On login, as setting buttons up, check if it's linked to a template and if so check if template has changed & update
  4. On template change, update all instances you can (aka the current toon only; other toons are updated on the login)

In theory this sounds like it'd work fine, but I don't remember why I gave up. Maybe it was just getting messy to keep it all clean at the time.

Edit: I'm also thinking the link in the template itself for the diff instances is pointless. Can just scan all the buttons and check for the template anyway. Unless the user has like hundreds of hundreds of populated buttons it shouldn't be bad on performance.

To be honest, after some thought, I think the reason I gave up on it was because I wasn't sure how I'd handle changing includeAllChars and includeBank. But now that those are button only, it might be irrelevant.

niketa-wow commented 3 years ago

No! I just remembered why I decided it was too complicated, because I couldn't make up my mind how to handle this issue:

  1. Say you have an objective, and you put it on the bar. But then later you delete the objective. Do you leave all the instances on all bars? (Maybe if the linked reference doesn't exist, remove the reference and treat it like a normal objective?) Or do you remove all instances of it?
  2. What if you rename it? This might be why I had the list of all buttons using the objective, because otherwise if you then created a new objective with the old name, it would then link to the wrong thing. The login process would then be more complicated, as you'd then have to check for the template ref, check the template itself to see if the button is included, then do whatever.
  3. Creates the extra step of when you move a button instance, you then have to go update the template.

I think basically all of these things combined was just too much for me to think about while still deciding how it SHOULD behave. I can probably still do it, but it'd be more ideal if I can think of a cleaner way to implement it than all this.

niketa-wow commented 3 years ago

This thread is getting a little hard to navigate, so I'm splitting it up by issue. For new issues, let's do separate trackers.