Moonlight Lib

Moonlight Lib

86M Downloads

Model JSONs being cleared after resource pack reload

embeddedt opened this issue ยท 23 comments

commented

Hi there, a player reported issues with hanging signs' item models not working when they enable the dynamic resources optimization in ModernFix. This patch causes models to load on the fly in-game instead of them all being loaded at once during resource pack reloads. After some digging I found that this issue was introduced by 19559dd as the model JSONs are now cleared from memory as soon as resource pack reloading finishes. Downgrading to Moonlight 2.2.37 fixed the issue (I was testing on 1.19.2, not sure if this applies elsewhere).

Is it possible to mark blockstates & model JSONs as persistent so they don't get cleared, or store the data on disk so that it can be reloaded if necessary? I would hope the JSONs alone are not too large unless there are thousands of them.

commented

Hm ironic that my change was set out to do a similar thing to what that modern fix thing does. I'm not sure how that works or what the best way to handle this is. I can just make the nose persist but I wonder if there's a different option. I guess checking for modern fix with direct integration is a viable option here but I wonder what other resources I should keep. Tags and textures have same issue?

commented

At the moment it only defers model loading; dynamically loading textures is a lot more complicated. And I haven't touched server data at all.

I should also note that this feature isn't on by default in ModernFix, precisely because it can have compatibility issues in scenarios like this.

commented

So I looked the other day at model bakery as found out that forge actually saves all unbaked models and their jsons in the bakery itself. Those would persiste after I clear the resources raw data from my pack. Is that how the mod is working? I'm interested in seeing what exactly is the step that is failing so I can add a solution that ideally is as lightweight as possible (with everycompat those things can be on the thousands)

commented

Also good to know you got such a system because I was working the other day on some sort of lazy model wrapper system that would just load stuff that's needed kind of like multipart does with its own parts but It was full of bugs and I kind of gave up

commented

That's exactly what ModernFix dynamic resources changes; now the unbaked & baked models are loaded/unloaded as they are referenced (which reduces the memory impact of having lots of models to almost nothing). The problem now is that there is no JSON to actually load the model from anymore, so it gets treated as a missing model.

The simplest solution here I can think of, besides persisting the JSON files, is to serialize the resource pack to a ZIP file and store it temporarily on the disk. Then it would't take up any space in memory, but can still be referenced at any time.

commented

Hm might look into that in the future. Reason I made that change was just to free up memory so there's no need for it if block models are loaded lazily so I'll just make it persist. You got an API (or stable enough classes) I can call to see if that option is on? Also sucks that even with lazy loading most stuff has to be loaded anyways as item models need at least one model and they all appear in jei

commented

Also sucks that even with lazy loading most stuff has to be loaded anyways as item models need at least one model and they all appear in jei

They get loaded as you move between pages and are stored in a Guava soft cache, which allows them to be GCed if they're not accessed for long enough or there is enough memory pressure.

You got an API (or stable enough classes) I can call to see if that option is on?

There is a bit of an API but it doesn't currently have a static method one can call to check the status of that option (what a silly oversight...). I will add that for the next release. In the meantime:

/* org.embeddedt.modernfix.core.ModernFixMixinPlugin */
if(ModernFixMixinPlugin.instance.isOptionEnabled("perf.dynamic_resources.SomeDummyClassNameHere")) {
    /* dynamic resources is on */
}

Package name for that is the same on Forge & Fabric and I don't see myself ever changing the name.

commented

Ok will do that ty

commented

so i just leave that code as is or need to change the dummy name?

commented

All added

commented

It doesn't get saved anywhere but for cleanliness/future tracking you could change it to MoonlightCompat or something. Code will behave the same either way.

commented
da402cf37f77ec5ff4a840dcdb7df1ef.1.mp4

Problem is seeming to still persist on the latest version

commented

If you look prior I think he did mention it being not enabled by default

At the moment it only defers model loading; dynamically loading textures is a lot more complicated. And I haven't touched server data at all.

I should also note that this feature isn't on by default in ModernFix, precisely because it can have compatibility issues in scenarios like this.

commented

or maybe theres something im not seeing. im not familiar wht the mod so idk if this option is enabled by default or not, I can just say that on default settings everything works as normal

It doesn't, go into the mod menu or go into your config for it and enable mixin.perf.dynamic_resources to true or look just in case to see if it's on

in the config file it'll be like this

image

commented

seems like the code you gave me didnt work ;_;
it just needs this ModernFixMixinPlugin.instance.isOptionEnabled("perf.dynamic_resources") not with the last bit

commented

or maybe theres something im not seeing. im not familiar wht the mod so idk if this option is enabled by default or not, I can just say that on default settings everything works as normal

commented

it just needs this ModernFixMixinPlugin.instance.isOptionEnabled("perf.dynamic_resources") not with the last bit

I believe the last bit also needs to be present.

commented

ok so i checked and the config check does work. unfortunately i cant test anymore in dev as I'm gettint this mixin error when i turn that feature on. At least this explain why i thought all was working as it was off by default

java.lang.NoSuchMethodError: 'net.minecraft.client.resources.model.UnbakedModel net.minecraft.client.resources.model.ModelBakery$ModelBakerImpl.m_245361_(net.minecraft.resources.ResourceLocation)'

from DynamicBakedModelProvider

commented

i see it useing the obfuscated name while in a non obfuscared environment so thats why its not finding it. somehow

commented

Not sure why deobfuscation is failing for that method specifically... that is not a lambda or anything.

commented
8e0eabe4ea4fa0dae22f10d30e27ab4f.mp4

How it looks on the new latest

commented

The non-modded variants except for the top of the oak one looks fine, but sprites are messed up still for everything else and the modded variants

commented

This should be resolved