amadornes / MCMultiPart

A universal multipart API for Modern Minecraft
Other
67 stars 23 forks source link

[MCMP2] Methods from TileEntity in IMultipartTile / Block in IMultipart #67

Closed bdew closed 7 years ago

bdew commented 7 years ago

Currently IMultipartTile has most of the TileEntity methods in it with implementation that does the call via getTileEntity.

This makes sense when you are implementing IMultipartTile in a separate class that wraps an existing TileEntity. But when they are implemented by the same class it gets very annoying, at least in scala, because you have to manually override every single method like this

override def getWorld: World = super[TileEntity].getWorld

Otherwise either the compiler complains about conflicting inheritance or you get infinite recursion that never proceeds to the base method in TileEntity (YourClass.whatever -> IMultipartTile.whatever -> calls getTileEntity() and gets YourClass ->YourClass.whatever -> repeat until StackOverFlowError)

Moreover i'm not sure why those methods have to be there to begin with. Anything that calls them can just call getTileEntity().whatever.

All of that also applies to Block/IMultipart, though to lesser extent as it only has a handful of methods like that, unlike IMultipartTile which has all of them.

Also i suspect having the same names will in some cases trigger this bug when reobfuscating - md-5/SpecialSource#12, which i don't think was ever fully fixed.

My suggestion is to remove all the vanilla methods from IMultipart/IMultipartTile and leave only stuff that needs to be aware of multipart data. The normal methods can be called via getBlock/getTileEntity.

Edit: It's less of a problem for java mods since it prioritizes the implementation in a superclass over the default in the interface.

bdew commented 7 years ago

Regarding the SS issue i mentioned, i did some testing...

It looks like the method names in IMultipartTile don't get obfuscated, but the calls in them do, e.g. looking at getWorld bytecode you see:

public net.minecraft.world.World getWorld();
  Code:
     0: aload_0
     1: invokeinterface #27,  1           // InterfaceMethod getTileEntity:()Lnet/minecraft/tileentity/TileEntity;
     6: invokevirtual #38                 // Method net/minecraft/tileentity/TileEntity.func_145831_w:()Lnet/minecraft/world/World;
     9: areturn

The methods in classes that extend TileEntity and implement IMultipartTile do get obfuscated. More fun ensues when calling those methods from elsewhere whether you get the obfuscated name or not depends on the type of the variable.

This doesn't directly break code - because a call to an unobfuscated name goes to the default method in the interface, which then will call the obfuscated name via getTileEntity. But it does introduce subtle differences in how things work between dev and obfuscated code, and might cause other problems down the line (though i can't think of any specific scenarios right now).

amadornes commented 7 years ago

I think I've renamed all the colliding methods by this point... If I haven't, I imagine someone will yell at me when they run into the issue, but I don't think I've missed any (if they don't have a different name, they have a different signature so it should be alright). I've added a helper method to IMultipartTile which wraps your TE if you don't need to override any of the special methods, so I imagine that'll also help :)