Skip to content

Move builtin transform selection to game provider - #1056

Merged
Player3324 merged 3 commits into
FabricMC:masterfrom
Player3324:gameSpecificBuiltinTransform
Jul 2, 2025
Merged

Move builtin transform selection to game provider#1056
Player3324 merged 3 commits into
FabricMC:masterfrom
Player3324:gameSpecificBuiltinTransform

Conversation

@Player3324

Copy link
Copy Markdown
Contributor

Fixes #1055 and #1043

Co-authored-by: Raik176 <raikmaus321@gmail.com>
Co-authored-by: Space Walker <spacedoesrs@gmail.com>
Comment on lines +166 to +167
|| className.startsWith("com.mojang.minecraft") // unobf classes in classic
|| className.startsWith("com.mojang.rubydung") // unobf classes in pre-classic

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should these both end with . to be consistent with the lines above and below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They should, thanks for spotting

boolean requiresUrlClassLoader();
Set<BuiltinTransform> getBuiltinTransforms(String className);

enum BuiltinTransform {

@Gamebuster19901 Gamebuster19901 Jun 22, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm unfamiliar with these terms. Specifically STRIP_ENVIRONMENT and CLASS_TWEAKS. I looked in FabricTransformer but the usage isn't immediately clear to me. Perhaps we could add some documentation?

EDIT: I put this note in the wrong spot. It's for the enum values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I adjusted it, class tweakers is a bit of a future term for this branch, it's the new and eventually more powerful replacement for access wideners.

@Gamebuster19901 Gamebuster19901 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Either way is fine for me. Just figured it might be useful to preserve backwards compatibility if possible.

Path getLaunchDirectory();
boolean isObfuscated();
boolean requiresUrlClassLoader();
Set<BuiltinTransform> getBuiltinTransforms(String className);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we make this a default method for backwards compat? Other games couldn't use transforms previously so an empty set would make sense.

Doesn't really matter too much though.

@Player3324 Player3324 Jun 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't usually do this for internal interfaces like this, a custom GameProvider is probably better off with thinking about what the game actually wants.

We don't know what the right package for an arbitrary game would be and I haven't implemented tracking what a class is yet (game, game lib, mod, maybe more types).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 makes sense.

@SpaceWalkerRS

Copy link
Copy Markdown
Contributor

lgtm

@Player3324
Player3324 merged commit c47fb15 into FabricMC:master Jul 2, 2025
Gamebuster19901 pushed a commit to WilderForge/fabric-loader that referenced this pull request Jul 3, 2025
Co-authored-by: Raik176 <raikmaus321@gmail.com>
Co-authored-by: Space Walker <spacedoesrs@gmail.com>
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.

5 participants