Fabric API

Fabric API

108M Downloads

`ScreenEvents.afterRender`'s screen null check fails when opening the disconnect screen

Minenash opened this issue ยท 22 comments

commented

Running the following in my screen's render method results in the null check failing, and crashing the game.

client.getInstance().world.disconnect();
client.getInstance().disconnect(new SaveLevelScreen(new TranslatableText("menu.savingLevel")));

My screen's code and the code that calls it
Crash Log

Though, if I strip everything and just have the code in the code block in the render, and nothing else, it still crashes. If I run the code instead of opening the screen, it doesn't crash.

The code in the quit method is practically identical to the code in the vanilla's exit to main menu button

commented

Someone should check if this still happens in 1.18+ given that the code was changed a bit iirc?

commented

The check should never occur except in cases of race condition in 1.17+.

commented
java.lang.NullPointerException: Screen cannot be null
	at java.base/java.util.Objects.requireNonNull(Objects.java:235)
	at net.fabricmc.fabric.api.client.screen.v1.ScreenEvents.afterRender(ScreenEvents.java:135)
	at net.minecraft.client.render.GameRenderer.handler$zli000$fabric-screen-api-v1$onAfterRenderScreen(GameRenderer.java:3108)
	at net.minecraft.client.render.GameRenderer.render(GameRenderer.java:929)
	at net.minecraft.client.MinecraftClient.render(MinecraftClient.java:1193)
	at net.minecraft.client.MinecraftClient.run(MinecraftClient.java:781)
	at net.minecraft.client.main.Main.main(Main.java:244)
	at net.minecraft.client.main.Main.main(Main.java:51)
	at net.fabricmc.loader.impl.game.minecraft.MinecraftGameProvider.launch(MinecraftGameProvider.java:462)
	at net.fabricmc.loader.impl.launch.knot.Knot.launch(Knot.java:74)
	at net.fabricmc.loader.impl.launch.knot.KnotClient.main(KnotClient.java:23)
	at net.fabricmc.devlaunchinjector.Main.main(Main.java:86)
commented

This is still happening in 1.19+, maybe re-open the issue?

commented

That is just not supposed to happen - you should at least provide more context.

commented

It's the exact same circumstance as Mineash

commented

Maybe you could move your logic somewhere else than the render screen method?

commented

I mean, when you exit the world, doesn't it set the screen to null? so you shouldn't touch the world etc. in render method, which is part of a frame in fps. You are supposed to do those logic in client ticks.

commented

You call quit in render, after which the screen supposingly should be still there?

I think vanilla calls screen quit in button handling. Button handling I recall is detected in a loop in the tick code. Consider moving your quit code there?

commented

For fixing the mod, I can try that.

After more testing (posted on the discord), I had found out that the current screen was never null during this, but for some reason, the beforeRender didn't get ran once, so therefore the afterRender got called twice in a row.

commented

I mean, when you exit the world, doesn't it set the screen to null?

Not that I could find. The disconnect method changes the screen into a save or progress screen.

Button handling I recall is detected in a loop in the tick code. Consider moving your quit code there?

How would you recommend I do this? I'm using the screen as a guaranteed sync to the very next render that's not the menu screen so it usable for the screenshot.

Even though I can get my mod to work with this, it is still slightly concerning that a inject is just not running in a single instant while another inject that has the same target runs fine.

commented

Like use an atomic boolean that is set to true in render method when the screen needs to quit. In tick method you detect if the boolean is true and quit accordingly if it is.

commented

Like use an atomic boolean that is set to true in render method when the screen needs to quit. In tick method you detect if the boolean is true and quit accordingly if it is.

If that is the case, it may be worth adding a line to the openScreen and reset screen methods noting there may be failure if you open a screen during the "render phase"

commented

Only right after a disconnect() though, just plain opening screens works fine

commented

I think what happens is when you go to disconnect on the client, what will happen is the client will set the new screen and then call render(Z) until netty has terminated and the integrated server has stopped. What this may do is while you are in a render phase, you send up starting another render phase while you are rendering.

Might be solvable if we track the currently open screen via some sort of stack?

commented

Yeah you should probably not touch client thread logic in render method

commented

you send up starting another render phase while you are rendering.

Hmm, it's possible, though the original screen's render method fully finishes executive before the null pointer

commented

So after afterRender is called, the tracking field for the currently open screen is set to null.

So you have this kind of call stack

renderPhase {
    renderScreen {
        disconnect {
            renderPhase {
                renderScreen()
                setToNull() <== not null
            }
        }
    }
    afterRender() <= screen is null
    setToNull() 
}
commented

Why wouldn't this happen with a normal open screen method though?

commented

Normally you are not nesting render phases with vanilla screens, and minecraft calls it's open/close methods on the tick loop, which is after the render phase.

commented

Since the field to track the current screen is static, you may end up having that field become null before the root render phase calls the after event

commented

Hmm, I guess the question is, since render nesting somehow works in vanilla, should the field be turned into a stack, or is this such a fringe case, and also not the proper way to handle it in general, that it's okay to break vanilla allowed behavior