Skip to content
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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Imisnew2
Copy link
Contributor

@Imisnew2 Imisnew2 commented Sep 2, 2021

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

@@ -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
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Collaborator

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

if settings.include_items then
validabils_it('items')
end
Copy link
Contributor Author

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.

Copy link
Contributor

@Byrth Byrth left a 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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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)
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants