Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

draw octants directly rather than relying on font #5433

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

dankamongmen
Copy link
Contributor

  • implement yQuads() and draw_octant(), pretty obvious extensions of existing code. to allocate up to 3 potential remainder lines, consider that octants will often appear in a rectangular subset of the terminal. we want the distributed excess uniformly distributed across such a region. so:
    • one excess row: break symmetry in any direction (pick an arbitrary tetrad and use it everywhere)
    • two excess rows: go to alternating tetrads
    • three excess rows: break symmetry, do not use three contiguous tetrads
  • our Octants are octary arrays of bool, provided as a somewhat opaque constant table
  • the 8-line copy-and-paste draw based on the Octant is not the prettiest thing in the known universe
    • we could generalize draw_sextant() and draw_octant() like notcurses did, almost certainly
    • oh bird thou never wert

with that said, i don't think draw_octant() is actually being called lol, so let's not merge this yet. happy to hear early feedback, though.

@dankamongmen
Copy link
Contributor Author

hrmmm, do the builtin draws only run when we can't find these glyphs in the font? that would explain things. i'd suggest giving them primacy should that be the case, but can understand going the other way.

@jcollie
Copy link
Collaborator

jcollie commented Jan 30, 2025

hrmmm, do the builtin draws only run when we can't find these glyphs in the font? that would explain things. i'd suggest giving them primacy should that be the case, but can understand going the other way.

Take a look at this. There are a couple of places in Box.zig and Face.zig where the glyphs need to be "linked in" in order for them to be drawn.

@dankamongmen
Copy link
Contributor Author

hrmmm, do the builtin draws only run when we can't find these glyphs in the font? that would explain things. i'd suggest giving them primacy should that be the case, but can understand going the other way.

Take a look at this. There are a couple of places in Box.zig and Face.zig where the glyphs need to be "linked in" in order for them to be drawn.

got it, much appreciated

@dankamongmen dankamongmen force-pushed the dankamongmen/octants branch 2 times, most recently from d58e04d to fafeb49 Compare February 6, 2025 07:13
@mitchellh mitchellh added this to the 1.1.1 milestone Feb 12, 2025
@mitchellh
Copy link
Contributor

Apologies for the delay, I plan to get this in for 1.1.1 (tagged with that milestone) and will review and modify soon.

@mitchellh mitchellh force-pushed the dankamongmen/octants branch from fafeb49 to ef64471 Compare February 12, 2025 22:14
@mitchellh mitchellh requested a review from a team as a code owner February 12, 2025 22:14
@mitchellh mitchellh force-pushed the dankamongmen/octants branch from ef64471 to f3a19fd Compare February 12, 2025 22:20
@mitchellh
Copy link
Contributor

Box_test_diff.ppm. Looks good.

Going to try to refactor your C-isms now 😆

image

@mitchellh mitchellh force-pushed the dankamongmen/octants branch from f3a19fd to 8b2f9ac Compare February 13, 2025 17:08
@mitchellh
Copy link
Contributor

Alright, updated it to autogenerate the table @dankamongmen made from a text file (at comptime) that is just a copy & paste from the symbols for legacy computing supplement. If we can replace that with a mathematical formula in the future, great. If not, I like this better than the hand-made table.

This is also all covered by our diffing unit test (as shown above) so any refactors in the future can ensure pixel-perfect matching is retained. Once unit tests pass will merge.

Copy link
Collaborator

@qwerasd205 qwerasd205 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mitchellh mitchellh merged commit 2a29f71 into ghostty-org:main Feb 13, 2025
30 checks passed
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.

4 participants