Bernasss12 / BetterEnchantedBooks

Makes it easier to identify different enchantment books.
MIT License
12 stars 9 forks source link

[Crash] Rendering issue with any book #64

Closed FrosteByte closed 5 months ago

FrosteByte commented 5 months ago

When I open any inventory with an enchanted book, minecraft crashes and reports that mode is null. I reverted all mod settings to default and that seemed to work for some time, but now I get the crash again. I have no idea how to reproduce or troubleshoot.

crash-2024-03-26_10.04.16-client.txt

Bernasss12 commented 5 months ago

I cannot think of any way that setting could've end up being null even after recheking the code right now.

Can you tell me what other mods you have installed if any? Also when you say that now you get it again, does it mean it happens everytime you play the game or does it start happening during a gameplay session?

Edit: Would also be nice if you could provide me the mod configuration file to see if that is also broken?

Fourmisain commented 5 months ago

I can see only one* way how this field could be null, that is if loadAndPopulateConfig() is never executed - which would mean that the TitleScreenMixin doesn't execute for some reason - like a canceling Mixin of another mod?

Do you have any mod that changes, replaces or skips the title screen?

Looking through your mod list (which is at the end of the crash log btw, Bernasss), nothing imediately jumps out at me.

I reverted all mod settings to default and that seemed to work for some time, but now I get the crash again.

That would make sense. Resetting the config sets the sorting order to a valid value, until you restart Minecraft, which I assume is "after some time".

I also assume by "I have no idea how to reproduce" you mean this doesn't happen every play session? That could be explained by randomness in the Mixin load order - sometimes our Mixin goes first and does its thing, sometimes the other Mixin goes first and cancels any other action including our Mixin.

Actually, our Mixin has priority 10, which is extremely high, so maybe I'm wrong?


(*) This could also happen if the config is loaded on another thread than the Render thread (where it crashed on) - but I don't see any reason why that would be the case, even in modded scenarios.

There's also the exceedingly unlikely possibility that some other mod tries to modify Better Enchanted Books via MixinSquared... nah.

Fourmisain commented 5 months ago

I created a small debug release which puts a bunch of log statements. That should help confirm the suspicion.

https://github.com/Fourmisain/BetterEnchantedBooks/releases/tag/debug

@FrosteByte Can you run this, make it crash and paste the latest.log file of that crash?

FrosteByte commented 5 months ago

Wow, I'm so sorry I wasn't available while you were working on this! Yeah, I use a lot of mods...

I figured out how to reproduce the crash: launch directly to the world via the quick play options in the default minecraft java launcher. When I launch the game normally and navigate to any world and find/hold/use enchanted books, there are no issues. When I use quick play with any version or config of BEbooks the game runs normally until any enchanted book needs to be rendered and then it crashes.

I ran a dozen tests to check different variables so let me know if you want more info. Now that I know what the issue is and have a bypass I'm good to go in the meantime :)

latest.log

Bernasss12 commented 5 months ago

Well that does it! I'll look for another place to hook the settings loading onto.

I can see only one* way how this field could be null, that is if loadAndPopulateConfig() is never executed - which would mean that the TitleScreenMixin doesn't execute for some reason - like a canceling Mixin of another mod?

You were right on the mark on this one hahaha

I'll see what I can do to make sure it loads correctly everytime!

Fourmisain commented 5 months ago

I figured out how to reproduce the crash: launch directly to the world via the quick play options in the default minecraft java launcher.

Well, damn! I know a lot of mods that rely on the title screen as a kind of late initialization point. Had no idea it was possible to load directly into the game!

Ah, and here's why, this feature was just introduced last month: https://www.minecraft.net/en-us/article/quick-play-coming-java-and-bedrock-edition

Wonder how this actually works, since I couldn't find any command line options (yet).

I'll see what I can do to make sure it loads correctly everytime!

I think the second easiest late init point is using a resource reloader, I added one in Falling Leaves for example. Needs some retesting.

Bernasss12 commented 5 months ago

Yeah I was unaware of it myself, apparently there are some launch arguments that enable it, but I haven't had the time to fully look into more of it myself. --quickPlayPath, --quickPlaySingleplayer, --quickPlayMultiplayer and --quickPlayRealms

But yeah I was thinking of trying to get a loading point directly connected with world loading, since there's no way they can skip that step 😂

Fourmisain commented 5 months ago

But yeah I was thinking of trying to get a loading point directly connected with world loading, since there's no way they can skip that step 😂

Don't forget the config needs to load before the config screen is opened too (which is why title screen was convenient). Pretty sure the resource reloader works for both cases. Gonna run some tests.

Fourmisain commented 5 months ago

Yep, can confirm that a synchronous client resource reload listener (that's a mouth full) works for both the title screen and quick play world load. Which makes sense, since client resources contain translations and textures. It runs on the render thread too (I think that's the "synchronous" part).

Adds a Fabric API dependency though - which shouldn't really be an issue.

Fourmisain commented 5 months ago

I tested some alternatives. The best one I found is a simple mixin into MinecraftClient:

@Inject(method = "<init>", at = @At("RETURN"))
public void lateInit(RunArgs args, CallbackInfo ci) { ... }

This should run after every mod's onInitializeClient and onInitialize, so all enchantments should be registered by then. It runs on the render thread - a bit earlier than the resource reload listener, but that shouldn't matter. It only ever runs once too, so we can get rid of the configsFirstLoaded check.

This should also work for every Minecraft version, which is a big plus compared to the other methods I tested (looking at you, onInitFinished...)

In before Mojang adds a way to add custom enchantments via data packs soon - wouldn't be surprised, they are cooking! 😄

Bernasss12 commented 5 months ago

In before Mojang adds a way to add custom enchantments via data packs soon - wouldn't be surprised, they are cooking! 😄

I do expect that eventually, judging by the way they have been adding data pack functionality 🤣