SuperMartijn642 / Rechiseled

12 stars 13 forks source link

[Bug] newest update breaks textures #36

Closed Leclowndu93150 closed 1 year ago

Leclowndu93150 commented 1 year ago

Version Info

SuperMartijn642 commented 1 year ago

It works fine with just Rechiseled.

Are you using any other mods? If so, could you narrow down which mods are needed to reproduce the issue?

Leclowndu93150 commented 1 year ago

https://pastebin.com/HjHRQZrr here

i can even send you the modpack if you dont mind

embeddedt commented 1 year ago

It's probably ModernFix. This mixin will not work correctly unless faster texture loading is disabled in ModernFix's config, as getBasicSpriteInfos is changed to read and cache the full image instead of using PngSizeInfo/PngInfo, which will bypass your change. This is similar to how Mojang handles textures in 1.19.3 and later.

Is it possible to avoid mixing into the texture atlas in Fusion? PngSizeInfo really slows down texture loading, so launch time will be impacted if I need to disable my optimization here when Fusion is present.

I think it should also be safe to do the replacement when getLoadedSprites returns instead of within getBasicSpriteInfos.

SuperMartijn642 commented 1 year ago

changed to read and cache the full image

Ah I was already confused when I saw vanilla loaded each texture just to get the size and then discarded it lol

The problem I have now is that I need access to the texture metadata so I can load Fusion's metadata. Vanilla already loads the metadata for each texture which then gets cached in Resource. I would preferably not have to re-request the resource, since that would lead to me having to read the metadata file from disk a second time.

I can move it to getLoadedSprites and re-request the resource and load the metadata again, but I don't know if that's worth it performance wise. Maybe you know if that's worth it (or if there's another solution)?

embeddedt commented 1 year ago

Ah I was already confused when I saw vanilla loaded each texture just to get the size and then discarded it lol

Indeed, the number of puzzling design choices like this is incredible.

I can move it to getLoadedSprites and re-request the resource and load the metadata again, but I don't know if that's worth it performance wise. Maybe you know if that's worth it (or if there's another solution)?

I think that is the cleanest solution. The only other approach I see would be for me to modify my mixin so that it doesn't completely replace getBasicSpriteInfos, but that requires injecting into a lambda which has generally been annoying to do in a way that works across modloaders and MC versions without a lot of fiddling.

You can parallelize it using Mojang's Util.backgroundExecutor() which should hopefully make it fast enough; generally speaking more time seems to be spent reading the full contents of resources than opening them.

SuperMartijn642 commented 1 year ago

Should be fixed now in version 1.0.1 of Fusion. Thanks for the help!

embeddedt commented 1 year ago

I think you applied the compatibility to a few too many versions. As of 1.19.3 Mojang finally fixed the texture system themselves so ModernFix no longer patches it, and your normal mixin should work fine. Someone reported https://github.com/embeddedt/ModernFix/issues/140 which seems to be caused by applying the ModernFix-specific mixin.

SuperMartijn642 commented 1 year ago

Not sure what's going on, I only added the ModernFix stuff to 1.19.2 and below. It isn't even present on the 1.20 branch 🤔

embeddedt commented 1 year ago

Weird, I will ask them to check they installed the correct version.

Leclowndu93150 commented 1 year ago

im fascinated by all this chinese