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

[1.2.0] New inventory system #46

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

[1.2.0] New inventory system #46

wants to merge 31 commits into from

Conversation

TheBusyBiscuit
Copy link
Member

Resolves #29

Rewriting the inventory system.

@TheBusyBiscuit TheBusyBiscuit marked this pull request as ready for review July 14, 2021 08:59
@TheBusyBiscuit TheBusyBiscuit changed the title Initial work on the new inventory system New inventory system Jul 14, 2021
@TheBusyBiscuit TheBusyBiscuit changed the title New inventory system [1.1.0] New inventory system Jul 14, 2021
@sonarcloud
Copy link

sonarcloud bot commented Jul 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

95.5% 95.5% Coverage
0.0% 0.0% Duplication

item.setAmount(amount - maxStackSize);
itemInSlot.setAmount(maxStackSize);
} else {
itemInSlot.setAmount(Math.min(amount, maxStackSize));
Copy link
Member

Choose a reason for hiding this comment

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

You can forget about the Math#min here for a slight gain in speed and just set to "amount": you have already checked that "amount" is at most maxStackSize so worst case scenario is the two are equal and Math.min will set exactly "amount" anyway, otherwise "amount" is the lower so it would again be the chosen one

if (holder instanceof Menu) {
Menu inv = (Menu) holder;

try {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this fail to stop shift-clicking into filled slots? Let's say I have an "apples" group which is not interactable and filled with itemtype APPLE and I shift click a stack of apples in my inventories, the if clause is false so the event is never checked and apples get stacked into the first "apples" slots in the menu

Copy link
Member

Choose a reason for hiding this comment

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

Might be a good idea to add something of the sort to TestMenuClicking too, just in case

Copy link
Contributor

@md5sha256 md5sha256 left a comment

Choose a reason for hiding this comment

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

Just these two and what Sfiguz has brought up, otherwise LGTM.

@md5sha256
Copy link
Contributor

LGTM bar the comments left by Sfiguz. I'll approve once those are resolved I suppose

@TheBusyBiscuit TheBusyBiscuit changed the title [1.1.0] New inventory system [1.2.0] New inventory system Jan 1, 2022
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.

Improve upon dough-inventories and perhaps rewrite that completely
3 participants