-
-
Notifications
You must be signed in to change notification settings - Fork 318
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
groups 2: dynamic boogaloo #3094
base: main
Are you sure you want to change the base?
Conversation
order = order, | ||
} | ||
order = order + 1 | ||
local icon, title, messages = OptionsPrivate.Private.AuraWarnings.FormatWarnings(data.uid) |
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.
You want a not isTmpGroup check here and change the old comment to something like:
-- Warnings
-- One Aura/Group: Show warning of aura/group
-- Multi-selection: Nothing
The story behind that is, that for the information tab almost every setting behaves in a non-standard way. E.g. url is set on everything selected, whereas description is only edited on one aura. That doesn't fit the old common options code, and I didn't want to add even more baggage to it.
So the information code is very bare-bones and does the merging itself with some duplicated code. The idea being that some day in the future I'll rewrite it to something better and then port the other pages to a new common infrastructure...
Here's a useful toy example.
|
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 read through all the changes once, there quite a few repeated comments, which I repeated just to mark all points where a similar change would be good.
There are a few areas which I want to study in more detail, hopefully in the next few days.
The general idea of introducing distribute functions, which are run per regionData, and thus make updates localized to a group is pretty good. Although the combination of distributor + anchorer is currently not as powerful as the previous version.
nameplate = function(self, anchorLocation) return WeakAuras.GetUnitNameplate(anchorLocation) end, | ||
unit = function(self, anchorLocation) return WeakAuras.GetUnitFrame(anchorLocation) end, | ||
frame = function(self, anchorLocation) return Private.GetSanitizedGlobal(anchorLocation) end, | ||
aura = function(self, anchorLocation) return WeakAuras.GetRegion(anchorLocation) end, --? do we actually want this? |
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.
Hmm, my first guess would be no.
local ok, result = xpcall(distributeFunc, geterrorhandler(), regionData) | ||
Private.ActivateAuraEnvironment() | ||
if ok then | ||
return result |
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.
As far as I can see this means that a custom distributor has to use a standard anchorer for the anchorLocation -> frame mapping. And due to how the "frame" anchorer works, that makes it impossible to anchor to frames that don't have global names?
Do we need a different anchorer or a custom anchorer for that.
The old anchorers returned actual frame, instead of a frame name.
E.g. LibGetFrame's GetUnit actually has a second parameter, a options table. Currently it looks impossible to use that.
(Or maybe I haven't dug deep enough into how everything fits together.)
The changes to dynamic groups from 30365b3 went missing. |
since we'll be doing incompatible changes
also begin removing perUnit stuff on "new" version
… handles this well
84a96c6
to
9bb9b39
Compare
local anchorers = { | ||
self = function(self, anchorLocation) return self end, | ||
nameplate = function(self, anchorLocation) return WeakAuras.GetUnitNameplate(anchorLocation) end, | ||
unit = function(self, anchorLocation) return WeakAuras.GetUnitFrame(anchorLocation) end, |
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 is unfourtunately a bit more involved than this.
Since some unit frames addons (Vuhdo!) have a delay until they create a frame matching a unit, the LibGetFrame library has a callback interface where we are informed if a unit -> frame mapping changed.
That's the code in the old dynamic group involving dyngroup_unitframe_monitor, which does need some massaging to make work. The existing code probably needs some changes to support you too. @mrbuds wrote that part of the code (and LibGetFrame).
Edit: That delay is also the point of the "WeakAuras.HiddenFrames" fallback in the old Unit frame anchorer.
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.
LibGetFrame fire 3 callbacks https://github.com/mrbuds/LibGetFrame/blob/master/LibGetFrame-1.0.lua#L134
GETFRAME_REFRESH
after each scan of all children of UIParent
FRAME_UNIT_UPDATE => frame, unit
when a frame
is associated with a new unit
FRAME_UNIT_REMOVED => frame, unit
when unit
is no longer associated with frame
IM BACK BABY!!!!!111
...and i have some new changes to dynamic groups to make them better. This is a preview of some new features, please feel free to checkout this branch (or install the build artifact) to try it out!
This is a mild rewrite of the Dynamic Group system, to improve perfomance in some areas, and flexibility/expressiveness in other areas. Specifically:
where anchorType is one of
"self", "nameplate", "unit"
, anchorLocation (for nameplate/unit anchor types) defines which unit frame/nameplate is desired, and anchorId is any string. The anchorType, anchorLocation, and anchorId together uniquely define an anchor, and every child of the group is distributed onto exactly one anchor. Each anchor is then sorted as per whatever options are chosen in the UI, just like normal. Then, once sorted, each anchor is fed into the grow function individually. The practical upshot is that the dynamic group will be split into multiple mini groups, each anchored to a different place.The benefits of this system are twofold: First, we can simplify layout functions somewhat, since the grow function is no longer responsible for choosing the correct frame to anchor to. Second, we can improve performance in some common use cases, such as putting buff icons on unit frames, since we no longer need to iterate over the majority of minigroups anytime any of the other minigroups is updated.
What about my existing dynamic groups??
In this revision, the new system exists specifically on a new region type, "dynamicgroup2". Existing dynamic groups are entirely unaffected.
Type of change
This PR is unfinished: Here's what I know needs to be done