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

Remove reflection for mixins #41

Closed
wants to merge 1 commit into from
Closed

Remove reflection for mixins #41

wants to merge 1 commit into from

Conversation

Alexdoru
Copy link
Member

@Alexdoru Alexdoru commented Oct 1, 2024

Doing that makes it faster during runtime when rendering the GUI. Also it "adds no extra code" since a bunch of other mods adds the exact same accessors therefore they are all merged as one.

@Lyfts
Copy link
Member

Lyfts commented Oct 1, 2024

Why not use Forge AT's? Those are some of the most commonly AT'd variables

@Alexdoru
Copy link
Member Author

Alexdoru commented Oct 1, 2024

idk I don't like forge AT and this mod is most likely going to be used with GT5u which uses modularui and modularui adds the same mixin accessor so it adds no extra code

@Lyfts
Copy link
Member

Lyfts commented Oct 1, 2024

ModularUI also adds and uses a bunch of other accessors/invokers for various things in GuiContainer, so they might as well add one for those too.
To me this just seems like an unnecessary dep & coremod for something that could be boiled down to 4 lines in an AT file.
In this instance the AT is as free as it can be since forge will already transform those 4 fields, having mixins & coremod resolved is not nearly as free.

@Alexdoru
Copy link
Member Author

Alexdoru commented Oct 1, 2024

ModularUI also adds and uses a bunch of other accessors/invokers for various things in GuiContainer, so they might as well add one for those too. To me this just seems like an unnecessary dep & coremod for something that could be boiled down to 4 lines in an AT file. In this instance the AT is as free as it can be since forge will already transform those 4 fields, having mixins & coremod resolved is not nearly as free.

it adds a dep that's already there since this mod is used for GT and GT has mixins, also I don't think it adds much loading time. And I'd rather have that than reflection during runtime in the render loop

@Dream-Master Dream-Master requested a review from a team October 2, 2024 13:18
@Lyfts Lyfts mentioned this pull request Oct 21, 2024
@Alexdoru Alexdoru closed this Oct 21, 2024
@glowredman
Copy link
Member

I agree with Lyfts. Accessors add a method call which isn't necessary with ATs.

@Alexdoru Alexdoru deleted the mixins branch October 21, 2024 15:58
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