Skip to content

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

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

@RubenatorX RubenatorX Dec 23, 2021

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

validabils_it('mounts')
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.

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
-- 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.

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.

3 participants