ImmediatelyFast

ImmediatelyFast

55M Downloads

[1.21.5] tryForceDrawHudBuffers breaks rendering

thexaero opened this issue ยท 15 comments

commented

In other words, the #263 issue is back on 1.21.5.
By the time tryForceDrawHudBuffers is triggered in MixinGlResourceManager.checkForDrawCallWhileBatching, the vertex data is already uploaded to the VBO but is then replaced by whatever in hudBatchingVertexConsumers uses the same vertex format and therefore uploads to the same "immediate" VBO.
Like with #263, I can work around this by rendering a 0x0 square before rendering anything important but it's still a huge issue.
You might want to catch direct renders in VertexFormat.uploadImmediateVertexBuffer or something, before the original data is uploaded. I think the one in MixinGlResourceManager.checkForDrawCallWhileBatching is still necessary to keep though, in case somebody uses a dedicated VBO, not the immediate ones.

commented

But that very often isn't enough because not everything uses specifically RenderLayer.MultiPhase as the implementation for RenderLayer, especially in 1.21.5, and not everything uses RenderLayers/RenderTypes to render things to begin with, like before 1.21.5. I create render passes directly in my mods so I can upload custom shader uniforms that RenderLayer.MultiPhase just doesn't work with. That is why I suggested VertexFormat.uploadImmediateVertexBuffer (not sure what it's called in yarn). The issue only happens with the shared immediate vertex buffers so it seems like the best place for it.

commented

Fair. I thought that if someone bypasses the whole RenderLayer system they probably also use their own buffers instead of the minecraft shared ones. I can add a hook in VertexFormat.uploadImmediateVertexBuffer as well to catch those instances

commented

Awesome, thank you! Nope, I don't really have a reason to use my own vertex buffers.

commented

How are you setting GL states like blend, depth test, ... Manually with GlStateManager? If so before or after uploading the vertex buffer?

This is relevant for me because ImmediatelyFast is going to submit draw calls when VertexFormat.uploadImmediateVertexBuffer is called which may override GL states which were set before. If you set your GL states after calling VertexFormat.uploadImmediateVertexBuffer this wouldn't be an issue, else I need to track and restore the GL states

commented

I fixed it by checking if RenderLayer.draw is called during a batch (This is before the GL state and vertex buffer are configured, so it shouldn't be overridden anymore)

commented

I'm not setting GL states directly. The vanilla code applies them from the render pipeline with GlCommandEncoder.applyPipelineState (official mappings) after the buffer is uploaded. I am still using one of these after the buffer is uploaded: "drawObjectsWithRenderPass", "drawBoundObjectWithRenderPass" (yarn mappings).

commented

I added a hook in VertexFormat.uploadImmediateVertexBuffer. Can you check if that works for you?

commented

Thanks! I am so sure it does but sadly don't have time to check right now. I'll let you know tomorrow as soon as I can. Hope you don't mind.

commented

No problem. Just answer here whether it works or not

commented

Sorry, I realized that there are things you should track and restore, but they aren't directly GL states. Basically the things in RenderSystem set by RenderLayer.MultiPhase, which I think only includes all shader textures, shader matrices (modelview, projection and texture matrix) and shaderLineWidth. Feel free to track other stuff in RenderSystem too I guess. Nothing should call GlStateManager or OpenGL directly though. That part is still handled by GlCommandEncoder after buffer upload.

I can work around this easily but others likely won't (because even vanilla code doesn't).

commented

I am already tracking shader textures and shader color. Line width is very rarely ever used in minecraft, so it hasn't been a problem yet (I can track it if it causes a problem for you). Tracking the matrices isn't something I really want to do because its pretty expensive to do often (I have to copy them everytime)

commented

Ok, I can confirm that the fix works in my case. However, to avoid compatibility issues with other mods I think you should track all 12 shader textures and at least the shader texture matrix because it is being left modified (technically reset) even by some vanilla RenderLayers (that may be included in hudBatchingVertexConsumers).
Are you sure it would be that expensive? I can't imagine it being that bad considering how much matrices are copied around already. Should be simple to throw the shaderLineWidth in there too. I am actually using that. I don't think it's that niche.

commented

Thank you for testing and highlighting the possible issues you found. I am going to add the shader line width to list of tracked states. I however won't add all 12 shader textures or the shader texture matrix as long as there is no need to. Vanilla Minecraft never uses more than 3 shader textures (For this to be an issue with ImmediatelyFast, a mod would need to use create a custom RenderLayer which uses more than 3 textures, use that in the GUI/HUD and there has to be another mod which also uses more than 3 shader texture to render in the GUI/HUD, but without using a RenderLayer) and also only uses the texture matrix for breeze rendering (Which isn't ever used in GUI rendering).

commented

No problem, thank you for the quick responses and fixes!
It's true that more than 3 textures aren't used often. It just seems simple to include all 12, just in case, but it's up to you.
The texture matrix is also used for the item glint effect (uses texturing states), which is part of the GUI, and the Breeze entity can in fact be rendered on the GUI. Mods that display what you're looking at do that, including one of my mods. Mobs not rendering correctly on the GUI is how I noticed that the issue has returned. Although I'm pretty sure both the glint effect and the breeze's wind are rendered after the solid part is rendered, so it coincidentally shouldn't affect them. I feel like tracking 16 numbers is worth avoiding any future trouble though.

commented

Oh, I see what you're saying about the Breeze. It wouldn't ever be rendered as part of hudBatchingVertexConsumers so won't be a cause of issues.
EDIT: And yeah, item glint and breeze not being affected was kind of irrelevant considering they are using normal RenderLayers. But something custom can still be broken by the item glint I think.