Several fixes / changes to the holder item action
#132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MerchantPug
suggested changes
Jul 25, 2023
There was a problem hiding this comment.
I don't quite see the point in keeping InventoryUtil#deduplicateSlots when we aren't using it anymore and when it's private.
If it is still used, please tell me and I'll approve. I can't quite check properly on my phone.
I feel like we could potentially implement these optimisations to the other InventoryUtil methods too.
MerchantPug
approved these changes
Jul 25, 2023
MerchantPug
added a commit
to MerchantPug/apoli
that referenced
this pull request
Jul 25, 2023
eggohito
approved these changes
Jul 26, 2023
Agreed on having a path alias. |
# Conflicts: # src/main/java/io/github/apace100/apoli/mixin/LivingEntityMixin.java # src/main/java/io/github/apace100/apoli/util/InventoryUtil.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes several changes regarding the holder item action:
InventoryUtil#forEachStackmethod to not allocate a new set each callsetHolderto our duck interface'sEntityLinkedItemStack#setEntity. This is better because it does not change vanilla semantics of the holder field.holdertoholder_actionand theentity_actionfield toaction. This is to make it in line with other actions, likeequipped_item_action,passenger_action, etc. I feel this is okay since the action hasn't been released for too long and is probably still barely used. If you think this is too late for that change, we could add an alias using the old names for backwards compatibility and make an announcement asking people to change over to the new names quickly.Thanks Pug for pointing out the optimization potential with the method.