Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1.20.1] Add mappings for AbstractNbtList.add/remove #3837

Open
wants to merge 4 commits into
base: 1.20.1
Choose a base branch
from

Conversation

NerjalNosk
Copy link

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 NerjalNosk requested a review from a team as a code owner April 6, 2024 11:54
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, the mapping change looks reasonable. I would sort the toolchain stuff out as a diffrent thing.

I see this is for 1.20.1 which is fine, just want to double check that these have been named in the main branch?

build.gradle Outdated Show resolved Hide resolved
@modmuss50 modmuss50 changed the title Nbt fix [1.20.1] Add mappings for AbstractNbtList.add/remove Apr 6, 2024
@Shnupbups
Copy link
Contributor

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

@Shnupbups
Copy link
Contributor

Shnupbups commented Apr 6, 2024

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...

@NerjalNosk
Copy link
Author

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.

Copy link
Author

@NerjalNosk NerjalNosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowkey wondering what's the hold-up here

@modmuss50
Copy link
Member

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.

@liach
Copy link
Contributor

liach commented May 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants