MinecraftForge / SrgUtils

A library for working with SRG, and other mapping variants.
GNU Lesser General Public License v2.1
12 stars 16 forks source link

Improve the IMappingFile.chain() method #9

Closed TheGlitch76 closed 1 year ago

TheGlitch76 commented 2 years ago

This new method has a clearly defined merge strategy, and properly preserves parameters and metadata.

This is needed for VanillaGradle, where Yarn, Parchment, and Quilt Mappings all have metadata and parameters, and VG uses the chain function to avoid remapping multiple times.

Also, the code is a little ugly (and based on the public API because it was prototyped outside of SRGUtils), but my brain is too fried to figure out a better way to structure it--if you have any ideas, please let me know.

LexManos commented 2 years ago

You seem to have a fundamental misunderstanding of the word chain. Chain links things together A->B->C What you want to do is introduce a new concept of 'merging' where you can do [A: Foo] + [B: Bar] = [A: Foo, B: Bar] Changing the concept of an existing function is not the way to do this. It's very much explicitly designed to be chaining, not merging.

TheGlitch76 commented 2 years ago

Some documentation would have been nice to clarify that.

I can change this to a new merge method, but it might be a couple days before I'm in front of a computer again.

zml2008 commented 2 years ago

chaining sounds like a subset of merging imo. in what sort of cases would you want to chain but not merge?

what about adding a new merge operation, with a MergeStrategy, where there are options of:

not sure if there's any other options I can think of, so an IMappingSet.merge(other) and IMappingSet.chain(other) would suit our usecases just as well.

LexManos commented 2 years ago

Yes we could have better documentation, but the word is clear in my mind. As for what cases I would want to chain and not merge, every single use case in Forge/FG. We don't care about adding javadocs or anything like that too our mappings as that's taken care of elseware. We just need too know mapped <-> srg <-> obf for fields/methods/classes.

SizableShrimp commented 1 year ago

Is this still necessary or is this covered by #15?

marchermans commented 1 year ago

Has been superseded by #15 IMHO

zml2008 commented 1 year ago

I agree too - the merge operation does what we were wanting in VG originally

SizableShrimp commented 1 year ago

Cool, I shall close this then.