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 support for items in shortcuts addon. #2076
base: dev
Are you sure you want to change the base?
Conversation
@@ -198,9 +198,10 @@ end | |||
-- Iterate through resources and make validabils. | |||
function validabils_it(resource) | |||
for id,v in pairs(res[resource]) do | |||
if (not v.monster_level and v.prefix) or (v.monster_level and v.monster_level ~= -1 and v.ja:sub(1,1) ~= '#' ) then | |||
if (not v.monster_level and v.prefix) or (v.monster_level and v.monster_level ~= -1 and v.ja:sub(1,1) ~= '#' ) or (v.category == 'Usable') 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 limits the items added to ones in the Usable
category, which seem to be items like medicines, food, and clusters, but doesn't include usable gear, like Warp Ring
. This seems fine to me, but highlighting this change in case someone else has a different opinion.
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 seems okay to me. If someone wants to revisit this decision, they can do it in another PR.
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 don't see a reason why it couldn't shouldn't check equipped items and make those usable via command.
or, just, checking the flags instead of the category to see if its "use"-able
addons/shortcuts/statics.lua
Outdated
if settings.include_items then | ||
validabils_it('items') | ||
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.
Guards against supporting items. Is false by default.
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.
Looks good. Just a few questions.
-- Monster Abilities contains a large number of player-usable moves (but not monstrosity-usable). This excludes them. | ||
make_abil(strip(v.english),resource,id) | ||
local name = resource ~= 'items' and strip_non_alphanumeric_convert_digits_to_roman(v.english) or strip_non_alphanumeric_keep_plus(v.english) |
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 there name collisions if you don't keep the plus and avoid converting digits to roman numerals?
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 didn't try, but I felt /rolandaifuku+1
felt more natural than /rolandaifuku1
.
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.
That's fine, but you can use /rolandaifuku+1
either way.
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.
My main concern here is that this adds complexity (two slug paths) without adding any obvious benefit.
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.
No. If we only used the original, it would be simply /rolandaifuku for both the normal and +1 versions: the plus gets stripped and the 1 gets omitted when converting to Roman numerals.
@@ -198,9 +198,10 @@ end | |||
-- Iterate through resources and make validabils. | |||
function validabils_it(resource) | |||
for id,v in pairs(res[resource]) do | |||
if (not v.monster_level and v.prefix) or (v.monster_level and v.monster_level ~= -1 and v.ja:sub(1,1) ~= '#' ) then | |||
if (not v.monster_level and v.prefix) or (v.monster_level and v.monster_level ~= -1 and v.ja:sub(1,1) ~= '#' ) or (v.category == 'Usable') 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 seems okay to me. If someone wants to revisit this decision, they can do it in another PR.
|
||
aliases = config.load('data\\aliases.xml',default_aliases) | ||
aliases = config.load('data/aliases.xml', default_aliases) | ||
settings = config.load('data/settings.xml', default_settings) |
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.
Adding another settings file just to allow people to disable compatibility with items seems like a lot of work for no obvious benefit. I would just make it an always-on feature. No one is going to accidentally type //proether+3
or //vileelixir+1
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 did this because of vague concerns that this would cause problems. https://discord.com/channels/338590234235371531/501099842097905675/883160617706483712
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 adds complexity without benefit, so I would eliminate it. Make items always-on and don't add a settings file.
Adds support for items so that
/<item name>
causes that item to be used -- i.e. translates to/item "<item name>" <me>
.Test cases:
/remedy
/rolandaifuku
/rolandaifuku+1