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

Using ModifyArgs causes a "ClassNotFoundException" under ModLauncher 9 #584

Closed
Speiger opened this issue Jun 16, 2022 · 15 comments
Closed

Comments

@Speiger
Copy link

Speiger commented Jun 16, 2022

o/ so its offical too, I tried using ModifyArgs to replace values in a constructor.
While it was working on startup when the code changed started to be accessed it caused a "ClassNotFoundException"

java.lang.NoClassDefFoundError: org/spongepowered/asm/synthetic/args/Args$1
at net.minecraft.server.WorldLoader$PackConfig.createResourceManager(WorldLoader.java:44) ~[forge-1.19-41.0.30_mapped_official_1.19-recomp.jar%2390!/:?] {re:classloading}
at net.minecraft.server.WorldLoader.load(WorldLoader.java:20) ~[forge-1.19-41.0.30_mapped_official_1.19-recomp.jar%2390!/:?] {re:classloading}
at net.minecraft.client.gui.screens.worldselection.CreateWorldScreen.openFresh(CreateWorldScreen.java:116) ~[forge-1.19-41.0.30_mapped_official_1.19-recomp.jar%2390!/:?] {re:mixin,pl:runtimedistcleaner:A,re:classloading,pl:mixin:APP:chunkpregen.mixins.json:client.CreateNewWorldMixin,pl:mixin:A,pl:runtimedistcleaner:A}

Here is the mixin being used

@Mixin(MinecraftServer.class)
public class ServerSeedMixin
{
    @ModifyArgs(method = "createLevels", at = @At(value = "INVOKE", target = "Lnet/minecraft/server/level/ServerLevel;<init>", ordinal = 1))
    public void onWorldLoad(Args args)
    {
        System.out.println("Size="+args.size()+", Dimension="+args.get(4)+", Seed="+args.get(7));
    }
}

I am using the forge beta for 1.19 mappings should be visible through the crashlog.

@SizableShrimp
Copy link
Contributor

This is a known bug with Forge 1.17+ and is not a bug with mixin

@Speiger
Copy link
Author

Speiger commented Jun 16, 2022

yep fully aware (discord)

@Speiger Speiger closed this as completed Jun 16, 2022
@LexManos
Copy link
Contributor

Just to clarify, from what I understand this NOT a Forge bug. But more an issue with Mixin and JPMS. These synthetic accessor classes need to be in the module that they are mixing into. Instead of trying to be defined in Mixin's module. As that entire module does not go through transformers and thus can't have dynamic code. The other potential solution is to manually define the class in the correct classloader yourself. But it probably would just be easier to change the package.

@Speiger
Copy link
Author

Speiger commented Jun 16, 2022

@LexManos might be the case or might be not the case, that is for you and @Mumfrey to figure out.
For me I already found a solution that works a lot better and does not have to deal with "Maybes or non maybes" of forge...

But my suggestion would be: Either implement a compiler warning that straight out disables that feature or actually fix the issue, whoever causes it.

@zml2008
Copy link
Member

zml2008 commented Jun 16, 2022

There's a solution long discussed and planned in McModLauncher/modlauncher#90, with a PR in McModLauncher/modlauncher#110 -it's already been figured out, just needs people to have time to review & implement. you're welcome to help out if you want things to improve.

@marchermans
Copy link

There's a solution long discussed and planned in McModLauncher/modlauncher#90, with a PR in McModLauncher/modlauncher#110 -it's already been figured out, just needs people to have time to review & implement. you're welcome to help out if you want things to improve.

Well this solves some of it, but Lexes argument still holds.
Mixin needs to generate it into the module namespace of the module it is targetting.
Even with that API you can't inject that org/spongepowered/asm/synthetic/args/ namespace into the Game layer in each module since that would violate JPMS

@Mumfrey
Copy link
Member

Mumfrey commented Jun 19, 2022

The reason it generates the args classes in a central package instead of in the target package is that args classes are shared and aren't target-specific. For example an Args for two strings is just an args for 2 strings and if another mixin for a different class uses the same signature then the same Args class will be used instead of generating a new one.

If they need to be generated somewhere else then that's doable, but generating them in the same package as the first target doesn't make particular sense either.

Even with that API you can't inject that org/spongepowered/asm/synthetic/args/ namespace into the Game layer in each module since that would violate JPMS

I don't really get what you mean by this since JPMS doesn't know whether the classes are loaded off disk or generated at runtime. As far as I can see this should be a perfectly valid operation in JPMS-land like it was pre-JPMS.

@marchermans
Copy link

Even with that API you can't inject that org/spongepowered/asm/synthetic/args/ namespace into the Game layer in each module since that would violate JPMS

I don't really get what you mean by this since JPMS doesn't know whether the classes are loaded off disk or generated at runtime. As far as I can see this should be a perfectly valid operation in JPMS-land like it was pre-JPMS.

The problem is that two modules can not have classes in the same package. That violates JPMS.
You should pick a central package like: net/minecraft/mixin/asm/synthetic for example and target the minecraft module.

@Speiger
Copy link
Author

Speiger commented Jun 19, 2022

You should pick a central package like: net/minecraft/mixin/asm/synthetic for example and target the minecraft module.

Yes that but not that package. It should be a package specific to that mod because the likelyhood of classes with anything else is to high. And that would make sense since the Class is loaded only if the mod is loaded anyways.
Best would be to move it actually into the class that has said mixins that causes it.

@Mumfrey
Copy link
Member

Mumfrey commented Jun 19, 2022

The problem is that two modules can not have classes in the same package. That violates JPMS.

There are no other classes in that package, org.spongepowered.asm.synthetic is used purely for synthetic classes, so the classes can all be in one module.

You should pick a central package like: net/minecraft/mixin/asm/synthetic for example and target the minecraft module.

I don't get to pick the module, that part is handled by modlauncher.

It should be a package specific to that mod because the likelyhood of classes with anything else is to high. And that would make sense since the Class is loaded only if the mod is loaded anyways.

I already said this above, Args classes are not mod-specific, they are shared.

@marchermans
Copy link

You can not use your Mixin package, it is part of your Mixin module which is not loaded into the game layer but into the plugins layer.

If you want it to work no matter what with a single package, but ML basically requires you to pick something under net.minecraft.

You could inject a "helper" mod with its own root package and use that as package for it, but it as long as it gets loaded into the right layer.

@Mumfrey Mumfrey changed the title [Bug?] using ModifyArgs causes a "ClassNotFoundException" Using ModifyArgs causes a "ClassNotFoundException" Jun 20, 2022
@Mumfrey Mumfrey changed the title Using ModifyArgs causes a "ClassNotFoundException" Using ModifyArgs causes a "ClassNotFoundException" under ModLauncher 9 Jun 20, 2022
@Speiger
Copy link
Author

Speiger commented Jul 8, 2022

@Mumfrey here is a question. How are the classnames generated for the Args class?
Is it just a increasing counter? Or is the name generated based on what is inside?

Assuming it is a counter what happens if 2 mods provide "Args$1" class into sponges mixin directory but have 2 completely different arguments. Wouldn't that cause problems?

This is what i mean with "Clashing"

@Speiger Speiger reopened this Jul 8, 2022
@Speiger
Copy link
Author

Speiger commented Jul 8, 2022

Opened because i am kinda interested in the inner workings and this doesn't look like a third party issue IMO.

@Mumfrey
Copy link
Member

Mumfrey commented Jul 8, 2022

@Mumfrey here is a question. How are the classnames generated for the Args class? Is it just a increasing counter? Or is the name generated based on what is inside?

Yes they're just generated in the order that they're disccovered.

Assuming it is a counter what happens if 2 mods provide "Args$1" class into sponges mixin directory but have 2 completely different arguments. Wouldn't that cause problems?

This is what I've said already, Args classes are not mod-specific, they are signature-specific. So if the first Args to be discovered is (string, string) then Args$1 is (string,string), if the next one is (int,float,Object) then Args$2 will be (int,float,Object), there's no opportunity to conflict because the signatures are different. It doesn't matter if the (int,float,Object) comes from twenty different mods, it'll still be Args$2.

This is what i mean with "Clashing"

Yes but your understanding somewhere is faulty because this statement: "mods provide "Args$1" class into sponges mixin directory" doesn't make any sense in the first place: mods do not "provide" anything, the classes are generated by Mixin.

Opened because i am kinda interested in the inner workings and this doesn't look like a third party issue IMO.

The inner workings are all public.

@Speiger
Copy link
Author

Speiger commented Jul 8, 2022

Nope this explains for me everything that I didn't fully know and only assumed.
I had just some miss interpretations on how mixins worked.

My assumption was that the classes were generated on build and not in runtime.
So the name mapping was done when everything is loaded which means de-duplication happens right out of the gate.

Thank you for explaining the extra info.

@Speiger Speiger closed this as completed Jul 8, 2022
Fallen-Breath added a commit to Fallen-Breath/classic-minecraft-icon that referenced this issue Jun 17, 2023
see SpongePowered/Mixin#584
also splits 1.19.3 version for MinecraftClient for neater look
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants