fix: critical Vulkan/Wayland bugs, resource leaks, and build improvements#1
Closed
daedaevibin wants to merge 17 commits into
Closed
fix: critical Vulkan/Wayland bugs, resource leaks, and build improvements#1daedaevibin wants to merge 17 commits into
daedaevibin wants to merge 17 commits into
Conversation
…ents - Fix descriptor set layout: store as class member, wire into allocation - Fix render pass format mismatch: use dynamic swapchain format - Fix Wayland event loop: proper prepare_read/cancel_read/read_events - Add queue family enumeration: find graphics+present capable family - Fix eventfd busy-loop: consume wake events after poll - Add SPIR-V shader compilation (glslc) to CMakeLists.txt - Fix full Vulkan resource cleanup in destructor (atlas, vertex buf, descriptors, Wayland objects) - Remove all debug std::cout logging, remove unused iostream includes - Fix CMake -Oz flag conflict with -O3 in release - Use $SHELL env with fish fallback instead of hardcoded /usr/bin/fish - Adopt Veridian Zenith 'Atmosphere' color palette (#050200 bg, #f3f4f6 fg, #FFB347 accent) - Add glslang-tools to CI dependencies - Add shaders/*.spv to .gitignore
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Warning Review limit reached
More reviews will be available in 6 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR adds automatic GLSL→SPIR-V shader compilation to the CMake build and CI. It replaces the ChangesKaelum runtime and build improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Add missing <cctype> include in nexus.cpp (fixes CI: std::isdigit) - Add wl_display_flush() to event loop (outgoing requests reach compositor) - Return bool from prepare_read() and handle failure (dispatch_pending loop) - Cancel Wayland read lock on poll error before breaking - Move vkResetFences after successful vkAcquireNextImageKHR (prevents deadlock) - Free old command buffers before re-allocation in record_command_buffers() - Fix shell fallback comment accuracy
- Add -O1 to debug flags (satisfies _FORTIFY_SOURCE=3 requirement) - Suppress unused parameter warnings in xdg_surface_handle_configure, parse_sgr - Remove unused current_frame_ member
- Use configured dimensions from xdg_toplevel configure event when Vulkan reports UINT32_MAX extent (standard wlroots behavior) - Defer resize handling until after initial Vulkan init is complete (prevents SIGSEGV when configure fires during init_wayland roundtrip) - Query supportedCompositeAlpha from surface capabilities instead of hardcoding VK_COMPOSITE_ALPHA_PRE_MULTIPLIED_BIT_KHR (not supported on many Hyprland/wlroots setups) - Guard against zero-extent swapchain creation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sigil.cpp (1)
843-855:⚠️ Potential issue | 🔴 CriticalRender pass and pipeline must be recreated during swapchain resize.
During
on_resize(),create_swapchain()can select a different surface format if the preferred format is no longer available. Sincecreate_render_pass()usesswapchain_format_directly in the color attachment description, andcreate_graphics_pipeline()references the render pass, the old render pass and pipeline will be incompatible with the new swapchain if the format changes. Currently, only framebuffers and command buffers are recreated;create_render_pass()andcreate_graphics_pipeline()must also be called to maintain compatibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sigil.cpp` around lines 843 - 855, In the on_resize() method, after successfully calling create_swapchain(), you must also call create_render_pass() and create_graphics_pipeline() before calling create_framebuffers(). The issue is that create_swapchain() may select a different surface format than previously used, and since create_render_pass() depends on swapchain_format_ for its color attachment description and create_graphics_pipeline() references the render pass, these functions must be recreated to maintain compatibility with the new swapchain format. Add these two function calls in the correct order after the create_swapchain() check and before create_framebuffers().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/codeql.yml:
- Line 39: In the apt-get install command on line 39 of the codeql.yml workflow
file, replace the package name glslang-tools with glslc. The CMakeLists.txt
build configuration searches for the glslc executable which is provided by the
glslc package (not glslang-tools), and without it shader compilation will be
silently skipped rather than failing the build, allowing incomplete binaries to
be shipped without valid SPIR-V artifacts.
In `@CMakeLists.txt`:
- Around line 171-173: The else() block currently issues only a warning when
glslc is not found, but since the renderer requires SPIR-V shaders at runtime, a
missing glslc should cause the build configuration to fail immediately rather
than defer failure to runtime. Replace the message(WARNING "glslc not found.
Shaders will not be compiled automatically.") call with message(FATAL_ERROR ...)
using an appropriate error message to prevent silent build failures and ensure
shader compilation is always available when needed.
- Around line 156-163: The generated shader SPV files are being output to the
source directory using CMAKE_SOURCE_DIR in the SHADER_DIR variable and the
SHADER_SPV output path. This violates best practices for out-of-tree builds and
can cause issues in read-only or packaged source environments. Modify the
SHADER_DIR variable or create a separate output directory variable that uses
CMAKE_BINARY_DIR or CMAKE_CURRENT_BINARY_DIR instead of CMAKE_SOURCE_DIR. Update
the SHADER_SPV output path and the add_custom_command OUTPUT directive to use
this new build directory path so generated .spv files are placed in the build
tree, not the source tree.
In `@src/loom.cpp`:
- Around line 37-42: The startup message in src/main.cpp that reports the PTY
being linked to fish is now inaccurate because Loom::initialize() in
src/loom.cpp dynamically selects a shell based on the SHELL environment variable
with fallbacks to fish and then sh. Update the user-facing message in
src/main.cpp to accurately reflect this dynamic shell selection behavior instead
of hardcoding it to fish, so users receive correct information about which shell
is actually being used.
In `@src/main.cpp`:
- Around line 92-96: Remove the [[maybe_unused]] attribute and add explicit
error handling to the ::read() call when draining the wake_fd. Check the return
value: if it returns -1 and errno equals EINTR, loop back and retry the read
operation, otherwise handle the error appropriately. This ensures that if the
read is interrupted by a signal, the eventfd is properly drained before poll
returns, preventing busy-loop scenarios where the signal handler keeps
triggering poll without actually consuming the wakeup event.
In `@src/sigil.cpp`:
- Around line 256-274: The code unconditionally selects the first physical
device without verifying it supports the required queue families. Refactor the
physical device selection to iterate through all enumerated devices and check
each one for queue family support. Move the queue family search logic (the loop
checking for both VK_QUEUE_GRAPHICS_BIT and presentation support) into the
device iteration loop, so that graphics_queue_family_ is populated per device
candidate. Select and assign physical_device_ only when a device is found that
has a queue family supporting both graphics and presentation capabilities,
rather than blindly using devices[0].
- Around line 79-86: The seven setup functions `create_render_pass`,
`create_framebuffers`, `create_sync_objects`, `create_command_pool`,
`create_graphics_pipeline`, `record_command_buffers`, and `create_vertex_buffer`
are all void functions that only log Vulkan errors to stderr without returning
error information. This allows the initialize() method to return success even
when these critical setups fail, leaving invalid Vulkan objects. Refactor each
of these seven functions to return std::expected with an appropriate error type
(such as SigilError) instead of void, and have them return the error when Vulkan
API calls fail. Then in the initialize() method, check the return value of each
function call using the expected result pattern and propagate any errors up
before returning success at line 91.
---
Outside diff comments:
In `@src/sigil.cpp`:
- Around line 843-855: In the on_resize() method, after successfully calling
create_swapchain(), you must also call create_render_pass() and
create_graphics_pipeline() before calling create_framebuffers(). The issue is
that create_swapchain() may select a different surface format than previously
used, and since create_render_pass() depends on swapchain_format_ for its color
attachment description and create_graphics_pipeline() references the render
pass, these functions must be recreated to maintain compatibility with the new
swapchain format. Add these two function calls in the correct order after the
create_swapchain() check and before create_framebuffers().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bcdd951-06d7-419e-bad7-ec57d2b1ec23
📒 Files selected for processing (10)
.github/workflows/codeql.yml.gitignoreCMakeLists.txtinclude/common.hppinclude/sigil.hppsrc/glyph_engine.cppsrc/loom.cppsrc/main.cppsrc/nexus.cppsrc/sigil.cpp
💤 Files with no reviewable changes (1)
- src/glyph_engine.cpp
Root causes: - on_resize was called synchronously inside the Wayland configure handler, but the compositor hadn't processed the surface commit yet, so vkGetPhysicalDeviceSurfaceCapabilitiesKHR still returned stale data causing swapchain creation to fail - After failed recreation, swapchain_ still held the old (destroyed) handle, so render() tried to use it → SIGSEGV - Old swapchain was not destroyed before creating new one in on_resize Fixes: - Defer resize: configure handler sets needs_resize_ flag; main loop calls process_pending_resize() after event dispatch (same pattern as Ghostty/Kitty) - Destroy old swapchain in on_resize before creating replacement - Commit wl_surface in xdg_surface_handle_configure after ack - Query supportedCompositeAlpha (OPAQUE preferred) - Add diagnostic VkResult/extent/format to swapchain failure message
io_uring fixes (root cause of blank window): - Use offset -1 for PTY read/write (PTYs are non-seekable; offset 0 caused -ESPIPE, so shell output never reached Nexus) - Tag SQEs with URING_TAG_READ/WRITE to distinguish completions - write() no longer blocks with io_uring_wait_cqe (was consuming read CQEs, causing terminal to go silent after first keypress) - poll_read() iterates CQ ring, consuming write CQEs and returning on the first read CQE Keyboard: - Added numbers, symbols, backspace, tab, escape to key map - Added backspace/DEL handling in Nexus (cursor_x_--) - Filter other control characters < 0x20 in ground state Swapchain recreation: - Pass oldSwapchain to vkCreateSwapchainKHR on resize (Vulkan spec allows driver to reuse resources) - Destroy old swapchain only after new one is created successfully
Dynamic resize: - Grid changed from fixed std::array<Cell, 80*24> to std::vector<Cell> with runtime cols_/rows_ members - Nexus::resize() copies existing content to new grid dimensions - Loom::set_pty_size() sends TIOCSWINSZ to the shell on resize - Sigil fires resize_callback_ from on_resize() with cols/rows calculated from pixel extent and cell dimensions - Cell dimensions derived from FreeType max_advance and line_height Rendering adapts to dynamic grid: - render() reads nexus.cols()/rows() instead of k_default_cols/rows - Cells fill full NDC range (-1..1) for the current grid size New CSI sequences: - J modes 0/1/2/3 (erase in display: cursor-to-end, start-to-cursor, full, full+scrollback) - K modes 0/1/2 (erase in line) - L/M (insert/delete lines) - P/@ (delete/insert characters) - d (vertical line position absolute) - G (cursor character absolute) Keyboard: - Arrow keys, Home, End, PgUp, PgDn, Delete emit escape sequences - Numbers, symbols, backspace, tab, escape mapped
Wayland protocol fixes: - Add xdg_wm_base ping/pong listener (Hyprland marks unresponsive apps that don't respond, may withhold configure events) - Handle xdg_toplevel close event (sets should_close flag, main loop exits cleanly) - Ignore 0x0 configure (compositor saying 'pick your own size') - Set xdg_toplevel_set_min_size(320,240) so compositor knows minimum - Main loop checks should_close() instead of while(true) Frame callback pacing: - After present, request wl_surface.frame callback - render() skips if frame_pending_ (waits for compositor to signal frame_done before drawing again — prevents overdriving) - Properly clean up frame_callback in destructor Also handle VK_SUBOPTIMAL_KHR on present by triggering resize
… diagnostics Per Wayland protocol (and Kitty's documented pattern): ack_configure → set_window_geometry → commit Without set_window_geometry, Hyprland doesn't know the app's drawing area and cannot properly tile/resize the window. Swapchain extent fixes: - Removed early bail on currentExtent == 0 (happens on Hyprland before first buffer attach, was causing swapchain creation to fail) - Fall through to use configured_width_/height_ when extent is 0 or UINT32_MAX, then clamp to driver-reported min/max Added configured_width()/configured_height() accessors so the xdg_surface configure handler can query current dimensions. Diagnostic logging in handle_configure shows compositor-sent vs stored dimensions and whether init is complete — helps debug resize chain.
…ess bugs Rendering: - Glyph quads now use FreeType bearing offsets + actual bitmap dimensions for proper per-glyph positioning within cells (fixes distorted text) - GlyphRect stores full pixel metrics (bearing_x/y, width, height, advance) - GlyphMetric bearing types changed from uint32_t to int32_t (FreeType bitmap_left can be negative) - Vertex positions computed in pixel-to-NDC space using extent dimensions - Vertex buffer enlarged to 20k cells, with overflow clamping guard Wayland/Hyprland: - Added wl_pointer listener (enter/leave/motion/button/axis) so Hyprland can properly track pointer focus — fixes 'focus swaps when moving cursor' - Capped wl_seat binding to version 5 (avoids needing v8+ pointer events) - Configure handler debounced: only sets needs_resize_ when dimensions actually change Terminal: - Added scroll_up(): shifts grid up one line when cursor overflows bottom, clears last line — output no longer overwrites bottom row - Fixed move_cursor() unsigned wraparound: arithmetic now done in int before clamping, prevents cursor teleporting to far edge on negative moves I/O: - Loom::write() now copies data into persistent write_buffer_ before submitting to io_uring, fixing dangling stack pointer when callers pass temporary buffers
… handlers wl_pointer version 5 requires all event handlers up to axis_discrete. Missing frame handler (opcode 5) caused SIGABRT on Hyprland.
… buffer map size Rendering: - Fixed Y-axis direction: Vulkan NDC has Y=-1 at top, Y=+1 at bottom. Row 0 now renders at top of screen instead of bottom. - Baseline positioned at bottom of each cell; bearing_y offsets glyph upward from baseline (correct for Y-down coordinate system). - Triangle winding order corrected for Y-down NDC. - Added codepoint==0 skip alongside space skip. - Fixed vertex buffer map size (was hardcoded 10k, now uses capacity). Pointer: - Added axis_value120 and axis_relative_direction handlers to complete the wl_pointer v5+ listener (fixes missing field warning).
- Grid and PTY size now synced to actual compositor dimensions at startup (was stuck at 80x24 until first resize event fired in main loop) - Added temporary render diagnostics: first 5 frames print grid dimensions, non-space cell count, vertex count, and swapchain extent - Fixed vertex buffer map size to use capacity variable
io_uring reads from non-blocking FDs complete immediately with -EAGAIN when no data is available, instead of waiting for the shell to produce output. This caused all io_uring read completions to fail silently, so shell output never reached the grid (confirmed by render diagnostics showing non_space=0 across all frames). With the FD in blocking mode, io_uring correctly parks the read in the kernel until the shell writes data, then signals the registered eventfd.
…designations The fish shell prompt was invisible because most of its output consists of escape sequences that the parser was mishandling: - CSI '?' prefix (DEC private modes like ?2004h bracketed paste, ?25h show cursor) was treated as the final byte, causing '2004h' to render as text - ESC ( ) * + charset designations (3-byte sequences) were partially consumed, leaking the final byte as visible text - OSC sequences only terminated on BEL; ESC \ (ST) wasn't handled, leaving the parser stuck in OSC state eating all subsequent output Also: - Stop hiding mouse cursor on pointer enter (was setting cursor to NULL) - Implement CSI f (HVP), X (ECH), S (SU), r/c/n/l/h/s/u stubs - Add EscapeSkip state for consuming charset designation final byte
io_uring reads from PTY file descriptors were silently failing — the eventfd was never signaled, so shell output never reached the grid (non_space=0 across all frames). Replaced with standard poll(master_fd)+read() pattern, which is what Ghostty, Kitty, and every production terminal emulator uses. io_uring is kept for async writes only. Also increased render diagnostic count to 10 frames for better debugging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sprint 1 critical fixes that resolve crashes, resource leaks, and build failures blocking any functional rendering.
Vulkan correctness fixes
Descriptor set layout crash —
create_graphics_pipeline()created aVkDescriptorSetLayoutas a local, destroyed it, thencreate_descriptor_set()tried to allocate from it (and was missingpSetLayoutsentirely). Now stored asdescriptor_set_layout_member, wired into both pipeline layout creation and descriptor allocation, destroyed in~Sigil().Render pass / swapchain format mismatch — render pass hardcoded
VK_FORMAT_B8G8R8A8_SRGBwhile swapchain picks format dynamically. Addedswapchain_format_member;create_render_pass()now uses it.Queue family hardcoded to 0 — no enumeration, fails on GPUs where family 0 isn't graphics+present. Now enumerates families via
vkGetPhysicalDeviceSurfaceSupportKHR. Stored asgraphics_queue_family_.Fence deadlock on swapchain out-of-date —
vkResetFenceswas called beforevkAcquireNextImageKHR; if acquire returnedVK_ERROR_OUT_OF_DATE_KHR, the fence stayed unsignaled and the nextvkWaitForFencesdeadlocked. Moved reset to after successful acquire.Wayland event loop (complete rewrite)
poll_events()calledwl_display_dispatch()in a blocking while-loop → replaced withwl_display_read_events()+dispatch_pending()wl_display_flush()to the loop so outgoing requests (e.g.xdg_surface_ack_configure) actually reach the compositorprepare_read()now returnsbool; callers handle failure by looping ondispatch_pendingcancel_read()on poll error path to release Wayland read lockResource cleanup
vkDeviceWaitIdlefirst, then proper teardown of glyph atlas (image/memory/view/sampler), vertex buffer (unmap/buffer/memory), descriptor pool/layout, all Wayland objectsrecord_command_buffers(): frees old command buffers before re-allocating (prevents leak on resize)Build system
-OzfromCOMMON_FLAGS(conflicted with-O3in release)glslcshader compilation: GLSL → SPIR-V viaadd_custom_target(shaders)glslang-toolsto CI deps;shaders/*.spvto.gitignore<cctype>include forstd::isdigitOther
$SHELL→/usr/bin/fish→/bin/shfallback chainvz_bg(#050200),vz_fg(#f3f4f6),vz_accent(#FFB347)std::coutdebug lines and unused<iostream>includesLink to Devin session: https://app.devin.ai/sessions/adbf553caeec4d948cdb3a25123d8b7e
Requested by: @daedaevibin