Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Crash] Rendering issue with any book #64

Closed
FrosteByte opened this issue Mar 26, 2024 · 11 comments · Fixed by #65
Closed

[Crash] Rendering issue with any book #64

FrosteByte opened this issue Mar 26, 2024 · 11 comments · Fixed by #65
Assignees
Labels
accepted Issue was confirmed and will be fixed. bug Something isn't working crash

Comments

@FrosteByte
Copy link

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

@FrosteByte FrosteByte added bug Something isn't working crash need-validation labels Mar 26, 2024
@Bernasss12
Copy link
Owner

Bernasss12 commented Mar 26, 2024

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?

@Bernasss12 Bernasss12 added accepted Issue was confirmed and will be fixed. and removed need-validation labels Mar 26, 2024
@Bernasss12 Bernasss12 self-assigned this Mar 26, 2024
Fourmisain added a commit to Fourmisain/BetterEnchantedBooks that referenced this issue Mar 27, 2024
@Fourmisain
Copy link
Contributor

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
Copy link
Contributor

Fourmisain commented Mar 27, 2024

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Contributor

Fourmisain commented Mar 27, 2024

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
Copy link
Contributor

Fourmisain commented Mar 27, 2024

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 added a commit that referenced this issue Mar 28, 2024
Fix #64, update build, compatibility with 1.20-1.20.4
@Bernasss12
Copy link
Owner

Bernasss12 commented Mar 28, 2024

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 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue was confirmed and will be fixed. bug Something isn't working crash
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants