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

Stack aware getFoodComponent #3295

Closed
wants to merge 23 commits into from
Closed

Stack aware getFoodComponent #3295

wants to merge 23 commits into from

Conversation

Phoupraw
Copy link
Contributor

@Phoupraw Phoupraw commented Sep 1, 2023

Similar to getRecipeRemainder.

apple502j
apple502j suggested changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@apple502j apple502j left a comment

Choose a reason for hiding this comment

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

Make sure Checkstyle passes before sending a PR...

Also, there are a couple copypastes you might want to fix.

@modmuss50 modmuss50 requested review from modmuss50 and a team September 11, 2023 22:20
modmuss50
modmuss50 requested changes Sep 20, 2023
View reviewed changes
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Please add a test to the testmod.

@Phoupraw
Copy link
Contributor Author

Please add a test to the testmod.

Added. The test works fine.

modmuss50
modmuss50 requested changes Oct 20, 2023
View reviewed changes
fabric-item-api-v1/src/testmod/java/net/fabricmc/fabric/test/item/gametest/FoodGameTest.java Show resolved Hide resolved
fabric-item-api-v1/src/testmod/java/net/fabricmc/fabric/test/item/gametest/FoodGameTest.java Show resolved Hide resolved
fabric-item-api-v1/src/testmod/java/net/fabricmc/fabric/test/item/CustomDamageTest.java Outdated Show resolved Hide resolved
...sfer-api-v1/src/main/java/net/fabricmc/fabric/impl/transfer/fluid/CombinedProvidersImpl.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/ItemStackMixin.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/ItemStackMixin.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/FoxEntityMixin.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/FoxEntityMixin.java Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/BlockItemMixin.java Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/api/item/v1/FabricItemStack.java Outdated Show resolved Hide resolved
@Phoupraw
Copy link
Contributor Author

wait, the game test doesn't act as I thought.

modmuss50
modmuss50 reviewed Oct 21, 2023
View reviewed changes
fabric-item-api-v1/src/testmod/java/net/fabricmc/fabric/test/item/gametest/FoodGameTest.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/testmod/java/net/fabricmc/fabric/test/item/gametest/FoodGameTest.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/testmod/java/net/fabricmc/fabric/test/item/CustomDamageTest.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/WolfEntityMixin.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/WolfEntityMixin.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/WolfEntityMixin.java Outdated Show resolved Hide resolved
@Juuxel Juuxel added the enhancement New feature or request label Nov 6, 2023
Juuxel
Juuxel reviewed Nov 6, 2023
View reviewed changes
Copy link
Member

@Juuxel Juuxel left a comment

Choose a reason for hiding this comment

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

Some notes about the javadoc

fabric-item-api-v1/src/main/java/net/fabricmc/fabric/api/item/v1/FabricItemStack.java Outdated Show resolved Hide resolved
fabric-item-api-v1/src/main/java/net/fabricmc/fabric/api/item/v1/FabricItem.java Outdated Show resolved Hide resolved
@Juuxel Juuxel added the priority:low Low priority PRs that don't immediately need to be resolved label Nov 6, 2023
modmuss50
modmuss50 approved these changes Dec 27, 2023
View reviewed changes
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

This PR looks good, my main worry is the amount of Mixins and @reDIrect's. I guess there isnt any other way around it. Might be nice to explore using Mixin Extras now that we have it.

fabric-item-api-v1/src/main/java/net/fabricmc/fabric/mixin/item/BlockItemMixin.java

@Mixin(BlockItem.class)
class BlockItemMixin {
@Redirect(method = "useOnBlock", at = @At(value = "INVOKE", target = "Lnet/minecraft/item/BlockItem;isFood()Z"))
Copy link
Member

Choose a reason for hiding this comment

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

Could a lot of these mixins be improved by using WrapOperation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At that moment IJ Idea and Minecraft Dev plugin doesn't support mixin extra well, so I use redirect instead to avoid potential a mass of errors.

Copy link

Choose a reason for hiding this comment

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

MCDev has had full mixinextras support for a long time now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah? I had a try just now, and found that it still suggests incorrect return type wrapping putting field.
For example, for the following wrap:

@WrapOperation(method = "<clinit>", at = @At(value = "FIELD", target = "Lnet/fabricmc/fabric/api/transfer/v1/fluid/FluidStorage;ITEM:Lnet/fabricmc/fabric/api/lookup/v1/item/ItemApiLookup;", opcode = Opcodes.PUTSTATIC))

the correct handler method signature should be

private static void customField(ItemApiLookup<Storage<FluidVariant>, ContainerItemContext> value, Operation<ItemApiLookup<Storage<FluidVariant>, ContainerItemContext>> original)

but minecraft dev plugin suggests that it should be

private static ItemApiLookup<Storage<FluidVariant>, ContainerItemContext> customField(ItemApiLookup<Storage<FluidVariant>, ContainerItemContext> value, Operation<ItemApiLookup<Storage<FluidVariant>, ContainerItemContext>> original)

The latter causes an error on game launching.

Copy link

Choose a reason for hiding this comment

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

Ok you might be right about that one… will fix
Don’t know of any other such cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just found that fabric api project still remains loader 0.14.21... And fabric item api still depends on fabricloader >=0.4.0 in fabric.mod.json... Shall we update them?

Copy link
Member

Choose a reason for hiding this comment

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

We had the same problem in #3403 (comment) and decided on using the @reDIrect's in 1.20.1/.2 and then for .4 using ME. Quite happy to leave this as-is and when cherry-picking to 1.20.4 I can change this to use ME.

@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Dec 28, 2023
@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Jan 5, 2024
@modmuss50
Copy link
Member

I plan to do another release very shortly, I will re-target this PR to 1.20.4 and use ME.

@modmuss50 modmuss50 self-assigned this Jan 11, 2024
@modmuss50 modmuss50 mentioned this pull request Jan 12, 2024
Merged
@modmuss50
Copy link
Member

I have created a 1.20.4 + ME version of this PR here: #3520

@modmuss50
Copy link
Member

Merged into 1.20.4 via #3520 Many thanks for this 👍

@modmuss50 modmuss50 closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LlamaLad7 LlamaLad7 LlamaLad7 left review comments

@Juuxel Juuxel Juuxel requested changes

@modmuss50 modmuss50 modmuss50 approved these changes

@apple502j apple502j apple502j requested changes

@Shnupbups Shnupbups Shnupbups requested changes

Assignees

@modmuss50 modmuss50

Labels
enhancement New feature or request last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge priority:low Low priority PRs that don't immediately need to be resolved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Phoupraw @modmuss50 @Shnupbups @Juuxel @LlamaLad7 @apple502j