IrisShaders / Iris

A modern shaders mod for Minecraft compatible with existing OptiFine shader packs
https://irisshaders.dev
GNU Lesser General Public License v3.0
3.35k stars 639 forks source link

Iris should use Mojang Mappings instead of Yarn Mappings #679

Closed coderbot16 closed 3 years ago

coderbot16 commented 3 years ago

Moving to Mojang mappings ("Mojmap") has some important upsides over using Yarn mappings ("Yarn"):

  1. Consistent name quality. Though there are some weird names in Mojmap, these names aren't incorrect, they're just a bit awkward to work with when they come up. Overall, nearly every name in Mojmap is "acceptably accurate." Meanwhile, a number of names in Yarn (especially in the 1.17 rendering areas) are blatantly incorrect. The absolute worst example was the swapping of bind() and unbind() on Shader, which means that updating mappings will silently introduce bugs into Iris!!!! This is completely unacceptable.

  2. Not closely connected to any specific toolchain. Yarn mappings are very closely tied to the Fabric toolchain, and as a result Iris is completely reliant on the continued updating of Yarn mappings to each new version of the game. If the unpaid volunteers maintaining Yarn decide to disappear, we're screwed! While Mojang could decide to stop updating Mojmap, I find that to be a lot less likely. Mojang mappings have gradually become a community standard. Modern versions of Forge now plan to use them by default, Fabric supports Mojang mappings, Quilt might use them by default, or at the very least it will support them, etc. There's also some interesting possibilities with projects like VanillaGradle which offer an incredibly barebones toolchain for working with Minecraft using Mojang mappings. In short, using Mojang mappings gives us options, whereas Yarn locks us in to the Fabric toolchain.

  3. Instant mapping updates. Mojang releases the new Mojmap file with each new update - it's unnecessary to wait for mappings to update since the updated mappings are shipped alongside updates.

  4. Name stability. Yarn names change periodically as people refine mappings. While this is nice, since the changes often aren't backported, it makes it more annoying to maintain Iris for multiple versions.

Of course, some people will object to this. Here's my responses to some common criticisms:

  1. Mojang mappings are restrictively licensed! Guess what? Minecraft is restrictively licensed too! People seem to forget that it's not an open source game. Using Mojang mappings to create mods for Minecraft doesn't have a huge practical difference from using Yarn mappings to create mods for Minecraft. Either way, we're modifying Mojang's game in a way that they've explicitly allowed us to in the EULA. Some have tried to point out subtleties in Mojang's mappings license that supposedly prove that you cant use them for modding. In reality, Mojang's intent is clear - they want to bring the modding community together by getting everyone to use the same mappings set.

  2. Changing mappings will break open PRs! You're absolutely right, which is why this change won't be made until most big important PRs have been merged and things have stabilized. It will certainly happen before Minecraft 1.18 releases since updating to newer versions of Yarn is dangerous and could easily introduce bugs.

  3. Yarn contributors will no longer be able to contribute to Iris / read Iris source code without seeing Mojmap! This isn't a big deal since the intersection between "people who contribute to Yarn" and "people who contribute to Iris" is basically nil. If they want to read through Iris's code, one option is to import it into their development environment as a dependency, where it will automatically be remapped to Yarn. Again, it doesn't make much sense to inhibit the development of Iris over this small group of people.

  4. Why don't you just contribute fixed names to Yarn? I'd rather spend time developing mods instead of bikeshedding mappings. Migrating to Mojang mappings is a one-time cost, fixing Yarn mappings is a continuous time sink.

If you decide to respond to this issue, please actually read it in full. People who just come here to spam objections without reading my reasoning in full will be removed from the issue tracker. I understand that using Mojang mappings is somewhat controversial in the Fabric community right now, and I hope that people take the time to read and understand my reasoning here. Thanks.

ImperatorStorm commented 3 years ago

Yarn contributors will no longer be able to contribute to Iris / read Iris source code without seeing Mojmap!

On top of this, https://github.com/QuiltMC/rfcs/pull/33

Yarn is incredibly tainted, and the cleanroom hasn't been necessary or effective in a long time. Most likely for the best to switch to Mojmap.

coderbot16 commented 3 years ago

That RFC is a great read, it's certainly a lot more detailed than what I have here. Thanks for linking it. It does a great job at explaining the historical context of Yarn and how things have drastically changed since then. If Quilt is going to make Mojmap a first-class part of their toolchain alongside Quilt Mappings, then that's another factor in favor of Iris using Mojmap.

That thread did bring up an important point - renaming local variables, parameter names, class names, and method names in Iris will be necessary in addition to auto-remapping in order to maintain code quality, and avoid having artifacts of Yarn remaining in Iris's codebase. I'll definitely have to think about how I want to address that.

IMS212 commented 3 years ago

ParchmentMC is a project working on parameter names and javadocs for the official mappings. Loom 0.9 has gained support for it.

ramidzkh commented 3 years ago

Blaze4d has been using mojmap for a few weeks now, and it's worked out fine for us. Since most of our problems aren't on the Minecraft end itself, so we barely have to touch it. The problems start to show when the mojmap are very different from the usual Yarn name which makes total sense (see RenderLayer), so you have mixed method names or nasty names.

However, mojmap was worth the switch for us.

IMS212 commented 3 years ago

While I do have some objections to the naming of many mojang names (for example I’ve practically memorized worldrenderer, but it’s totally different in mojmap), not having to constantly “migrate mappings” seems like a big upside. Overall quite a learning curve, but would likely be better in the long run.

coderbot16 commented 3 years ago

Most of the names seem acceptable to me. But going from Identifier to ResourceLocation is probably my least favorite migration.

This kind of thing will be annoying to clean up (manually doing "layer" -> "type" / "renderType"):

-   private boolean isTransparent(RenderLayer layer) {
-       if (layer == RenderLayer.getWaterMask()) {
+   private boolean isTransparent(RenderType layer) {
+       if (layer == RenderType.waterMask()) {

This will definitely have a sizable upfront time cost. I'm hopeful that the long term benefits will outweigh that.

IMS212 commented 3 years ago

After a few experiences with loom, I do not trust migrateMappings at all, so I’m working on an intermediary mojmap version of Iris manually. It’s definitely time consuming, but hopefully doing it sooner rather than later will help us reach a decision faster based on looking at the final code.

coderbot16 commented 3 years ago

Personally I think the most reasonable approach would be to use migrateMappings and then manually check the changes. Manually remapping and then checking could be very time consuming since Iris is about 17k lines of code.

IMS212 commented 3 years ago

Fair, however migrateMappings does not work for mixin injects or other mixin related functions, so a lot of the benefit is lost there.

coderbot16 commented 3 years ago

Well, you'd be migrating those manually either way. But migrateMappings can be very helpful for "conventional" code.

Genau6502 commented 3 years ago

So I'd like to chime in here as a dev, but not an iris contributor. Until Mojmap becomes the standard, I could see many fabric authors deciding against contributing to iris, simple on the grounds of unfamiliar names. I know there are loads of tools to convert between them, but still - spending double the time on the same code is an issue. Of course, this would be solved by more and more mods switching over to mojamp, which I can't see happen on the current fabric toolchain. We're yet to see what Quilt do, but it's likely both Mojmap and QM will be used.

I'm half and half on this change, and it's impossible to deny that a lot of the yarn names are sub-standard, especially in rendering. Go with what's best for iris, which looks in this case to be mojmap

sylv256 commented 3 years ago

So I'd like to chime in here as a dev, but not an iris contributor. Until Mojmap becomes the standard, I could see many fabric authors deciding against contributing to iris, simple on the grounds of unfamiliar names. I know there are loads of tools to convert between them, but still - spending double the time on the same code is an issue. Of course, this would be solved by more and more mods switching over to mojamp, which I can't see happen on the current fabric toolchain. We're yet to see what Quilt do, but it's likely both Mojmap and QM will be used.

I'm half and half on this change, and it's impossible to deny that a lot of the yarn names are sub-standard, especially in rendering. Go with what's best for iris, which looks in this case to be mojmap

This is currently happening on Quilt.

IMS212 commented 3 years ago

I have a WIP conversion here: https://github.com/IMS212/Iris/tree/mojmap

IMS212 commented 3 years ago

I believe we should switch to using ParchmentMC (Mojang mappings + optional parameter names and javadocs), however this is only supported in Loom 0.9, thus we would have to bump the minimum java version requirement to 16. The best course may be only having Parchment for 1.17+, as it's simply cosmetic for ease of use.

coderbot16 commented 3 years ago

Since ParchmentMC is mostly cosmetic for development instead of being critical for compilation, it doesn't necessarily have to happen at the same time as the mojmap migration.

coderbot16 commented 3 years ago

Iris now uses Mojang Mappings on 1.16. The 1.17 & 1.18 branches still use Yarn mappings, but the Mojang Mappings change will be ported forward soon.

IMS212 commented 3 years ago

This has since been ported to 1.17 and 1.18, can we close this?