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.

Sign up for GitHub

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

Jump to bottom

Add item setting for bundle occupancy #1181

Open
wants to merge 12 commits into
base: 1.17
Choose a base branch
from
Open

Add item setting for bundle occupancy #1181

wants to merge 12 commits into from

Conversation

BoogieMonster1O1
Copy link

@BoogieMonster1O1 BoogieMonster1O1 commented Nov 25, 2020 •
edited
Loading

Adds an item setting for bundle occupancy to FabricItemSettings.

Fixes #1167

@i509VCB
Copy link
Contributor

i509VCB commented Nov 25, 2020

Well it looks like you beat me to opening the PR. Mine is here for reference on docs if you want some (https://github.com/i509VCB/fabric/tree/ft/bundle-item-weight)

There are a few things you need to probably suffice for, such as using a specific interface over a ToIntFunction and some javadoc explaining how the bundle weight is calculated to some extent

@i509VCB i509VCB added enhancement New feature or request in progress This issue has PR(s) open to resolve it (or a PR that is WIP) labels Nov 25, 2020
@BoogieMonster1O1 BoogieMonster1O1 marked this pull request as ready for review November 25, 2020 07:22
@i509VCB i509VCB added reviews needed This PR needs more reviews and removed in progress This issue has PR(s) open to resolve it (or a PR that is WIP) labels Nov 25, 2020
@BoogieMonster1O1
Copy link
Author

Well it looks like you beat me to opening the PR. Mine is here for reference on docs if you want some (https://github.com/i509VCB/fabric/tree/ft/bundle-item-weight)

There are a few things you need to probably suffice for, such as using a specific interface over a ToIntFunction and some javadoc explaining how the bundle weight is calculated to some extent

Thanks! I've added your docs

i509VCB
i509VCB reviewed Nov 25, 2020
View reviewed changes
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/api/item/v1/BundleOccupancyProvider.java
* @param stack the item stack
* @return the occupancy size of the item stack.
*/
int getBundleOccupancy(ItemStack stack);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't recall off head, does the bundle logic here assume the item stack is 1 and then later the count is multiplied onto the value? Could you check on that?

Copy link
Author

Choose a reason for hiding this comment

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

the bundle occupancy returned here isn't multiplied with the stack size

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm must have remembered something different then

@Mysterious-Dev
Copy link

So is there any change?

@BoogieMonster1O1
Copy link
Author

So is there any change?

none for now

@i509VCB i509VCB added the priority:low Low priority PRs that don't immediately need to be resolved label Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@i509VCB i509VCB i509VCB left review comments

@oliviathevampire oliviathevampire oliviathevampire approved these changes

@Mysterious-Dev Mysterious-Dev Mysterious-Dev approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request priority:low Low priority PRs that don't immediately need to be resolved reviews needed This PR needs more reviews
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BoogieMonster1O1 @i509VCB @Mysterious-Dev @oliviathevampire