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

Add proper RGB color conversion for CLI #1289

Closed
wants to merge 1 commit into from
Closed

Add proper RGB color conversion for CLI #1289

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 22, 2021

This solution (mostly) addresses #1282, only diverging in the fact that the colors weren't being stripped due to a minor bug.

@LadyCailin
Copy link
Member

Hmm, doesn’t this logic need to go in the Static.MCColorToANSI function, rather than here? As far as I know, the in game colors were correct, and support any given RGB value, and don’t need to be perceptually scaled, however, the colors in a terminal do. Or maybe I’m misunderstanding what the PR does.

@ghost
Copy link
Author

ghost commented Aug 22, 2021

Hmm, doesn’t this logic need to go in the Static.MCColorToANSI function, rather than here? As far as I know, the in game colors were correct, and support any given RGB value, and don’t need to be perceptually scaled, however, the colors in a terminal do. Or maybe I’m misunderstanding what the PR does.

Maybe I misunderstood what the purpose of fromRGBValue was?
Actually, MCToANSI merely does a replacing of the game codes to TermColors. Nonetheless, it still needs fromRGBValue output from color() and colorize().
The old method merely splitted the given hex code and making the last the one in effect.

@LadyCailin
Copy link
Member

Well, maybe there’s an additional bug I’m not aware of, but at least the point of #1282 was that using color(‘#123456’) in cmdline would end up with the color being stripped out. #1246 originally supported the RGB format in Minecraft, but since there’s a small finite set of valid colors for the terminal, they would have to be scaled or removed, and the first attempt was just to remove them. So that PR was merged in and the #1282 feature request created for cmdline.

But if you’re suggesting that #1256 has a bug, and this fixes it, that’s great, but the title is misleading. In any case, I’ll have to take a deeper look at this later, when I’m not on my phone :D

@PseudoKnight
Copy link
Contributor

fromRGBValue converts hex #FF0000 to the special Spigot supported format §xFF0000, where Spigot converts this later to a text component with an RGB color that MC can use. Changing this would break RGB colors in Minecraft.

Static.MCColorToANSI is called to convert legacy MC text formatting codes, like §a and now this special Spigot RGB format, to terminal formatting codes.

I think perhaps some of this should be moved to MCColorToANSI, and some to TermColors, where a new method can be added to do the actual conversion of RGB to TermColor.

@ghost
Copy link
Author

ghost commented Aug 22, 2021

So it looks like I committed (PR'ed, at that) a mistake.

The output of printing to the terminal with color0 and/or colorize(), contrary to the stated at #1282, is not stripped of colors, but also accompanied by the §x[$...] pattern. My solution aimed to take advantage of MCColorToANSI also, which maps Minecraft (and conveniently Windows) to TermColor instances. However, a solution to the wrong problem in the wrong circumstances.

@ghost ghost changed the title Addresses #1282 Add proper RGB color conversion for CLI Aug 25, 2021
@LadyCailin LadyCailin closed this Mar 21, 2022
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