-
Notifications
You must be signed in to change notification settings - Fork 114
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
Move letters off the grid to make text more readable #656
base: release
Are you sure you want to change the base?
Conversation
dest.x += offsetForEnd / 2; | ||
} | ||
|
||
// int diff = abs(tile->foreRed - tile->backRed) + abs(tile->foreGreen - tile->backGreen) + abs(tile->foreBlue - tile->backBlue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is some debugging logic, I'll remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the foresight to know how this will be received - it definitely alters the aesthetics of things, but it is easier to read. And the implementation is quite minimal! So nice work. I think we'll give it a go
/// 1 : Left-aligned text [startColumn must be set] | ||
/// 2 : Middle-aligned text [startColumn and endColumn must be set] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 1 : Left-aligned text [startColumn must be set] | |
/// 2 : Middle-aligned text [startColumn and endColumn must be set] | |
/// 1 : Left-aligned text [firstColumn must be set] | |
/// 2 : Middle-aligned text [firstColumn and lastColumn must be set] |
/// 0 : Normal terminal output | ||
/// 1 : Left-aligned text [startColumn must be set] | ||
/// 2 : Middle-aligned text [startColumn and endColumn must be set] | ||
int8_t mode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: what about an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that would be a good idea, I was being lazy.
I'll fix this after I rebase the PR to fix the conflicts.
I'm just a player and a fan of Brogue, so I'm not sure if my comment is useful, but I find the text in the "after" picture to look uneven. For a fixed-width typeface (the original solution), it's expected to have gaps between certain letters, but the horizontal rhythm of the text is still there due to each letter being shown in a grid. In the "after" version, the tracking (the distance between each character) is too tight (e.g., "Some food"), while the word "entitled" breaks the rhythm due to the lack of kerning pairs, especially because of the larger gaps between the "tit" letters. Of course, I truly appreciate the effort and the intent; however, I think this change is not a clear improvement. |
This PR augments the
cellDisplayBuffer
struct with a new field,CellTextInfo textInfo
that tracks information about the text qualities of the cell. When drawing text to the screen,textInfo.textMode
is set to a non-zero value:The platform can read this information, and adjust the placement of letters to make text more readable:
As shown in the screenshot above, most interfaces and log messages now make use of this fancier text-printing info.
I think I've ironed out all of the bugs, but it's possible that there's still some screens which display incorrectly.
Platforms must be modified to accept the extra
CellTextInfo
parameter in theirplotChar
implementation - though it can simply be ignored if the platform doesn't want to or can't support moving the letters around. The actual display logic change is pretty small - seetiles.c
for what the platform actually does to make use of this new metadata.