MinecraftForge / CoreMods

CoreMods
GNU Lesser General Public License v2.1
39 stars 25 forks source link

Allow remapping class names #25

Open Chocohead opened 4 years ago

Chocohead commented 4 years ago

Somewhat related to #10, an INameMappingService can map class names just like it can do methods and fields but there's nothing exposed in the coremod API to be able to use this. Whilst obviously not useful from the context of using MCP names it does open up using other possible (class) names in dev (say Yarn or Mojang's if legal ever allow it).

LexManos commented 4 years ago

In my opinion this shouldn't be supported as it's just asking for people to do weird things. Class names are meant to be stable to give some semblance of logic to the loading order/management. And honestly, we are a MCP/SRG ecosystem, class names are stable on this end, and thus allows easier translation to different people's workspaces. I have never been a fan of allowing one system to call the Block class "Block" and another to call it "TiddelyBorp". Keeping class names consistent saves an immeasurable amount of headake when having to work with other people's code. As well as saving issues related to actual code level changes related to class names, access levels, etc... And finally, I don't particularly care about Yarn. And it doesn't seem like we'll ever get Mojang's legal to give us the green light. So... my vote is no. But as cpw deals with this stuff i'll leave it up to him.

cpw commented 4 years ago

I would ask a more essentially practical question: how could this even work? Class name transformation is a static operation, one of the primary goals of the installation.

If you can give me a way this could possibly work, without rewriting everything in modlauncher and forge, please do explain..

Chocohead commented 4 years ago

There are only two scenarios which need be considered really, production and development.

In production, Forge runs with SRG names. The game is remapped and patched by the installer and that is the last changing of names that needs to be done. Any mod loaded by FML is also using SRG names so the world is happy. Any calls to the ASM API to remap a class/method/field are nooped to return the SRG string (as they are already).

In development, Forge runs with whatever names it has been mapped to. The mod being actively developed runs with whatever mappings Forge is using. Calls to the ASM API to remap a class/method/field are handled by the associated INameMappingService from the NameMappingServiceHandler. In MCP's case it will map methods and fields according to the csvs on the classpath and the class names can be nooped. For mapping sets which have different class names too an appropriate INameMappingService could handle them differently as well.


In fact the only change really needed (here at the very least) is:

//In ASMAPI
public static String mapClass(String name) {
    //Using ASMAPI#map(String, INameMappingService.Domain)
    return map(name, INameMappingService.Domain.CLASS);
}

Given that MCPNamingService already noops class domain map requests.

For Forge (and any other compiled mods you might want running in dev) to then ensure any Minecraft class names used for targets/owners used this would essentially solve any residual mapping difference problems for coremods. Conceptually it's no different to method or field names having to use SRG rather than MCP names. It comes with the bonus that the class name is already the input, so unlike methods or fields there's no additional obscuration if someone forgets to leave a comment.

It would only need be as intrusive as

function initializeCoreMod() {
    return {
        'flowerpotblock': {
            'target': {
                'type': 'CLASS',
                'name': 'net.minecraft.block.FlowerPotBlock'
            },
            ...
//to
function initializeCoreMod() {
    var mapClass = Java.type('net.minecraftforge.coremod.api.ASMAPI').mapClass;
    return {
        'flowerpotblock': {
            'target': {
                'type': 'CLASS',
                'name': mapClass('net.minecraft.block.FlowerPotBlock')
            },
            ...

This is merely a request for fully fleshing out the interface over to the INameMappingService in dev (given it doesn't need to do anything in production). The logistics of getting the rest of Forge running in alternative mappings (in dev of course) is not especially difficult in the grand scheme of things, although not completely without challenges, but that is not the point of this request. Given that coremods are written in JS anyway said logistics could be done even if this is rejected; that just leads to a bit more of a mess in order to do it, which feels like it could be reasonably avoided from what INameMappingService is temptingly dangling already.