MoreMcmeta / core

Animate almost any Minecraft texture with more options. Texture configuration API. 1.16-1.20
GNU Lesser General Public License v3.0
27 stars 6 forks source link

Incompatibility with Continuity #3

Closed XanderCreates closed 2 years ago

XanderCreates commented 2 years ago

There appears to be an incompatibility with MoreMcMeta and Continuity that prevents Continuity from being able to load their built in resource packs properly

To reproduce Steps to reproduce the behavior:

  1. Have MoreMcMeta
  2. Install Continuity
  3. Load the Built-in Packs from Continuity
  4. Latest.log Snippet whilst loading said packs

Expected behavior Connected Textures from the built in packs Example: 2021-10-13_22 16 14

Screenshots or videos See Continuity Issue 15 and take a look at the first post

Which Minecraft versions does this bug affect? 1.17.1

Mod loader Which mod loaders does this bug affect? Fabric

Mods list image

Resource pack They're Built-in, So here is a screenshot of the loaded packs image

Additional context The Errors in the Log Snippet vanished once disabling/removing MoreMcMeta

soir20 commented 2 years ago

I don't have information about this currently, but I want to note that I've seen this. Due to a death in the family and a lot of other things going on, I haven't had time to look into the issue. Hopefully, I will be able to investigate it next week.

soir20 commented 2 years ago

I have finally been able to identify the cause of this. Continuity is using a mixin on Minecraft's resource manager. In short, mixins alter Minecraft's code when the game runs, and these are usually tricky to debug. The seemingly-gibberish file names in the logs you are seeing show up because Continuity's mixin that transforms file names is not being applied to MoreMcmeta's custom resource manager.

Technical explanation: Continuity's ReloadableResourceManagerImplExtension interface is applied to MoreMcmeta's SizeSwappingResourceManager, preventing the SimpleReloadableResourceManager (AKA ReloadableResourceManagerImpl) from receiving redirects/mappings. Therefore, Continuity's sprite identifier mappings are disabled, and the hashed file names are not found.

I'm deciding how I want to implement a fix.

PepperCode1 commented 2 years ago

Hello. Please let know if something can be done on Continuity's part to make this easier to resolve as well. I know the redirect system I'm using isn't great, but it's the only way I know of to do it that prevents invalid identifiers and textures being outside the textures/ directory.

soir20 commented 2 years ago

Hey, thanks for reaching out @PepperCode1.

I've been experimenting with different options for improving compatibility with mods that use mixins on the resource manager. I have Continuity and MoreMcmeta working together in my dev environment, but the methods I've been using are extremely fragile or create resource leaks and other issues. The other option is to use mixins/coremodding, but that obviously creates more compatibility concerns. I've been trying to look for other options instead of requiring other mods to change their mixins when issues like this come up, but I've mostly exhausted the options besides coremodding over the last few days.

It's not really the redirects themselves that cause the problem. Continuity's redirects are added to the map of MoreMcmeta's resource manager wrapper. But MoreMcmeta's wrapper for the resource manager delegates to Minecraft's original one--the wrapper doesn't call its own getResource method. Therefore, the original manager has no knowledge of the redirects.

I believe the simplest fix on Continuity's end would be to make the map you use to store the redirects static so that they are shared between both resource managers. Another option would be to remove the mixin and create a similar wrapper for the resource manager, replacing the existing manager with reflection. The wrappers would then simply delegate to each other and Minecraft's original manager.

I just searched through GitHub for other mods that mixin to the resource manager. There are several, but I don't think they would conflict with MoreMcmeta's wrapper if I kept that. The risk is that I might have to revisit this again if MoreMcmeta doesn't change and something similar pops up in the future.

Please let me know if you want to apply a fix on Continuity's end. I can submit a PR with one of those solutions if that would be helpful.

soir20 commented 2 years ago

Closed as an alternative fix I found for this has been merged--I will reply to this thread again once a new version has been formally released.

soir20 commented 2 years ago

Version 1.17.1-3.0.1, which contains a fix for this issue, has been released and approved for Forge and Fabric on CurseForge and Modrinth.

Changelog for this version:

Corresponding versions for 1.16.5 will be released Wednesday or Thursday pending final testing.