Allstates helper methods

[mrbuds]

Add methods to the states/allstates table that helps with creating, updating or remove states in an optimized way

Advantage of using this function instead of doing a states[key] = { ... }

• If already created, update existing state, and return true if any value was changed, this can help reduce amount of resources an aura use
• Automatically add some fields: progressType, expirationTime, icon, show, changed
• Automatically return true when using these functions and any change was made

Examples

function(states, event, ...)
    if event == "PLAYER_TARGET_CHANGED" then
        if UnitExists("target") then
            -- if state exists it's updated, not replaced
            -- show & changed fields can be skipped
            states:Update("", {
                    name = UnitName("target"),
                    duration = 5,
                    expirationTime = GetTime() + 5,
                    progressType = "timed",
                    autoHide = true
            })
        else
            -- wipe
            states:RemoveAll()
        end
    end
    -- no need to return true
end

with clones

function(states, event, ...)
    local currentEssence = UnitPower("player", Enum.PowerType.Essence)
    local maxEssence = UnitPowerMax("player", Enum.PowerType.Essence)
    for i = 1, 6 do
        if i > maxEssence then
            states:Remove(i) -- wipe allstates[6]
        else
            local value = currentEssence >= i and 1 or 0
            local newState = {
                progressType = "static",
                value = value,
                total = 1
            }
            states:Update(i, newState)
        end
    end
    -- no need to return true
end

maintaining value of the variable changed when editing multiple clones is not great :(

[github-actions]

An experimental build of WeakAuras with the changes in this pull request is available here.
Build Time: Fri Jun 28 23:37:31 UTC 2024

[emptyrivers]

I like the idea, in general.

have you considered "just" making those functions be part of a mixin so that they magically appear as methods of tsu states param? or is that maybe going too far 🤔

[InfusOnWoW]

That feels like the wrong order to me, "key, state" feels a lot more natural to me.

I know you did that for key to be optional, but I still feel that its the wrong order.

[mrbuds]

swapped them, and made update function check differences recursively (eg. for overlays)

[emptyrivers]

maintaining value of the variable changed when editing multiple clones is not great :(

It's not impossible that we could magic that inconvenience away, though it might be considered breaking.

Currently, we apply new state to its region if:

• state.changed is truthy, and
• state updater function returns truthy

In order for changes to be applied, author must return true. Common shibboleth is to just always return true and the end of TSU.

But consider this:
apply new state to region if:

• state.changed is truthy, and
• state updater function does not return false

In order for changes to be applied, author can simply let the function fall through without returning (thus, no changed tracking needed). Some few authors who found the previous behavior useful can still explicitly return false to suppress state updates.

... so after writing it out, it's definitely a breaking change (though, possibly low impact - how many people actually use the TSU return value as designed?), but it does afford us the nice advantage of not having to manually track if any state is changed.

main downsides:

• it is still breaking
• potential performance problems could happen in TSU with many states. Not impossible for author to solve (the same risks exist with current behavior). But there are potential magics we could do to improve that too.

[emptyrivers]

well, that is all probably beyond the scope of this ticket anyways.

[mrbuds]

What if states:Update/Remove/RemoveAll set a hidden flag allStates.__changed, to make auras using these new functions not having to return anything without breaking (afaik __fields are ignored from pairs(allStates) right?)

RunTriggerFunc :

--    if( (ok and returnValue) or optionsEvent) then
++    if( (ok and (returnValue or (returnValue ~= false and allStates.__changed))) or optionsEvent) then
++      allStates.__changed = nil
        for id, state in pairs(allStates) do
          if (state.changed) then
            if (Private.ActivateEvent(id, triggernum, data, state)) then
              updateTriggerState = true;

[mrbuds]

ha shit All States table contains a non table at key: '__changed'. [string "@Interface/AddOns/WeakAuras/GenericTrigger.lua"]:647: in

edit: i'm an idiot, should have edited the code block a few lines up

[mrbuds]

@emptyrivers with 3b04d7a there is no need to return true when Update|Remove|RemoveAll functions made a change to allStates table.
It doesn't change behavior of code that don't use these functions

[emptyrivers]

it is perhaps a tiny bit weird that code which doesn't use the utility functions can set __changed = true but on balance i think it's fine

[emptyrivers]

technically a breaking change for any code that used states["__changed"] state

[InfusOnWoW]

So, I have lots of thoughts about this. When I designed the new trigger state mechanism a long tiem ago, I did consider various ways in which triggers communicate what has changed with the rest of WA. The ideal interface is that the user just writes to the states table and each state, and WA handles the syncing of that to the display/conditions/text replacement/dynamic groups and so on.

I rejected that, because this makes the case that nothing has actually changed pretty expensive, and that's not an uncommon case at all. That's why the user and triggers have to return true, and why the have to set .changed to true. Similarly the only purpose of .show = false, is that it's easier to check for than a missing cloneId.

The second tricky question back then, how granular should the change tracking be. I quickly settled on one changed flag.

I've been thinking about a new trigger state framework, which rexamines those two things and maybe gets us that ideal interface and a finer change tracking and even some further optimization potential, though it's been on the always long future ideas list. (It's not too unsimilar to what rivers did to the dynamic group,)

Now, so the question for me is, is that an idea I'll ever come around to, and if so, how does this PR interact with that. I'll try to do a bit of experimentation

[mrbuds]

I rejected that, because this makes the case that nothing has actually changed pretty expensive, and that's not an uncommon case at all. That's why the user and triggers have to return true, and why the have to set .changed to true

In practice people are lazy and i see all the time auras that overwrite allstates table without caring if anything was changed, and return true in any case, even if allstates table wasn't touched (like CLEU:SPELL_AURA_APPLIED => guid ~= myGUID => return true)

This keep old behavior, while giving access to simplified method to make code that doesn't fall in these traps (and you can still return false)

I was also thinking this would help with idea to convert current generic triggers to small/efficient/readable enough TSU

[InfusOnWoW]

Yes I agree that in pratice people don't use changed very much, so in that sense the API is less than ideal and certainly your changes are an improvement as that makes editing states easier.

The concern I have is that introducing such a convenient API might clash with future changes on how TSU triggers work. That is if we add a TSUv2, where the api is just setting up states and WA handles the rest automatically, without any requirement to set .changed or even return true, then this might be a dead-end API, and adding it will just confuse users.

I've started a bit of research if that "perfect" api is possible and so far so good. My small prototype does behave like I want it to behave, and I want to push it a bit further to decide whether that's going to be the next "big" project I want to tackle. It's something I have been considering for a while and it would unlock a bunch of potential features. In any case, I don't intend to take too long to figure out what the right direction is.

[mrbuds]

I don't intend to take too long to figure out what the right direction is.

I'm glad this PR is giving you motivation for greater changes, but take the time you need