-
Notifications
You must be signed in to change notification settings - Fork 417
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
Add support for Item-containing Items #4083
Conversation
5f3d7c8
to
2889334
Compare
The PR this depends on has been merged. |
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.
A few comments, but it looks good overall. There are no implementations that are usable by other mods, but that can come in a different PR if someone is feeling motivated. 😉
fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/item/ItemStorage.java
Show resolved
Hide resolved
...nsfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/BundleContentsStorage.java
Show resolved
Hide resolved
...nsfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/BundleContentsStorage.java
Outdated
Show resolved
Hide resolved
...nsfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/BundleContentsStorage.java
Show resolved
Hide resolved
...nsfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/BundleContentsStorage.java
Show resolved
Hide resolved
...r-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/ContainerComponentStorage.java
Outdated
Show resolved
Hide resolved
...r-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/ContainerComponentStorage.java
Show resolved
Hide resolved
...r-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/ContainerComponentStorage.java
Outdated
Show resolved
Hide resolved
...pi-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/ingame/TransferTestInitializer.java
Show resolved
Hide resolved
...pi-v1/src/testmod/java/net/fabricmc/fabric/test/transfer/ingame/TransferTestInitializer.java
Show resolved
Hide resolved
…transfer/item/ContainerComponentStorage.java Co-authored-by: Technici4n <13494793+Technici4n@users.noreply.github.com>
2f04b44
to
49c8d1e
Compare
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 great! Thank you.
fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/api/transfer/v1/item/ItemStorage.java
Outdated
Show resolved
Hide resolved
...nsfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/BundleContentsStorage.java
Outdated
Show resolved
Hide resolved
...nsfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/BundleContentsStorage.java
Outdated
Show resolved
Hide resolved
...nsfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/BundleContentsStorage.java
Outdated
Show resolved
Hide resolved
Well, the ContainerComponentStorage impl could probably be opened up for mods to use, considering it's suitably generic. I made it internal for now because I didn't want to have to figure out how to properly expose it. |
Yes it's fine to keep it internal like that. At this point just having |
* first steps toward container item support * add bundle support * Update fabric-transfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/item/ContainerComponentStorage.java Co-authored-by: Technici4n <13494793+Technici4n@users.noreply.github.com> * address reviews + TIL ContainerItemContext#find exists * address reviews of bundle code --------- Co-authored-by: Technici4n <13494793+Technici4n@users.noreply.github.com> (cherry picked from commit 4054068)
Currently draft, sinceonly Shulker Boxes are supportedthe PR this depends on hasn't been merged.Based on #4082, until that is merged changes from that PR will show up in the diff and commit log.Actual diff