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

Equipment: add equip functions #259

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

svanheulen
Copy link
Contributor

This version doesn't do any extra checking.
I have a version here that only sends packets like the official client would: https://github.com/svanheulen/packages/blob/equip-functions-extra-checks/equipment/equipment.lua

Copy link
Member

@z16 z16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some notes, one more thing I'd like to discuss is what I mentioned the other day about error checking. I'm not as comfortable with my opinion as I used to be, and I know other people on the team feel differently. I'll discuss it with them some more to decide on what to do, then I'll get back to you.

equipment/equipment.lua Outdated Show resolved Hide resolved
equipment/equipment.lua Show resolved Hide resolved
equipment/manifest.xml Outdated Show resolved Hide resolved
local bag = item and item.bag or 0
local index = item and item.index or 0
assert(resources.bags[bag].equippable, "Can not equip from this bag (bag = " .. bag .. ")")
assert(not item or item.id ~= 0, "Can not equip from an empty bag slot (bag = " .. bag .. ", index = " .. index .. ")")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not item can be removed, it will always be false, unless item is actually the boolean value false. And if that's the case, it will error further down. You should be able to use ffi to check the type and see if it was an actual item type passed in. Then you can remove lines 37 and 38 as well, since an item_type will always have .bag and .index set. You'd only need to check that the item.id is not zero and you could add a check that the item is actually equippable in that slot (can be done via resources).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually pass false instead of an item_type object. I did this to match the functionality of /equipset in the game where each slot can be equipped, unequipped or not touched. So if you pass false for an equipment slot it will unequip that slot. And that's why lines 37 and 38 are needed.

I can do it a different way if you prefer, but I feel like it's pretty important to have that ability in some way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point that I had not considered. Though I would check item == false in that case to make it more obvious.

Thinking about it though, we do have an empty item type, which already has those fields set to 0. We could let users use that instead of false, I feel that would make for a neater API:

equipment:equip({
    [0] = items.bags[0][12],
    [1] = items.bags[0][37],
    [9] = items.empty,
    [10] = items.bags[8][44],
})

I'm liking this idea, but the items_service does not expose its empty item yet. This could be changed very easily though. How do you feel about this? The entire checking code (and the function's code in general) would be reduced very heavily. Could also remove the slot index checks by changing the loop itself to for i = 0, 15 do. It would also be faster and process the set in order, though that likely won't matter much, but if you get determinism for free, I'd take it :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like this plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made those changes but I still need to differentiate between an empty bag slot and items.empty (also I think items.none might be a better name?) so it doesn't really reduce the code that much. But I still like it better.

equipment/equipment.lua Outdated Show resolved Hide resolved
equipment/equipment.lua Outdated Show resolved Hide resolved
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

2 participants