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

0.2.7 refactors #178

Merged
merged 16 commits into from
Mar 6, 2020
Merged

0.2.7 refactors #178

merged 16 commits into from
Mar 6, 2020

Conversation

modmuss50
Copy link
Member

@modmuss50 modmuss50 commented Feb 17, 2020

This PR contains a bit of cleanup to loom, as well as almost drop in support for access winders, and fixes for idea 2020.

This has the abbility to generate project based jars.

@modmuss50
Copy link
Member Author

modmuss50 commented Feb 18, 2020

Ok, I have something mostly working (not tested a lot, inner classes dont work for example).

I have been testing with the following file:

ae\v1	v1	named
public	class	net/minecraft/client/MinecraftClient
public	field	net/minecraft/client/MinecraftClient	resourcePackDir	Ljava/io/File;
protected	method	net/minecraft/client/MinecraftClient	initializeSearchableContainers	()V
public	class	net/minecraft/client/render/chunk/ChunkBuilder$BuiltChunk$SortTask
public	field	net/minecraft/client/render/chunk/ChunkBuilder$BuiltChunk$SortTask	data	Lnet/minecraft/client/render/chunk/ChunkBuilder$ChunkData;

Loom remaps this and places it in META-INF of the jar:

ae\v1	intermediary
public	class	net/minecraft/class_846$class_851$class_4579
public	class	net/minecraft/class_310
protected	method	net/minecraft/class_310	method_1546	()V
public	field	net/minecraft/class_310	field_1757	Ljava/io/File;
public	field	net/minecraft/class_846$class_851$class_4579	field_20841	Lnet/minecraft/class_846$class_849;

The format is still wide open for changes, im already not sure on forcing tabs as my ide sometimes changes them to tabs.

@2xsaiko
Copy link
Contributor

2xsaiko commented Feb 18, 2020

im already not sure on forcing tabs as my ide sometimes changes them to tabs.

Why not accept any amount of whitespace as a seperator?

@modmuss50
Copy link
Member Author

Ok, done that for now, Im still not super happy with the file format. Right now there are no limits on what you can make public, people seem undecided on allowing methods/fields to only be protected.

switch (split[1]) {
case "class":
if (split.length != 3) {
throw new RuntimeException("Failed to parse access escalator. at line:" + line);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of aborting immediately, I'd add the encountered errors to a list and continue trying to parse the file, then abort after it's done, that way you can see all the errors at once (and on top of that you don't get a completely irrelevant stack trace except for the message)
I'd also make the error more descriptive, something like

syntax error: expected '<class name> <access level>' after 'class'
at /path/to/inputfile.txt:40:
    |
 40 | class sdas as das das d asd as
    |       ^~~~~~~~~~~~~~~~~~~~~~~~

Same goes for all the other parse errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can easily make the erros more descriptive, im not sure if its worth the effort to go to make it super detailed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it doesn't have to be this detailed, one line error messages would definitely be enough, the main thing I would want is that it doesn't immediately throw when it encounters the first error.

@modmuss50
Copy link
Member Author

Updated file format:

ae\v1	named
# Comments like this are supported
public	class	net/minecraft/client/MinecraftClient # And comments can be put here as well
public	field	net/minecraft/client/MinecraftClient	resourcePackDir	Ljava/io/File;
protected	method	net/minecraft/client/MinecraftClient	initializeSearchableContainers	()V
mutable	field	net/minecraft/client/MinecraftClient	COMPLETED_UNIT_FUTURE	Ljava/util/concurrent/CompletableFuture;
public	field	net/minecraft/client/MinecraftClient	COMPLETED_UNIT_FUTURE	Ljava/util/concurrent/CompletableFuture;

public	class	net/minecraft/client/render/chunk/ChunkBuilder$BuiltChunk$SortTask
public	field	net/minecraft/client/render/chunk/ChunkBuilder$BuiltChunk$SortTask	data	Lnet/minecraft/client/render/chunk/ChunkBuilder$ChunkData;

Tabs are required, it will complain quite quickly if you use spaces.

…and will benefit idea 2020

Add some basic validation to the AccessWidenerRemapper, will present any issues with the mappings when building (May need a way to disable?)
+ Some bugs fixes
@modmuss50
Copy link
Member Author

Im going to move out the AccessWinder stuff into another PR, as my reactors should be good enough for 0.2.7 even without them.

@modmuss50 modmuss50 changed the title Rough work on project based jars, skeleton for AccessEscalators? 0.2.7 refactors Feb 28, 2020
Copy link

@sfPlayer1 sfPlayer1 left a comment

Choose a reason for hiding this comment

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

No obvious issues

@liach
Copy link
Contributor

liach commented Mar 5, 2020

Access changer should be ok as we don't recompile decompiled code like the other silly project does

Copy link
Contributor

@liach liach left a comment

Choose a reason for hiding this comment

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

No major problems

@modmuss50 modmuss50 merged commit ee462f8 into FabricMC:dev/0.2.7 Mar 6, 2020
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.

4 participants