search

bergerhealer / BKCommonLib

An extensive library used in bergerhealer's plugins
Other
181 stars 45 forks source link

Forge Support #105

Open bergerkiller opened 4 years ago

bergerkiller commented 4 years ago
BkCommonLib version: 1.16.2-v1-SNAPSHOT
Spigot version: 1.12.2 / 1.15.2 / 1.16

Problem or bug:

BKCommonLib has limited to no support for Bukkit + Forge server implementations.

Progress

Recent development builds have integrated remapping handlers for supporting forge servers, such as Magma, Mohist and CatServer.

TODO

Other Forge server implementations that need work supporting:

Related

Issue with Magma: https://github.com/bergerhealer/TrainCarts/issues/390

vico93 commented 4 years ago

I dont think Kettle is active anymore... i thought in that case the "torch" was passed to Mohist/Magma..

ArcLight would be interesting to have support when 1.16.x builds start appearing, that would be a amazing headstart to have your plugins support in 1.16 so early.

bergerkiller commented 4 years ago

Also note that forge support discussions have been ongoing on the Discord for a while now, just added this issue ticket so that this progress is visible on the outside. There's already 99% completed Forge support for 1.12.2 / Magma/Mohist/CatServer, with a new Remapper API in Mountiplex used for example here: https://github.com/bergerhealer/BKCommonLib/blob/master/src/main/java/com/bergerkiller/bukkit/common/server/MohistServer.java#L108

To support ArcLight would involve adding a Server detection for it, plus registering a remapper for it. If it's similar to the remappers used by the other servers, this shouldn't be hard. Do note however that so far I've only tested/worked on Forge 1.12.2 support. 1.16 probably has a lot more problems.

bergerkiller commented 4 years ago

https://github.com/bergerhealer/BKCommonLib/commit/536a29d24ffd9ad47ee5a6b75f018d7bdf713986

Arclight 1.14 is having too many problems in the remapper code itself. Theres a bug with an internal cache that I can't control, getName() fails randomly on field objects and theres TileEntityRenderer client code mixed in with server code causing a ClassNotFound error at runtime. I don't know if 1.15/1.16 is any better, since this is 'legacy'. But currently it's not looking rosy.

Will try 1.15/1.16 tomorrow, to see if it fares any better.

https://paste.traincarts.net/cuxenagiba.rb

vico93 commented 4 years ago

Just a question: where is your discord server? I tried searching on spigot home from each of your plugins but cant find any invites...

bbayu123 commented 4 years ago

It's on the traincarts page, right below the video showcases

IzzelAliz commented 3 years ago

Can you take a look at this report? My users are experiencing errors when running latest BKCommonLib.

bergerkiller commented 3 years ago

@IzzelAliz what server is that on? It looks like the remapper isnt working at all

IzzelAliz commented 3 years ago

@IzzelAliz what server is that on? It looks like the remapper isnt working at all

It is Arclight 1.15

bergerkiller commented 3 years ago

I am working on this

bergerkiller commented 3 years ago

Should be all good now with both 1.15 and 1.16 arclight. https://github.com/bergerhealer/BKCommonLib/commit/5d9c27d1da052ca18dffe830ee8f4029aa5eabc1

https://ci.mg-dev.eu/job/BKCommonLib/998/

IzzelAliz commented 3 years ago

Some news for you. Since this commit https://github.com/IzzelAliz/Arclight/commit/65ed2913f683cc6a28dfd89c28477d18224ceec6 Arclight can handle remapped sources so you don't need to manually call the remapper(by simply "remove" Arclight support). However there are some error I'm not sure how to deal with. https://pastebin.com/MphhLzXR If anything I can do on my side I would happy to help.

bergerkiller commented 3 years ago

What exactly was changed? I doubt I can stop calling the remapper directly, because the JVM/javassist must see the 'real' symbols or it fails to compile. So some translation is inevitable here.

Maybe about how the remapper would remap generated classes? Nothing really needs to be changed for that, all it does right now is call the defineClass method using reflection, which doesn't add enough overhead to remove. Works either way :p

The error usually is preceded by a warning or error about a missing requirement, but its absent from the log. Maybe further up? I can look at it later today too

IzzelAliz commented 3 years ago

Some modifications made to remapper and plugins now can read a net/minecraft/server/v1_16_R1/Xxxxx class and get remapped byte source, so your compilers will find all symbols, and defineClass is handled and generated classes will be remapped.

bergerkiller commented 3 years ago

Ah I see, that's pretty nifty. If this gives javaassist class symbols that are remapped to be bukkit-like then that should work, yeah. Only difficulty is that right now, in part because of other forge server projects, it has to make sure a class remapper doesn't remap generated classes. But I can maybe add a global toggle somewhere so for Arclight it allows it.

bergerkiller commented 3 years ago

So the problem was a little bit hidden.

Thing of note: The mpl$ prefix is just a little flag indicator to tell the javaassist resolver to treat what follows as the final name. This prevents double-remapping. I use this so that I can use the compiled names together with to-be-resolved names in the source code.

The reason it says ??worldserver?? is because it failed to find the class worldserver. Which makes sense, because it is a field name.

It actually didnt manage to find a local field matching 'spigotConfig' in class 'WorldServer'. This was further confirmed when I added some extra logging in lookupField, which reveals the first try fails: FAILED TO LOOKUP LOCAL FIELD net.minecraft.server.v1_16_R3.WorldServer -> spigotConfig

So for some reason, it is unable to find that field. It did manager to find the class names just fine though, so the remapper is doing what it is intended to do.

Any idea what this might be? The spigotConfig field can be found through reflection, since it passed the #exists check in the template.

bergerkiller commented 3 years ago

This version, besides adding support, also prints a better error revealing this local field lookup failed as well. https://github.com/bergerhealer/BKCommonLib/commit/e583241036d1889957a3b14bf20b0efef6c84549

What I think is happening: I think spigotConfig is defined in nms.World rather than nms.WorldServer, and for some reason the bytecode generating by Arclight does something wrong to cause this to fail. Might be a remapping class inheritance failure.

Error: https://paste.traincarts.net/enabohoqag.java

https://ci.mg-dev.eu/job/BKCommonLib/1008/

Any input is appreciated, or @mention me on discord and we can discuss there when you have time :)