BakedModelManagerMixin.getModel mixin overwrites Forge's equivalent patch

[SquidDev]

Describe the bug

Tested with:

• Minecraft 1.20.1
• Forge 47.2.20
• Forgified Fabric API 0.91.0+1.10.4+1.20.1
• Connector 1.0.0-beta.32+1.20.1

---

Fabric API (and thus Forgified Fabric API) adds a method to ModelManager/BakedModelManagerMixin to get the model from a ResourceLocation/Identifier:

ForgifiedFabricAPI/fabric-model-loading-api-v1/src/client/java/net/fabricmc/fabric/mixin/client/model/loading/BakedModelManagerMixin.java

Lines 50 to 53 in 641809b

	@Override
	public BakedModel getModel(Identifier id) {
	return models.get(id);
	}

However, this shadows a similar method within Forge:

https://github.com/MinecraftForge/MinecraftForge/blob/45b00b000411d9c7b2e090359b051dc60a73628a/patches/minecraft/net/minecraft/client/resources/model/ModelManager.java.patch#L23-L25

While this would be fine, the two methods have different implementations - the Forge method is guaranteed to not return null, while the Fabric one does. This means that any mod which expects this method to not return null, will crash if the model cannot be loaded.

This has been seen in cc-tweaked/CC-Tweaked#1626, where (for unrelated reasons) a model could not be loaded, which caused a crash when trying to use that model.

Logs

No response

Additional context

Just to confirm that which method implementation was present in the class, I enabled mixin dumping in an environment with just Forgified Fabric API installed:

$ javap -c net.minecraft.client.resources.model.ModelManager | grep --after=10 getModel\(
  public net.minecraft.client.resources.model.BakedModel getModel(net.minecraft.resources.ResourceLocation);
    Code:
       0: aload_0
       1: getfield      #92                 // Field f_119397_:Ljava/util/Map;
       4: aload_1
       5: invokeinterface #421,  2          // InterfaceMethod java/util/Map.get:(Ljava/lang/Object;)Ljava/lang/Object;
      10: checkcast     #126                // class net/minecraft/client/resources/model/BakedModel
      13: areturn

I wonder if it's worth having something as part of the build process which ensures that this does not happen again. Unfortunately I don't believe any of mixin's debug options will catch this :(

[Su5eD]

Sorry about the confusion, it should be fixed now.

[SquidDev]

Thanks!