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

Adds true color support #1628

Closed
wants to merge 18 commits into from
Closed

Conversation

veeman
Copy link

@veeman veeman commented Mar 7, 2022

Fixes #48

Added true color support.
Based on the #103 pull request implementation.

As in this implantation the struct Attribute changed to class Attribute.
Also there are now TrueColorAttribute and TrueColor definitions wich can be used as dropin classes to provide the new feature.

The abstract ConsoleDriver has two more properties which determinates the true color support state of the driver:

bool SupportsTrueColorOutput { get; }
bool UseTrueColor { get; set; }

The drivers WindowsDriver and NetDriver are extended to render the new true colors.
Also the WindowsDriver is optimized to do the rendering faster.
The CursesDriver is not touched yet and would run only in the basic color mode.

Moreover i added an exampel scenario and some basic unit test for this feature.

Currently only tested on Windows 10.

Any feedback is appreciated.

@BDisp
Copy link
Collaborator

BDisp commented Mar 8, 2022

I'm loving your work on this, thanks. I think for CursesDriver you will need to use can_change_color and init_color. See https://stackoverflow.com/questions/25091638/other-colors-in-ncurses and https://invisible-island.net/ncurses/man/curs_color.3x.html#h3-can_change_color.

@NoUDerp
Copy link

NoUDerp commented Mar 30, 2022

I'm using this to render simple canvases as a window. I made a modification for a custom constructor for TrueColorAttribute to allow providing the linux 256 color (calculated on a GPU) to avoid calculating it on the CPU, for flexibility. I'm taking quite a GC beating over the high frequency allocations of TrueColorAttribute being a reference type when I update the frame (it's video). The best solution I came up with was making the base Attributes Foreground, Background, and Value into read/write, but requires I run a modified library.

public TrueColorAttribute(TrueColor foreground, Color foreground_color4, TrueColor background, Color background_color4)
: base(foreground_color4, background_color4)
{
TrueColorForeground = foreground;
TrueColorBackground = background;
}

@veeman
Copy link
Author

veeman commented Apr 28, 2022

I looked into the ncurses library.
They provide for true color support the methods init_color and init_extended_color.
However this methods are used to define indexed rgb values which in turn can then be used to identify the color.

With the current implementation, we need some sort of mapping to these indexed colors and keep track of them.

Currently this sounds very inefficient to me. So I will not do this.

The true collor support works just fine. The NetDriver also works on Linux and Mac.

In my experience, the bottleneck for rendering various things is in the drivers using StringBuilder
that are instantiated on each run and the entire screen is reprinted.

The whole Drivers could be optimized further, but this should be another task.

@tig tig changed the title true color support Adds true color support May 7, 2022
@tig
Copy link
Collaborator

tig commented May 7, 2022

This PR is pretty darn exciting. I'd love to get it merged as soon as it is ready.

@veeman could you update us on your ability/plans to complete this awesome work?

Thanks!

@JessicaMulein
Copy link
Contributor

Awesome, I still have to keep my promise to make HACC support Spectre.Console and it supports true color so this will be hot as snot!

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

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

This PR is a big deal. And great.

  1. I believe this is a breaking change and that there's probably no way to NOT make it a breaking change. Thus this PR will likely define Terminal.Gui v2.x.
  2. The ColorPicker and other Scenarios that demo colors should be updated to use TrueColor support. I suggest that be done as separate PR's that are dependent on this one.
  3. I've asked for some changes throughout. I did not run this code and my review was fairly superficial. We'll need many more eyes on this given the magnitude of the changes.

@@ -82,6 +82,86 @@ public enum Color {
White
}

/// <summary>
/// 24bit color. Support translate it to 4bit(Windows) and 8bit(Linux) color
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change summary to:

/// Represents a 24bit color. Supports translating to 4bit (Windows) and 8bit (Linux) colors.

/// <param name="red"></param>
/// <param name="green"></param>
/// <param name="blue"></param>
public TrueColor (int red, int green, int blue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please apply correct source code formatting to all files in this PR (hit Ctrl-K-D in VS).

Copy link
Author

Choose a reason for hiding this comment

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

Auto format applied.

Terminal.Gui/Core/ConsoleDriver.cs Show resolved Hide resolved
/// <summary>
/// Defines a true color attribute.
/// </summary>
public class TrueColorAttribute : Attribute {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused as to why this class is needed? Why not just put this into Attribute and make it so true color is the default/assumed, supporting non-truecolor as a fallback?

When would someone use this class?

Copy link
Author

@veeman veeman Nov 29, 2022

Choose a reason for hiding this comment

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

As describe before we need to decide which color escape sequence we should use. To keep backward compatibility i just introduced a new TrueColorAttribute class which holds the rgb values, like in the other PR.

This method has also the advantage that you only need to create new color escape sequences when the instance reference changes. However i can not evaluate if this has further side effects.

Furthermore there is no easy way to merge basic color and truecolor attribute class because any platform defines its own basic color range. The later must be unified first.

Copy link
Collaborator

@tznind tznind Nov 29, 2022

Choose a reason for hiding this comment

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

Would it be possible to write a mapping algorithm for true color to Console Color (and vice versa)?

An approximation would be fine, I'm guessing someone has already done this elsewhere.

If we could translate between the two then we could just add the properties to Attribute and add a new true color constructor. So the pseudocode would be something like:

public class Attribute
{
	public int RedForeground { get; }
	public int GreenForeground { get; }
	public int BlueForeground { get; }

	public int RedBackground { get; }
	public int GreenBackground { get; }
	public int BlueBackground { get; }

	public Color Foreground { get; }
	public Color Background { get; }

	public Attribute (Color foreground, Color background)
	{
		Foreground = foreground;
		Background = background;

		var m = MapToTrueColor (foreground);
		RedForeground = m [0];
		GreenForeground = m [1];
		BlueForeground = m [2];

		m = MapToTrueColor(background);
		RedBackground = m[0];
		GreenBackground = m[1];
		BlueBackground = m[2];
	}

	public Attribute (int redForeground, int greenForeground, int blueForeground, int redBackground, int greenBackground, int blueBackground)
	{
		RedForeground = redForeground;
		GreenForeground = greenForeground;
		BlueForeground = blueForeground;


		RedBackground = redBackground;
		GreenBackground = greenBackground;
		BlueBackground = blueBackground;

		Foreground = MapToBasicColor (redForeground,greenForeground,blueForeground);			

		Background = MapToBasicColor (redBackground,greenBackground,blueBackground);			
	}

    [...]
}

That would allow clients to mix and match true color whenever they wanted. And even have an app that supports true color but falls back on basic colors when deployed to an older machine (with zero effort on the API consumers behalf)?

EDIT: Just realized Attribute describes both foreground and background so have updated the pseudocode.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, something like that could work. Maybe we can track the index by the lifetime of the Attribute class and use a database approach to select a free index by incrementing a static next-index value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently this is the RGB breakdown. I might pull your branch and see if a 'transparent' and backwards compatible approach can be adopted following this approach.

Name R G B
Black 00 00 00
DarkBlue 00 00 80
DarkGreen 00 80 00
DarkCyan 00 80 80
DarkRed 80 00 00
DarkMagenta 80 00 80
DarkYellow 80 80 00
DarkGray 80 80 80
Blue 00 00 FF
Green 00 FF 00
Cyan 00 FF FF
Red FF 00 00
Magenta FF 00 FF
Yellow FF FF 00
Gray C0 C0 C0
White FF FF FF

Copy link
Collaborator

Choose a reason for hiding this comment

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

@veeman I've started working on this approach in https://github.com/tznind/gui.cs/tree/true-color

Once its ready (and assuming its viable) I will open a PR to your branch.

@@ -246,109 +370,109 @@ Attribute SetAttribute (Attribute attribute, [CallerMemberName] string callerMem
case "TopLevel":
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole switch statement scares me. So much brute-force code! It makes me wonder if there's a way of re-factoring it as part of this PR to use reflection or something to radically simplify it?

/// <summary>
/// Determinates if the current console driver supports TrueColor output-
/// </summary>
public virtual bool SupportsTrueColorOutput { get => false; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused as to why there's ALSO a method in CursesDriver.cs called CanColorTermTrueColor. Why not just have CursesDriver implement an override of this method?

I think this method should be named SupportsTrueColor for tersness.

Copy link
Collaborator

@BDisp BDisp Aug 4, 2022

Choose a reason for hiding this comment

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

This must be abstract in the ConsoleDriver.cs to force to be implemented in all drivers.

Copy link
Author

Choose a reason for hiding this comment

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

The CursesDriver currently does not implement the TrueCollorAttribute feature.
On *unix, there is no easy way to determinate if the current terminal supports color output.
Despite ncruses provides a check for color support, it tells only if the library is compiled with color output support.

The unofficial community way is to check the environment variable COLORTERM for some magic values. This does the static CursesDriver.CanColorTermTrueColor function. This variable is also set under WSL, so i just retrive this information in the NetDriver.

The virtual property SupportsTrueColorOutput tells if the used driver and the used terminal environment supports color output.

The property UseTrueColor is used to decide which mode should be used. If SupportsTrueColorOutput is set then UseTrueColor is also set, but can be uesed to enter baisc color rendering.

@tig tig added enhancement breaking-change For PRs that introduces a breaking change (behavior or API) labels Oct 14, 2022
@tig tig added this to the v2.0 milestone Oct 14, 2022
@tig
Copy link
Collaborator

tig commented Oct 14, 2022

Marked for the v2.0 milestone.

rheaney added a commit to rheaney/GalacticFilaments.Terminal.Gui that referenced this pull request Nov 16, 2022
Using the PR "Adds true color support gui-cs#1628" from gui-cs/Terminal.Gui (https://github.com/gui-cs/Terminal.Gui/pull/1628/files), I added true color support to the Terminal.
@veeman
Copy link
Author

veeman commented Nov 29, 2022

I merged this PR on the latest devel branch.

The WindowsDriver and NetDriver is feature complete however should be performance tested.

Attached an image of the True Colors ui catalogue scenario:
screen

The CursesDriver can currently not be extended (i have no plan how to do this) with the current attributes architecture to provide color output. ncurses needs pre initialized indexed color values which is not compatible with the current implementation.

@BDisp
Copy link
Collaborator

BDisp commented Nov 29, 2022

The CursesDriver can currently not be extended (i have no plan how to do this) with the current attributes architecture to provide color output. ncurses needs pre initialized indexed color values which is not compatible with the current implementation.

Great work thanks. Do you have any idea how to achieve that?

@tznind
Copy link
Collaborator

tznind commented Dec 4, 2022

@veeman I've created a PR here veeman#2 with some improvements and changes. Including this new Scenario for opening images:

Note: You can see that when toggling true/non-true color the exact RGB values rendered on screen change. I need to dig into why this is but I think its because each console will render Color.X to a slightly different RGB value (which might prove a problem).

sauron

@tig tig removed this from the v2.0 milestone Feb 28, 2023
@tig
Copy link
Collaborator

tig commented Mar 3, 2023

We need to retarget this PR at the v2_develop branch.

@tig
Copy link
Collaborator

tig commented Aug 9, 2023

Done! See #2612

(and #2729)

@tig tig closed this Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change For PRs that introduces a breaking change (behavior or API) enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Truecolors (direct RGB colors) support
6 participants