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

utilize loom injected interfaces #1730

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

Conversation

SpaceWalkerRS
Copy link
Contributor

Utilize Loom's injected interfaces feature to avoid excessive casting to carpet's "fake"s interfaces.

@gnembon
Copy link
Owner

gnembon commented May 19, 2023

Seems to be a convenient mechanic albeit much more 'magical' than the 'good old interfaces' which were messy, but more understandable from Java point of view, but if its natively now supported by loom, so be it.

But don't change the contract of Vanilla.java and Carpet.java - these need to stay put (i.e. no modded code in the scarpet area)

@SpaceWalkerRS
Copy link
Contributor Author

I figured the extra layer was unnecessary if the code could be called directly, but it makes sense to contain references to modded code in their own classes. Should I restore those two classes and its references then?

@SpaceWalkerRS SpaceWalkerRS force-pushed the injected-interfaces branch 2 times, most recently from 48eaefd to 2d055c0 Compare May 22, 2023 00:18
@gnembon
Copy link
Owner

gnembon commented May 22, 2023

Well, these two interfaces are needed in situations where you need it without carpet or without modding in general if you know what I mean.

Regarding these injected interfaces, is there any gist / docs on it? Is it done though defining them as defaults? Would IDE pick that up and not screw up references? (is that done via MinecraftDevelopment plugin for example). Also is carpet$ part of that requirement, or you just renamed these methods to make sure they don't collide ever?

@SpaceWalkerRS
Copy link
Contributor Author

Regarding these injected interfaces, is there any gist / docs on it? Is it done though defining them as defaults?

There is some documentation on the Fabric wiki, though it doesn't go into detail as to how it works, but that can be seen in Loom's source code. It uses a jar processor to add the interfaces to the source class file before it is remapped. This has the same effect as applying the interface through Mixin, but with the added benefit that it is already applied at compile time, circumventing the need to cast any objects to the interface. This is also why the methods in the interface must have a default implementation, because each class that implements an interface must have an implementation for its methods available in order for the project to compile.

Would IDE pick that up and not screw up references? (is that done via MinecraftDevelopment plugin for example).

I don't know how the MinecraftDevelopment plugin deals with this (I have never used it, as I don't use Intellij). You will have to rebuild the project any time you add a new injected interface, because Loom will have to re-process the source files to add it. Your IDE will just link any references to the default implementations.

Also is carpet$ part of that requirement, or you just renamed these methods to make sure they don't collide ever?

Sorry, I should have clarified those changes. They were indeed purely to avoid collisions with other mods. There were already a few conventions in place. Some fakes used carpet$ in front of the method name, some fakes added CM at the end or in the middle of the method name, and some fakes didn't do anything to avoid collisions.

@gnembon
Copy link
Owner

gnembon commented May 24, 2023

ok, great.

  1. I think I got that - by defining defaults, they get injected on rebuilding the project so they are part of the native classes. That makes sense. Only annoyance is that you need to rebuild the project when adding a method, but that's minor. Maybe we should a how to on working on this repo. I always tried to avoid non-standard procedures so you could build it as any other 'examplemod', but hey - this seems convenient.

  2. Also gotcha, if loom injects these to native classes, it makes sense that any IDE should just see it.

  3. re the names - I guess its good to have a convention, using 'carpet$' prefix is good. Thanks for unifying that.

Will merge that, but after 1.20, to avoid confusion

@altrisi
Copy link
Collaborator

altrisi commented May 24, 2023

(shortly mentioned on discord) From what I read (very long time ago though, on the fabric discord iirc), this is mostly thought for library mods (like fabric api) so when others depend on them have these easy at compile time, and you already need to change the compile time deps when changing that. IMO it's not very convenient to have to regenerate the minecraft jar and sources every time you want to add an accessor interface, not sure if worth it.

Also makes it easier to not realize a method is not implemented given they now all have default (failing) implementations. This has happened in the past with fakes implemented in separate mixins to the same class (class was abstract, therefore no compile error). That time it crashed with AbstractMethodError, this seems more difficult to track (and potentially eaten by whatever calling them, resulting in silent errors).

@SpaceWalkerRS
Copy link
Contributor Author

IMO it's not very convenient to have to regenerate the minecraft jar and sources every time you want to add an accessor interface, not sure if worth it.

While that's true, IMO it's very minor. It takes <15s (on my old laptop) to re-process sources and it's only needed when adding/removing an interface. And with how many "fake"s carpet already has, you're more likely to be adding/removing methods to existing interfaces instead.

Also makes it easier to not realize a method is not implemented

This is a fair point though. I did try to remedy this somewhat by throwing an error in the default implementation, but it's probably still more error-prone than having abstract methods.

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