Add fractional scroll offsets for embedders#61
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR adds fractional-offset scrolling to Ghostty's surface API. It introduces renderer state fields to track smooth scroll position, updates Metal shaders to apply offsets, syncs state to GPU uniforms each frame, implements ChangesSmooth Scroll Fractional Offset
Sequence DiagramsequenceDiagram
participant Client as Embedding Client
participant CAPI as C API Layer
participant Surface as Surface
participant RendererState as RendererState
participant Renderer as Renderer
participant GPU as GPU/Shaders
Client->>CAPI: ghostty_surface_scroll_to_offset(offset)
CAPI->>Surface: scrollToOffset(offset)
Surface->>RendererState: resetSmoothScrollOffset()
RendererState->>RendererState: clear smooth_scroll_offset to 0
Surface->>Surface: split offset into row + fraction
Surface->>Surface: scroll by integral rows
Surface->>RendererState: update smooth_scroll_offset, image_scroll_offset
Surface->>Renderer: queue render
Renderer->>RendererState: read smooth_scroll_offset
Renderer->>Renderer: sync to uniforms
Renderer->>GPU: upload uniforms
GPU->>GPU: apply offset in cell_bg, cell_text, image shaders
GPU->>Client: rendered frame
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/renderer/generic.zig`:
- Around line 1236-1237: The two assignments to self.uniforms
(self.uniforms.smooth_scroll_offset and self.uniforms.image_scroll_offset) are
under state.mutex and can race with drawFrame readers; move these uniform writes
so they occur while holding draw_mutex instead of state.mutex. Specifically,
after updating any state under state.mutex keep the new values and then acquire
draw_mutex before writing into self.uniforms (and release it immediately after),
ensuring drawFrame only reads uniforms while draw_mutex is held; reference the
symbols self.uniforms, state.mutex, draw_mutex, and drawFrame when locating
where to move the writes.
In `@src/Surface.zig`:
- Around line 3524-3535: The code currently truncates pixel_offset to an integer
which drops sub-pixel fractional scroll and causes visible stair-stepping;
change pixel_offset to remain a float (set pixel_offset = -fractional_offset *
cell_height_float as f64), assign that float into
renderer_state.smooth_scroll_offset (no `@trunc`), then compute extra_rows_float =
`@ceil`(`@abs`(pixel_offset) / cell_height_float) and clamp/convert to u16 the same
way (using maxInt(u16) and `@intFromFloat`) before storing into
renderer_state.image_scroll_offset; update references to pixel_offset,
smooth_scroll_offset, and extra_rows_float accordingly so fractional offsets are
preserved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52b460e9-edf0-45f9-b8e9-2fc58618078d
⛔ Files ignored due to path filters (4)
src/renderer/shaders/glsl/cell_bg.f.glslis excluded by!**/*.glslsrc/renderer/shaders/glsl/cell_text.v.glslis excluded by!**/*.glslsrc/renderer/shaders/glsl/common.glslis excluded by!**/*.glslsrc/renderer/shaders/glsl/image.v.glslis excluded by!**/*.glsl
📒 Files selected for processing (9)
include/ghostty.hsrc/Surface.zigsrc/apprt/embedded.zigsrc/renderer/State.zigsrc/renderer/generic.zigsrc/renderer/metal/shaders.zigsrc/renderer/opengl/shaders.zigsrc/renderer/shaders/shaders.metalsrc/termio/Termio.zig
| self.uniforms.smooth_scroll_offset = state.smooth_scroll_offset; | ||
| self.uniforms.image_scroll_offset = state.image_scroll_offset; |
There was a problem hiding this comment.
Move uniform writes under draw_mutex.
self.uniforms is draw-time shared state; writing it here under only state.mutex can race with drawFrame reads and violates the file’s own lock contract.
🔧 Suggested fix
const Critical = struct {
links: terminal.RenderState.CellSet,
mouse: renderer.State.Mouse,
preedit: ?renderer.State.Preedit,
scrollbar: terminal.Scrollbar,
+ smooth_scroll_offset: f32,
+ image_scroll_offset: [2]u16,
overlay_features: []const Overlay.Feature,
};
...
-const scrollbar = state.terminal.screens.active.pages.scrollbar();
-self.uniforms.smooth_scroll_offset = state.smooth_scroll_offset;
-self.uniforms.image_scroll_offset = state.image_scroll_offset;
+const scrollbar = state.terminal.screens.active.pages.scrollbar();
+const smooth_scroll_offset = state.smooth_scroll_offset;
+const image_scroll_offset = state.image_scroll_offset;
...
break :critical .{
.links = links,
.mouse = state.mouse,
.preedit = preedit,
.scrollbar = scrollbar,
+ .smooth_scroll_offset = smooth_scroll_offset,
+ .image_scroll_offset = image_scroll_offset,
.overlay_features = overlay_features,
};
};
...
self.draw_mutex.lock();
defer self.draw_mutex.unlock();
+self.uniforms.smooth_scroll_offset = critical.smooth_scroll_offset;
+self.uniforms.image_scroll_offset = critical.image_scroll_offset;🤖 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/renderer/generic.zig` around lines 1236 - 1237, The two assignments to
self.uniforms (self.uniforms.smooth_scroll_offset and
self.uniforms.image_scroll_offset) are under state.mutex and can race with
drawFrame readers; move these uniform writes so they occur while holding
draw_mutex instead of state.mutex. Specifically, after updating any state under
state.mutex keep the new values and then acquire draw_mutex before writing into
self.uniforms (and release it immediately after), ensuring drawFrame only reads
uniforms while draw_mutex is held; reference the symbols self.uniforms,
state.mutex, draw_mutex, and drawFrame when locating where to move the writes.
| const cell_height_float: f64 = @floatFromInt(self.size.cell.height); | ||
| if (fractional_offset > 0 and cell_height_float > 0) { | ||
| const pixel_offset = @trunc(-fractional_offset * cell_height_float); | ||
| self.renderer_state.smooth_scroll_offset = @floatCast(pixel_offset); | ||
|
|
||
| const extra_rows_float = @ceil(@abs(pixel_offset) / cell_height_float); | ||
| const max_u16_float: f64 = @floatFromInt(std.math.maxInt(u16)); | ||
| const extra_rows: u16 = if (extra_rows_float >= max_u16_float) | ||
| std.math.maxInt(u16) | ||
| else | ||
| @intFromFloat(extra_rows_float); | ||
| self.renderer_state.image_scroll_offset = .{ extra_rows, extra_rows }; |
There was a problem hiding this comment.
Keep the pixel offset fractional.
Line 3526 truncates the requested smooth-scroll displacement to a whole pixel, so small offset updates are dropped and the new API will visibly stair-step instead of tracking the embedder’s fractional position.
Proposed fix
const cell_height_float: f64 = `@floatFromInt`(self.size.cell.height);
if (fractional_offset > 0 and cell_height_float > 0) {
- const pixel_offset = `@trunc`(-fractional_offset * cell_height_float);
+ const pixel_offset = -fractional_offset * cell_height_float;
self.renderer_state.smooth_scroll_offset = `@floatCast`(pixel_offset);
const extra_rows_float = `@ceil`(`@abs`(pixel_offset) / cell_height_float);
const max_u16_float: f64 = `@floatFromInt`(std.math.maxInt(u16));
const extra_rows: u16 = if (extra_rows_float >= max_u16_float)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cell_height_float: f64 = @floatFromInt(self.size.cell.height); | |
| if (fractional_offset > 0 and cell_height_float > 0) { | |
| const pixel_offset = @trunc(-fractional_offset * cell_height_float); | |
| self.renderer_state.smooth_scroll_offset = @floatCast(pixel_offset); | |
| const extra_rows_float = @ceil(@abs(pixel_offset) / cell_height_float); | |
| const max_u16_float: f64 = @floatFromInt(std.math.maxInt(u16)); | |
| const extra_rows: u16 = if (extra_rows_float >= max_u16_float) | |
| std.math.maxInt(u16) | |
| else | |
| @intFromFloat(extra_rows_float); | |
| self.renderer_state.image_scroll_offset = .{ extra_rows, extra_rows }; | |
| const cell_height_float: f64 = `@floatFromInt`(self.size.cell.height); | |
| if (fractional_offset > 0 and cell_height_float > 0) { | |
| const pixel_offset = -fractional_offset * cell_height_float; | |
| self.renderer_state.smooth_scroll_offset = `@floatCast`(pixel_offset); | |
| const extra_rows_float = `@ceil`(`@abs`(pixel_offset) / cell_height_float); | |
| const max_u16_float: f64 = `@floatFromInt`(std.math.maxInt(u16)); | |
| const extra_rows: u16 = if (extra_rows_float >= max_u16_float) | |
| std.math.maxInt(u16) | |
| else | |
| `@intFromFloat`(extra_rows_float); | |
| self.renderer_state.image_scroll_offset = .{ extra_rows, extra_rows }; |
🤖 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/Surface.zig` around lines 3524 - 3535, The code currently truncates
pixel_offset to an integer which drops sub-pixel fractional scroll and causes
visible stair-stepping; change pixel_offset to remain a float (set pixel_offset
= -fractional_offset * cell_height_float as f64), assign that float into
renderer_state.smooth_scroll_offset (no `@trunc`), then compute extra_rows_float =
`@ceil`(`@abs`(pixel_offset) / cell_height_float) and clamp/convert to u16 the same
way (using maxInt(u16) and `@intFromFloat`) before storing into
renderer_state.image_scroll_offset; update references to pixel_offset,
smooth_scroll_offset, and extra_rows_float accordingly so fractional offsets are
preserved.
Greptile SummaryThis PR exposes a new
Confidence Score: 3/5The core smooth-scrolling path works correctly for text, but the Kitty image rendering at fractional scroll positions is incomplete — image_scroll_offset is computed and uploaded but no code ever acts on it. The smooth_scroll_offset wiring through shaders and all reset-on-scroll paths looks solid. However, image_scroll_offset is declared, populated, and written into GPU uniforms every frame yet is never read by any GLSL or Metal shader, and kittyUpdate is not adjusted to include the extra rows that become partially visible due to the fractional shift. Any terminal with Kitty inline images near the viewport boundary will show a missing image strip during smooth scrolling. src/renderer/generic.zig (image_scroll_offset copy with no consumer) and src/Surface.zig (scrollToOffset pixel-offset calculation) deserve the most attention before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant Embedder as Embedder (cmux)
participant CAPI as ghostty_surface_scroll_to_offset
participant Surface as Surface.scrollToOffset
participant State as renderer.State
participant Renderer as generic.Renderer
participant Shader as GPU Shader
Embedder->>CAPI: scrollToOffset(surface, 2.73)
CAPI->>Surface: scrollToOffset(2.73)
Surface->>Surface: clamp offset to 2.73
Surface->>Surface: "floor to row=2, frac=0.73"
Surface->>State: "terminal.scroll(.{.row=2})"
Surface->>State: resetSmoothScrollOffset()
Surface->>State: "smooth_scroll_offset = trunc(-0.73 x cell_h)"
Surface->>State: "image_scroll_offset = {1,1}"
Surface->>Renderer: queueRender()
Renderer->>State: read smooth_scroll_offset
Renderer->>State: read image_scroll_offset (unused by shaders)
Renderer->>Shader: "uniforms.smooth_scroll_offset = -14px"
Shader->>Shader: "cell_pos.y += smooth_scroll_offset"
Note over Shader: image_scroll_offset uploaded but never read
Reviews (1): Last reviewed commit: "Add fractional scroll offsets for embedd..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // cross-thread mailbox message within the IO path. | ||
| const scrollbar = state.terminal.screens.active.pages.scrollbar(); | ||
| self.uniforms.smooth_scroll_offset = state.smooth_scroll_offset; | ||
| self.uniforms.image_scroll_offset = state.image_scroll_offset; |
There was a problem hiding this comment.
image_scroll_offset uniform is never read by any shader
image_scroll_offset is stored in the GPU uniform buffer here, but neither the GLSL image.v.glsl, the Metal image_vertex function, nor any fragment shader ever reads it. The kittyUpdate CPU path also doesn't receive the value to expand the image draw range. In practice, a Kitty image whose top row is just above the fractional-scroll boundary will be omitted from the draw list, leaving a blank strip where the partially-visible image row should appear.
|
|
||
| const cell_height_float: f64 = @floatFromInt(self.size.cell.height); | ||
| if (fractional_offset > 0 and cell_height_float > 0) { | ||
| const pixel_offset = @trunc(-fractional_offset * cell_height_float); |
There was a problem hiding this comment.
@trunc quantises the pixel offset to whole pixels, so smooth-scroll animations will visibly step in 1-pixel increments instead of moving continuously. Because smooth_scroll_offset is a f32 uniform the shader can already accept fractional pixels; drop the truncation to preserve sub-pixel precision.
| const pixel_offset = @trunc(-fractional_offset * cell_height_float); | |
| const pixel_offset = -fractional_offset * cell_height_float; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/renderer/opengl/shaders.zig">
<violation number="1" location="src/renderer/opengl/shaders.zig:185">
P2: `image_scroll_offset` is wired into uniforms but never consumed by OpenGL shaders, so the extra image-row range for fractional scrolling is not actually applied.</violation>
</file>
<file name="src/renderer/generic.zig">
<violation number="1" location="src/renderer/generic.zig:1236">
P1: Write `self.uniforms` fields only while `draw_mutex` is held; updating them under `state.mutex` introduces a race with draw-time uniform reads.</violation>
</file>
<file name="src/Surface.zig">
<violation number="1" location="src/Surface.zig:3526">
P2: Do not truncate the computed pixel offset here; truncation removes sub-pixel smooth-scroll movement and causes visible stair-stepping.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // can be expensive) and also makes it so we don't need another | ||
| // cross-thread mailbox message within the IO path. | ||
| const scrollbar = state.terminal.screens.active.pages.scrollbar(); | ||
| self.uniforms.smooth_scroll_offset = state.smooth_scroll_offset; |
There was a problem hiding this comment.
P1: Write self.uniforms fields only while draw_mutex is held; updating them under state.mutex introduces a race with draw-time uniform reads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/renderer/generic.zig, line 1236:
<comment>Write `self.uniforms` fields only while `draw_mutex` is held; updating them under `state.mutex` introduces a race with draw-time uniform reads.</comment>
<file context>
@@ -1230,6 +1233,8 @@ pub fn Renderer(comptime GraphicsAPI: type) type {
// can be expensive) and also makes it so we don't need another
// cross-thread mailbox message within the IO path.
const scrollbar = state.terminal.screens.active.pages.scrollbar();
+ self.uniforms.smooth_scroll_offset = state.smooth_scroll_offset;
+ self.uniforms.image_scroll_offset = state.image_scroll_offset;
</file context>
| smooth_scroll_offset: f32 align(4), | ||
|
|
||
| /// Extra image-row range to draw while smooth scrolling. | ||
| image_scroll_offset: [2]u16 align(4), |
There was a problem hiding this comment.
P2: image_scroll_offset is wired into uniforms but never consumed by OpenGL shaders, so the extra image-row range for fractional scrolling is not actually applied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/renderer/opengl/shaders.zig, line 185:
<comment>`image_scroll_offset` is wired into uniforms but never consumed by OpenGL shaders, so the extra image-row range for fractional scrolling is not actually applied.</comment>
<file context>
@@ -178,6 +178,12 @@ pub const Uniforms = extern struct {
+ smooth_scroll_offset: f32 align(4),
+
+ /// Extra image-row range to draw while smooth scrolling.
+ image_scroll_offset: [2]u16 align(4),
+
/// Bit mask defining which directions to
</file context>
|
|
||
| const cell_height_float: f64 = @floatFromInt(self.size.cell.height); | ||
| if (fractional_offset > 0 and cell_height_float > 0) { | ||
| const pixel_offset = @trunc(-fractional_offset * cell_height_float); |
There was a problem hiding this comment.
P2: Do not truncate the computed pixel offset here; truncation removes sub-pixel smooth-scroll movement and causes visible stair-stepping.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/Surface.zig, line 3526:
<comment>Do not truncate the computed pixel offset here; truncation removes sub-pixel smooth-scroll movement and causes visible stair-stepping.</comment>
<file context>
@@ -3491,6 +3492,53 @@ const ScrollAmount = struct {
+
+ const cell_height_float: f64 = @floatFromInt(self.size.cell.height);
+ if (fractional_offset > 0 and cell_height_float > 0) {
+ const pixel_offset = @trunc(-fractional_offset * cell_height_float);
+ self.renderer_state.smooth_scroll_offset = @floatCast(pixel_offset);
+
</file context>
| const pixel_offset = @trunc(-fractional_offset * cell_height_float); | |
| const pixel_offset = -fractional_offset * cell_height_float; |
1 participant
Adds a libghostty embedded API for fractional scrollback offsets used by cmux smooth scrolling.
Changes:
ghostty_surface_scroll_to_offset.Testing:
./scripts/ensure-ghosttykit.shfrom the cmux worktree.nm -gU ghostty/macos/GhosttyKit.xcframework/macos-arm64_x86_64/ghostty-internal.a | rg "ghostty_surface_scroll_to_offset|ghostty_surface_mouse_scroll".Need help on this PR? Tag
@codesmithwith what you need.Summary by cubic
Enables smooth, sub-row scrolling for embedders by adding a fractional scroll offset API. Renderer and shaders now apply a pixel offset while keeping the terminal viewport row-based.
ghostty_surface_scroll_to_offset(surface, double offset)to scroll to a fractional row offset from the top of scrollback.smooth_scroll_offsetandimage_scroll_offsetin renderer state; passed as uniforms to Metal/OpenGL.Written for commit 8577c2b. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements