Skip to content

WorldEdit transformers for 1.8.8 + give more control to transformers (wrapper) - #1429

Merged
0utplay merged 3 commits into
CloudNetService:nightlyfrom
DasBabyPixel:fix/classtransformer
Jul 15, 2024
Merged

0utplay merged 3 commits into
CloudNetService:nightlyfrom
DasBabyPixel:fix/classtransformer

Conversation

@DasBabyPixel

Copy link
Copy Markdown
Contributor

Motivation

A few servers running bukkit 1.8.8 still use worldedit. Only since the change to modern java on those old versions, worldedit stopped to work. This PR fixes worldedit and FAWE on 1.8.8.
The necessary class transformers also uncovered a problem in the class transformer api. Converting a ClassNode back to a byte[] was not possible (in some cases), because of COMPUTE_FRAMES|COMPUTE_MAXS.

Some might also want to make use of the wrapper transformer API in wrapper modules, so the custom byte[] creation might come in handy.
As an example, I have another project that needs transforming of classes, but uses a byte[] instead of asm.
The new API allows something like this:

@Override
public byte[] toByteArray(ClassNode classNode) {
    var bytes = Transformer.super.toByteArray(classNode);
    return CustomTransformer.transform(bytes);
}

Modification

Added transformers for WorldEdit / FAWE
Added a custom opt-in serializer option for transformers.
Added better logging for transformer failures.

Result

FAWE / WorldEdit supported.
Proper handling of transformer errors / soft failure - not doing this causes the JVM to freeze on errors iirc.
Give more control to transformers, without changing their default behaviour.

Other context

This is something I concocted some ago and kept in my fork, thought this might be of interest.
I did also test worldedit/fawe on 1.20 to make sure this doesn't cause any problems. Didn't notice anything, but as always with transformers, future changes on worldedit (they'd have to do something odd, but still) could cause the transformer to break worldedit functionality.

If the worldedit transformers aren't merged, I'd still love to see the API to give more control to transformers merged, because I use the API changes privately. WorldEdit transformers could be re-added by a wrapper module, but only with the more powerful api.

@DasBabyPixel
DasBabyPixel force-pushed the fix/classtransformer branch from 68bb84a to 1774e83 Compare July 5, 2024 19:11
@0utplay

0utplay commented Jul 8, 2024

Copy link
Copy Markdown
Member

I like the idea, but this might have to wait until we've replaced the networking and asm

@DasBabyPixel
DasBabyPixel force-pushed the fix/classtransformer branch from 1774e83 to 7eb950d Compare July 9, 2024 13:51
@0utplay

0utplay commented Jul 14, 2024

Copy link
Copy Markdown
Member

If you are still willing to do these changes: You have to use the new classfile api

@DasBabyPixel

DasBabyPixel commented Jul 14, 2024 •

Copy link
Copy Markdown
Contributor Author

Can I give the ClassTransform class the option (a default method, or sth else) of manually modifying the byte[] of the class after using java's api? I mostly did the whole thing to be able to manually apply papermc transforms to classes contained in a wrapper module (thus not transformed by paper's classloader).
To do something like this:

@Override
public byte[] toByteArray(ClassNode classNode) {
    var bytes = Transformer.super.toByteArray(classNode);
    return CustomBytecodeModifier.modify(bytes);
}

@DasBabyPixel

Copy link
Copy Markdown
Contributor Author

Maybe I'm also missing a way to do the same thing using java's new api. If so, let me know

@0utplay

0utplay commented Jul 14, 2024

Copy link
Copy Markdown
Member

cc @derklaro

@derklaro

derklaro commented Jul 14, 2024 •

Copy link
Copy Markdown
Member

The wrapper transformer api is build to transform classes not to implement custom transformers which, imo, is done with the changes made to the api recently.

If you want to transform the original class bytes of a class, I suggest to either write your own java agent and attach it to register custom transformers, or, another way would be to use a (kinda) hacky approach to get the original class bytes, pass them to your transformer like this:

static class TestClassTransform implements ClassTransform {

  @Override
  public void atStart(ClassBuilder builder) {
    var classModel = builder.original().orElseThrow(); // transforming, so this is always present
    var classReader = (ClassReader) classModel.constantPool();
    var originalClassBytes = classReader.readBytes(0, classReader.classfileLength());
    // pass to custom transformer, parse the class file & extract the instructions to call from it
  }

  @Override
  public void accept(ClassBuilder builder, ClassElement element) {
    // builder.accept(element); - use this to accept the old class
    // or use the extracted elements above and pass them to the builder
  }
}

@DasBabyPixel

Copy link
Copy Markdown
Contributor Author

The wrapper transformer api is build to transform classes not to implement custom transformers which, imo, is done with the changes made to the api recently.

If you want to transform the original class bytes of a class, I suggest to either write your own java agent and attach it to register custom transformers, or, another way would be to use a (kinda) hacky approach to get the original class bytes, pass them to your transformer like this:

static class TestClassTransform implements ClassTransform {

  @Override
  public void atStart(ClassBuilder builder) {
    var classModel = builder.original().orElseThrow(); // transforming, so this is always present
    var classReader = (ClassReader) classModel.constantPool();
    var originalClassBytes = classReader.readBytes(0, classReader.classfileLength());
    // pass to custom transformer, parse the class file & extract the instructions to call from it
  }

  @Override
  public void accept(ClassBuilder builder, ClassElement element) {
    // builder.accept(element); - use this to accept the old class
    // or use the extracted elements above and pass them to the builder
  }
}

Allright, I'm gonna finish the transformers in this PR (they don't need byte[] manipulation). Going to try your suggestion, if an issue arises, I'm gonna open a new issue or PR

@derklaro

Copy link
Copy Markdown
Member

I'm happy to pull in the WorldEdit transformers. If there are issues feel free to contact me on Discord :)

@DasBabyPixel
DasBabyPixel force-pushed the fix/classtransformer branch 2 times, most recently from 77e2680 to c905d86 Compare July 14, 2024 19:10
@DasBabyPixel

Copy link
Copy Markdown
Contributor Author

Finished the transformers, tested //pos1 //pos2 //set <block> on paper 1.8.8, 1.12.2, 1.16.5 and 1.21.
See commit message for exact versions used

@derklaro derklaro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor nits, but generally looking good

tested on paper 1.8.8 and 1.12.2 with FastAsyncWorldEdit-bukkit-21.03.26-5ff3a9b-1286-22.3.9
on paper 1.16.5 with FAWE 2.8.0
on paper 1.21 with latest snapshot FastAsyncWorldEdit-Bukkit-2.11.1-SNAPSHOT-847
@DasBabyPixel
DasBabyPixel force-pushed the fix/classtransformer branch from 3d65c54 to 04ff43f Compare July 15, 2024 12:07
@DasBabyPixel

Copy link
Copy Markdown
Contributor Author

Done

@derklaro derklaro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

didn't test this, but code LGTM

@0utplay

0utplay commented Jul 15, 2024

Copy link
Copy Markdown
Member

I will do some small testing later and merge afterwards. Thank you!

@0utplay

0utplay commented Jul 15, 2024

Copy link
Copy Markdown
Member

So I did face an issue using fawe and the //set Command.
image

But worldedit was working fine and fawe is also working and I think you can argue that people would have to live with that

@DasBabyPixel

Copy link
Copy Markdown
Contributor Author

So I did face an issue using fawe and the //set Command. image

But worldedit was working fine and fawe is also working and I think you can argue that people would have to live with that

I didn't notice that because I had anti xray turned off. Gonna see if there is an easy fix, otherwise oh well...
I do want to point out that this also happens with java 8 on a pure paper server without anything else

@DasBabyPixel

Copy link
Copy Markdown
Contributor Author

After further investigation, this happens because timings are stopped while async (duh).
FAWE sends chunk updates async and the PacketPlayOutMapChunk hides blocks async, causing this.
This is not actually an error but rather a warning that also up screws timings a little

image

I see 3 options
Option 1: Disable timings in config
Option 2: Disable anti xray in config
Option 3: Add transformer disabling either

I think it is best to ignore this and tell ppl to change their config to disable timings. We have spark for a reason.

@0utplay
0utplay merged commit 202961f into CloudNetService:nightly Jul 15, 2024
@DasBabyPixel
DasBabyPixel deleted the fix/classtransformer branch August 1, 2024 14:13
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