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

Reconsider package structure #550

Open
LambdAurora opened this issue Feb 4, 2024 · 8 comments
Open

Reconsider package structure #550

LambdAurora opened this issue Feb 4, 2024 · 8 comments
Labels
discussion changes that need discussion before being implemented s: large PRs with more than 700 lines t: refactor proposes a refactor

Comments

@LambdAurora
Copy link
Contributor

For quite a long time QM has followed Yarn's package structure and came up with its own rules for the package structure.

Lately I've been looking a bit more closely at the Mojang mappings, and given the recent refactors on Mojang's part to increase the data-drivability of Minecraft, I'm finally starting to understand better the Mojang package structure.

So now we probably should ask ourselves, is the QM package structure still making sense given the refactors?

Advantages of using Mojang's package structure

  • Ability to map the already existing package-info.class files;
  • Not have to wonder where to put classes when mapping;
  • Mappings are easier to juggle with mojmap, making it easier to understand for modders if accustomed with mojmap;
  • Package structure might make more sense given the direction taken by Mojang for the future of the game.

Disadvantages of using Mojang's package structure

  • We constrain ourselves to the Mojang package structure.

Unresolved Questions

  • Do we use the exact same package names or do we adapt to our naming convention?
  • Do we reconsider some of our naming convention in the process?
  • What will be the public reception?
@LambdAurora LambdAurora added t: refactor proposes a refactor discussion changes that need discussion before being implemented s: large PRs with more than 700 lines labels Feb 4, 2024
@FirstMegaGame4
Copy link
Contributor

FirstMegaGame4 commented Feb 4, 2024

I agree that using the Yarn's package structure is less good than using Mojang's one because of the recent... Codecify? of minecraft code. But I do not think that following exactly the same package structure as Mojang is a good idea. IMO, Quilt Mappings should be able to change some parts of the Mojang package structure to use it's own conventions.

As for the the public reception, I do think that this change should be applied for Quilt Mappings 2.0. People currently knows that this version will bring a lot of changes to these mappings, so I think that it's the best version to consider a package structure refactor.

@Pyrofab
Copy link
Member

Pyrofab commented Feb 4, 2024

To be perfectly honest, as a modder I do not look at package names much. That said, I believe following mojang's package structure would be a good idea for at least 2 reasons :

  • tracking mojang's intent more closely
  • eliminating concerns about package-private elements

For consistency, I would say that packages should be renamed when they are about a concept that has been remapped (e.g. world).

@LambdAurora
Copy link
Contributor Author

eliminating concerns about package-private elements

Having the same package structure as Mojang also heavily simplifies the usage of QM on NeoForge for those who dare want to do that as NeoForge is using Mojmap as the runtime mapping meaning there's no visibility remap step.

@ix0rai
Copy link
Member

ix0rai commented Feb 4, 2024

I support this move, just wanted to chime in with a few points:

  • if we choose not to copy mojang's names (imo we should continue using our own) what do we do about net/minecraft/unmapped? we would need to establish some sort of manual map from moj package names to QM package names, defaulting to moj names for new, unmapped, packages
  • whether we reconsider our conventions can be considered once we start refactoring. it would be fun to have the whole team on an enigma multiplayer doing the work together!
  • as pyro said, I don't think any mappings users will be opposed

@OroArmor
Copy link
Member

OroArmor commented Feb 4, 2024

I mostly like the idea, i would need to see the mappings with the new structure to really get a feel for it.

@ix0rai
Copy link
Member

ix0rai commented Mar 5, 2024

so. plan time
this will require a few things on the tech side to assure validity. in terms of setting up packages, my proposal is that:

  • we establish a JSON file (like simple_type_field_names) that acts as a map of mojang package names to QM versions. all classes stay organised the same way as mojang, but we get to rename packages.
  • we build an enigma plugin that disallows changing package names from enigma, redirecting mappers to said file. this plugin also automatically fills out the JSON file as new mojang packages are added, simply adding an entry of "x/y/z": "x/y/z" whenever a package is missing. this plugin also handles automatically moving new classes to the proper package as dictated by mojmap.
    • this will require some new API in enigma: plugins that add checks for the validity of a rename. should be fairly simple to implement
  • we automatically download a mojmap JAR as part of QM setup. analysing this this will allow the plugin to get all those fancy features that were just described.

@OroArmor
Copy link
Member

OroArmor commented Mar 5, 2024

We will definitely want a plugin that auto-moves classes to the correct package. As for already named classes, it should be fairly simple and probably not need a full plugin, but definitely a bunch of code to read the files and move them to the correct directories. Since it's such a one-off thing I don't think it'll be that useful to keep around.

@ix0rai
Copy link
Member

ix0rai commented Mar 5, 2024

update since I'm incredibly wise. instead of building new enigma APIs, we simply automatically correct packages on everything using the power of dynamic name proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion changes that need discussion before being implemented s: large PRs with more than 700 lines t: refactor proposes a refactor
Projects
None yet
Development

No branches or pull requests

5 participants