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

Optimized GuigraphicsMixin's code #956

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

westernat
Copy link

Description

use WrapWithCondition instead of multiple Inject

Other details

@ChiefArug
Copy link
Contributor

Why is this using @Local(argsOnly = true) rather than simply appending the enclosing methods parameters, as you can do with most injectors (including WrapWithConditions according to the wiki: https://github.com/LlamaLad7/MixinExtras/wiki/WrapWithCondition)?

@pietro-lopes
Copy link
Contributor

Why is this using @Local(argsOnly = true) rather than simply appending the enclosing methods parameters, as you can do with most injectors (including WrapWithConditions according to the wiki: https://github.com/LlamaLad7/MixinExtras/wiki/WrapWithCondition)?

Do you have an example?
I don't see what you mean as his target is something that does not have those on the methods parameters.

@ChiefArug
Copy link
Contributor

@pietro-lopes
Copy link
Contributor

Do you have an example

I found this in GeckoLib: https://github.com/bernie-g/geckolib/blob/refs%2Fheads%2Fmain/common%2Fsrc%2Fmain%2Fjava%2Fsoftware%2Fbernie%2Fgeckolib%2Fmixin%2Fclient%2FTextureManagerMixin.java#L42-L47

Looks like normal to me, the invoke target contains the same args as the injected method, not extra args from parent method

@ChiefArug
Copy link
Contributor

Oh, I missed that the first parameter was the instance being called on and just counted the params.

if (kjs$itemSize[0] != 0) {
cir.setReturnValue(KubeJSClient.drawStackSize((GuiGraphics) (Object) this, font, kjs$itemSize[0], kjs$itemSize[1], kjs$itemSize[2], color, dropShadow));
@WrapWithCondition(method = "renderItemDecorations(Lnet/minecraft/client/gui/Font;Lnet/minecraft/world/item/ItemStack;IILjava/lang/String;)V", at= @At(value = "INVOKE", target = "Lnet/minecraft/client/gui/GuiGraphics;drawString(Lnet/minecraft/client/gui/Font;Ljava/lang/String;IIIZ)I"))
private boolean kjs$drawSize(GuiGraphics instance, Font font, String text, int x, int y, int color, boolean dropShadow, @Local(argsOnly = true) ItemStack stack, @Local(argsOnly = true, ordinal = 0) int pX, @Local(argsOnly = true, ordinal = 1) int pY, @Local(argsOnly = true) String pText) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private boolean kjs$drawSize(GuiGraphics instance, Font font, String text, int x, int y, int color, boolean dropShadow, @Local(argsOnly = true) ItemStack stack, @Local(argsOnly = true, ordinal = 0) int pX, @Local(argsOnly = true, ordinal = 1) int pY, @Local(argsOnly = true) String pText) {
private boolean kjs$drawSize(GuiGraphics instance, Font font, String text, int x, int y, int color, boolean dropShadow, Font font2, ItemStack stack, int pX, int pY, String pText) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how good using the arg append method is vs using specific @Local(argsOnly = true) parameters, but this is what I was referring to above.

Copy link
Author

Choose a reason for hiding this comment

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

The change you're suggesting is just to make the method have one more Font parameter to be passed in

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it avoids using @Local which in some cases can be a bad thing. I'm not sure if it is here though.

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.

3 participants