LivingEntityRenderer.addLayer called on wrong thread

[TheDGOfficial]

Forgive me if this not an EntityCulling issue, i'm only opening it here since it looks like the CullTask.run is the start of the stacktrace and I assume it runs on an another thread than the game thread and so might be the culprit, but there's multiple mods in stacktrace so if this not an EntityCulling issue feel free to close this.

[10:57:19] [CullThread/ERROR]: LivingEntityRenderer.addLayer called on wrong thread
java.lang.Exception
	at knot//net.minecraft.client.renderer.entity.LivingEntityRenderer.wrapMethod$dkk000$modernfix$handleOffThreadLayerAdd(LivingEntityRenderer.java:2529)
	at knot//net.minecraft.client.renderer.entity.LivingEntityRenderer.addLayer(LivingEntityRenderer.java)
	at knot//net.minecraft.client.renderer.entity.HorseRenderer.<init>(HorseRenderer.java:40)
	at knot//org.embeddedt.modernfix.entity.EntityRendererMap$RenderConstructor.load(EntityRendererMap.java:40)
	at knot//org.embeddedt.modernfix.entity.EntityRendererMap$RenderConstructor.load(EntityRendererMap.java:31)
	at knot//com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3556)
	at knot//com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2307)
	at knot//com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2180)
	at knot//com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2071)
	at knot//com.google.common.cache.LocalCache.get(LocalCache.java:3985)
	at knot//com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4006)
	at knot//com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4946)
	at knot//org.embeddedt.modernfix.entity.EntityRendererMap.get(EntityRendererMap.java:74)
	at knot//org.embeddedt.modernfix.entity.EntityRendererMap.get(EntityRendererMap.java:20)
	at knot//net.minecraft.client.renderer.entity.EntityRenderDispatcher.bo$getOtherRenderer(EntityRenderDispatcher.java:549)
	at knot//net.minecraft.client.renderer.entity.EntityRenderDispatcher.getRenderer(EntityRenderDispatcher.java:535)
	at knot//dev.tr7zw.entityculling.NMSCullingHelper.getCullingBox(NMSCullingHelper.java:45)
	at knot//dev.tr7zw.entityculling.CullTask.cullEntities(CullTask.java:126)
	at knot//dev.tr7zw.entityculling.CullTask.run(CullTask.java:79)
	at java.base/java.lang.Thread.run(Thread.java:1516)
[10:57:19] [CullThread/INFO]: Loaded entity minecraft:horse

bo$getOtherRenderer is from BadOptimizations:

https://github.com/imthosea/BadOptimizations/blob/5de4a3ad4299909178d8995dc0bc80626be48d44/common/src/main/java/me/thosea/badoptimizations/mixin/renderer/entity/MixinEntityRendererDispatcher.java#L39-L51

The handleOffThreadLayerAdd is from ModernFix:

https://github.com/coredex-source/ModernFix---mVUS/blob/420d70f5fb501a5a1ac6cee2ca096c2b91a52a9a/src/main/java/org/embeddedt/modernfix/common/mixin/safety/LivingEntityRendererMixin.java#L22-L34

The rest seems vanilla code and EntityCulling code.

The relevant versions are, MC 26.1.2, EntityCulling 1.10.5, ModernFix mVUS 5.27.19, BadOptimizations 2.4.1.

The error prints a few times right after you join a world and otherwise does not occur during gameplay again per my testing. It could be related to the ModernFix option "Dynamic entity renderers" but I have not tested this.

It seems like a possible fix would be to just do what ModernFix does inside within EntityCulling - doing a isSameThread check and calling mc.schedule otherwise, as seen in the ModernFix mixin.

To clarify, this does not cause a crash and I just wanted to report in case it wasn't known. It seems ModernFix just prints the stacktrace so mod authors can fix it in their side instead of relying of ModernFix's mixin fix.

[tr7zw]

EntityCulling intentionally runs Async not on the main thread, and the only thing it calls here is EntityRenderDispatcher.getRenderer which usually is thread safe and should not cause any side effects. Modernfix seems to hijack the entire Rendermap that gets called, lazy creates the renderers which in turn then async calls logic that they defined to not be allowed to be called async. (Ironically in the comment saying that its broken mods doing it, but in this case its Modernfix itself). I can probably work around this crash by extracting relevant information sync when modernfix is loaded, but that will have a slight negative impact on performance, which is a bit counterproductive when the goal is to get performance improving mods running.
I'm a bit unsure what Modernfix is trying to do here/why it replaces that map with an apparently lazy loading implementation for no apparent reason. Is it to save memory? Startup time?

[tr7zw]

I guess I'll do some more reworkings to dummy this down. It will have a tiny impact on performance, but no can do.

[TheDGOfficial]

If it is normally safe and not cause any side effects without ModernFix, and since ModernFix fixes it via a mixin, i guess EntityCulling would not need changing anything, except if we want to get rid of the debug exception stack trace ModernFix prints when fixing a call this way to be sync. I originally reported this since I thought ModernFix was trying to report a stacktrace for a thread-safety issue that would be unnoticed normally in main game - but if it's verified to be safe to call EntityRenderDispatcher.getRenderer (ModernFix assumes the mod is broken for calling addLayer on the wrong thread, but if ModernFix itself causes addLayer to be called when calling getRenderer, then that's their issue as you said), then it can stay as is. Let me know if you want me to close the issue in that case. To clarify it does not cause a crash and it seems to only happen a few times right after joining a world and does not spam, so always forcing it to be a sync call (even if addLayer is not going to be called) might just make performance worse for no reason as you said.

Could it be a race where EntityCulling thread calls getRenderer before the game calls it for the first time causing the addLayer code path to run on the culling thread? I'd assume without ModernFix dynamic entity renderer the layers are already loaded by the moment culling task runs.

I assume this can't really be fixed without always forcing sync as you said or making the game render the first-seen entity types for at least 1 frame (to make it so layer is initialized on game thread) before checking if they should be culled in the culling thread.

As for the reason of ModernFix doing this, I looked at the Dynamic entity renderers option in ModernFix and the description reads "Constructs entity models the first time they are seen instead of during launch.", so i guess it speeds up startup and reduces memory usage for entities that you would otherwise not see in the world since their models would not be created till they appear in world.

If you want, I can report this to ModernFix repository as well, maybe they can tell us what EntityCulling is supposed to do in this case code-wise to not make ModernFix print this stacktrace.