Fix crash when a graphics pipeline is bound with no active render pass#871
Fix crash when a graphics pipeline is bound with no active render pass#871Chaos02 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses two window-occlusion/minimize edge cases in VulkanMod’s frame/render lifecycle: a crash caused by attempting to bind/compile graphics pipelines without an active render pass, and a “clear-color flash” caused by presenting frames that were only cleared (but not fully rendered) while occluded.
Changes:
- Skip
Renderer.bindGraphicsPipeline(...)when no render pass is currently bound (prevents NPE during pipeline handle creation). - Change the main swapchain render pass color attachment load-op to
VK_ATTACHMENT_LOAD_OP_LOADto preserve prior contents on GUI-only / occluded frames. - Stop clearing the color attachment every tick (clear depth only) to avoid presenting the global clear color on occluded frames.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/main/java/net/vulkanmod/vulkan/Renderer.java |
Adds an early-out in bindGraphicsPipeline when no render pass is bound to prevent a null render-pass pipeline-state crash. |
src/main/java/net/vulkanmod/vulkan/pass/DefaultMainPass.java |
Switches the main pass color load-op to LOAD so GUI-only frames can draw over prior contents instead of a freshly-cleared color. |
src/main/java/net/vulkanmod/mixin/render/frame/MinecraftMixin.java |
Changes per-tick clears to depth-only to pair with the render-pass LOAD and eliminate the clear-color flash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Don't clear COLOR each frame — the world/GUI overwrites it on normal frames, | ||
| // and preserving it lets the main render pass's LOAD keep the last good image | ||
| // (fixes the tab-out clear-color flash). Depth is still cleared. | ||
| Renderer.clearAttachments(GL11.GL_DEPTH_BUFFER_BIT); |
| // Main swapchain pass LOADs the previously-presented frame so GUI-only / occluded | ||
| // frames (where the world render is skipped) composite over the last good image | ||
| // instead of showing the stale clear color (fixes the tab-out flash). The color | ||
| // attachment's initialLayout is COLOR_ATTACHMENT_OPTIMAL (not UNDEFINED) and begin() | ||
| // transitions the image to that layout preserving contents, so LOAD retains the | ||
| // prior frame. On normal frames the world/sky fully overwrites color, so this is a no-op. |
| if (boundRenderPass == null) | ||
| return; |
|
Related PRs from the same effort (getting a VulkanMod based 1.21.1 pack working). Both are independent of this one, which is purely the tab-out path: |
Rendering the GUI while no render pass is bound (window occluded, minimized, or the title bar being dragged) makes bindGraphicsPipeline build a PipelineState from a null render pass, which then NPEs in GraphicsPipeline.createGraphicsPipeline at state.renderPass.getId(). Skip the bind when nothing is bound. Without a render pass there is no valid target to record a draw into, so there is nothing useful to do here anyway. getHandle is only reached from bindGraphicsPipeline, and the eager compile in the GraphicsPipeline constructor is gated on builder.renderPass, so this covers the whole path to the NPE.
Chaos02
changed the title
|
I am withdrawing the flash half of this PR. It is wrong, and the verification I claimed for it The stated rationale is factually wrong. The PR body claims It also reads undefined memory after a swapchain recreate. The "Verified in-game on MC 1.21.1" claim is not supportable, and I should not have made
So the test that "verified" this exercised neither the clear-strip nor the The crash guard is unaffected by any of this. Rather than close this PR and scatter the Sorry for the round trip. |
|
Separate finding from the same tracing session, flagging it here since Beryl has no issue Beryl (0.2.0-alpha) replaces The practical consequence for review here: any fix made at the One related data point, since it points at Beryl rather than VulkanMod: A/B-ing Beryl off Beryl is closed-source so I only have stack frames here, and I am not speculating past them. |
Fixes an intermittent
NullPointerExceptionthat kills the game when the window isoccluded, minimized, or the title bar is being dragged.
Symptom
Repro
In-game, open chat, then tab out or minimize the window. Crashes intermittently.
It is a race between three conditions: the window stops having a valid render target,
the GUI (chat) keeps rendering anyway, and a GUI shader is used whose pipeline is not
yet in the cache.
Cause
The GUI still renders while no render pass is bound.
bindGraphicsPipelinebuilds aPipelineStatefrom the current (null)boundRenderPass; the first use of anuncached GUI shader then reaches
createGraphicsPipeline, which dereferencesstate.renderPass.getId()and throws. A cached pipeline survives this path, which iswhy the crash only shows up when a not-yet-compiled shader happens to be used during
the occluded frames.
Fix
Skip the bind when no render pass is bound. Without a pass there is no valid target to
record a draw into, so there is nothing useful to do in this method anyway.
This covers the whole path to the NPE:
createGraphicsPipelinehas two call sites.getHandle, is only reached fromRenderer.bindGraphicsPipeline, which thisguard covers.
GraphicsPipelineconstructor, is gated onPipeline.Builder.renderPass.Notes
One unrelated observation found while tracing this, left alone here:
PipelineState.DEFAULTcarries a null
renderPass, so if the constructor's eager-compile path were ever wired up itwould hit the same dereference on non-
DYNAMIC_RENDERINGdevices. Not reachable today; happyto open a separate issue if that is worth tracking.
Verification
Repro steps above against the patched build: no crash across repeated tab-outs.