Ret FCFS Rotation Helper - Page 14

Heck · 08/13/09, 3:55 PM

Just so I can follow what's been going on for the last several posts, we've now got two conversations going on about two different FCFS rotation helper mods at the same time...?

Hard to keep this all straight.

burghy · 08/13/09, 4:07 PM

Yeah probably my bad, just thought is better to continue here since the title of the thread is quite general.

WoW AddOns - Retribution FCFS helper (clcret) - r15 - CurseForge.com
Added the command line request, you can see in the changelog the syntax.

Couldn't reproduce the death bug you mentioned and it wasn't fun testing it.

Neraya · 08/14/09, 3:50 AM

Originally Posted by burghy

Added the command line request, you can see in the changelog the syntax.

Originally Posted by burghy

Couldn't reproduce the death bug you mentioned and it wasn't fun testing it.

Hmm I can imagine... Raiding freya 25 last night (+3, still working on the 1st kill for that) I noticed it stayed up even when I had no target. I always 'noticed' it when getting back inside and seeing the frame still there. The circomstances may be a bit more involved than just dying. But I'd guess there's cases where you loose your current target whithout the PLAYER_TARGET_CHANGED event firing (?).

Dying in an instance and releasign does mean a zoning out of the instance and into the outside world... Maybe this could be the cause ? how/where did you test this yourself ?

I have both shit and clcret running now. With the change to the timing thing they suggest the same spells most of the time. There's discrepancies, and I'm homing in on the circumstances this happens. Either shit is wrong, clcret is wrong, ... or both are.

clcRet's got the better looks... But until proven otherwise I still "trust" the results of 'shit' more for the time being. I did a lot of testing with shit, and clcret's the new kid on the block afterall.

burghy · 08/14/09, 4:07 AM

Tested for like 10 min on dummies last night and couldn't find any discrepancy between it and shit with the latest build (has only first 2 skills shown). I'm running without dp and ss added to the rotation but unless SHIT does something special for them (apart from conditions if usable or not) shouldn't be a difference.

Zaloog · 08/14/09, 4:55 AM

One feature request I would love to see for either clcret or S.H.I.T. is added support for ButtonFacade.

Neraya · 08/14/09, 5:48 AM

Code checkup (007) & bugs/annoyances I noticed (in 006, haven't ran 007 yet).

1) The function MySort() is no longer being used and can be removed...

2) CheckSov()
You're testing the variable caster but this wasn't ever initialised anywhere in the rest of the code. From what I can tell this should have been the next variable declared (and initialised) a bit higher at the UnitDebuff() call.

3) Interesting to see you removed the 3nd ability in line... It's kinda annoying (and time consuming) to predic future events...

4) The range check currently is annoyingin that it checks melee range on all abilities.
I guess it depends what you really expect here... Do you expect a 'in melee range' indicator or a 'out of range for this ability'. It does the 'in melee range' now, which I find annoying if the current ability has a longer range than that.
Add an option for range checking... "Disabled", "Melee range" (and "Spell range").

5) GetBest()

xprio = 1
xcd = pq[1].xcd

I could be missing the point or the clever trick here, but... it would appear to me this should either be...
- xprio = 1 and xcd = <ArbitraryLargeValue> to make sure you'll be taking the 'if' branch to set the x-variables to the 1st priority item but then you can better do is the next way...
- xprio = pq[i].priority, xcd = pq[1].xcd, pq[1].xcd = max(0, pq[i]xcd] - 1.5) and start the for loop at 2 instead of 1.

Looking at it now. I'm getting the impression it will sometimes have xprio set to 1 where it should have it set to pq[1].priority

6) since the (2) items in the qd table are intended as copies of an item of the pq table...
wouldn't it be more optimal to have qd just be indexes into the pq table rather than actual copies ?

instead of the self:QD() call:

qd[pos] = xindex;

with appropriate changes in UpdateUi() et al.

7) CheckQueue()
v = pq[i]
you're creating a reference to the pq[i] item, but you still end up using pq[i] (as well as v) in the rest of the for loop.

8) CenterHorizontally()
db.x = (UIParent:GetWidth() / 2 - 110 * db.scale) / db.scale
that line (the 110) got me boggled for a while until I realised it's a hardcoded reference to the width of the frame (half of 220)
Hardcoded values are bad, mkay

-> db.x = ( (UIParent:GetWidth() - (self.frame:GetWidth() * db.scale) / 2 ) / db.scale ? (the self.frame:GetWidth() could be wrong though).

Neraya · 08/14/09, 6:26 AM

9) InitUI()

	local texture = frame:CreateTexture(nil, "BACKGROUND")
	texture:SetAllPoints()
	texture:SetTexture("Interface\\AddOns\\clcret\\textures\\minimalist")
	texture:SetVertexColor(0, 0, 0, 1)
	texture:Hide()
	frame.texture = texture

Is this needed for anything ? I can't recall seeing that minimalist bar in my last nights raid and clcret test...

which leads me to...
local clcret = LibStub("AceAddon-3.0"):NewAddon("clcret", "AceEvent-3.0", "AceConsole-3.0", "AceTimer-3.0")
Are all 3 of those libs needed ? I can't seem to see any usage of them (?)

burghy · 08/14/09, 7:30 AM

1 - true, but I'm still changing a lot of stuff so I postponed code cleanup
2 - is fixed in one of the later alpha builds
4 - first time I implemented it was done per ability but I changed it because without cs displayed you will not have any information regarding your melee range, could add a toggle sure
5 - the code is good, just a lot of unnecesary from old builds, for example priority field is unnecesary since pq is sorted according to priority
6 - pq gets changed so I didn't want to link to it, maybe later after I'm sure important info isn't affected
7 - yeah cleanup is needed
8 - true
9 - is the background shown when you can manualy move the frame
10* - aceevent is needed for talent change, target change, combat status, aceconsole for console commands, ace timer for the delayed init

Exemplar · 08/14/09, 8:33 AM

I tested this briefly the other evening. Seems pretty solid in performing its assigned task.

The frames not disappearing appears to occur if your target despawns or you move out of range without changing target. I was beating on test dummies, then hearthed away, it was still open until I selected a new non-combat target (at which point it vanished). I suspect his "on death" issue was the target despawning before release (bosses/adds as opposed to world mobs, hence your inability to replicate).

Divine Plea and Avenging Wrath on the left never showed on cooldown in clcret once used. I'd appreciate an option to not display these (if I use this in the future I'd just like the current use and next ability, the DP and AW just use screen real estate as I manage them manually elsewhere).

Not necessarily germane and potentially beyond the scope of the addon, but any plans to add a digital countdown to remaining cooldown? I use a mod to add numeric cooldown remaining to my buttons (I find the swirl imprecise and like to know to the tenth of a second what's going on), but of course this mod doesn't appear on clcret (as they're not, and cannot be, actual buttons).

burghy · 08/14/09, 8:43 AM

I'll look into it but you should give OmniCC a try. I like the numbers too but I didn't pay much attention to it because OmniCC works on them.

Update (v8):
- replaced DP and AW buttons with 10 configurable ones
- more layout options (position is relative to the black box behind main skill when you unlock the frame)

Neraya · 08/14/09, 8:32 PM

With 006, dual swapping to holy spec from ret, inside TotC normal (no idea if it matters)

Date: 2009-08-15 02:28:51
ID: 1
Error occured in: Global
Count: 1
Message: ..\AddOns\clcret\core.lua line 498:
attempt to perform arithmetic on field 'cdStart' (a nil value)
Debug:
[C]: ?
clcret\core.lua:498: CheckQueue()
clcret\core.lua:384:
clcret\core.lua:380

• frmorrison · 08/14/09, 8:37 PM

Originally Posted by Neraya

With 006

The current version on Curse is 008, may try that out?

burghy · 08/15/09, 5:25 AM

Hopefully that talent change bug is fixed in 009.

cremor · 08/15/09, 8:51 AM

In addition to the problem that the frame stays visible sometimes when it shouldn't (happend for me when I left the instance while I had an enemy in target) I noticed the following bugs with the "valid target" setting:
* The frame shows if the target is dead (when looting).
* The frame doesn't show if the target changes from friendly to enemy (e.g. in Trial of the Champion).

burghy · 08/15/09, 10:44 AM

Tested with last version (10 release) and seemed fine in ToC. Also changed so it disables the frame in vehicles.