-
Notifications
You must be signed in to change notification settings - Fork 20
Equipment: add equip functions #259
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
base: master
Are you sure you want to change the base?
Conversation
z16
left a comment
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.
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
| 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 .. ")") |
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.
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).
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.
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.
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 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
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.
Yeah, I like this plan.
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.
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.
eada87b to
f65fa35
Compare
952649f to
f51d3c9
Compare
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