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

Fill existing stacks first when adding to player inventory. #3025

Merged
merged 2 commits into from Aug 14, 2017

Conversation

Projects
None yet
2 participants
@fscan
Member

fscan commented Aug 11, 2017

I also removed the .copy() in AddItems because i think it's not needed. IItemHandler insertItem only returns the same stack if it hasn't changed (disregarding buggy implementations). Am i missing something here?

Obviously needs a lot of testing.

@yueh

This comment has been minimized.

Show comment
Hide comment
@yueh

yueh Aug 12, 2017

Member

Buggy implementations are sadly quite common. But there is also the chance that we do not know the source of an itemstack, so they might expect that we do not modify the itemstack similar the an itemhandler, which must not changed to passed one at all.

Also copying an itemstack is not that bad in controlled cases. It only gets out of hand when moving a couple of 10k item/tick around. And it usually needs very good optimitized inventories to even allow it.

Therefore I would err on the side of caution and keep it.

Member

yueh commented Aug 12, 2017

Buggy implementations are sadly quite common. But there is also the chance that we do not know the source of an itemstack, so they might expect that we do not modify the itemstack similar the an itemhandler, which must not changed to passed one at all.

Also copying an itemstack is not that bad in controlled cases. It only gets out of hand when moving a couple of 10k item/tick around. And it usually needs very good optimitized inventories to even allow it.

Therefore I would err on the side of caution and keep it.

fscan added some commits Aug 11, 2017

Fill existing stacks first when adding to player inventory.
TEST: remove copy and stack content check on AddItems, should not be
needed with IITemHandler

@yueh yueh added this to the rv5.alpha - 1.12 milestone Aug 14, 2017

@yueh yueh merged commit 9022150 into AppliedEnergistics:rv5-1.12 Aug 14, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins Success
Details
sonarqube SonarQube reported no issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment