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

New feature: xterm grayscale palette (2+24 colors) #1199

Closed
wants to merge 5 commits into from

Conversation

worstje
Copy link
Contributor

@worstje worstje commented Feb 5, 2017

Brief overview of PR changes/additions

This PR adds support for the grayscale range of the 256-color palette (palette entries 232-255). It makes these available for both foreground and background colors. Because the numbers worked out nicely (24 grayscale xterm entries @ index 232..255 + pure black @ index 0 + pure white @ index 15), the roman alphabet is leveraged to index the entire grayscale spectrum from one command.

The new markup should match the existing logic behind the formatting of existing Evennia-specific markup; first the PIPE / ACCOLADE, then a possible BACKGROUND marker, then the specification of the color. For existing markup codes, this could be an ANSI 'letter' or xterm RGB number.

I have chosen the EQUALS SIGN (=) as the 'grayscale marker'; it is not yet used, easily accessible on most keyboards and the two-lines give me a feeling of belonging with the monochrome nature of the palette.

All of the above reasoning combined gives the following new markup codes which are implemented in this PR:

|=a thru |=z affect the foreground from BLACK to WHITE.
|[=a thru |[=z affect the background from BLACK to WHITE.

{= and {[= varieties have also been implemented.

Motivation for adding to Evennia

The game I play on (ARXmush) has a bunch of pretty good crafters, and while the six 'rgb' grayscale shades work alright for most cases, I do hear the occasional grumble about some gradients not being able to look nice. Which reminded me (having messed with the xterm spec on several occasions in the past) that it is a bit wasteful not to have the rest of the palette available for use. Having checked the Evennia codebase at an earlier date, I could verify that Evennia has zero support for these shades itself, thus making upstream the logical place to implement it.

Additionally, Tehom is a wise man and wants to avoid editing stock files as much as possible to keep merging and conflicts to a minimum. ;-) Thus my arrival at the source of it all to help the greater world.

Other info (issues closed, discussion etc)

I noticed doubled entries in xterm256_map when I started; I have removed those because they seemed to serve no purpose. (Everything still works fine from what I could test.)

The implementation for this new functionality was added inside ANSIParser.sub_xterm256() for several reasons:

  1. It minimally affects the rest of the codebase; otherwise I would have had to figure out the best way to add the regexes for these particular markup sequences.
  2. It fits the name; it is still a xterm256 feature, and the mapping xterm256_map remains accurate.
  3. The ansi-fallback code is inside of ANSIParser.sub_xterm256(); duplicating that into my own function would just cause code-duplication and make it harder to maintain the aspect of gracefully downgrading to ANSI in the future.

A grayscale color-swatch has also been added to @Colors xterm256. I went through a couple of testing variations and ended up with something that matches the existing 6x6x6 color cube swatches in layout. However, due to this new palette covering 26 colors, there was no way to make it look perfect. Had I only included the grayscale palette entries (232..255) number 24 in total, it would have been perfect... save for the missing 'a' and 'z' entries. This could possibly lead to the confusion of users, so instead I decided to add the two extra colors as well for the sake of familiarity.

I opted for plain bright red (|500)as the foreground color for the background swatches; it appeared to me as the most legible out of a few things I tried.

Finally ...

Thanks to the Evennia project, its contributors and maintainers for allowing the game I play today to exist. (Also thanks to the actual people running it, but they might not have gotten it off the ground yet if it wasn't for you guys!)

If this PR is in any way lacking, please let me know and if it is in my capabilities, I'll do my best to alleviate / address you concerns and needs.

@Griatch
Copy link
Member

Griatch commented Feb 5, 2017

Thanks for a well-written and well described PR! I looked some more at it and it seems like a reasonable syntax to use |=a-|=z for greyscale. I have a few comments on the code:

This does not properly convert the greyscale to HTML markers for the webclient. That is, it converts almost all the greyscale since it turns out the webclient actually has CSS support for the entire 255 table even though Evennia didn't implement this in its color markup. What is missing is solid black and solid white, which you in ansi.py set to 0 and 15. Change that to xterm 000 and 555 and the webclient text2html converter should be able to pick it up without further modification I think.


# ansi fallback logic expects r,g,b values in [0..5] range
gray = (ord(letter)-97)/5.0
r, g, b = gray, gray, gray
Copy link
Member

Choose a reason for hiding this comment

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

This should be red, green, blue to work with the ansi fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I spotted it right before you commented. Already fixed it.

@worstje
Copy link
Contributor Author

worstje commented Feb 5, 2017

I made the changes needed for the webclient. Initially I thought that {000 and {555 might not be absolute colors which is why I went for the easy indexes that match those in the ANSI standard. But that was a mistaken impression on my part.

As I tested the fix I noticed there's another bug that needs fixing in the WebClient. However, I don't think this patch is to blame so I haven't touched it. I think the nature of HTML is collapsing multiple spaces, which throws off the alignment of the table since my commands require an extra space to fill their 'swatch' up for visual purposes. Likely it would be fixed by having multiple spaces get replaced with ' ' instead; but that is just guesswork on my part without looking into it. (It requires a bug report, no doubt, but that'll come later.)

I also just noticed I dragged down the coverage percentage a bit; if I see relevant tests for this markup stuff I'll make sure to add mine to the collection.

Edit: The closest test I see is ANSIStringTestCase.test_instance(), which has a few color strings but tests a whole bunch of other things at the same time. As I don't quite get how that class works internally, I'd rather not touch it. In theory, trivially adding a few colors and their encoded forms ought to be easy, but when you don't quite understand the class you are editing the test case for, you might just invalidate or silently break some aspect you are not aware of. I'd rather not touch it and leave it to someone more knowledgable.

@Griatch
Copy link
Member

Griatch commented Feb 5, 2017

Thanks, that looks correct now! No worries about the unittests for colors, there are no unittests specifically for this (although there will eventually be); the ANSIString is a custom string type that correctly handles color tags when it comes to measuring its length etc, it is not directly related to the testing of colors themselves.

Merged with a rebase. I did take the liberty of reformatting the @color xterm output to make it more consistent with the rest of the table. This is of course just a matter of taste, but even without writing out the value for absolute black one can deduce what it must be easily. Also, I changed the foreground color of the background-greys from red to the inverse. This means the mid-color fades out but it's consistent with the rest of the table (which uses the same trick) and one can easily deduce the sequence anyway. Here's how it looks for me now:
example screenshot

You are correct that the webclient does not format this quite so neatly due to HTML swallowing blankspace. It is rarely noticeable but for cases like this one - and probably does deserve its own issue report.

@Griatch Griatch closed this Feb 5, 2017
@worstje
Copy link
Contributor Author

worstje commented Feb 5, 2017

I think I tried the inverse grayscale for the background colors at some point, but I think I worried about the middle-of-the-pack legibility. It probably is more acceptable than I gave it credit for, but I kept messing around with the two extra values at the same time so it kinda got snowed over.

Your final look is pretty decent. I'd probably be tempted to put in an extra blank space for the sheer sake of looking nice; the grayscale just doesn't seem to fit very well with the rest of the color swatches due to the lack of saturation.

Potato, potato. Thanks for the merge!

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.

None yet

2 participants