Skip to content

refactor: Remove vertex SSBO req, restructure CellText#10952

Closed
NateSmyth wants to merge 3 commits intoghostty-org:mainfrom
NateSmyth:vertex-attribute
Closed

refactor: Remove vertex SSBO req, restructure CellText#10952
NateSmyth wants to merge 3 commits intoghostty-org:mainfrom
NateSmyth:vertex-attribute

Conversation

@NateSmyth
Copy link

@NateSmyth NateSmyth commented Feb 22, 2026

Purpose

  1. Remove the vertex SSBO requirement causing crashes on cards with OpenGL > 4.3 but vertex SSBO < 1
  2. Compensate for the bytes added to the CellText struct by narrowing glyph_pos and glyph_size

Implementation

1. Replace vertex SSBO with per-instance vertex attribute
  • Change the cell text vertex shader to remove the SSBO layout binding & lookup
  • bg_colors[] SSBO becomes bg_color attribute
  • Add bg_color directly to all CellText construction sites
  • Update metal shader to reflect changes in the shared renderer
2. Refactor `CellText` to compensate for the 4 added bytes
  • After adding bg_color, CellText goes to 40 bytes
  • glyph_size and glyph_pos are [2]u32 align(8), but fit easily in [2]u16 align(4)
  • Update the struct in metal & opengl shaders.zig, add intCasts to the CellText construction sites

Net: No more vertex SSBO. CellText ends up at 28 bytes

Verify

  • ghostty now works on my HD 7650A machine, fixing the crash on startup
  • Side by side render tests show identical rendering/contrast
  • Tried my absolute hardest to overflow glyph_pos and I think its literally impossible

AI Usage Disclosure per `AI_POLICY.md`

Used AI (opencode) for:

  • repetitive edits ("update the other 5 construction sites to match")
  • background research (e.g. finding/pulling exact GL 4.3 min vertex SSBO spec)
  • review (e.g. cross check metal and opengl changes, make sure no sites still rely on the old struct)
Also...

...used AI to make some silly stress test scenarios.

Render as many unicode codepoints as possible in all 4 styles (trying to trip the u16 intcast)

unicode-spam.mp4

Side-by-side regression test (Linux)

ghostty-comparison.mp4

Smoke check nothing broke on metal (Mac)

metal-render-compressed.mp4

But at this point I had devolved to using the AI like a toy. Point being it works fine.

Fixes #8836

@NateSmyth NateSmyth marked this pull request as ready for review February 22, 2026 19:47
@NateSmyth NateSmyth requested a review from a team as a code owner February 22, 2026 19:47
@qwerasd205
Copy link
Member

I haven't looked at this thoroughly, but I would feel more inclined to - if we want to remove SSBO usage to allow better compatibility with old hardware - use a buffer texture instead. The spec allows for a GL_MAX_VERTEX_SHADER_STORAGE_BLOCKS of 0, but requires that GL_MAX_VERTEX_TEXTURE_IMAGE_UNITS is 16.

I've experimented in the past with replacing the BG cell buffer with a texture, and it worked fine, but I never brought it in to Ghostty main because I didn't like the increased complexity it brought with it. There's probably a nicer way to do it though that would be fine, or- alternatively- maybe something could be done to transparently (so, only as a part of the OpenGL backend, without affecting the generic code) create a buffer texture for vertex shader access to passed buffers.

(Info on buffer textures: https://wikis.khronos.org/opengl/Buffer_Texture)

@NateSmyth
Copy link
Author

@qwerasd205 Yeah I think you're right.

Initially thought about this but decided it added too much complexity for what I was trying to fix. But looking at it again, if you want to remove both SSBOs the TBO is cleaner, and definitely can be done transparently in the opengl backend.

Although the original issue was because min vertex SSBO = 0 in opengl 4.3, so there were in-spec cards that would pass the version 430 core check then crash when trying to access the block. So vertex attribute seemed cleaner if scope was limited to the vertex shader.

I don't think going no SSBO entirely would expand compatibility further because min fragment SSBOs = 8 in 4.3. But if you also removed the vertex attrib bindings you could support 4.2. And from there its just layout(binding = N) stuff if you want to support 3.3 I think

I'll rework this and probably split into two scoped PRs since the TBO approach wouldn't rely on the CellText glyph narrowing.

@NateSmyth
Copy link
Author

Reworked to #10994

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants