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

To upsample, or not to upsample? #370

Closed
pedrottimark opened this issue Oct 14, 2019 · 4 comments
Closed

To upsample, or not to upsample? #370

pedrottimark opened this issue Oct 14, 2019 · 4 comments

Comments

@pedrottimark
Copy link
Contributor

Please read this as an observation from a thankful user, instead of a late request for chalk 3

It might be relevant input to your discussion about performance and simplification

Colors are downsampled from 16 million RGB values to an ANSI color format that is supported by the terminal emulator

While studying bgAnsi256 as a visual enhancement when available, I noticed that chalk also upsamples from lower-level color models to higher-level ANSI color formats

It looks like a side-effect of the implementation more than an intended feature, because:

  • less predictable result (see ansi below)
  • more complicated to test
  • obstacle to faster paths
  • more than necessary dependence on color-convert

green

Here is (new chalk.Instance({level)).green('…') with ^ as placeholder for escape:

level output
0
1 ^[32m…^[39m
2 ^[32m…^[39m
3 ^[32m…^[39m

This is predictable: it uses lowest-level format and downsamples when needed

ansi

Here is (new chalk.Instance({level)).ansi(32)('…') which is a synonym for green:

level output
0
1 ^[32m…^[39m
2 ^[38;5;34m…^[39m
3 ^[38;2;0;128;0m…^[39m

The visual result is less predictable because on macOS 10.12.6 Terminal:

  • level 1 output has color #17a41a from default theme
  • level 2 output has color #19ad1d

This is research, because I don’t need to use ansi nor bgAnsi

ansi256

Here is (new chalk.Instance({level)).bgAnsi256(194)('…')

level output
0
1 ^[107m…^[49m
2 ^[48;5;194m…^[49m
3 ^[48;2;204;255;204m…^[49m

Because the contrasting background colors that I intend to use both downsample to 107 I would use chalk.level >= 2 as condition for foreground color with or without background color

However, it would eliminate a test case if levels 2 and 3 have the same format

@pedrottimark
Copy link
Contributor Author

Here are the relevant code snippets from the index.js file:

const levelMapping = [
  'ansi',
  'ansi',
  'ansi256',
  'ansi16m'
];

Here are places for potential short-cut to avoid upsample for some level and model:

  • ansiStyles.color[levelMapping[level]][model](...arguments_)
  • ansiStyles.bgColor[levelMapping[level]][model](...arguments_)

@Qix-
Copy link
Member

Qix- commented Oct 14, 2019

The dependency is there anyway, and ansi to ansi in color-convert is a no-op.

I'd like to see benchmarking results to see if this is really a bottleneck.

Also something worth mentioning: the 16 basic (level 1) color palette is considered sacred; many users override the default colors in their emulator and rely on applications to differentiate between "I'm using the green palette color" and "I seriously need this to be green on the screen", because those are not equal use cases.

In the former case, one would use chalk.green. In the latter, one might use chalk.ansi(32).

Here's kind of where things get less-than-ideal (this isn't a dig at you, I'm merely stating this here to explain why Chalk's API might seem ugly or counterintuitive): Chalk is dealing with a horribly outdated and under-documented standard that is decades old and mainly laid out by a wikipedia article, completely ignoring the terminfo database (which would make it functionally 'correct') - which would be worse if it weren't for the fact that the terminfo database's format is also horribly outdated (e.g. 256 colors nor truecolor are represented in the database format) and getting PRs into that project can take longer than some Chalk users have been alive. This is all exasperated by the fact that most emulators and shells don't adhere to any single terminfo database entry and generally implement the version on Wikipedia (which loosely resembles the official Xterm terminfo profile). And even worse, most modern applications that don't use a wrapper just assume the emulator supports these codes, too - so the terminfo database is quickly becoming obsolete and irrelevant.

However, there is no replacement - there is (currently) no standard for control sequences, and given that most applications use text-only streams, there's no great way to make an alternative programmatic interface to the enclosing emulator (assuming there is an enclosing emulator, which is not always the case when the application runs under e.g. systemd, docker, etc.)

Further, there is no "intended use" of the color sequences. As I mentioned above, the basic 16 color palette has no guarantee that e.g. 32m is green on the screen because, prior to widespread ansi256 and truecolor support in emulators, developers wanted to match their emulators to their display managers' skin/schemes, which means a lot of those colors get completely redefined.

Conversely, I've yet to see an emulator that supports re-defining the 256 color palette or re-interpret truecolor control sequences. Since both ansi256 and truecolor escapes give the application developer reasonable near-guarantees that the color they specified is the color that will show up on the screen, they might choose those colors over the basic 16-color palette in cases where accurate colors are important.

Fast-forward to the Chalk library's API design. We have to support this kind of bizarre, rock-and-a-hard-place use case if we're going to support higher level color models (e.g. ansi256 or truecolor).

If there is a better way to handle the API given all of this information, please by all means suggest it, because I would love to see the UX improved if it's possible.

At the very least, if nothing functional comes out of this issue, we should document this since I also think this is an important observation. Thank you for bringing it up :)

@pedrottimark
Copy link
Contributor Author

Josh, thanks for sharing your experience with ANSI escape codes

Two thoughts jumped out:

the 16 basic (level 1) color palette is considered sacred; many users override the default colors in their emulator

I've yet to see an emulator that supports re-defining the 256 color palette

  1. You redirected my path of exploration:

    • To mix 16-color color palette with 256-color palette might be harmful instead of helpful to some people

    • For background color as visual cue whether or not adjacent lines are related, I should explicitly design for Truecolor through a conditional expression:

    level alternative comment
    3 chalk.rgb(…).bgRgb(…) acceptable fg-bg contrast
    2 chalk.ansi256(…).bgAnsi256(…) marginal fg-bg contrast
    <= 1 basic foreground color respect user configuration

    It’s predictable, testable, and 3.0.0-beta.2 supports it from TypeScript ❤️

  2. Your comment can be read as explaining why not to convert upward to a higher-level format more clearly than my comment did

    After the following code snippet, I will be quiet, and leave it in your capable hands

    const convert = (colorObject, model, level) => {
      if (model === 'ansi' || model === 'ansi256' && level >= 2) {
        // Avoid converting model to higher-level format.
        return colorObject[model][model];
      }
    
      // Convert model to equal-or-lower-level format.
      return colorObject[[levelMapping[level]][model]];
    };
    • const styler = createStyler(convert(ansiStyles.color, model, level)(...arguments_), …);
    • const styler = createStyler(convert(ansiStyles.bgColor, model, level)(...arguments_), …);
  3. My only suggestion to improve the already excellent UX of chalk is in Add ansi and bgAnsi to TypeScript declaration #369 (comment) to remove ansi and bgAnsi from its public interface because limited use and potential misuse

    The preceding code snippet would change: if (model === 'ansi256' && level >= 2) {

  4. I am happy to lend a hand with technical writing under your direction

@Qix- Qix- mentioned this issue Nov 20, 2020
7 tasks
@Qix-
Copy link
Member

Qix- commented Apr 19, 2021

Hey there, thanks for the detailed response. 2020 was a wild ride - sorry for the insane delay on this.

I really appreciate all of the exploration and suggestions. A few others (including Sindre) have since agreed, and we're removing the upscaling by removing the color conversion altogether (save for a few simple methods).

Going to go ahead and close this to clean up a bit - thanks again for making me think quite a lot about this 😃

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

No branches or pull requests

2 participants