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

[1.20.4] ClasspathLocator is no longer disabled in production env, causing OptiFine to crash while installing in the classpath #9899

Closed
burningtnt opened this issue Apr 6, 2024 · 6 comments
Assignees
Labels
1.20 Assigned This request has been assigned to a core developer. Will not go stale.

Comments

@burningtnt
Copy link

burningtnt commented Apr 6, 2024

Minecraft Version: 1.20.4

Forge Version: 49.0.39

Hello, I'm a developer of Hello Minecraft! Launcher https://github.com/HMCL-dev/HMCL.

In Forge versions for Minecraft 1.20.4, ClasspathLocator is always enabled in any environment, unlike the old implementation which was only enabled in development environments.

Therefore, while installing some mods (e.g. OptiFine) in the classpath, it worked in the legacy Forge versions as following reasons.

  • ClasspathLocator is disabled in production environment.
  • ModsFolderLocator only scaned files under mods folder.

However, while ClasspathLocator is enabled in any environment, this no longer works as following reasons.

  • ClasspathLocator loads all the JARs containing mods.toml
  • It doesn't execlude the files that contains an implementation of ITransformationService what ModsFolderLocator does.
  • OptiFine contains mod.toml without a Class annotated by @Mod("optifine")

See: HMCL-dev/HMCL#2975 (Sorry for it's in Chinese :(

I suggest ClasspathLocator execlude the files that contains an implementation of ITransformationService, just like ModsFolderLocator.

(Apologize for my poor English.)

@burningtnt burningtnt added the Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. label Apr 6, 2024
@autoforge autoforge bot added the 1.20 label Apr 6, 2024
@burningtnt
Copy link
Author

burningtnt commented Apr 7, 2024

The removal of the enabled field happended in 6f9f0f0#diff-07e70ca800aaf1bbe0de02598e83c730c98fac2dacfc0e9c7be43c4a66dc27edL27

I'm not sure about whether this change is for some special purposes or not.
In my opinion, once Forge is enabled to load mods from cp, we should keep the behavior same as how we load mods from the mods folder.

@PaintNinja PaintNinja added Assigned This request has been assigned to a core developer. Will not go stale. and removed Triage This request requires the active attention of the Triage Team. Requires labelling or reviews. labels Apr 13, 2024
@LexManos
Copy link
Member

It being enabled at runtime is an intended change. But it does seem I should re-work it to be a two phase system to be sure to not locate things that have already been located such as optifine.

The locations services is still a work in progress as I was trying to clean up the clusterfuck of a codebase/design while breaking as little as possible. I will make another pass when i get some time.

@burningtnt
Copy link
Author

Thanks for you reply.

If any plans about the feature to disable ClasspathLocator locating JARs which contains ITransformationService are made, please let us know.

@LexManos
Copy link
Member

LexManos commented May 9, 2024

Was simpler then I expected. And was just another random list of hardcoded things that I intend to cleanup at some point in the future.

@zkitefly
Copy link

Excuse me, this modification was applied in 1.20.4-49.0.50 right?

@LexManos
Copy link
Member

Thats what the changelog says yes.
https://maven.minecraftforge.net/net/minecraftforge/forge/1.20.4-49.0.50/forge-1.20.4-49.0.50-changelog.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20 Assigned This request has been assigned to a core developer. Will not go stale.
Projects
None yet
Development

No branches or pull requests

4 participants