-
Notifications
You must be signed in to change notification settings - Fork 7
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
ItemStack refactor #12
Conversation
I think it's a good idea to provide "native" support for an ItemBuilder utility. |
That's a good idea. I'll try to implement one once the |
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.
Respect stack size, other than that it looks pretty good.
@Defman I will add the stack limitations now. |
…te required trait impl and derives.
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, though I'm not sure how we should handle operations that can lead to ItemStacks having self.count = 0
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.
Great work, thanks! I've left some comments.
crates/items/src/item_stack.rs
Outdated
pub fn take(&mut self, amount: u32) -> Result<ItemStack, ItemStackError> { | ||
if self.count <= amount { | ||
return Err(if self.count == amount { | ||
ItemStackError::EmptyStack |
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.
We do need to provide a good solution for taking the whole stack—this is useful for inventory handling in Feather, for example. I know there are talks of refactoring ItemStack
to properly account for empty stacks, so we should revisit this point.
…, make fields private, clarify docs.
Hi @PauMAVA! We decided on Discord to merge If this PR is nearly ready, we can merge it now and finish the remaining work in a separate PR. Alternatively, we can move the PR to Feather. |
Hey! There are still lots of things to implement (mainly Item Stack Metadata). I think we could merge this and open a new PR on the feather repo and continue the implementation there. What do you think about it? I can open a new Draft PR there if you want. |
Sounds good to me. I'll merge the PR as is, and we can finish up the work in the Feather repo. |
Let me know when the repo has been moved to |
This draft PR is related to #6. We can discuss the implementation of the new ItemStack type here.