New addon: Debuffed #1630
New addon: Debuffed #1630
Conversation
There was a problem hiding this comment.
Overall it looks pretty good, you just need to fix the copyright notice. Aside from that all the things I commented on are minor suggestions, although I would like to see the debuff file committed to the RE repository. But even if you decide to do that, it wouldn't have to be now, can do it at any other point in time.
So once you fix the copyright notice let me know if you want me to merge it as is, or if you want to implement any of the other suggestions.
addons/Debuffed/Debuffed.lua
Outdated
| @@ -0,0 +1,250 @@ | |||
| --[[ | |||
| Copyright (c) 2018, Auk | |||
There was a problem hiding this comment.
Please use the proper copyright sign ©.
addons/Debuffed/Debuffed.lua
Outdated
| * Redistributions in binary form must reproduce the above copyright | ||
| notice, this list of conditions and the following disclaimer in the | ||
| documentation and/or other materials provided with the distribution. | ||
| * Neither the name of <addon name> nor the |
addons/Debuffed/Debuffed.lua
Outdated
| THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND | ||
| ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED | ||
| WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE | ||
| DISCLAIMED. IN NO EVENT SHALL <your name> BE LIABLE FOR ANY |
addons/Debuffed/debuffs.lua
Outdated
| @@ -0,0 +1,54 @@ | |||
| return { | |||
There was a problem hiding this comment.
You know, I think this would be a useful addition to Windower resources. Feel free to add this to the ResourceExtractor repository (in the fixes.xml file). That way you could access that then.
I would also leave out the name field (both if you go the ResourceExtractor route or if you leave it here). You can use the ID to access the name from the buff resources, and then you can use the .name property to localize it based on client language (so it shows the japanese name on the japanese client). You can still use the english name in a comment at the end of the line for easier maintenance (both here and in the fixes.xml file).
addons/Debuffed/Debuffed.lua
Outdated
| end | ||
| end | ||
| end | ||
| box.current_string = count > 0 and current_string or '' |
There was a problem hiding this comment.
This is minor suggestion, but repeated expanding string concatenation can be "very" expensive (for certain definitions of "very"). I would probably use a list to append lines to and later join them all by \n.
Has the even minorer additional benefit, that you won't need the count variable - at the end of the function you could just check if the table is empty. If so - return. If not, update the box.current_string with Debuffed [' .. target.name ..']\n' .. list:concat('\n'), where lines is the table used to hold all the lines.
That makes it a tad more efficient, but I doubt it would make much of a difference unless it's on the lowest of low boxes.
addons/Debuffed/Debuffed.lua
Outdated
| end | ||
|
|
||
| function handle_overwrites(target, new, t) | ||
| if debuffed_mobs[target] then |
There was a problem hiding this comment.
Can invert the condition to reduce nesting:
if not debuffed_mobs[target] then
return true
end
addons/Debuffed/Debuffed.lua
Outdated
| end | ||
|
|
||
| function inc_action(act) | ||
| if act.category == 4 then |
There was a problem hiding this comment.
Same as above, can invert the condition:
if act.category ~= 4 then
return
end
addons/Debuffed/Debuffed.lua
Outdated
| print(' \\cs(255,255,255)interval <value>\\cr - Allows you to change the refresh interval (default: 0.1)') | ||
| print(' \\cs(255,255,255)blacklist|whitelist add|remove <name>\\cr - Adds or removes the spell <name> to the specified list') | ||
| end | ||
| end) |
There was a problem hiding this comment.
No new line at the end of the file. This is critical.
|
Implemented all of those suggestions. I'll definitely look into the resources but I still need to add a few other spells like songs yet. |
|
Switched to using resources now and I've made a pull request with an updated fixes.xml. Windower/ResourceExtractor@39b324a |
|
Just so you know, I want to refactor RE first to handle tables within the fixes file and interpret it as an array. As it is now we end up with the string |
|
No worries, I did take a look at RE myself but it's a bit out of my expertise. Should be a lot cleaner than the hacky workaround I used for the string issue. |
|
Sorry this took so long, we went over a few things and resources were adjusted again. Now the overwrites should be an actual table instead of a string containing a table definition. Also adjusted some duration values, but not many. Some seem to be off still, but it should be usable. Let me know when you've upgraded the addon to handle the new resources. |
|
Fixed for the new resources now but I also added a check for Light Shot to upgrade Dia. I've made two more pull requests to add the spell attributes for Dia IV so overwrites work properly for it. Windower/ResourceExtractor#54 I'll look into fixing the durations if I ever get chance, haven't been actively playing recently. |
No description provided.