yarn icon indicating copy to clipboard operation
yarn copied to clipboard

[1.20.1] Add mappings for AbstractNbtList.add/remove

Open NerjalNosk opened this issue 10 months ago • 5 comments

Fixes NBT lists add and remove methods getting mixed up with their synthetic obfuscation.

Ensures Gradle to pick JVM 17 when the local default is set to another version (prevents Enigma from running properly)

NerjalNosk avatar Apr 06 '24 11:04 NerjalNosk

We usually don't add mappings to older versions of the game...

Shnupbups avatar Apr 06 '24 14:04 Shnupbups

More importantly, I don't think it's necessary (or desirable) to map these synthetic bridge methods. I believe in practice, in an actual dev environment, the proper names should already show up just fine. The only place where the intermediaries are shown is in Enigma, in places where the methods are called. This is a bug with Enigma, but we probably shouldn't band-aid that bug with unnecessary mappings. And actually, from my testing, after making the requested change to apply the mappings the intermediaries (method_10531 add instead of add add) it doesn't even actually band-aid the Enigma issue anyway, instead doing nothing...

Shnupbups avatar Apr 06 '24 15:04 Shnupbups

More importantly, I don't think it's necessary (or desirable) to map these synthetic bridge methods.

Well as meaningless as it may seem, I do need them to be mapped, in order to extend the class, all so to provide a mirror instance of another, albeit with some form of "listeners", in order to make each changes be reflected on another object within the mod I am currently working on.

And actually, from my testing, after making the requested change to apply the mappings the intermediaries (method_10531 add instead of add add) it doesn't even actually band-aid the Enigma issue anyway, instead doing nothing...

Honestly I do not know enough of dealing with mappings to know really what should or shouldn't be done, I am only trying to do what feels the most logical.

Also, additional point, the original mapping was only add, which Enigma first changed to add add upon trying to fix the issue on the NbtByteArray and NbtLongArray's ends (simply renaming the method_10531 to add, and respectively for method_10536 to remove). I only then manually made the other change as modmuss's comment on that matter made me think it might not be the right thing to do.

NerjalNosk avatar Apr 06 '24 17:04 NerjalNosk

lowkey wondering what's the hold-up here

Sorry, this needs manually testing IMO to make sure its the correct change. As its for an older minecraft version its lower on the priority list, not helped by the fact I have been busy with 1.20.5 stuff and my laptop broke. Sorry again, ill try and get to this soon.

modmuss50 avatar Apr 22 '24 13:04 modmuss50

I think the existence of these mappings are solely for naming the parameters; these synthetic bridge methods should have their names auto-populated by our tools (enigma, stitch/loom) already. I recall enigma has some problem with populating names while stitch/loom works totally fine.

liach avatar May 05 '24 12:05 liach