-
Notifications
You must be signed in to change notification settings - Fork 432
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 2nd cmd param to specify dim item. #2070
base: dev
Are you sure you want to change the base?
Conversation
Added damaging drain msg to mydmg coloring.
[Battlemod] Added damaging drain msg to dmg coloring
ID for voracious trunk was incorrect changed from 576 to 579
Added 3 missing traits
Correct error in id for voracious trunk
Burden tracker needed updating and corrections: * Use non-0 manuever action param to infer burden. * Correct activate and zone burden. * improved encapsulation and abstration of burden tracker. * improved automaton ability timer handling.
Regulator was missing from orignal table.
Add a missing local to a variable. Add comment for timer creation/deletion
This always returns nil
Arcon said so
register login event to update `player_id`
bluguide add missing traits
improve code that gets `pet_index`
use `get_mob_by_index` rather than `get_mob_by_id`
Autocontrol v2.0
playermob was moved inside of an statement when it should be before that statement.
Fix issues when player/playermob is not avalible.
Check if `attachments` table is `nil`
* correct `attachements` to `attachments` * add `local` to `attachments` definition
* burden.lua fix typo * autocontrol bump revision * update readme
`player_id` was nil if addon was loaded after login event.
Autocontrol: Add load event to `autoabils.lua`
* hide burden display when job is not pup. * remove debug `add_to_chat` statement
…ker-hide autocontrol hide burden display
Initiate menu interaction with Moogles before using Mog House bags.
Further improved Nomad/Pilgrim Moogle check
Update roe.lua
Typo fix `clear_moogle` to `clear_moogles`
Added a comment instructing users not to add Pilgrim Moogles manually, as it won't work.
Version bump and Pilgrim Moogle removal.
[Battlemod] Several updates and fixes.
Update craft.lua
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.
The intention of the addon is fine. There are some stylistic changes that are necessary (we use four space indentation everywhere, and preferably spaces after commas, although this was here even before your edit), and I'm not sure why, but the Stubborn
addon got caught in this PR as well, despite only having whitespace-changes. Would be cool if you could remove it from the PR, just so the change history doesn't get messed up.
The refactoring suggestion is just that, a suggestion. Your code is not wrong, and if you prefer it that way let me know, I'll merge it after the other things have been adjusted.
addons/Dimmer/Dimmer.lua
Outdated
-- Narrow list of teleport options | ||
local item_options = {} | ||
if name then | ||
for index,stats in pairs(item_info) 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.
Use four space indentation everywhere.
addons/Dimmer/Dimmer.lua
Outdated
if name then | ||
for index,stats in pairs(item_info) do | ||
if stats.short_name == name then | ||
table.append(item_options, stats) |
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 can be simplified quite a bit. This entire code block could be condensed into one line if you require('tables')
at the top and prepend the item_info
table with a T
like this:
item_info = T{
...
}
Then you can write this:
local item_options = name
and item_info:filter(function(item) return item.short_name == name end)
or item_info
However, this entire thing asks to be refactored imo. I would probably move the loop contents into its own function process_item
or whatever, then do this:
if name then
local _, stats = item_info:find(function(item) return item.short_name == name end)
process_item(stats)
else
for _, stats in pairs(item_options) do
process_item(stats)
end
end
This could be simplified further if you removed the short_name
property, and instead just keyed the table by the command:
item_info = {
holla={id=26176,japanese='D.ホラリング',english='"Dim. Ring (Holla)"',slot=13},
dem ={id=26177,japanese='D.デムリング',english='"Dim. Ring (Dem)"',slot=13},
mea ={id=26178,japanese='D.メアリング',english='"Dim. Ring (Mea)"',slot=13},
mask ={id=10385,japanese="キュムラスマスク+1",english="Cumulus Masque +1",slot=4},
}
This can be done because the index is not used anywhere. Now you can simplify it further to:
if name then
process_item(item_info[name])
else
for _, stats in pairs(item_options) do
process_item(stats)
end
end
addons/Dimmer/Dimmer.lua
Outdated
@@ -104,7 +119,7 @@ windower.register_event('addon command',function(...) | |||
windower.chat.input('//dimmer') |
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 know this was here before your edit, but I'm not liking how it inputs the command again instead of just doing search_item()
. Would be cool if you could change that.
I was trying to make as few changes as possible to not step on anyone's toes (changing the structure of the table, importing other libraries). But at your suggestion, I'll go ahead and make those updates. |
Don't worry about that, we're always trying to improve older code of ours. Both to ease future maintenance as well as to increase performance for users. |
General cleanup.
dcce6b2
to
b822639
Compare
I made the changes requested. Most of the changes were made as you suggested. However, as for using short_name for the item_info table's indices, I first did that as you suggested, but later decided to use the item ID instead. This allowed me to do efficient lookups to add data later. Previously, two tables were maintained in order to pull data, which was confusing to me and also caused scope to be annoying after moving the loop contents to a function. I combined these into one table. |
bump |
I normally carry all 3 dimensional rings with me. I understand that the addon was originally written with the idea in mind that you are using dim rings to get to Reisenjima or Al'Taeiu, and it works great for that purpose. However, I have sometimes desired that I be able to use them simply to get to specific crags, such as while farming void NMs for empyrean trials. This change allows us to specify a second parameter to accept the type of dim item such as
//dim mea
while leaving the original//dim
functionality intact.I included a small version bump in this change. Not sure if that is standard practice or not; this is my first pull request.