-
-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add warband currency support #5229
base: main
Are you sure you want to change the base?
Conversation
ee62f78
to
995a03c
Compare
995a03c
to
5881517
Compare
"ACCOUNT_CHARACTER_CURRENCY_DATA_RECEIVED", | ||
"CURRENCY_TRANSFER_LOG_UPDATE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was expecting this to error on cataclysm, but it didn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-retail flavors don't have currencyInfo.isAccountTransferable
which is what is gatekeeping any retail code from running on cata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do call RegisterEvent() on all of these evenrs, but the RegisterEvents is in a xpcall:
WeakAuras2/WeakAuras/GenericTrigger.lua
Line 1421 in 660a6cb
xpcall(frame.RegisterEvent, trueFunction, frame, event) |
|
"ACCOUNT_CHARACTER_CURRENCY_DATA_RECEIVED", | ||
"CURRENCY_TRANSFER_LOG_UPDATE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do call RegisterEvent() on all of these evenrs, but the RegisterEvents is in a xpcall:
WeakAuras2/WeakAuras/GenericTrigger.lua
Line 1421 in 660a6cb
xpcall(frame.RegisterEvent, trueFunction, frame, event) |
return Private.AccountCurrencyData[currencyId] | ||
end | ||
|
||
Private.AccountCurrencyData[currencyId] = C_CurrencyInfo.FetchCurrencyDataFromAccountCharacters(currencyId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you need this caching? Did you measure whether C_CurrencyInfo.FetchCurrencyDataFromAccountCharacter is actually expensive.
Also note that the way you implemented caching is currently faulty, because:
- Multiple triggers for the same currency, will receive the same CURRENCY_TRANSFER_LOG_UPDATE event and thus refresh. (That's probably not a problem.)
- Relying on the trigger receiving the event does not work with auras loading/unloading, that is:
** Assume an aura only loaded in say the capital city
** The aura is initially loaded, the AccountCurrencyData is updated
** The user leaves that zone, the aura unloads
** The user transfers currency, resulting in CURRENCY_TRANSFER_LOG_UPDATE, but no aura is loaded so no cache update happens
** The user reenters the capital city, the trigger receives WA_DELAYED_PLAYER_ENTERING_WORLD, WA_DELAYED_PLAYER_ENTERING_WORLD doesn't get the cache updated, so old stale data is used.
(I didn't test that scenario, but I think that's how it works, or rather doesn't.)
Imho check first whether we need the caching, I suspect we don't.
end | ||
end | ||
else | ||
C_CurrencyInfo.RequestCurrencyDataForAccountCharacters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes each trigger call C_CurrencyInfo.RequestCurrencyDataForAccountCharacters(). I think you should move the handling of the api to an outside system in GenericTrigger.lua, which handles that for every trigger.
quantityEarnedThisWeek = %q, | ||
totalEarned = %q | ||
} | ||
local discovered = %q |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using %q for these seems wrong, %s should be the better choice, %q should only be used for strings.
state.show = active | ||
state.value = currencyInfo.quantity | ||
state.realCharacterQuantity = currencyInfo.realCharacterQuantity | ||
state.capped = currencyInfo.maxQuantity and currencyInfo.maxQuantity > 0 and currencyInfo.quantity >= currencyInfo.maxQuantity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These capped calculation is duplicated above in the checkTriState calls, the code above would be much easier to read if you calculated it just once.
|
||
ret = ret .. [=[ | ||
-- Gather currency data for account characters | ||
if currencyInfo and currencyInfo.isAccountTransferable then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole code probably should check for "clone" somewhere
trigger.use_maxQuantity and trigger.maxQuantity_operator or "<", | ||
trigger.use_quantityEarnedThisWeek and trigger.quantityEarnedThisWeek_operator or "<", | ||
trigger.use_totalEarned and trigger.totalEarned_operator or "<", | ||
trigger.use_discovered ~= nil and (trigger.use_discovered and "true" or "false") or "nil", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using something like "trigger.use_discovered and "true" or "false" " is fine, if the check is not set at all, we can treat it as "false".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you are right here, this is a tristate value, which is the stupid kind of unreadable code we sometimes create.
state.mainCharacter = true | ||
|
||
if clone then | ||
for i, currencyData in ipairs(accountCurrencyData) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transfering everything from a character doesn't correctly update the trigger, the clone for the character who know has 0 is still displayed.
I think this because this loop here runs over accountCurrencyData which no longer contains any data for that character, so the existing state is kept unmodified even though it should have been dropped.
local primaryCheckFailed = false | ||
|
||
-- Check quantity-related values | ||
for attribute, triggerValue in pairs(triggerQuantities) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be rewritten so that checking the triggerValue happens at function creation time, not every time the trigger runs.
So the looping should be outside of the string, and the string should only include a check if the triggerValue is not nil. This makes the resulting function much smaller in the usual case that there's no or one check.
checkTristate(discovered, currencyInfo.discovered, true, true) | ||
checkTristate(capped, currencyInfo.quantity >= (currencyInfo.maxQuantity or 0), currencyInfo.maxQuantity and currencyInfo.maxQuantity > 0, false) | ||
checkTristate(seasonCapped, currencyInfo.totalEarned >= (currencyInfo.maxQuantity or 0), currencyInfo.useTotalEarnedForMaxQty and currencyInfo.maxQuantity and currencyInfo.maxQuantity > 0, true) | ||
checkTristate(weeklyCapped, currencyInfo.quantityEarnedThisWeek >= (currencyInfo.maxWeeklyQuantity or 0), currencyInfo.maxWeeklyQuantity and currencyInfo.maxWeeklyQuantity > 0, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, the checking whether to include the check for discovered/capped/seasonCapped/weeklyCapped should probably be done at function creation time.
I still plan on updating this with the feedback from code review, but I'm just enjoying playing the game a bit too much right now :D |
That's not a problem. I can also take a look at some of the changes I suggested, but similarly not now.. |
Description
TWW brings warband currencies that can be transferred over to other characters within your warband.
The C_CurrencyInfo API contains currencyData for all warband characters, and so we're able to create a cloning trigger that shows currency info for each character in the warband.
This is a full rework of the currency trigger prototype as this needs to use
statesParameter = "full"
.Several state values for account wide currencies have been added:
Example