Alvinn8 / paper-nms-maven-plugin

A maven plugin for using NMS on paper with Mojang mappings.
MIT License
115 stars 8 forks source link

Remap artifact/classes twice instead of once #9

Closed stefvanschie closed 2 years ago

stefvanschie commented 2 years ago

Currently to remap the code from Mojang mappings to Spigot mappings, the two mappings are merged together and then applied as one mapping. I unfortunately ran into an improperly mapped method on 1.17 due to this strategy. The situation is as follows. The class PlayerConnection extends ServerPlayerConnection. Spigot's mappings define a mapping for a method called sendPacket from a on ServerPlayerConnection, but not on PlayerConnection. Mojang's mappings define a mapping from send to a on ServerPlayerConnection as well as PlayerConnection. Merging these two leaves us with the following mapping:

mojang  spigot
net/minecraft/server/network/ServerGamePacketListenerImpl   net/minecraft/server/network/PlayerConnection
    send    a
net/minecraft/server/network/ServerPlayerConnection net/minecraft/server/network/ServerPlayerConnection
    send    sendPacket

Since Lorenz does not have context for the mappings, it does not know that these two classes are related. Therefore, we now have two mappings for the same method, which are actually named differently. When using this with tiny-remapper, it ends up using the a name. The server jar however, uses the sendPacket name, meaning that you end up with a NoSuchMethodError when calling this method after remapping.

This pull request addresses this by not merging the mappings, but instead mapping the classes twice. The issue here, as mentioned earlier, is that Lorenz cannot know that these classes are related. However, tiny-remapper does know this. By mapping the jar twice, this means that it can properly use this context in the second step. When it maps from Mojang to obfuscated, it properly maps send to a. When mapping from Spigot it will now also map a to sendPacket, because it knows that the parent class of this method has this name and propagates it to its children. This means that this two-step process properly maps from send to sendPacket.

The initialization phase remains largely the same with the change that the merged mappings are no longer stored. Instead the Spigot and Mojang mappings are stored to be used in the remapping phase. Users that currently have full mappings stored need to re-initialize 1.17 again if this pull request is accepted. The remapping phase now first maps the final artifact/classes from Mojang names to obfuscated names as specified by the Mojang mappings. It then does this for all of the dependencies as well. We need to do this, since the dependencies are needed as the classpath for the second remapping step. Once all dependencies have been mapped to obfuscated names, the initial artifact/classes are mapped from obfuscated names to Spigot names as specified by the Spigot mappings. This is the final result of this plugin and the resulting artifact/classes can be further used by Maven. The temporarily remapped dependencies are deleted, as well as other temporary files. This process only happens when no dev-bundle is present; versions which have a dev-bundle behave the same as before.

This change resolves the issue mentioned earlier. I've compared the final output of this plugin with and without these changes for my code and, with the exception of send now being mapped properly, the other names remain identical. I cannot say with complete certainty that this is the case for all names, but testing seems to indicate that this is the case.